From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C0167C48BE5 for ; Sat, 12 Jun 2021 01:46:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 936CE611CD for ; Sat, 12 Jun 2021 01:46:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230297AbhFLBsE (ORCPT ); Fri, 11 Jun 2021 21:48:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33796 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229622AbhFLBsD (ORCPT ); Fri, 11 Jun 2021 21:48:03 -0400 Received: from zeniv-ca.linux.org.uk (zeniv-ca.linux.org.uk [IPv6:2607:5300:60:148a::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9DFBAC061574; Fri, 11 Jun 2021 18:46:04 -0700 (PDT) Received: from viro by zeniv-ca.linux.org.uk with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1lrsic-007A4F-9m; Sat, 12 Jun 2021 01:45:50 +0000 Date: Sat, 12 Jun 2021 01:45:50 +0000 From: Al Viro To: Ian Kent Cc: Greg Kroah-Hartman , Tejun Heo , Eric Sandeen , Fox Chen , Brice Goglin , Rick Lindsley , David Howells , Miklos Szeredi , Marcelo Tosatti , "Eric W. Biederman" , Carlos Maiolino , linux-fsdevel , Kernel Mailing List Subject: Re: [PATCH v6 5/7] kernfs: use i_lock to protect concurrent inode updates Message-ID: References: <162322846765.361452.17051755721944717990.stgit@web.messagingengine.com> <162322868275.361452.17585267026652222121.stgit@web.messagingengine.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <162322868275.361452.17585267026652222121.stgit@web.messagingengine.com> Sender: Al Viro Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 09, 2021 at 04:51:22PM +0800, Ian Kent wrote: > The inode operations .permission() and .getattr() use the kernfs node > write lock but all that's needed is to keep the rb tree stable while > updating the inode attributes as well as protecting the update itself > against concurrent changes. Huh? Where does it access the rbtree at all? Confused... > diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c > index 3b01e9e61f14e..6728ecd81eb37 100644 > --- a/fs/kernfs/inode.c > +++ b/fs/kernfs/inode.c > @@ -172,6 +172,7 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode) > { > struct kernfs_iattrs *attrs = kn->iattr; > > + spin_lock(&inode->i_lock); > inode->i_mode = kn->mode; > if (attrs) > /* > @@ -182,6 +183,7 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode) > > if (kernfs_type(kn) == KERNFS_DIR) > set_nlink(inode, kn->dir.subdirs + 2); > + spin_unlock(&inode->i_lock); > } Even more so - just what are you serializing here? That code synchronizes inode metadata with those in kernfs_node. Suppose you've got two threads doing ->permission(); the first one gets through kernfs_refresh_inode() and goes into generic_permission(). No locks are held, so kernfs_refresh_inode() from another thread can run in parallel with generic_permission(). If that's not a problem, why two kernfs_refresh_inode() done in parallel would be a problem? Thread 1: permission done refresh, all locks released now Thread 2: change metadata in kernfs_node Thread 2: permission goes into refresh, copying metadata into inode Thread 1: generic_permission() No locks in common between the last two operations, so we generic_permission() might see partially updated metadata. Either we don't give a fuck (in which case I don't understand what purpose does that ->i_lock serve) *or* we need the exclusion to cover a wider area.