Re: [PATCH 67/73] ext2: Remove target inode pointer from ext2_add_entry() [ver #2]

[Ted Ts'o <tytso@mit.edu>]

On Tue, Feb 21, 2012 at 06:05:54PM +0000, David Howells wrote:
>  
> -static inline void ext2_set_de_type(ext2_dirent *de, struct inode *inode)
> +static inline void ext2_set_de_type(ext2_dirent *de, struct super_block *sb,
> +				    umode_t mode, unsigned char file_type)
>  {
> -	umode_t mode = inode->i_mode;
> -	if (EXT2_HAS_INCOMPAT_FEATURE(inode->i_sb, EXT2_FEATURE_INCOMPAT_FILETYPE))
> -		de->file_type = ext2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
> -	else
> +	if (!EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE)) {
>  		de->file_type = 0;
> +		return;
> +	}
> +
> +	if (file_type)
> +		de->file_type = file_type;
> +	else
> +		de->file_type = ext2_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
>  }

It would be simpler to drop the umode_t mode parameter, and just
always make the caller pass in the correct file_type.  In fact, this
manual calculation only needs to happen in one place, ext2_set_link(),
so might as well move "ext2_type_by_mode[(mode & S_IFMT) >> S_SHIFT]"
to ext2_set_link().

See below....

> -int ext2_add_entry(struct dentry *dentry, struct inode *inode)
> +/*
> + * Called from three settings:
> + *
> + * - Creating a regular entry - de/page NULL, doesn't exist
> + * - Creating a fallthru - de/page NULL, doesn't exist
> + * - Creating a whiteout - de/page set if it exists
> + *
> + * @new_file_type is either EXT2_FT_WHT, EXT2_FT_FALLTHRU, or 0.  If
> + * 0, file type is determined by inode->i_mode.
> + */

One of the things that confused me at first when I reviewed these
patch series is that the patch that introduces whiteouts and
fallthroughs haven't been introduced yet; they come later.  Yet in
this patch and in others, sometimes the comments mention fallthru and
whiteouts earlier.

It might be simpler just to fold the commits that adds fallthrus and
whiteouts into a single patch; and then make sure all of the comments
that mention fallthrus and whiteouts are included there.  I'll
mentioned later, but it's also not clear it makes sense to have
separate INCOMPAT options whiteouts and fallthrus.  Is there any time
when you would have one, but not another?  And why can't it just be an
RO_INCOMPAT option?  As long as the inode number field is zero, older
kernels will simply assume those directory entries represent deleted
entries, which should be fine.

> @@ -660,14 +684,14 @@ int ext2_make_empty(struct inode *inode, struct inode *parent)
>  	de->rec_len = ext2_rec_len_to_disk(EXT2_DIR_REC_LEN(1));
>  	memcpy (de->name, ".\0\0", 4);
>  	de->inode = cpu_to_le32(inode->i_ino);
> -	ext2_set_de_type (de, inode);
> +	ext2_set_de_type (de, inode->i_sb, inode->i_mode, 0);

In this and the following ext2_set_de_type() (for adding the "." and
".." entries in a new directory), the type can *only* be EXT2_FT_DIR.
So why not pass it in explicitly, and save the table lookup?  This is
also why we should be able to drop passing in inode->i_mode to
ext2_set_de_type entirely --- the only place where we really need to
calculate file_type is ext2_set_link().

						- Ted