On Thu, Mar 16, 2017 at 9:58 AM, Jan Kara <jack@xxxxxxx> wrote:
> On Wed 15-03-17 21:19:13, Amir Goldstein wrote:
>> On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@xxxxxxx> wrote:
>> > +static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
>> > + struct inode *inode, struct vfsmount *mnt,
>> > + int allow_dups)
>> > {
>> > struct fsnotify_mark *lmark, *last = NULL;
>> > struct fsnotify_mark_connector *conn;
>> > + struct fsnotify_mark_connector **connp;
>> > + spinlock_t *lock;
>> > int cmp;
>> > + int err = 0;
>> > +
>> > + if (WARN_ON(!inode && !mnt))
>> > + return -EINVAL;
>> > + if (inode) {
>> > + connp = &inode->i_fsnotify_marks;
>> > + lock = &inode->i_lock;
>> > + } else {
>> > + connp = &real_mount(mnt)->mnt_fsnotify_marks;
>> > + lock = &mnt->mnt_root->d_lock;
>> > + }
>> >
>> > if (!*connp) {
>> > conn = kmem_cache_alloc(fsnotify_mark_connector_cachep,
>> > - GFP_ATOMIC);
>> > + GFP_KERNEL);
>> > if (!conn)
>> > return -ENOMEM;
>> > INIT_HLIST_HEAD(&conn->list);
>> > @@ -350,8 +363,17 @@ int fsnotify_add_mark_list(struct fsnotify_mark_connector **connp,
>> > * lockless_dereference() in fsnotify().
>> > */
>> > smp_wmb();
>> > - *connp = conn;
>> > + spin_lock(&mark->lock);
>> > + spin_lock(lock);
>> > + if (!*connp) {
>> > + *connp = conn;
>> > + } else {
>> > + kmem_cache_free(fsnotify_mark_connector_cachep, conn);
>> > + conn = *connp;
>> > + }
>> > } else {
>> > + spin_lock(&mark->lock);
>> > + spin_lock(lock);
>> > conn = *connp;
>> > }
>> >
>>
>> This chunk has become overly convoluted IMO.
>> How about something like this:
>>
>> conn = fsnotify_connector_get(connp, lock);
>>
>> spin_lock(&mark->lock);
>> spin_lock(lock);
>>
>> Where fsnotify_connector_get() acquires and releases only the object lock
>> and only for the case of allocating a new connector.
>
> Agreed and changed.
>
Looks very neat now.
You may add
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
(based on commit 6e23640 on your testing branch)
I believe that addresses the last of my review comments.
Let me know if I missed any Rvb tag.
Amir.
