On Wed 25-01-17 10:41:12, Miklos Szeredi wrote:

On Fri, Jan 20, 2017 at 2:21 PM, Jan Kara [off-list ref] wrote:

quoted

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_connector) 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.

I wonder if this big patch could be conveniently split up into
structural and functional parts.

I.e. first add the connector structure but make its lifetime match
that of the inode.  That would mean just adding the extra
dereferences, but not the tricky rcu-conformant creation and
destruction of the connecor.

Yeah, probably I could split it into three parts. The addition of
dynamically added (but freed only on object free) connector structure,
the change of locking from i_lock/d_lock into connector->lock, the freeing
of connector when the last mark on the list gets freed (RCU bits go there).

Also there seems to be a fair amount of moving code around, which
could warrant a separate patch for clarity.

I don't think there's substantial moving of code. However the locking
change above results in some functions becoming redundant so I just deleted
them. After the split it should be easier to understand.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR