| Message ID | 1646440633-3542-12-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: Initial implementation of NFSv4 Courteous Server | expand |
> 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"); > + 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). > 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
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. > > >> + 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 > > >
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 >> >> >>
> On Mar 9, 2022, at 10:09 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > >> 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. 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 >>> >>> >>>
> On Mar 9, 2022, at 10:09 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > > 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. The "status:" information was added just a year ago by 472d155a0631 ("nfsd: report client confirmation status in "info" file"). I don't see any concern in recent commit history about making the information in this file machine-readable or backwards-compatible. Therefore IMO it should be safe and acceptable to use: status: confirmed|unconfirmed|courtesy > -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 >>> >>> >>> -- Chuck Lever
On Thu, Mar 10, 2022 at 03:27:58AM +0000, Chuck Lever III wrote: > > > On Mar 9, 2022, at 10:09 PM, Dai Ngo <dai.ngo@oracle.com> wrote: > > > > > >> 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. > > 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? It's not versioned. It'd be good to document some rules; nfsd(7) seems like the logical place to put that, though probably knows about it. Pointers to it from kernel comments and elsewhere might help? I suppose the absolute safest option would be adding a new line, but I like the idea of adding the possibility of "courtesy" to the existing line as you suggest, and that seems very low risk. There is one utility, see nfs-utils/tools/nfsdclnt. I'd forgotten about it untill I looked just now.... --b.
On 3/10/22 7:00 AM, Bruce Fields wrote: > On Thu, Mar 10, 2022 at 03:27:58AM +0000, Chuck Lever III wrote: >>> On Mar 9, 2022, at 10:09 PM, Dai Ngo <dai.ngo@oracle.com> wrote: >>> >>> >>>> 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. >> 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? > It's not versioned. It'd be good to document some rules; nfsd(7) seems > like the logical place to put that, though probably knows about it. > Pointers to it from kernel comments and elsewhere might help? > > I suppose the absolute safest option would be adding a new line, but I > like the idea of adding the possibility of "courtesy" to the existing > line as you suggest, and that seems very low risk. > > There is one utility, see nfs-utils/tools/nfsdclnt. I'd forgotten about > it untill I looked just now.... I think nfsdclnt does not parse the 'status' field so we're safe to update the status to show 'confirmed/unconfirmed/courtesy'. -Dai > > --b.
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"); + 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); 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);
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(-)