Re: [PATCH v3 09/13] fanotify: cache fsid in fsnotify_mark_connector

[Jan Kara <jack@suse.cz>]

On Sun 25-11-18 15:43:48, Amir Goldstein wrote:
> For FAN_REPORT_FID, we need to encode fid with fsid of the filesystem on
> every event. To avoid having to call vfs_statfs() on every event to get
> fsid, we store the fsid in fsnotify_mark_connector on the first time we
> add a mark and on handle event we use the cached fsid.
> 
> Subsequent calls to add mark on the same object are expected to pass the
> same fsid, so the call will fail on cached fsid mismatch.
> 
> Similarly, if an event is reported on several mark types (inode, mount,
> filesystem), all connectors should already have the same fsid and we
> warn if we detect a mismatch.

Aha, that's what I was missing when I commented on inode & mount marks
working fine with fid. Couldn't we define inode->mount->sb ordering of
marks and just fetch fsid from the most specific one?

> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fanotify/fanotify.c      | 42 ++++++++++------
>  fs/notify/fanotify/fanotify.h      |  5 +-
>  fs/notify/fanotify/fanotify_user.c | 62 ++++++++++++++----------
>  fs/notify/mark.c                   | 77 ++++++++++++++++++++++++++----
>  include/linux/fsnotify_backend.h   | 24 ++++++++--
>  5 files changed, 154 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 844b748f0b74..90bac98dd8c7 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -103,13 +103,13 @@ static int fanotify_get_response(struct fsnotify_group *group,
>   * requested by the user, will not be present in the returned mask.
>   */
>  static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
> -				       u32 event_mask, const void *data,
> -				       int data_type)
> +				     u32 event_mask, const void *data,
> +				     int data_type, __kernel_fsid_t *fsid)
>  {
>  	__u32 marks_mask = 0, marks_ignored_mask = 0;
>  	const struct path *path = data;
>  	struct fsnotify_mark *mark;
> -	int type;
> +	int type, err;
>  
>  	pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
>  		 __func__, iter_info->report_mask, event_mask, data, data_type);
> @@ -136,6 +136,13 @@ static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
>  		     !(mark->mask & FS_EVENT_ON_CHILD)))
>  			continue;
>  
> +		if (fsid) {
> +			err = fsnotify_get_conn_fsid(mark->connector, fsid);
> +			/* Should we skip mark with null or conflicting fsid? */
> +			pr_debug("%s: fsid=%x.%x type=%u err=%d\n", __func__,
> +				 fsid->val[0], fsid->val[1], type, err);
> +		}
> +
>  		marks_mask |= mark->mask;
>  		marks_ignored_mask |= mark->ignored_mask;
>  	}

I would just move getting of fsid into a separate function. I does not seem
to fit well into fanotify_group_event_mask() except for the fact that we
iterate through marks there... Also is the pr_debug() long-term useful
there?

> @@ -480,8 +481,54 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
>  	return -1;
>  }
>  
> +/*
> + * Check consistent fsid for all marks on the same object.
> + * It is expected to always get the same fsid (or no fsid) for all
> + * marks added to object.
> + */
> +static int fsnotify_test_conn_fsid(const struct fsnotify_mark_connector *conn,
> +				   const __kernel_fsid_t *fsid,
> +				   const char *where)
> +

Call this fsnotify_conn_fsid_consistent()?

> +{
> +	/*
> +	 * Backend is expected to check for non uniform fsid (e.g. btrfs),
> +	 * but maybe we missed something?
> +	 */
> +	if (fsid->val[0] != conn->fsid.val[0] ||
> +	    fsid->val[1] != conn->fsid.val[1]) {
> +		pr_warn_ratelimited("%s: fsid mismatch on object of type %u: %x.%x != %x.%x\n",
> +				    where, conn->type,
> +				    fsid->val[0], fsid->val[1],
> +				    conn->fsid.val[0], conn->fsid.val[1]);
> +		return -EXDEV;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Get cached fsid of filesystem containing object from connector.
> + */
> +int fsnotify_get_conn_fsid(const struct fsnotify_mark_connector *conn,
> +			   __kernel_fsid_t *fsid)
> +{
> +	if (WARN_ON(!conn->fsid.val[0] && !conn->fsid.val[1]))

WARN_ON_ONCE so that we don't spam logs please.

> +		return -ENODEV;
> +
> +	if (!fsid->val[0] && !fsid->val[1]) {
> +		*fsid = conn->fsid;
> +		return 0;
> +	}
> +
> +	return fsnotify_test_conn_fsid(conn, fsid, __func__);
> +}

The validation in fsnotify_get_conn_fsid() is weird. This function should
just return the fsid whatever it is. Caller can check if he wants. I know
this just does the right thing for the place where you use
fsnotify_get_conn_fsid() but it is just a surprising behavior.

> +
> +static const __kernel_fsid_t null_fsid;
> +
>  static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
> -					       unsigned int type)
> +					       unsigned int type,
> +					       __kernel_fsid_t *fsid)
>  {
>  	struct inode *inode = NULL;
>  	struct fsnotify_mark_connector *conn;
> @@ -493,6 +540,8 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
>  	INIT_HLIST_HEAD(&conn->list);
>  	conn->type = type;
>  	conn->obj = connp;
> +	/* Cache fsid of filesystem containing the object */
> +	conn->fsid = fsid ? *fsid : null_fsid;

This is unusual. I'd just do:

	if (fsid)
		conn->fsid = *fsid;
	else
		conn->fsid->val[0] = conn->fsid->val[1] = 0;

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