Re: [PATCH v3 04/13] fanotify: define the structures to report a unique file identifier

[Jan Kara <jack@suse.cz>]

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: <linux-api@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

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 <jack@suse.com>
SUSE Labs, CR