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

[Chuck Lever III <chuck.lever@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