Re: [PATCH 10/22] fsnotify: Detach mark from object list when last reference is dropped

[Jan Kara <jack@suse.cz>]

On Fri 23-12-16 12:51:28, Amir Goldstein wrote:
> On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara <jack@suse.cz> wrote:
> > Instead of removing mark from object list from fsnotify_detach_mark(),
> > remove the mark when last reference to the mark is dropped. This will
> > allow fanotify to wait for userspace response to event without having to
> > hold onto fsnotify_mark_srcu.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> ...
> 
> +/* Called with mark->obj_list_head->lock held, releases it */
> +static void fsnotify_detach_from_object(struct fsnotify_mark *mark)
>  {
> 
> IMO, the implicit release in this function makes the code using it hard
> to read and maintain. Please consider splitting it into 2 functions
> to be called from code that explicitly unlocks, e.g.:
> 
>      free_list = fsnotify_detach_from_object_locked(mark, &inode);
>      spin_unlock(&list->lock);
>      if (inode)
>         iput(inode);
>      if (free_list)
>         fsnotify_free_list(list);

Maybe I'll instead move atomic_dec_and_lock() into
fsnotify_detach_from_object(). I'll just have to find good name for that
function.

> ...
> > +               inode = fsnotify_detach_list_from_object(list);
> >                 free_list = true;
> >         } else
> >                 __fsnotify_recalc_mask(list);
> >         mark->obj_list_head = NULL;
> >         spin_unlock(&list->lock);
> >
> > +       if (inode)
> > +               iput(inode);
> > +
> 
> Question: if list is holding inode anyway, what's the use of
> FSNOTIFY_MARK_FLAG_OBJECT_PINNED?
> or maybe you are removing it later on in the series?

It is removed (maybe later, but certainly I remember dropping it).

> ...
> > +       /*
> > +        * We have to be careful since we can race with e.g.
> > +        * fsnotify_clear_marks_by_group() and once we drop the list->lock, the
> > +        * list can get modified. However we are holding mark reference and
> > +        * thus our mark cannot be removed from obj_list so we can continue
> > +        * iteration after regaining list->lock.
> > +        */
> > +       hlist_for_each_entry(mark, &list->list, obj_list) {
> >                 fsnotify_get_mark(mark);
> > -               fsnotify_put_list(list);
> > +               spin_unlock(&list->lock);
> > +               if (old_mark)
> > +                       fsnotify_put_mark(old_mark);
> > +               old_mark = mark;
> >                 fsnotify_destroy_mark(mark, mark->group);
> > -               fsnotify_put_mark(mark);
> > +               spin_lock(&list->lock);
> >         }
> > +       /*
> > +        * Detach list from object now so that we don't pin inode until all
> > +        * mark references get dropped. It would lead to strange results such
> > +        * as delaying inode deletion or blocking unmount.
> > +        */
> > +       inode = fsnotify_detach_list_from_object(list);
> > +       fsnotify_put_list(list);
> > +       if (inode)
> > +               iput(inode);
> > +       if (old_mark)
> > +               fsnotify_put_mark(old_mark);
> 
> I must be missing something subtle here. I don't see where the list->lock
> is unlocked.

fsnotify_put_list() - fsnotify_grab_list() will get list->lock,
fsnotify_put_list() drops it.

> Also, I am not sure if you placed put old_mark after
> fsnotify_put_list
> for a reason. If you did, I did not find that reason in the comments. If you
> didn't I think it would be more appropriate after the list iteration
> ends, although
> it appear that put old_mark should be called after list->lock unlock.

Exactly. You have to drop mark reference after unlocking list for obvious
reasons.

> > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > index 6086fc7ff6df..76b3c34172c7 100644
> > --- a/include/linux/fsnotify_backend.h
> > +++ b/include/linux/fsnotify_backend.h
> > @@ -244,9 +244,9 @@ struct fsnotify_mark {
> >         struct list_head g_list;
> >         /* Protects inode / mnt pointers, flags, masks */
> >         spinlock_t lock;
> > -       /* List of marks for inode / vfsmount [obj_list_head->lock] */
> > +       /* List of marks for inode / vfsmount [obj_list_head->lock, mark ref] */
> >         struct hlist_node obj_list;
> > -       /* Head of list of marks for an object [mark->lock, group->mark_mutex] */
> > +       /* Head of list of marks for an object [mark ref] */
> >         struct fsnotify_mark_list *obj_list_head;
> 
> What is the meaning of [mark ref] here?
> If the mark is on the obj_list its refcount is already elevated.
> I thought it's the mark that is holding a ref on the list_head (or tap
> if you accept my suggestion)
> and not the other way around.

So [mark ref] here means that if you hold mark reference, obj_list_head
cannot change and mark is pinned in the object list. Probably I can put
more detailed explanation above the structure declaration.

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