Re: [PATCH RFC v15 11/11] NFSD: Show state of courtesy clients in client info

[dai.ngo@oracle.com]

On 3/9/22 12:51 PM, dai.ngo@oracle.com wrote:
>
> On 3/9/22 12:14 PM, Chuck Lever III wrote:
>>
>>> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>>
>>> Update client_info_show to show state of courtesy client
>>> and time since last renew.
>>>
>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>> ---
>>> fs/nfsd/nfs4state.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index bced09014e6b..ed14e0b54537 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -2439,7 +2439,8 @@ static int client_info_show(struct seq_file 
>>> *m, void *v)
>>> {
>>>     struct inode *inode = m->private;
>>>     struct nfs4_client *clp;
>>> -    u64 clid;
>>> +    u64 clid, hrs;
>>> +    u32 mins, secs;
>>>
>>>     clp = get_nfsdfs_clp(inode);
>>>     if (!clp)
>>> @@ -2451,6 +2452,12 @@ static int client_info_show(struct seq_file 
>>> *m, void *v)
>>>         seq_puts(m, "status: confirmed\n");
>>>     else
>>>         seq_puts(m, "status: unconfirmed\n");
>>> +    seq_printf(m, "courtesy client: %s\n",
>>> +        test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags) ? "yes" : 
>>> "no");
>> I'm wondering if it would be more economical to combine this
>> output with the status output just before it so we have only
>> one of:
>>
>>     seq_puts(m, "status: unconfirmed\n");
>>
>>     seq_puts(m, "status: confirmed\n");
>>
>> or
>>
>>     seq_puts(m, "status: courtesy\n");
>
> make sense, will fix.

On second thought, I think it's safer to keep this the same
since there might be scripts out there that depend on it.

-Dai

>
>>
>>
>>> +    hrs = div_u64_rem(ktime_get_boottime_seconds() - clp->cl_time,
>>> +                3600, &secs);
>>> +    mins = div_u64_rem((u64)secs, 60, &secs);
>>> +    seq_printf(m, "time since last renew: %02ld:%02d:%02d\n", hrs, 
>>> mins, secs);
>> Thanks, this seems more friendly than what was here before.
>>
>> However if we replace the fixed courtesy timeout with a
>> shrinker, I bet some courtesy clients might lie about for
>> many more that 99 hours. Perhaps the left-most format
>> specifier could be just "%lu" and the rest could be "%02u".
>>
>> (ie, also turn the "d" into "u" to prevent ever displaying
>> a negative number of time units).
>
> will fix.
>
> I will wait for your review of the rest of the patches before
> I submit v16.
>
> Thanks,
> -Dai
>
>>
>>
>>>     seq_printf(m, "name: ");
>>>     seq_quote_mem(m, clp->cl_name.data, clp->cl_name.len);
>>>     seq_printf(m, "\nminor version: %d\n", clp->cl_minorversion);
>>> -- 
>>> 2.9.5
>>>
>> -- 
>> Chuck Lever
>>
>>
>>