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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 12 Aug 2019 17:24:39 -0700
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     "Theodore Ts'o" <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Andreas Dilger <adilger@...ger.ca>
Subject: Re: [PATCH v3] ext4: shrink directory when last block is empty

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