On Thu, Mar 16, 2017 at 10:54 AM, Jan Kara <jack@xxxxxxx> wrote:
> 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...
>
Right... sorry for the noise.
You may add
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
