Re: [PATCH 12/33] fsnotify: Move locking into fsnotify_find_mark()

[Jan Kara <jack@suse.cz>]

On Thu 16-03-17 10:41:09, Amir Goldstein wrote:
> On Thu, Mar 16, 2017 at 10:24 AM, Jan Kara <jack@suse.cz> wrote:
> > On Wed 15-03-17 22:08:00, Amir Goldstein wrote:
> >> On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@suse.cz> wrote:
> >> > Move locking of a mark list into fsnotify_find_mark(). This reduces code
> >> > churn in the following patch changing lock protecting the list.
> >> >
> >> > Signed-off-by: Jan Kara <jack@suse.cz>
> >> > ---
> >> >  fs/notify/inode_mark.c    | 8 +-------
> >> >  fs/notify/mark.c          | 8 ++++++++
> >> >  fs/notify/vfsmount_mark.c | 7 +------
> >> >  3 files changed, 10 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c
> >> > index 9b2f4e6eb8eb..f05fc49b8242 100644
> >> > --- a/fs/notify/inode_mark.c
> >> > +++ b/fs/notify/inode_mark.c
> >> > @@ -71,13 +71,7 @@ void fsnotify_clear_inode_marks_by_group(struct fsnotify_group *group)
> >> >  struct fsnotify_mark *fsnotify_find_inode_mark(struct fsnotify_group *group,
> >> >                                                struct inode *inode)
> >> >  {
> >> > -       struct fsnotify_mark *mark;
> >> > -
> >> > -       spin_lock(&inode->i_lock);
> >> > -       mark = fsnotify_find_mark(inode->i_fsnotify_marks, group);
> >> > -       spin_unlock(&inode->i_lock);
> >> > -
> >> > -       return mark;
> >> > +       return fsnotify_find_mark(inode->i_fsnotify_marks, group);
> >> >  }
> >> >
> >> >  /**
> >> > diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> >> > index e7929539203a..1113dd8b2f82 100644
> >> > --- a/fs/notify/mark.c
> >> > +++ b/fs/notify/mark.c
> >> > @@ -478,16 +478,24 @@ struct fsnotify_mark *fsnotify_find_mark(struct fsnotify_mark_connector *conn,
> >> >                                          struct fsnotify_group *group)
> >> >  {
> >> >         struct fsnotify_mark *mark;
> >> > +       spinlock_t *lock;
> >> >
> >> >         if (!conn)
> >> >                 return NULL;
> >> >
> >> > +       if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE)
> >> > +               lock = &conn->inode->i_lock;
> >> > +       else
> >> > +               lock = &conn->mnt->mnt_root->d_lock;
> >> > +       spin_lock(lock);
> >>
> >> This is becoming a repeating pattern.
> >> Perhaps:
> >>
> >>            lock = fsnotify_object_lock(conn);
> >
> > Yes, but it will all go away in couple of patches so I don't think it's
> > worth the churn in the patch series...
> >
> 
> All I am saying is it would have been better if we had a better
> abstraction of the "object" interface.
> Since we are not going to base a new base struct for inode and vfsmount,
> the next best thing is to have wrapper macros to access the lock
> and the connector instead of open coding the type flag test everywhere
> it is needed.
> 
> But if you are saying that the object lock is going to referred to rarely,
> then I see your point, but then again, it doesn't hurt to have an
> inline wrapper.

The object lock is accessed exactly 0-times after "fsnotify: Lock object
list with connector lock" in the series which is why I find the wrapper
pointless...

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