On Tue, Feb 26, 2019 at 08:01:18PM -0800, harshadshirwadkar@xxxxxxxxx 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
