Re: [PATCH 08/22] fsnotify: Attach marks to object via dedicated head structure

[Jan Kara <jack@suse.cz>]

On Fri 23-12-16 07:48:43, Amir Goldstein wrote:
> On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara <jack@suse.cz> wrote:
> > Currently notification marks are attached to object (inode or vfsmnt) by
> > a hlist_head in the object. The list is also protected by a spinlock in
> > the object. So while there is any mark attached to the list of marks,
> > the object must be pinned in memory (and thus e.g. last iput() deleting
> > inode cannot happen). Also for list iteration in fsnotify() to work, we
> > must hold fsnotify_mark_srcu lock so that mark itself and
> > mark->obj_list.next cannot get freed. Thus we are required to wait for
> > response to fanotify events from userspace process with
> > fsnotify_mark_srcu lock held. That causes issues when userspace process
> > is buggy and does not reply to some event - basically the whole
> > notification subsystem gets eventually stuck.
> >
> > So to be able to drop fsnotify_mark_srcu lock while waiting for
> > response, we have to pin the mark in memory and make sure it stays in
> > the object list (as removing the mark waiting for response could lead to
> > lost notification events for groups later in the list). However we don't
> > want inode reclaim to block on such mark as that would lead to system
> > just locking up elsewhere.
> >
> > This commit tries to pave a way towards solving these conflicting
> > lifetime needs. Instead of anchoring the list of marks directly in the
> > object, we anchor it in a dedicated structure (fsnotify_mark_list) and
> > just point to that structure from the object. Also the list is protected
> > by a spinlock contained in that structure. With this, we can detach
> > notification marks from object without having to modify the list itself.
> >
> 
> The structural change looks very good to.
> It makes the code much easier to manage IMO.
> 
> I am only half way though this big change, but I wanted to make one meta
> comment.
> 
> I have a problem with the choice of naming for the new struct.
> 'list' is really an overloaded term and the use of 'list' as a name of
> a class that
> contains a list head makes for some really confusing constructs like
> list->list and mark->obj_list_head which is not a list_head struct.

OK, I'll think about better naming. I agree it may be slightly confusing.

> For future generations, I suggest that we invest the effort in choosing
> a name that makes more sense. I do realize how annoying it would be to
> fix the entire series now, so it's not a problem if renaming is done in the end
> of the series as long as we agree on the end result.
> 
> May I suggest the name fsnotify_tap to describe the new struct.
> I know it is arbitrary, but not more arbitrary then fsnotify_mark and certainly
> not any more arbitrary then fsnotify_group.
> 
> Here are some examples of constructs that will make more sense:
> 
> <+#define FSNOTIFY_LIST_TYPE_INODE       0x01
> <+#define FSNOTIFY_LIST_TYPE_VFSMOUNT    0x02
> >+#define FSNOTIFY_TAP_TYPE_INODE       0x01
> >+#define FSNOTIFY_TAP_TYPE_VFSMOUNT    0x02
> 
> LIST_TYPE_INODE implies this is a list of inodes
> TAP_TYPE_INODE implies this is a tap on an inode

Frankly, I don't like 'TAP' much because as you say it is rather arbitrary
(or maybe I'm just missing a point as a non-native speaker). I'd prefer
something more descriptive.

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