> On May 23, 2022, at 12:37 PM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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
