From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb1-f194.google.com ([209.85.219.194]:36556 "EHLO mail-yb1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727979AbeK2D0g (ORCPT ); Wed, 28 Nov 2018 22:26:36 -0500 MIME-Version: 1.0 References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-5-amir73il@gmail.com> <20181128152730.GI15604@quack2.suse.cz> In-Reply-To: <20181128152730.GI15604@quack2.suse.cz> From: Amir Goldstein Date: Wed, 28 Nov 2018 18:24:11 +0200 Message-ID: Subject: Re: [PATCH v3 04/13] fanotify: define the structures to report a unique file identifier To: Jan Kara Cc: Matthew Bobrowski , linux-fsdevel , linux-api@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Nov 28, 2018 at 5:27 PM Jan Kara wrote: > > On Sun 25-11-18 15:43:43, Amir Goldstein wrote: > > When user requests the flag FAN_REPORT_FID in fanotify_init(), > > a unique file indetifier of the event target object will be reported > > with the event. > > > > This commit only defines the internal and user visible structures used > > to store and report the unique file identifier. > > > > The file identifier includes the filesystem's fsid (i.e. from statfs(2)) > > and an NFS file handle of the file (i.e. from name_to_handle_at(2)). > > > > The file identifier makes holding the path reference and passing a file > > descriptor to user redundant, so those are disabled in a group with > > FAN_REPORT_FID. > > > > Cc: > > Signed-off-by: Amir Goldstein > > On a general note I'd fold patches 4-6 into one patch as separating them > looks weird to me and does not really simplify the review (I had to jump > among these three patches frequently to understand what's going on). > > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > > index fb84dd3289f8..2e4fca30afda 100644 > > --- a/fs/notify/fanotify/fanotify.h > > +++ b/fs/notify/fanotify/fanotify.h > > @@ -7,6 +7,14 @@ extern struct kmem_cache *fanotify_mark_cache; > > extern struct kmem_cache *fanotify_event_cachep; > > extern struct kmem_cache *fanotify_perm_event_cachep; > > > > +/* The size of the variable length buffer storing fsid and file handle */ > > +#define FANOTIFY_FID_LEN(handle_bytes) \ > > + (sizeof(struct fanotify_event_fid) + (handle_bytes)) > > + > > +struct fanotify_info { > > + struct fanotify_event_fid *fid; > > +}; > > + > > Hum, lot of file handles actually fit in 24 bytes and separate allocation > for such small things is really an overkill. And the rate at which we > produce events can be relatively large. So I'd prefer if could embed such > small handles inside the struct fanotify_event. Probably as a separate > optimization patch. > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index 2dbb2662a92f..93e1aa2a389f 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -133,9 +133,10 @@ static int fill_event_metadata(struct fsnotify_group *group, > > metadata->reserved = 0; > > metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS; > > metadata->pid = pid_vnr(event->pid); > > - if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID) || > > + unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) { > > metadata->fd = FAN_NOFD; > > - else { > > + } else { > > Use FANOTIFY_HAS_FID() helper here? Not here. FAN_REPORT_FID implies that event->fd will never be used. But I need to check FANOTIFY_HAS_FID() before copy_fid_to_user(), because we could fail to decode fid and still report the event without fid. > > > @@ -106,6 +107,24 @@ struct fanotify_event_metadata { > > __s32 pid; > > }; > > > > +#define FAN_EVENT_INFO_TYPE_FID 1 > > + > > +/* Variable length info record header following event metadata */ > > +struct fanotify_event_info { > > + __u8 info_type; > > + __u8 reserved; > > + __u16 info_len; > > + unsigned char info[0]; > > +}; > > + > > +/* Unique file identifier info record */ > > +struct fanotify_event_fid { > > + __kernel_fsid_t fsid; > > + __u32 handle_bytes; > > + __s32 handle_type; > > + unsigned char f_handle[0]; > > +}; > > + > > Hum, I find another generic embedded fanotify_event_info an > overengineering. fanotify_event_metadata with embedded versioning and > length was supposed to be extensible enough without further generic headers > being embedded... I know you had ideas for further extension of reported > information so probably that is the reason but at least from my POV why not > just bump the 'vers' field to 4 for groups with FAN_REPORT_FID and create > struct fanotify_event_metadata_v4 which would have all necessary fields for > passing the filehandle (and probably it does not have to have 'fd' field)? > Two reasons mainly: 1. Considering further extensions, this design looks like a better fit to me. 2. Related to #1, I don't like the way uapi gets bloated with all definition of past format versions, so IMO format bump should be last resort I'm perfectly fine with getting rid of fanotify_event_info record header. I agree that it is overengineering. How about: struct fanotify_event_info_fid { struct fanotify_event_metadata hdr; struct fanotify_event_fid fid; }; Then fanotify_example program from man fanotify(7) is legit even when adding FAN_REPORT_FID to fanotify_init(). Programs that want to access fid can cast to (struct fanotify_event_info_fid *) and access fid info. This will make the process of adapting existing programs to FAN_REPORT_FID smoother IMO. And the only cost we need to pay is carry event->fd = FAN_NOFD in wasted event space. Thoughts? Amir.