Re: [PATCH v3 20/20] fanotify: no external fh buffer in fanotify_name_event

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jul 15, 2020 at 6:34 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 08-07-20 14:11:55, Amir Goldstein wrote:
> > The fanotify_fh struct has an inline buffer of size 12 which is enough
> > to store the most common local filesystem file handles (e.g. ext4, xfs).
> > For file handles that do not fit in the inline buffer (e.g. btrfs), an
> > external buffer is allocated to store the file handle.
> >
> > When allocating a variable size fanotify_name_event, there is no point
> > in allocating also an external fh buffer when file handle does not fit
> > in the inline buffer.
> >
> > Check required size for encoding fh, preallocate an event buffer
> > sufficient to contain both file handle and name and store the name after
> > the file handle.
> >
> > At this time, when not reporting name in event, we still allocate
> > the fixed size fanotify_fid_event and an external buffer for large
> > file handles, but fanotify_alloc_name_event() has already been prepared
> > to accept a NULL file_name.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Just one tiny nit below:
>
> > @@ -305,27 +323,34 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >   * Return 0 on failure to encode.
> >   */
> >  static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
> > -                           gfp_t gfp)
> > +                           unsigned int fh_len, gfp_t gfp)
> >  {
> > -     int dwords, type, bytes = 0;
> > +     int dwords, bytes, type = 0;
> >       char *ext_buf = NULL;
> >       void *buf = fh->buf;
> >       int err;
> >
> >       fh->type = FILEID_ROOT;
> >       fh->len = 0;
> > +     fh->flags = 0;
> >       if (!inode)
> >               return 0;
> >
> > -     dwords = 0;
> > +     /*
> > +      * !gpf means preallocated variable size fh, but fh_len could
> > +      * be zero in that case if encoding fh len failed.
> > +      */
> >       err = -ENOENT;
> > -     type = exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
> > -     if (!dwords)
> > +     if (!gfp)
> > +             bytes = fh_len;
> > +     else
> > +             bytes = fanotify_encode_fh_len(inode);
>
> Any reason why proper fh len is not passed in by both callers?

No good reason.
It's just how the function evolved and I missed this simplification.

> We could then get rid of this 'if' and 'bytes' variable.

Yap. sounds good.
I will test and push the branches.
Let me know if you want me to re-post anything.

Thanks,
Amir.