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

[Jan Kara <jack@suse.cz>]

On Thu 29-11-18 13:42:00, Amir Goldstein wrote:
> On Thu, Nov 29, 2018 at 1:12 PM Jan Kara <jack@suse.cz> wrote:
> > > 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.

Sounds good to me.

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

OK.

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