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.
