On Thu 16-03-17 10:41:09, Amir Goldstein wrote:
> On Thu, Mar 16, 2017 at 10:24 AM, Jan Kara <jack@xxxxxxx> wrote:
> > On Wed 15-03-17 22:08:00, Amir Goldstein wrote:
> >> On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> >> > ---
> >> > 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@xxxxxxxx>
SUSE Labs, CR
