Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <380a3ac6d07832c4c7bef84615bc5c5d47be30ac.camel@kernel.org>
Subject: Re: [RFC PATCH 2/3] nfsd: rework arguments to nfs4_set_delegation
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Neil Brown <neilb@suse.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Thu, 14 Jul 2022 14:59:15 -0400
In-Reply-To: <E8CCB7EF-242C-4530-96E2-ACC7B5CB3163@oracle.com>
References: <20220714152819.128276-1-jlayton@kernel.org>
         <20220714152819.128276-3-jlayton@kernel.org>
         <CDD9B96C-3CFC-4BA5-A71C-6C2BFAC2B227@oracle.com>
         <476892362c94debad589af79ff7d6766f5ca8c85.camel@kernel.org>
         <E8CCB7EF-242C-4530-96E2-ACC7B5CB3163@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.3 (3.44.3-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2022-07-14 at 17:14 +0000, Chuck Lever III wrote:
> =20
>=20
> > On Jul 14, 2022, at 1:12 PM, Jeff Layton <jlayton@kernel.org> wrote:
> >=20
> > On Thu, 2022-07-14 at 16:47 +0000, Chuck Lever III wrote:
> > >=20
> > > > On Jul 14, 2022, at 11:28 AM, Jeff Layton <jlayton@kernel.org> wrot=
e:
> > > >=20
> > > > We'll need the nfs4_open to vet the filename. Change nfs4_set_deleg=
ation
> > > > to take the same arguments are nfs4_open_delegation.
> > >=20
> > > ^are^as
> > >=20
> > > Nit: Considering that in the next patch you change the synopsis of
> > > nfs4_open_delegation again but not nfs4_set_delegation, this
> > > description causes a little whiplash.
> > >=20
> > >=20
> >=20
> > Yeah, I should have squashed a couple of those together. I _did_ say it
> > was an RFC. I can resend a cleaned-up version later if you want to take
> > this in.
>=20
> I'm interested in Neil's thoughts about this approach, but I'm
> willing to run with it unless test results show a regression.
>=20
>=20

...and there is a regression. Partial lockdep pop here:

Jul 14 14:46:54 quad3 kernel: =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
Jul 14 14:46:54 quad3 kernel: WARNING: possible recursive locking detected
Jul 14 14:46:54 quad3 kernel: 5.19.0-rc5+ #316 Tainted: G           OE   =
=20
Jul 14 14:46:54 quad3 kernel: --------------------------------------------
Jul 14 14:46:54 quad3 kernel: nfsd/1148 is trying to acquire lock:
Jul 14 14:46:54 quad3 kernel: ffff88812a3a7388 (&inode->i_sb->s_type->i_mut=
ex_dir_key/1){++++}-{3:3}, at: nfsd4_process_open2+0x1890/0x2710 [nfsd]
Jul 14 14:46:54 quad3 kernel:=20
                              but task is already holding lock:
Jul 14 14:46:54 quad3 kernel: ffff88812a3a7388 (&inode->i_sb->s_type->i_mut=
ex_dir_key/1){++++}-{3:3}, at: nfsd_lookup_dentry+0x16f/0x6a0 [nfsd]
Jul 14 14:46:54 quad3 kernel:=20
                              other info that might help us debug this:
Jul 14 14:46:54 quad3 kernel:  Possible unsafe locking scenario:
Jul 14 14:46:54 quad3 kernel:        CPU0
Jul 14 14:46:54 quad3 kernel:        ----
Jul 14 14:46:54 quad3 kernel:   lock(&inode->i_sb->s_type->i_mutex_dir_key/=
1);
Jul 14 14:46:54 quad3 kernel:   lock(&inode->i_sb->s_type->i_mutex_dir_key/=
1);
Jul 14 14:46:54 quad3 kernel:=20
                               *** DEADLOCK ***
Jul 14 14:46:54 quad3 kernel:  May be due to missing lock nesting notation
Jul 14 14:46:54 quad3 kernel: 1 lock held by nfsd/1148:
Jul 14 14:46:54 quad3 kernel:  #0: ffff88812a3a7388 (&inode->i_sb->s_type->=
i_mutex_dir_key/1){++++}-{3:3}, at: nfsd_lookup_dentry+0x16f/0x6a0 [nfsd]
Jul 14 14:46:54 quad3 kernel:=20


The core problem is the unclear locking in nfsd_lookup_dentry. Sometimes
that returns with the i_rwsem held, but there's no clear indication of
whether that's the case when the function returns. I guess fh_unlock
just takes care of that (which is a little scary, tbqh).

Now that I've taken a stab at it, I don't see how we can fix this w/o
taking Neil's locking cleanups series first. I think I'll pull that in
and try to redo this series on top of it.

Cheers,
--=20
Jeff Layton <jlayton@kernel.org>