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

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Nov 29, 2018 at 1:12 PM Jan Kara <jack@suse.cz> wrote:
>
> 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?
>

You probably already realized that we are racing on two threads ;-)
so see my reply to this in question on thread of patch 8.
Short: I don't want to change fsid reported on same object via same
path depending on event type and marks existing at that time.


> > 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?
>

Don't mind to get rid of it.
How about fsnotify_get_event_fsid(iter_info) will just get the
first fsid it finds from any connector or via prioirty and not enforce conflicts
here at all?
Conflicts are supposed to be enforces when adding the mark anyway.

> > @@ -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()?
>

As written above, I am considering dropping the call to this function from
get_fsid path and use it only from set_fsid, so I might as well just open code
this test in set_fsid.

> > +{
> > +     /*
> > +      * 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.
>

OK.

> > +             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.
>

OK.

> > +
> > +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;
>

Sure.

Thanks,
Amir.