On Fri, Dec 13, 2019 at 10:13 PM Bruce Fields firstname.lastname@example.org wrote:
On Fri, Dec 13, 2019 at 01:23:08PM -0500, Chuck Lever 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.