On Wed, Jan 4, 2017 at 3:38 PM, Jan Kara <jack@xxxxxxx> wrote: > On Fri 23-12-16 14:34:07, Jan Kara wrote: >> On Fri 23-12-16 07:48:43, Amir Goldstein wrote: >> > On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara <jack@xxxxxxx> 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. > > So how about naming the type fsnotify_mark_connector? We can use 'conn' as > a name for local variables. I think that is not as overloaded as 'list' > and it describes that it is a structure used for connecting marks with > inode / vfsmount. Would that make things more comprehensive for you? > connector sounds good. As long as it is not list->list it works for me ;-) -- 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
