On Fri, Dec 13, 2019 at 10:13 PM Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> On Fri, Dec 13, 2019 at 01:23:08PM -0500, Chuck Lever wrote:
> > > On Dec 13, 2019, at 11:40 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> > >>> +
> > >>> + /* nfsd4_lease is set to at most one hour */
> > >>> + if (WARN_ON_ONCE(nn->nfsd4_lease > 3600))
> > >>> + return 360 * HZ;
> > >>
> > >> Why is the WARN_ON_ONCE added here? Is it really necessary?
> > >
> > > This is to ensure the kernel doesn't change to a larger limit that
> > > requires a 64-bit division on a 32-bit architecture.
> > >
> > > With the old code, dividing by 10 was always fast as
> > > nn->nfsd4_lease was the size of an integer register. Now it
> > > is 64 bit wide, and I check that truncating it to 32 bit again
> > > is safe.
> >
> > OK. That comment should state this reason rather than just repeating
> > what the code does. ;-)
>
> Note that __nfsd4_write_time() already limits nfsd4_lease to 3600.
>
> We could just use a smaller type for nfsd4_lease if that'd help.
I think it's generally clearer to have only one type to store the lease
time, and time64_t is the most sensible one, even if the range is a
bit excessive.
I've seen too many time related bugs from mixing integer types
incorrectly.
Arnd
