Re: [PATCH v3 2/2] NFSD: enable support for write delegation

From: Chuck Lever III <chuck.lever@oracle.com>
To: Dai Ngo <dai.ngo@oracle.com>
Cc: Jeff Layton <jlayton@kernel.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] NFSD: enable support for write delegation
Date: Thu, 18 May 2023 18:03:33 +0000	[thread overview]
Message-ID: <3A7E2D1B-B24B-4920-85EF-D6AE029B57D6@oracle.com> (raw)
In-Reply-To: <5fab1724-090b-9c22-5555-bf3df7ea165c@oracle.com>

> On May 18, 2023, at 2:01 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> 
> On 5/18/23 10:16 AM, Chuck Lever III wrote:
>> 
>>> On May 18, 2023, at 1:11 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>> 
>>> 
>>> On 5/18/23 6:23 AM, Chuck Lever III wrote:
>>>>> On May 17, 2023, at 7:38 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>>>> 
>>>>> This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>>>>> if there is no conflict with other OPENs.
>>>>> 
>>>>> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
>>>>> are handled the same as read delegation using notify_change,
>>>>> try_break_deleg.
>>>> Very clean. A couple of suggestions, one is down below, and here is
>>>> the other:
>>>> 
>>>> I was thinking we should add one or two counters in fs/nfsd/stats.c
>>>> to track how often read and write delegations are offered, and
>>>> perhaps one to count the number of DELEGRETURN operations. What do
>>>> you think makes sense?
>>> I'm not sure what these counters will tell us, currently we already
>>> has a counter for number of delegations handed out.
>> I haven't found that, where is it? Certainly, if NFSD already
>> has one, then no need to add more.
> 
> num_delegations in nfs4state.c
> 
>> 
>> It would be nice one day, perhaps, to have a metric of how many
>> delegations a client holds. That's not for this series.
> 
> okay.
> 
>> 
>> 
>>> I think a counter
>>> on how often nfsd has to recall the write delegation due to GETATTR can
>>> be useful to know whether we should implement CB_GETATTR.
>> I hesitated to mention that because I wonder if that's something
>> that would be interesting only for defending a design choice,
>> not for site-to-site tuning. In other words, after we plumb it
>> into NFSD, it will never actually be used after CB_GETATTR
>> support is added.
>> 
>> Do you believe it's something that administrators can use to
>> help balance or tune their workloads?
> 
> You're right. That is just for ourselves to determine if CB_GETATTR
> is needed.

To be clear, such a counter, I agree, would be useful /to us/.
I'm just not sure how we could add something that would not
become part of the kernel/userspace API. Anyone have thoughts
about that?

--
Chuck Lever