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);
...
> + 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?
...
> + /*
> + * 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. 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.
Please untangle this knot for me.
> 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.
--
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
