Re: [PATCH RFC] NFSD: Fix possible sleep during nfsd4_release_lockowner()

From: Chuck Lever III <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH RFC] NFSD: Fix possible sleep during nfsd4_release_lockowner()
Date: Mon, 23 May 2022 17:25:48 +0000	[thread overview]
Message-ID: <FF7F2939-C3DE-4584-BFFA-13B554706B9C@oracle.com> (raw)
In-Reply-To: <c357e63272de96f9e7595bf6688680879d83dc83.camel@kernel.org>

> On May 23, 2022, at 12:37 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2022-05-23 at 15:41 +0000, Chuck Lever III wrote:
>> 
>>> On May 23, 2022, at 11:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> 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.
>> 
>> Trond doesn't like that approach; see the e-mail thread.
> 
> I didn't see him saying that that would be wrong, per-se, but the
> initial implementation was racy.

I proposed this for check_for_locks() to use:

> void nfsd_file_put_async(struct nfsd_file *nf)
> {
> 	if (refcount_dec_and_test(&nf->nf_ref))
> 		nfsd_file_close_inode(nf->nf_inode);
> }

Trond's response was:

> That approach moves the sync to the garbage collector, which was
> exactly what we're trying to avoid in the first place.

Basically he's opposed to any flush work being done by
the garbage collector because that has a known negative
performance impact.

> 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.

locks_copy_conflock() would need to take a boolean parameter
that callers would set when they actually manipulate a lock.

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.

--
Chuck Lever