Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     "Al Viro" <viro@zeniv.linux.org.uk>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org,
        "LKML" <linux-kernel@vger.kernel.org>,
        "Daire Byrne" <daire@dneg.com>,
        "Andreas Dilger" <adilger.kernel@dilger.ca>
Subject: Re: [PATCH/RFC] VFS: support parallel updates in the one directory.
In-reply-to: <20220222190751.GA7766@fieldses.org>
References: <164549669043.5153.2021348013072574365@noble.neil.brown.name>,
 <20220222190751.GA7766@fieldses.org>
Date:   Thu, 24 Feb 2022 16:08:36 +1100
Message-id: <164567931673.25116.15009501732764258663@noble.neil.brown.name>
Precedence: bulk

On Wed, 23 Feb 2022, J. Bruce Fields wrote:
> For what it's worth, I applied this to recent upstream (038101e6b2cd)
> and fed it through my usual scripts--tests all passed, but I did see
> this lockdep warning.
>=20
> I'm not actually sure what was running at the time--probably just cthon.
>=20
> --b.
>=20
> [  142.679891] =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D
> [  142.680883] WARNING: possible circular locking dependency detected
> [  142.681999] 5.17.0-rc5-00005-g64e79f877311 #1778 Not tainted
> [  142.682970] ------------------------------------------------------
> [  142.684059] test1/4557 is trying to acquire lock:
> [  142.684881] ffff888023d85398 (DENTRY_PAR_UPDATE){+.+.}-{0:0}, at: d_lock=
_update_nested+0x5/0x6a0
> [  142.686421]=20
>                but task is already holding lock:
> [  142.687171] ffff88801f618bd0 (&type->i_mutex_dir_key#6){++++}-{3:3}, at:=
 path_openat+0x7cb/0x24a0
> [  142.689098]=20
>                which lock already depends on the new lock.
>=20
> [  142.690045]=20
>                the existing dependency chain (in reverse order) is:
> [  142.691171]=20
>                -> #1 (&type->i_mutex_dir_key#6){++++}-{3:3}:
> [  142.692285]        down_write+0x82/0x130
> [  142.692844]        vfs_rmdir+0xbd/0x560
> [  142.693351]        do_rmdir+0x33d/0x400

Thanks.  I hadn't tested rmdir :-)

"rmdir" and "open(O_CREATE)" take these locks in the opposite order.

I think the simplest fix might be to change the inode_lock(_shared) taken
on the dir in open_last_Lookups() to use I_MUTEX_PARENT.  That is
consistent with unlink and rmdir etc which use I_MUTEX_PARENT on the
parent.

open() doesn't currently use I_MUTEX_PARENT because it never needs to
lock the child.  But as it *is* a parent that is being locked, using
I_MUTEX_PARENT probably make more sense.

--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3513,9 +3513,9 @@ static const char *open_last_lookups(struct nameidata *=
nd,
 	}
 	shared =3D !!(dir->d_inode->i_flags & S_PAR_UPDATE);
 	if ((open_flag & O_CREAT) && !shared)
-		inode_lock(dir->d_inode);
+		inode_lock_nested(dir->d_inode, I_MUTEX_PARENT);
 	else
-		inode_lock_shared(dir->d_inode);
+		inode_lock_shared_nested(dir->d_inode, I_MUTEX_PARENT);
 	dentry =3D lookup_open(nd, file, op, got_write);
 	if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED))
 		fsnotify_create(dir->d_inode, dentry);

Thanks,
NeilBrown