Thanks for the review Ted and I'm sorry for the delay in handling
these comments. I have sent out a V4 of this patch where these
comments are handled.
- Harshad
On Sun, Apr 7, 2019 at 4:58 PM Theodore Ts'o <tytso@xxxxxxx> wrote:
>
> 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
