From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:42376 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750730AbcLWNnb (ORCPT ); Fri, 23 Dec 2016 08:43:31 -0500 Date: Fri, 23 Dec 2016 14:42:12 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , linux-fsdevel , Lino Sanfilippo , Miklos Szeredi , Paul Moore Subject: Re: [PATCH 10/22] fsnotify: Detach mark from object list when last reference is dropped Message-ID: <20161223134212.GF22679@quack2.suse.cz> References: <20161222091538.28702-1-jack@suse.cz> <20161222091538.28702-11-jack@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri 23-12-16 12:51:28, Amir Goldstein wrote: > On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara 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 > > --- > ... > > +/* 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 SUSE Labs, CR