2008-10-06 10:31:38

by Kalpak Shah

[permalink] [raw]
Subject: [PATCH][5/15] e2fsprogs-uninit-fixes.patch

Bug fixes for uninit_bg feature:
- display correct inode number during BG_INO_UNINIT and INOREF_IN_USED
errors
- restart e2fsck only once in case of above errors
- initialize bitmaps properly considering whether block group is uninit
or not (needed for e2freefrag)

Signed-off-by: Kalpak Shah <[email protected]>


Attachments:
e2fsprogs-uninit-fixes.patch (15.36 kB)

2008-10-10 18:26:23

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH][5/15] e2fsprogs-uninit-fixes.patch

On Mon, Oct 06, 2008 at 04:01:10PM +0530, Kalpak Shah wrote:
> Bug fixes for uninit_bg feature:
> - display correct inode number during BG_INO_UNINIT and INOREF_IN_USED
> errors
> - restart e2fsck only once in case of above errors
> - initialize bitmaps properly considering whether block group is uninit
> or not (needed for e2freefrag)
>
> Signed-off-by: Kalpak Shah <[email protected]>

> Bug fixes for uninit_bg feature:
> - display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
> - restart e2fsck only once in case of above errors
> - initialize bitmaps properly considering uninit or not (needed for e2freefrag)
>
> Index: e2fsprogs-1.41.1/debugfs/set_fields.c
> ===================================================================
> --- e2fsprogs-1.41.1.orig/debugfs/set_fields.c
> +++ e2fsprogs-1.41.1/debugfs/set_fields.c
> @@ -566,6 +566,7 @@ void do_set_block_group_descriptor(int a
> }
>
> set_gd = current_fs->group_desc[set_bg];
> + set_sb = *current_fs->super;
>
> if (ss->func(ss, argv[3]) == 0) {
> current_fs->group_desc[set_bg] = set_gd;

Why is this needed? None of the table entries used by
set_block_group_descriptor requires set_sb to be set?


> Index: e2fsprogs-1.41.1/e2fsck/problem.c
> ===================================================================
> --- e2fsprogs-1.41.1.orig/e2fsck/problem.c
> +++ e2fsprogs-1.41.1/e2fsck/problem.c
> @@ -1301,12 +1301,12 @@ static struct e2fsck_problem problem_tab
>
> /* Inode found in group where _INODE_UNINIT is set */
> { PR_2_INOREF_BG_INO_UNINIT,
> - N_("@i %i found in @g %g where _INODE_UNINIT is set. "),
> + N_("@i %N found in @g %g where _INODE_UNINIT is set. "),
> PROMPT_FIX, PR_PREEN_OK },
>
> /* Inode found in group unused inodes area */
> { PR_2_INOREF_IN_UNUSED,
> - N_("@i %i found in @g %g unused inodes area. "),
> + N_("@i %N found in @g %g unused inodes area. "),
> PROMPT_FIX, PR_PREEN_OK },
>

This doesn't look right. %N prints pctx.num, which is set to the
directory entry's offset. It looks like %Di is the correct expansion,
and maybe something like this:

@E references @i %Di in @g %g where _INODE_UNINIT is set.\n

and

@E references @i %Di in @g %g unused inods area.\n

- Ted

2008-10-11 13:43:06

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH][5/15] e2fsprogs-uninit-fixes.patch

On Mon, Oct 06, 2008 at 04:01:10PM +0530, Kalpak Shah wrote:
> Bug fixes for uninit_bg feature:
> - display correct inode number during BG_INO_UNINIT and INOREF_IN_USED
> errors
> - restart e2fsck only once in case of above errors
> - initialize bitmaps properly considering whether block group is uninit
> or not (needed for e2freefrag)
>
> Signed-off-by: Kalpak Shah <[email protected]>

> Bug fixes for uninit_bg feature:
> - display correct inode number during BG_INO_UNINIT and INOREF_IN_USED errors
> - restart e2fsck only once in case of above errors
> - initialize bitmaps properly considering uninit or not (needed for e2freefrag)
>

Note: While I really do very much appreciate your diffs against the
luster e2fsprogs tree, in the future, it would be *really* helpful if
you could separate out patches so that each patch does only one thing.
It makes it much easier to audit patches, and then if I end up taking
some parts of a patch, but not all of it, it becomes easier on your
end to match up what I've taken from what I haven't taken. There are
some hunks of this patch I which I will probably never take, such as
the debugfs/set_fields.c patch, which as far as I can tell serves no
purpose, and the addition of ext2fs_super_and_bgd_loc2() in
lib/ext2fs/closefs.c (which as near as I can tell was taken from an
earlier version of jose's 64-bit patches):

> + * XXX: Remove this when we upgrade our e2fsprogs patches. Also blk64_t has been
> + * changed to blk_t here since this version does not have 64-bit support.
> + */
> +errcode_t ext2fs_super_and_bgd_loc2(ext2_filsys fs,


If this had been separated into a second patch, it becomes easier to
drop that one patch hunk from the patch.

Anyway, just a suggestion....

I will be trying to separate out a few of the bug fixes for the maint
branch for an e2fsprogs 1.41.3 release, but most of this is going to
be reserved for later in the master branch, after quite a bit of
refinement.

- Ted