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@xxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> 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@xxxxxxxx>
SUSE Labs, CR
