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=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,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 9049FC4708F for ; Tue, 1 Jun 2021 13:19:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6F03A613B1 for ; Tue, 1 Jun 2021 13:19:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233922AbhFANUx (ORCPT ); Tue, 1 Jun 2021 09:20:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38082 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233064AbhFANUv (ORCPT ); Tue, 1 Jun 2021 09:20:51 -0400 Received: from mail-vs1-xe2e.google.com (mail-vs1-xe2e.google.com [IPv6:2607:f8b0:4864:20::e2e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9B9D2C06174A for ; Tue, 1 Jun 2021 06:19:08 -0700 (PDT) Received: by mail-vs1-xe2e.google.com with SMTP id e2so5512212vsr.7 for ; Tue, 01 Jun 2021 06:19:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vG9XhD9yPILV9Qp7pU9v5y+PXRxebG5XrL79GQd10ds=; b=neTIcs2KViMm4PAkTdmxpRRJPGBIVB2P4CHkfj09jaZmc1cNfB/2QmyFtmvoW7bYQy GtvP361YVlji7lbMB2azJaAw2gzsb/VFEPoJEkBlkfCk4SK0uTG+rMKP64goXu5R4rmz tzKEkH0HmGAHRz7TThyTiMeMYADAWjvD2GDi8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vG9XhD9yPILV9Qp7pU9v5y+PXRxebG5XrL79GQd10ds=; b=IZFoTMafpGPhhA3NcU3S641Kztv9fgcMItSN9dtw1mVHzPY2eDcrmiMwuKL+OOTmHs 99RWT1Ue543sQ+b79EA57VcrRTLFn76ImQFU8XlRz4rXaV+tJ0yxAxK7EqffDRva2F04 wwE0iA4+LDRb7aW3w5WvmzcJpG1gY+efI2pnrgaXK8yBFclw1+vyNHa+ilZsve173Z7g iCKk/1iC4ygWw8e7RipfH8mqVMPP1YWmhtQ2cdOVs4J/vO81ytCvJkcp548LYrcptj7S WylWaXVAB2saKfIxQV2H7/AYzFZckcRz9zqQDhNhdcHlkVGtR0FLp06Z5d0Ood9V+49Q JGHw== X-Gm-Message-State: AOAM532gB0JiGSnuoDbAN27+zXbbBfX6WnkJx6WBHw5hjtxNWrG/YKzT Jqu6wbKndu2MUWvz9HKex1jyFB78O8rfkxrN/AA1Bg== X-Google-Smtp-Source: ABdhPJxjmup0Pyq9xuCSMQbidyEK5x0ytyC4tnXrH70thXtlATobCKr/Au/GM/YPozoWlqxFsY73VBD2sR2OJ0RLFLA= X-Received: by 2002:a05:6102:b06:: with SMTP id b6mr18171215vst.21.1622553545662; Tue, 01 Jun 2021 06:19:05 -0700 (PDT) MIME-Version: 1.0 References: <162218354775.34379.5629941272050849549.stgit@web.messagingengine.com> <162218366632.34379.11311748209082333016.stgit@web.messagingengine.com> In-Reply-To: <162218366632.34379.11311748209082333016.stgit@web.messagingengine.com> From: Miklos Szeredi Date: Tue, 1 Jun 2021 15:18:54 +0200 Message-ID: Subject: Re: [REPOST PATCH v4 4/5] kernfs: use i_lock to protect concurrent inode updates To: Ian Kent Cc: Greg Kroah-Hartman , Tejun Heo , Eric Sandeen , Fox Chen , Brice Goglin , Al Viro , Rick Lindsley , David Howells , Marcelo Tosatti , linux-fsdevel , Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 28 May 2021 at 08:34, 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. > > And .permission() is called frequently during path walks and can cause > quite a bit of contention between kernfs node operations and path > walks when the number of concurrent walks is high. > > To change kernfs_iop_getattr() and kernfs_iop_permission() to take > the rw sem read lock instead of the write lock an additional lock is > needed to protect against multiple processes concurrently updating > the inode attributes and link count in kernfs_refresh_inode(). > > The inode i_lock seems like the sensible thing to use to protect these > inode attribute updates so use it in kernfs_refresh_inode(). > > Signed-off-by: Ian Kent > --- > fs/kernfs/inode.c | 10 ++++++---- > fs/kernfs/mount.c | 4 ++-- > 2 files changed, 8 insertions(+), 6 deletions(-) > > 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); > } > > int kernfs_iop_getattr(struct user_namespace *mnt_userns, > @@ -191,9 +193,9 @@ int kernfs_iop_getattr(struct user_namespace *mnt_userns, > struct inode *inode = d_inode(path->dentry); > struct kernfs_node *kn = inode->i_private; > > - down_write(&kernfs_rwsem); > + down_read(&kernfs_rwsem); > kernfs_refresh_inode(kn, inode); > - up_write(&kernfs_rwsem); > + up_read(&kernfs_rwsem); > > generic_fillattr(&init_user_ns, inode, stat); > return 0; > @@ -284,9 +286,9 @@ int kernfs_iop_permission(struct user_namespace *mnt_userns, > > kn = inode->i_private; > > - down_write(&kernfs_rwsem); > + down_read(&kernfs_rwsem); > kernfs_refresh_inode(kn, inode); > - up_write(&kernfs_rwsem); > + up_read(&kernfs_rwsem); > > return generic_permission(&init_user_ns, inode, mask); > } > diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c > index baa4155ba2edf..f2f909d09f522 100644 > --- a/fs/kernfs/mount.c > +++ b/fs/kernfs/mount.c > @@ -255,9 +255,9 @@ static int kernfs_fill_super(struct super_block *sb, struct kernfs_fs_context *k > sb->s_shrink.seeks = 0; > > /* get root inode, initialize and unlock it */ > - down_write(&kernfs_rwsem); > + down_read(&kernfs_rwsem); > inode = kernfs_get_inode(sb, info->root->kn); > - up_write(&kernfs_rwsem); > + up_read(&kernfs_rwsem); > if (!inode) { > pr_debug("kernfs: could not get root inode\n"); > return -ENOMEM; > This last hunk is not mentioned in the patch header. Why is this needed? Otherwise looks good. Thanks, Miklos