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@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
