From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:60318 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726949AbeK2WRx (ORCPT ); Thu, 29 Nov 2018 17:17:53 -0500 Date: Thu, 29 Nov 2018 11:48:35 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v3 09/13] fanotify: cache fsid in fsnotify_mark_connector Message-ID: <20181129104835.GJ31087@quack2.suse.cz> References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-10-amir73il@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181125134352.21499-10-amir73il@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > Signed-off-by: Amir Goldstein > --- > 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 SUSE Labs, CR