Re: [PATCH v3 3/3] fanotify: add API to attach/detach super block mark

[Jan Kara <jack@suse.cz>]

On Thu 30-08-18 18:15:51, Amir Goldstein wrote:
> Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush
> of super block mark type.
> 
> A super block watch gets all events on the filesystem, regardless of
> the mount from which the mark was added, unless an ignore mask exists
> on either the inode or the mount where the event was generated.
> 
> Only one of FAN_MARK_MOUNT and FAN_MARK_FILESYSTEM mark type flags
> may be provided to fanotify_mark() or no mark type flag for inode mark.
> 
> Cc: <linux-api@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Just one nit below, otherwise the patch look good to me.

> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index 74247917de04..7345b9a57f66 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -43,6 +43,7 @@
>  				 FAN_UNLIMITED_MARKS)
>  
>  /* flags used for fanotify_modify_mark() */
> +#define FAN_MARK_INODE		0x00000000
>  #define FAN_MARK_ADD		0x00000001
>  #define FAN_MARK_REMOVE		0x00000002
>  #define FAN_MARK_DONT_FOLLOW	0x00000004
> @@ -51,6 +52,7 @@
>  #define FAN_MARK_IGNORED_MASK	0x00000020
>  #define FAN_MARK_IGNORED_SURV_MODIFY	0x00000040
>  #define FAN_MARK_FLUSH		0x00000080
> +#define FAN_MARK_FILESYSTEM	0x00000100
>  
>  #define FAN_ALL_MARK_FLAGS	(FAN_MARK_ADD |\
>  				 FAN_MARK_REMOVE |\
> @@ -59,7 +61,10 @@
>  				 FAN_MARK_MOUNT |\
>  				 FAN_MARK_IGNORED_MASK |\
>  				 FAN_MARK_IGNORED_SURV_MODIFY |\
> -				 FAN_MARK_FLUSH)
> +				 FAN_MARK_FLUSH|\
> +				 FAN_MARK_FILESYSTEM)
> +
> +#define FAN_ALL_MARK_TYPE_FLAGS (FAN_MARK_MOUNT | FAN_MARK_FILESYSTEM)
>  
>  /*
>   * All of the events - we build the list by hand so that we can add flags in

So for completeness I'd add FAN_MARK_INODE to FAN_ALL_MARK_FLAGS and
FAN_ALL_MARK_TYPE_FLAGS. I know it doesn't change the actual value but
logically it belongs there when you defined it...

Also one more thing to consider: Different mark types cannot be combined.
So it could save some bits in 'flags' in future if we had something like
FAN_MARK_TYPE_MASK and (flags & FAN_MARK_TYPE_MASK) would enumerate
different mark types - 0 for inode mark, FAN_MARK_MOUNT for mount mark,
FAN_MARK_FILESYSTEM for superblock mark etc. Again, currently there's no
practical difference in the values, just the names and tests for validity
would be slightly different.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR