[7/7] tune2fs: Update dir checksums when clearing dir_index feature

When clearing dir_index feature while metadata_csum is enabled, we have
to rewrite checksums of all indexed directories to update checksums of
internal tree nodes.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 misc/tune2fs.c | 143 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 95 insertions(+), 48 deletions(-)

On Feb 13, 2020, at 3:16 AM, Jan Kara <jack@suse.cz> wrote:
> 
> When clearing dir_index feature while metadata_csum is enabled, we have
> to rewrite checksums of all indexed directories to update checksums of
> internal tree nodes.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> 
> +#define REWRITE_EA_FL		0x01	/* Rewrite EA inodes */
> +#define REWRITE_DIR_FL		0x02	/* Rewrite directories */
> +#define REWRITE_NONDIR_FL	0x04	/* Rewrite other inodes */
> +#define REWRITE_ALL (REWRITE_EA_FL | REWRITE_DIR_FL | REWRITE_NONDIR_FL)
> +
> +static void rewrite_inodes_pass(struct rewrite_context *ctx, unsigned int flags)
> {

My preference these days is to put constants like the above into a named
enum, and then use the enum as the argument to the function rather than
a very generic "int flags" argument.  That makes it clear to the reader
what values the flags may hold, and can immediately tag to the enum, like:

enum rewrite_inodes_flags {
	REWRITE_EA_FL	  = 0x01	/* Rewrite EA inodes */
	REWRITE_DIR_FL	  = 0x02	/* Rewrite directories */
	REWRITE_NONDIR_FL = 0x04	/* Rewrite other inodes */
	REWRITE_ALL	  = REWRITE_EA_FL | REWRITE_DIR_FL | REWRITE_NONDIR_FL
};

static void rewrite_inodes_pass(struct rewrite_context *ctx,
				enum rewrite_inodes_flags rif_flags)
static void rewrite_inodes(ext2_filsys fs, enum rewrite_inodes_flags rif_flags)
static void rewrite_metadata_checksums(ext2_filsys fs,
				       enum rewrite_inodes_flags rif_flags)

Otherwise, when looking at a function that takes "int flags" as an argument,
you have to dig through the code to see what kind of flags these are, and
what possible values they might have.  This is often even more confusing when
there are multiple different kinds of flags accessed within a single function
(not the case here, but happens often enough).

I'm not _against_ the patch, just thought I'd suggest an improvement and see
what people think about it.

Cheers, Andreas

On Tue 18-02-20 13:50:33, Andreas Dilger wrote:
> On Feb 13, 2020, at 3:16 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > When clearing dir_index feature while metadata_csum is enabled, we have
> > to rewrite checksums of all indexed directories to update checksums of
> > internal tree nodes.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > 
> > +#define REWRITE_EA_FL		0x01	/* Rewrite EA inodes */
> > +#define REWRITE_DIR_FL		0x02	/* Rewrite directories */
> > +#define REWRITE_NONDIR_FL	0x04	/* Rewrite other inodes */
> > +#define REWRITE_ALL (REWRITE_EA_FL | REWRITE_DIR_FL | REWRITE_NONDIR_FL)
> > +
> > +static void rewrite_inodes_pass(struct rewrite_context *ctx, unsigned int flags)
> > {
> 
> My preference these days is to put constants like the above into a named
> enum, and then use the enum as the argument to the function rather than
> a very generic "int flags" argument.  That makes it clear to the reader
> what values the flags may hold, and can immediately tag to the enum, like:
> 
> enum rewrite_inodes_flags {
> 	REWRITE_EA_FL	  = 0x01	/* Rewrite EA inodes */
> 	REWRITE_DIR_FL	  = 0x02	/* Rewrite directories */
> 	REWRITE_NONDIR_FL = 0x04	/* Rewrite other inodes */
> 	REWRITE_ALL	  = REWRITE_EA_FL | REWRITE_DIR_FL | REWRITE_NONDIR_FL
> };
> 
> static void rewrite_inodes_pass(struct rewrite_context *ctx,
> 				enum rewrite_inodes_flags rif_flags)
> static void rewrite_inodes(ext2_filsys fs, enum rewrite_inodes_flags rif_flags)
> static void rewrite_metadata_checksums(ext2_filsys fs,
> 				       enum rewrite_inodes_flags rif_flags)
> 
> Otherwise, when looking at a function that takes "int flags" as an argument,
> you have to dig through the code to see what kind of flags these are, and
> what possible values they might have.  This is often even more confusing when
> there are multiple different kinds of flags accessed within a single function
> (not the case here, but happens often enough).
> 
> I'm not _against_ the patch, just thought I'd suggest an improvement and see
> what people think about it.

Yeah, the documentation with enum type is nice. What I somewhat dislike is
that enum suggests 'enumeration' but we actually use values (like say
REWRITE_EA_FL | REWRITE_DIR_FL) which are not really enumerated in the type
definitition. So your scheme works only because enum in C is just an int
with a lipstick on it. So I'm somewhat undecided. Ted, what's your opinion
on this?

								Honza

On Thu, Feb 13, 2020 at 11:16:02AM +0100, Jan Kara wrote:
> When clearing dir_index feature while metadata_csum is enabled, we have
> to rewrite checksums of all indexed directories to update checksums of
> internal tree nodes.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

With regards to the enum, I agree with Jan that using an enum for
bitfields isn't a great fit.  Also, in this case, where it's for a
static function and the definitions don't go beyond a single file, the
advantages of using an enum so we can have strong typing is much less
useful.

One thing which I did notice when trying to test this patch is that

mke2fs -t ext4 -d /usr/projects/e2fsprogs /tmp/foo.img 1G

...does not create any indexed directories.  That's because the
changes to ext2fs_link() only teach e2fsprogs how to add a link to a
directory which is already indexing.  We don't have code which takes a
directory with a single directory block and which doesn't have
directory indexing flag enabled, and converts to a indexed directory.

That might be a nice thing to add at some point.

     	      	     	      	     - Ted

On Mar 15, 2020, at 11:15 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> 
> With regards to the enum, I agree with Jan that using an enum for
> bitfields isn't a great fit.  Also, in this case, where it's for a
> static function and the definitions don't go beyond a single file, the
> advantages of using an enum so we can have strong typing is much less
> useful.

I don't think that "enum" has to mean "sequential integers", but rather
"an listing of related constants" as defined by Wikipedia:

    An enumeration is a complete, ordered listing of all
    the items in a collection.

Giving a list of related constants a name makes the code easier to
understand, especially when the variables have totally generic names
like "flags".  I'm not saying it isn't possible to figure out what
the possible values of that variable are, by hunting around the code
to see what is assigned to it, but making the code easier to understand
at first reading should have value by itself?

Cheers, Andreas

On Sun 15-03-20 13:15:20, Theodore Y. Ts'o wrote:
> On Thu, Feb 13, 2020 at 11:16:02AM +0100, Jan Kara wrote:
> > When clearing dir_index feature while metadata_csum is enabled, we have
> > to rewrite checksums of all indexed directories to update checksums of
> > internal tree nodes.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Thanks, applied.
> 
> With regards to the enum, I agree with Jan that using an enum for
> bitfields isn't a great fit.  Also, in this case, where it's for a
> static function and the definitions don't go beyond a single file, the
> advantages of using an enum so we can have strong typing is much less
> useful.
> 
> One thing which I did notice when trying to test this patch is that
> 
> mke2fs -t ext4 -d /usr/projects/e2fsprogs /tmp/foo.img 1G
> 
> ...does not create any indexed directories.  That's because the
> changes to ext2fs_link() only teach e2fsprogs how to add a link to a
> directory which is already indexing.  We don't have code which takes a
> directory with a single directory block and which doesn't have
> directory indexing flag enabled, and converts to a indexed directory.

Yes, I know and it's kind of deliberate because not all users of
ext2fs_link() want that to happen (e.g. linking into lost+found). So I
wasn't sure what a proper API for this functionality would be and decided
to leave that decision for later. Maybe we could export the "optimize dir"
functionality from e2fsck to be generally available in libext2fs?

								Honza

On Sun 15-03-20 13:15:20, Theodore Y. Ts'o wrote:
> On Thu, Feb 13, 2020 at 11:16:02AM +0100, Jan Kara wrote:
> > When clearing dir_index feature while metadata_csum is enabled, we have
> > to rewrite checksums of all indexed directories to update checksums of
> > internal tree nodes.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Thanks, applied.

Hum, I'm still not seeing this series in e2fsprogs git. Did it get stuck
somewhere?

								Honza