Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <f20de886f02402970c86c5195ea344de128afd91.camel@kernel.org>
Subject: Re: [PATCH RFC] NFSD: Fix possible sleep during
 nfsd4_release_lockowner()
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Mon, 23 May 2022 13:38:59 -0400
In-Reply-To: <FF7F2939-C3DE-4584-BFFA-13B554706B9C@oracle.com>
References: <165323344948.2381.7808135229977810927.stgit@bazille.1015granger.net>
         <fe3f9ece807e1433631ee3e0bd6b78238305cb87.camel@kernel.org>
         <510282CB-38D3-438A-AF8A-9AC2519FCEF7@oracle.com>
         <c3d053dc36dd5e7dee1267f1c7107bbf911e4d53.camel@kernel.org>
         <1A37E2B5-8113-48D6-AF7C-5381F364D99E@oracle.com>
         <c357e63272de96f9e7595bf6688680879d83dc83.camel@kernel.org>
         <FF7F2939-C3DE-4584-BFFA-13B554706B9C@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.1 (3.44.1-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-05-23 at 17:25 +0000, Chuck Lever III wrote:
>=20
> > On May 23, 2022, at 12:37 PM, Jeff Layton <jlayton@kernel.org> wrote:
> >=20
> > On Mon, 2022-05-23 at 15:41 +0000, Chuck Lever III wrote:
> > >=20
> > > > On May 23, 2022, at 11:26 AM, Jeff Layton <jlayton@kernel.org> wrot=
e:
> > > >=20
> > > > What I was going to suggest is a nfsd_file_put variant that takes a
> > > > list_head. If the refcount goes to zero and the thing ends up being
> > > > unhashed, then you put it on the dispose list rather than doing the
> > > > blocking operations, and then clean it up later.
> > >=20
> > > Trond doesn't like that approach; see the e-mail thread.
> >=20
> > I didn't see him saying that that would be wrong, per-se, but the
> > initial implementation was racy.
>=20
> I proposed this for check_for_locks() to use:
>=20
> > void nfsd_file_put_async(struct nfsd_file *nf)
> > {
> > 	if (refcount_dec_and_test(&nf->nf_ref))
> > 		nfsd_file_close_inode(nf->nf_inode);
> > }
>=20
>=20
> Trond's response was:
>=20
> > That approach moves the sync to the garbage collector, which was
> > exactly what we're trying to avoid in the first place.
>=20
> Basically he's opposed to any flush work being done by
> the garbage collector because that has a known negative
> performance impact.
>=20
>=20

Fair enough. I like his other suggestion better anyway.

> > His suggestion was just to keep a counter in the lockowner of how many
> > locks are associated with it. That seems like a good suggestion, though
> > you'd probably need to add a parameter to lm_get_owner to indicate
> > whether you were adding a new lock or just doing a conflock copy.
>=20
> locks_copy_conflock() would need to take a boolean parameter
> that callers would set when they actually manipulate a lock.
>=20

Yep. You'd also have to add a bool arg to lm_put_owner so that you know
whether you need to decrement the counter.

> I would feel more comfortable with that approach if
> locks_copy_conflock() was a private API used only in fs/locks.c
> so we can audit its usage to ensure that callers are managing
> the lock count correctly.
>=20
>=20

It basically is. In fact, I'm not sure why it's exported since nothing
outside of locks.c calls it. Looking at the git history, it probably got
exported by mistake when we split up locks_copy_lock and
locks_copy_conflock, as the former was exported.

I think this approach is quite doable...
--=20
Jeff Layton <jlayton@kernel.org>