From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:41682 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbdAaPmO (ORCPT ); Tue, 31 Jan 2017 10:42:14 -0500 Date: Tue, 31 Jan 2017 16:41:31 +0100 From: Jan Kara To: Miklos Szeredi Cc: Jan Kara , linux-fsdevel@vger.kernel.org, Amir Goldstein , Paul Moore Subject: Re: [PATCH 06/22] fsnotify: Attach marks to object via dedicated head structure Message-ID: <20170131154131.GB15249@quack2.suse.cz> References: <20170120132123.9670-1-jack@suse.cz> <20170120132123.9670-7-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 Wed 25-01-17 10:41:12, Miklos Szeredi wrote: > On Fri, Jan 20, 2017 at 2:21 PM, 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_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 SUSE Labs, CR