On Fri 23-12-16 12:51:28, Amir Goldstein wrote:
> On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> > ---
> ...
>
> +/* 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@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
