From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:33818 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727428AbeIDORb (ORCPT ); Tue, 4 Sep 2018 10:17:31 -0400 Date: Tue, 4 Sep 2018 11:53:07 +0200 From: Jan Kara To: Paul Moore Cc: jack@suse.cz, linux-audit@redhat.com, linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk, rgb@redhat.com, amir73il@gmail.com Subject: Re: [PATCH 01/10] audit_tree: Remove mark->lock locking Message-ID: <20180904095307.GE9444@quack2.suse.cz> References: <20180710100217.12866-1-jack@suse.cz> <20180710100217.12866-2-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: Hi, sorry for getting to this so late but I was catching up after vacation and your replies got burried in my inbox. On Fri 27-07-18 00:47:04, Paul Moore wrote: > On Tue, Jul 10, 2018 at 6:02 AM Jan Kara wrote: > > Currently, audit_tree code uses mark->lock to protect against detaching > > of mark from an inode. In most places it however also uses > > mark->group->mark_mutex (as we need to atomically replace attached > > marks) and this provides protection against mark detaching as well. So > > just remove protection with mark->lock from audit tree code and replace > > it with mark->group->mark_mutex protection in all the places. It > > simplifies the code and gets rid of some ugly catches like calling > > fsnotify_add_mark_locked() with mark->lock held (which cannot sleep only > > because we hold a reference to another mark attached to the same inode). > > > > Signed-off-by: Jan Kara > > --- > > kernel/audit_tree.c | 24 ++++-------------------- > > 1 file changed, 4 insertions(+), 20 deletions(-) > > ... > > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index 02feef939560..1c82eb6674c4 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -360,12 +355,12 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) > > return -ENOSPC; > > } > > > > - spin_lock(&entry->lock); > > + mutex_lock(&entry->group->mark_mutex); > > I wonder if we could move the lock up above the > fsnotify_add_inode_mark() earlier in create_chunk() and use > fsnotify_add_mark_locked()? Possibly, but I didn't want to do this in this patch as that's a separate change. Also this is what in fact happens in later patches. Honza -- Jan Kara SUSE Labs, CR