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

[Jan Kara <jack@suse.cz>]

On Sun 08-01-17 14:10:42, Amir Goldstein wrote:
> > +void fsnotify_put_mark(struct fsnotify_mark *mark)
> > +{
> > +       /* Catch marks that were actually never attached to object */
> > +       if (!mark->connector && atomic_dec_and_test(&mark->refcnt)) {
> > +               fsnotify_final_mark_destroy(mark);
> > +               return;
> > +       }
> > +
> > +       /*
> > +        * We have to be careful so that traversals of obj_list under lock can
> > +        * safely grab mark reference.
> > +        */
> > +       if (atomic_dec_and_lock(&mark->refcnt, &mark->connector->lock)) {
> 
> Style nit: maybe if (!atomic_dec_and_lock(...)) return; ?

OK, done.

> >  /*
> > - * Remove mark from inode / vfsmount list, group list, drop inode reference
> > - * if we got one.
> > + * Mark mark as dead, remove it from group list. Mark still stays in object
> 
> Nit: marking mark as "dead" would imply
> mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE
> which does not happen here. need to be careful with wording...
> 
> You probably meant to write "Mark mark as detached...".

Yeah, good catch. Thanks.

> > @@ -613,23 +637,39 @@ void fsnotify_detach_group_marks(struct fsnotify_group *group)
> >  void fsnotify_destroy_marks(struct fsnotify_mark_connector **connp)
> >  {
> >         struct fsnotify_mark_connector *conn;
> > -       struct fsnotify_mark *mark;
> > +       struct fsnotify_mark *mark, *old_mark = NULL;
> > +       struct inode *inode;
> >
> > -       while ((conn = fsnotify_grab_connector(connp))) {
> > -               /*
> > -                * We have to be careful since we can race with e.g.
> > -                * fsnotify_clear_marks_by_group() and once we drop the list
> > -                * lock, mark can get removed from the obj_list and destroyed.
> > -                * But we are holding mark reference so mark cannot be freed
> > -                * and calling fsnotify_destroy_mark() more than once is fine.
> > -                */
> > -               mark = hlist_entry(conn->list.first, struct fsnotify_mark,
> > -                                  obj_list);
> > +       conn = fsnotify_grab_connector(connp);
> > +       if (!conn)
> > +               return;
> > +       /*
> > +        * We have to be careful since we can race with e.g.
> > +        * fsnotify_clear_marks_by_group() and once we drop the conn->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 conn->lock.
> > +        */
> > +       hlist_for_each_entry(mark, &conn->list, obj_list) {
> >                 fsnotify_get_mark(mark);
> > -               fsnotify_put_connector(conn);
> > +               spin_unlock(&conn->lock);
> > +               if (old_mark)
> > +                       fsnotify_put_mark(old_mark);
> > +               old_mark = mark;
> >                 fsnotify_destroy_mark(mark, mark->group);
> > -               fsnotify_put_mark(mark);
> > +               spin_lock(&conn->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_connector_from_object(conn);
> > +       fsnotify_put_connector(conn);
> 
> In this code, it is extra dissonant that spin_unlock(&conn->lock);
> is obfuscated by fsnotify_put_connector(conn);
> since you (rightfully) used spin_unlock() inside the for loop.
> As I suggested in earlier patch - get rid of fsnotify_put_connector()
> helper is the easy road.

Done.

> > +       if (inode)
> > +               iput(inode);
> > +       if (old_mark)
> > +               fsnotify_put_mark(old_mark);
> 
> It is not clear to me from the comment above if the order of
> iput(inode) and fsnotify_put_mark(old_mark) is meaningful
> or arbitrary, because af far as I understand "until all mark
> references get dropped" does not refer to "old_mark".
> 
> If the order is arbitrary, to me it would have looked better
> to see the snippet missing from within the for loop here:
> 
>                spin_unlock(&conn->lock);
>                if (old_mark)
>                        fsnotify_put_mark(old_mark);
> 
> And iput(inode) as the final chord of this function.
> 
> But if order is not arbitrary, please elaborate in comment.

The order is arbitrary. Frankly, I can see reasons for any ordering - my
original thinking was to have iput() closer to where we get inode pointer.
But whatever. Reordered.

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