Re: [PATCH v3] ext4: shrink directory when last block is empty

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

On Tue, Feb 26, 2019 at 08:01:18PM -0800, harshadshirwadkar@gmail.com wrote:
>
+static bool
+ext4_dx_delete_entry(handle_t *handle, struct inode *dir,
+		     struct dx_frame *dx_frame, __le64 block)
+{
>

The function name is a bit problematic.  The ext4_find_entry,
ext4_dx_find_entry, ext4_delete_entry() all operate on the directory
entry.  This is doing something different --- it's operating to remove
the hash tree dx entry.  So something maybe like
ext4_remove_dx_entry()?  And we definitely need some documentation for
this function.

Also, I think we can drop last argument, since you can get it from
 cpu_to_le64(dx_get_block(dx_frame->at)).

> +
> +static inline bool should_try_dx_delete(struct dx_frame *dx_frame,
> +					struct buffer_head *bh,
> +					struct inode *dir)
> +{
> +	return dx_frame && dx_frame->bh && is_empty_dirent_block(dir, bh) &&
> +			dx_get_block(dx_frame->at) ==
> +			(dir->i_size - 1) >> dir->i_sb->s_blocksize_bits;
> +}

As above, I'm not sure this is a great name for the function --- and
for that matter, I'm not sure it's worth it to separate this out.
First of all, we want to be able to truncate non-indexed directory,
not just indexed directories.  So moving this logic into
ext4_delete_entry() probably makes sense.

If the directory is not indexed, it's really trivial to truncate it
--- and the xfstests change you submitted would fail on file system
configurations if the dir_index feature is not set, so we should
really do that simple case while we're at it.


> -static struct buffer_head * ext4_find_entry (struct inode *dir,
> -					const struct qstr *d_name,
> -					struct ext4_dir_entry_2 **res_dir,
> -					int *inlined)
> +static struct buffer_head *ext4_find_entry(struct inode *dir,
> +					   const struct qstr *d_name,
> +					   struct ext4_dir_entry_2 **res_dir,
> +					   int *inlined,
> +					   struct dx_frame *dx_frame)

Could you add some documentation for this function --- specifically,
why a caller might want to pass in dx_frame, and what it's used for?

BTW, ext4_rmdir() should have also been modified to pass in dx_frame
when calling ext4_find_entry().  Right now with this patch, if the
last entry is a directory, when it's rmdir'ed, since ext4_rmdir()
doesn't have the plumbing to pass dx_frame to ext4_delete_entry(),
we'll end up leaving an empty directory entry on the directory entry.

Thanks,

      	     	     	      		- Ted