Re: [Y2038] [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
On Fri, Dec 13, 2019 at 03:10:44PM +0100, Arnd Bergmann wrote:
> @@ -5212,8 +5211,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> struct nfs4_ol_stateid *stp;
> struct nfsd4_blocked_lock *nbl;
> struct list_head *pos, *next, reaplist;
> - time_t cutoff = get_seconds() - nn->nfsd4_lease;
> - time_t t, new_timeo = nn->nfsd4_lease;
> + time64_t cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease;
For some reason the version I was testing still had this as
get_seconds(), which was what was causing test failures. I'm not quite
sure what happened--I may have just typo'd something while fixing up a
conflict.
--b.
> + time64_t t, new_timeo = nn->nfsd4_lease;
>
> dprintk("NFSD: laundromat service - starting\n");
>
> @@ -5227,7 +5226,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> spin_lock(&nn->client_lock);
> list_for_each_safe(pos, next, &nn->client_lru) {
> clp = list_entry(pos, struct nfs4_client, cl_lru);
> - if (time_after((unsigned long)clp->cl_time, (unsigned
> long)cutoff)) {
> + if (clp->cl_time > cutoff) {
> t = clp->cl_time - cutoff;
> new_timeo = min(new_timeo, t);
> break;
> @@ -5250,7 +5249,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> spin_lock(&state_lock);
> list_for_each_safe(pos, next, &nn->del_recall_lru) {
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> - if (time_after((unsigned long)dp->dl_time, (unsigned
> long)cutoff)) {
> + if (dp->dl_time > cutoff) {
> t = dp->dl_time - cutoff;
> new_timeo = min(new_timeo, t);
> break;
> @@ -5270,8 +5269,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> while (!list_empty(&nn->close_lru)) {
> oo = list_first_entry(&nn->close_lru, struct nfs4_openowner,
> oo_close_lru);
> - if (time_after((unsigned long)oo->oo_time,
> -(unsigned long)cutoff)) {
> + if (oo->oo_time > cutoff) {
> t = oo->oo_time - cutoff;
> new_timeo = min(new_timeo, t);
> break;
> @@ -5301,8 +5299,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> while (!list_empty(&nn->blocked_locks_lru)) {
> nbl = list_first_entry(&nn->blocked_locks_lru,
> struct nfsd4_blocked_lock, nbl_lru);
> - if (time_after((unsigned long)nbl->nbl_time,
> -(unsigned long)cutoff)) {
> + if (nbl->nbl_time > cutoff) {
> t = nbl->nbl_time - cutoff;
> new_timeo = min(new_timeo, t);
> break;
> @@ -5319,7 +5316,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> free_blocked_lock(nbl);
> }
> out:
> - new_timeo = max_t(time_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> + new_timeo = max_t(time64_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> return new_timeo;
> }
>
> @@ -5329,13 +5326,13 @@ static void laundromat_main(struct work_struct *);
> static void
> laundromat_main(struct work_struct *laundry)
> {
> - time_t t;
> + time64_t t;
> struct delayed_work *dwork = to_delayed_work(laundry);
> struct nfsd_net *nn = container_of(dwork, struct nfsd_net,
> laundromat_work);
>
> t = nfs4_laundromat(nn);
> - dprintk("NFSD: laundromat_main - sleeping for %ld seconds\n", t);
> + dprintk("NFSD: laundromat_main - sleeping for %lld seconds\n", t);
> queue_delayed_work(laundry_wq, &nn->laundromat_work, t*HZ);
> }
>
> @@ -6549,7 +6546,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct
> nfsd4_compound_state *cstate,
> }
>
> if (fl_flags & FL_SLEEP) {
> - nbl->nbl_time = get_seconds();
> + nbl->nbl_time = ktime_get_boottime_seconds();
> spin_lock(&nn->blocked_locks_lock);
> list_add_tail(&nbl->nbl_list, &lock_sop->lo_blocked);
> list_add_tail(&nbl->nbl_lru, &nn->blocked_locks_lru);
> @@ -7709,7 +7706,7 @@ nfs4_state_start_net(struct net *net)
> nfsd4_client_tracking_init(net);
> if (nn->track_reclaim_completes && nn->reclaim_str_hashtbl_size == 0)
> goto skip_grace;
> - printk(KERN_INFO "NFSD: starting %ld-second grace period (net %x)\n",
> + printk(KERN_INFO "NFSD: starting %lld-second grace period (net %x)\n",
> nn->nfsd4_grace, net->ns.inum);
> queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace *
> HZ);
> return 0;
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 11b42c523f04..aace740d5a92 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -956,7 +956,7 @@ static ssize_t write_maxconn(struct file *file, char
> *buf, size_t size
Re: [Y2038] [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
On Fri, Dec 13, 2019 at 10:13 PM Bruce Fields wrote: > On Fri, Dec 13, 2019 at 01:23:08PM -0500, Chuck Lever wrote: > > > On Dec 13, 2019, at 11:40 AM, Arnd Bergmann wrote: > > > On Fri, Dec 13, 2019 at 5:26 PM 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. Arnd ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
On Fri, Dec 13, 2019 at 01:23:08PM -0500, Chuck Lever wrote:
>
>
> > On Dec 13, 2019, at 11:40 AM, Arnd Bergmann wrote:
> >
> > On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever wrote:
> >>> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann wrote:
> >
> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> >>> index 24534db87e86..508d7c6c00b5 100644
> >>> --- a/fs/nfsd/nfs4callback.c
> >>> +++ b/fs/nfsd/nfs4callback.c
> >>> @@ -823,7 +823,12 @@ static const struct rpc_program cb_program = {
> >>> static int max_cb_time(struct net *net)
> >>> {
> >>> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> >>> - return max(nn->nfsd4_lease/10, (time_t)1) * HZ;
> >>> +
> >>> + /* 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.
--b.
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
On Fri, Dec 13, 2019 at 7:23 PM Chuck Lever wrote: > > On Dec 13, 2019, at 11:40 AM, Arnd Bergmann wrote: > > > > On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever wrote: > >>> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann wrote: > > > >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > >>> index 24534db87e86..508d7c6c00b5 100644 > > > > 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. ;-) I changed the comment now to: + /* +* nfsd4_lease is set to at most one hour in __nfsd4_write_time, +* so we can use 32-bit math on it. Warn if that assumption +* ever stops being true. +*/ Modified branch pushed to git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git y2038-nfsd-v2 Arnd ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
> On Dec 13, 2019, at 11:40 AM, Arnd Bergmann wrote:
>
> On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever wrote:
>>> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann wrote:
>
>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>>> index 24534db87e86..508d7c6c00b5 100644
>>> --- a/fs/nfsd/nfs4callback.c
>>> +++ b/fs/nfsd/nfs4callback.c
>>> @@ -823,7 +823,12 @@ static const struct rpc_program cb_program = {
>>> static int max_cb_time(struct net *net)
>>> {
>>> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>> - return max(nn->nfsd4_lease/10, (time_t)1) * HZ;
>>> +
>>> + /* 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. ;-)
>> (Otherwise these all LGTM).
>
> Thanks for taking a look.
>
> Arnd
--
Chuck Lever
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever wrote:
> > On Dec 13, 2019, at 9:10 AM, Arnd Bergmann wrote:
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 24534db87e86..508d7c6c00b5 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -823,7 +823,12 @@ static const struct rpc_program cb_program = {
> > static int max_cb_time(struct net *net)
> > {
> > struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > - return max(nn->nfsd4_lease/10, (time_t)1) * HZ;
> > +
> > + /* 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.
> (Otherwise these all LGTM).
Thanks for taking a look.
Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
Hi Arnd-
> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann wrote:
>
> A couple of time_t variables are only used to track the state of the
> lease time and its expiration. The code correctly uses the 'time_after()'
> macro to make this work on 32-bit architectures even beyond year 2038,
> but the get_seconds() function and the time_t type itself are deprecated
> as they behave inconsistently between 32-bit and 64-bit architectures
> and often lead to code that is not y2038 safe.
>
> As a minor issue, using get_seconds() leads to problems with concurrent
> settimeofday() or clock_settime() calls, in the worst case timeout never
> triggering after the time has been set backwards.
>
> Change nfsd to use time64_t and ktime_get_boottime_seconds() here. This
> is clearly excessive, as boottime by itself means we never go beyond 32
> bits, but it does mean we handle this correctly and consistently without
> having to worry about corner cases and should be no more expensive than
> the previous implementation on 64-bit architectures.
>
> The max_cb_time() function gets changed in order to avoid an expensive
> 64-bit division operation, but as the lease time is at most one hour,
> there is no change in behavior.
>
> Signed-off-by: Arnd Bergmann
> ---
> fs/nfsd/netns.h| 4 ++--
> fs/nfsd/nfs4callback.c | 7 ++-
> fs/nfsd/nfs4state.c| 41 +++--
> fs/nfsd/nfsctl.c | 6 +++---
> fs/nfsd/state.h| 8
> 5 files changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 29bbe28eda53..2baf32311e00 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -92,8 +92,8 @@ struct nfsd_net {
> bool in_grace;
> const struct nfsd4_client_tracking_ops *client_tracking_ops;
>
> - time_t nfsd4_lease;
> - time_t nfsd4_grace;
> + time64_t nfsd4_lease;
> + time64_t nfsd4_grace;
> bool somebody_reclaimed;
>
> bool track_reclaim_completes;
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 24534db87e86..508d7c6c00b5 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -823,7 +823,12 @@ static const struct rpc_program cb_program = {
> static int max_cb_time(struct net *net)
> {
> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> - return max(nn->nfsd4_lease/10, (time_t)1) * HZ;
> +
> + /* 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?
(Otherwise these all LGTM).
> +
> + return max(((u32)nn->nfsd4_lease)/10, 1u) * HZ;
> }
>
> static struct workqueue_struct *callback_wq;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9a063c4b4460..6afdef63f6d7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -170,7 +170,7 @@ renew_client_locked(struct nfs4_client *clp)
> clp->cl_clientid.cl_boot,
> clp->cl_clientid.cl_id);
> list_move_tail(&clp->cl_lru, &nn->client_lru);
> - clp->cl_time = get_seconds();
> + clp->cl_time = ktime_get_boottime_seconds();
> }
>
> static void put_client_renew_locked(struct nfs4_client *clp)
> @@ -2612,7 +2612,7 @@ static struct nfs4_client *create_client(struct
> xdr_netobj name,
> gen_clid(clp, nn);
> kref_init(&clp->cl_nfsdfs.cl_ref);
> nfsd4_init_cb(&clp->cl_cb_null, clp, NULL, NFSPROC4_CLNT_CB_NULL);
> - clp->cl_time = get_seconds();
> + clp->cl_time = ktime_get_boottime_seconds();
> clear_bit(0, &clp->cl_cb_slot_busy);
> copy_verf(clp, verf);
> memcpy(&clp->cl_addr, sa, sizeof(struct sockaddr_storage));
> @@ -4282,7 +4282,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net
> *net)
> last = oo->oo_last_closed_stid;
> oo->oo_last_closed_stid = s;
> list_move_tail(&oo->oo_close_lru, &nn->close_lru);
> - oo->oo_time = get_seconds();
> + oo->oo_time = ktime_get_boottime_seconds();
> spin_unlock(&nn->client_lock);
> if (last)
> nfs4_put_stid(&last->st_stid);
> @@ -4377,7 +4377,7 @@ static void nfsd4_cb_recall_prepare(struct
> nfsd4_callback *cb)
>*/
> spin_lock(&state_lock);
> if (dp->dl_time == 0) {
> - dp->dl_time = get_seconds();
> + dp->dl_time = ktime_get_boottime_seconds();
> list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
> }
> spin_unlock(&state_lock);
> @@ -5183,9 +5183,8 @@ nfsd4_end_grace(struct nfsd_net *nn)
> */
> static bool clients_still_reclaiming(struct nfsd_net *nn)
> {
> - unsigned long now = (unsigned long) ktime_get_real_seconds();
> - unsigned long double_grace_period_end = (unsigned long)nn->boot_time +
> -2 * (unsigned long)nn->nfsd4_lease;
> + time64_t double_grace_period_end = nn->boot_time +
> +
