From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:41574 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728300AbeK2C3e (ORCPT ); Wed, 28 Nov 2018 21:29:34 -0500 Date: Wed, 28 Nov 2018 16:27:30 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org Subject: Re: [PATCH v3 04/13] fanotify: define the structures to report a unique file identifier Message-ID: <20181128152730.GI15604@quack2.suse.cz> References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-5-amir73il@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181125134352.21499-5-amir73il@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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? > @@ -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)? Honza -- Jan Kara SUSE Labs, CR