From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:38751 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030753AbdADNiU (ORCPT ); Wed, 4 Jan 2017 08:38:20 -0500 Date: Wed, 4 Jan 2017 14:38:17 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , linux-fsdevel , Lino Sanfilippo , Miklos Szeredi , Paul Moore Subject: Re: [PATCH 08/22] fsnotify: Attach marks to object via dedicated head structure Message-ID: <20170104133817.GT3780@quack2.suse.cz> References: <20161222091538.28702-1-jack@suse.cz> <20161222091538.28702-9-jack@suse.cz> <20161223133407.GE22679@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161223133407.GE22679@quack2.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 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? Honza -- Jan Kara SUSE Labs, CR