> On Mar 9, 2022, at 10:09 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:
>
>
>> On 3/9/22 12:51 PM, dai.ngo@xxxxxxxxxx wrote:
>>
>>> On 3/9/22 12:14 PM, Chuck Lever III wrote:
>>>
>>>> On Mar 4, 2022, at 7:37 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:
>>>>
>>>> Update client_info_show to show state of courtesy client
>>>> and time since last renew.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
>>>> ---
>>>> 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.
I agree we should be sensitive to potential users of this information. However…
Without having one or two examples of such scripts in front of us, it’s hard to say whether my suggestion (a new keyword after “status:”) or your original (a new line in the file) would be more disruptive.
Also I’m not seeing exactly how the output format is versioned… so what’s the safest way to make changes to the output format of this file? Anyone?
When I get in front of a computer again, I’ll have a look at the change history of this function.
> -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
>>>
>>>
>>>
