On Wed 15-07-20 19:05:52, Amir Goldstein wrote:
> On Wed, Jul 15, 2020 at 6:34 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx>
> >
> > 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.
So I've just picked up patches 1-9 (I took patches 8 and 9 from your git)
and 17 to my fsnotify branch because they are completely stand-alone
cleanups and I didn't see a reason to delay them further. All the other
patches in this series look fine to me but I didn't pick them up yet
because they are more tightly related to the name event series and could
possibly change. So I'll pick them up once I feel name event series is more
stable...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
