diff mbox series

[v2,10/12] nfsd: use boottime for lease expiry alculation

Message ID 20191213141046.1770441-11-arnd@arndb.de
State New
Headers show
Series nfsd: avoid 32-bit time_t | expand

Commit Message

Arnd Bergmann Dec. 13, 2019, 2:10 p.m. UTC 
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 <arnd@arndb.de>

---
 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(-)

-- 
2.20.0

Comments

Chuck Lever Dec. 13, 2019, 4:26 p.m. UTC | #1 
Hi Arnd-

> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de>

> ---

> 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 +

> +					   2 * nn->nfsd4_lease;

> 

> 	if (nn->track_reclaim_completes &&

> 			atomic_read(&nn->nr_reclaim_complete) ==

> @@ -5198,12 +5197,12 @@ static bool clients_still_reclaiming(struct nfsd_net *nn)

> 	 * If we've given them *two* lease times to reclaim, and they're

> 	 * still not done, give up:

> 	 */

> -	if (time_after(now, double_grace_period_end))

> +	if (ktime_get_boottime_seconds() > double_grace_period_end)

> 		return false;

> 	return true;

> }

> 

> -static time_t

> +static time64_t

> nfs4_laundromat(struct nfsd_net *nn)

> {

> 	struct nfs4_client *clp;

> @@ -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;

> +	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)

> 

> #ifdef CONFIG_NFSD_V4

> static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,

> -				  time_t *time, struct nfsd_net *nn)

> +				  time64_t *time, struct nfsd_net *nn)

> {

> 	char *mesg = buf;

> 	int rv, i;

> @@ -984,11 +984,11 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,

> 		*time = i;

> 	}

> 

> -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);

> +	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%lld\n", *time);

> }

> 

> static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,

> -				time_t *time, struct nfsd_net *nn)

> +				time64_t *time, struct nfsd_net *nn)

> {

> 	ssize_t rv;

> 

> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h

> index 03fc7b4380f9..e426b22b5028 100644

> --- a/fs/nfsd/state.h

> +++ b/fs/nfsd/state.h

> @@ -132,7 +132,7 @@ struct nfs4_delegation {

> 	struct list_head	dl_recall_lru;  /* delegation recalled */

> 	struct nfs4_clnt_odstate *dl_clnt_odstate;

> 	u32			dl_type;

> -	time_t			dl_time;

> +	time64_t		dl_time;

> /* For recall: */

> 	int			dl_retries;

> 	struct nfsd4_callback	dl_recall;

> @@ -310,7 +310,7 @@ struct nfs4_client {

> #endif

> 	struct xdr_netobj	cl_name; 	/* id generated by client */

> 	nfs4_verifier		cl_verifier; 	/* generated by client */

> -	time_t                  cl_time;        /* time of last lease renewal */

> +	time64_t		cl_time;	/* time of last lease renewal */

> 	struct sockaddr_storage	cl_addr; 	/* client ipaddress */

> 	bool			cl_mach_cred;	/* SP4_MACH_CRED in force */

> 	struct svc_cred		cl_cred; 	/* setclientid principal */

> @@ -449,7 +449,7 @@ struct nfs4_openowner {

> 	 */

> 	struct list_head	oo_close_lru;

> 	struct nfs4_ol_stateid *oo_last_closed_stid;

> -	time_t			oo_time; /* time of placement on so_close_lru */

> +	time64_t		oo_time; /* time of placement on so_close_lru */

> #define NFS4_OO_CONFIRMED   1

> 	unsigned char		oo_flags;

> };

> @@ -606,7 +606,7 @@ static inline bool nfsd4_stateid_generation_after(stateid_t *a, stateid_t *b)

> struct nfsd4_blocked_lock {

> 	struct list_head	nbl_list;

> 	struct list_head	nbl_lru;

> -	time_t			nbl_time;

> +	time64_t		nbl_time;

> 	struct file_lock	nbl_lock;

> 	struct knfsd_fh		nbl_fh;

> 	struct nfsd4_callback	nbl_cb;

> -- 

> 2.20.0

> 


--
Chuck Lever
Arnd Bergmann Dec. 13, 2019, 4:40 p.m. UTC | #2 
On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > On Dec 13, 2019, at 9:10 AM, Arnd Bergmann <arnd@arndb.de> 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
Chuck Lever Dec. 13, 2019, 6:23 p.m. UTC | #3 
> On Dec 13, 2019, at 11:40 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> 

> On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever <chuck.lever@oracle.com> wrote:

>>> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann <arnd@arndb.de> 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
Arnd Bergmann Dec. 13, 2019, 9:09 p.m. UTC | #4 
On Fri, Dec 13, 2019 at 7:23 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > On Dec 13, 2019, at 11:40 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> >

> > On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever <chuck.lever@oracle.com> wrote:

> >>> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann <arnd@arndb.de> 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
J. Bruce Fields Dec. 13, 2019, 9:13 p.m. UTC | #5 
On Fri, Dec 13, 2019 at 01:23:08PM -0500, Chuck Lever wrote:
> 

> 

> > On Dec 13, 2019, at 11:40 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> > 

> > On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever <chuck.lever@oracle.com> wrote:

> >>> On Dec 13, 2019, at 9:10 AM, Arnd Bergmann <arnd@arndb.de> 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.
Arnd Bergmann Dec. 13, 2019, 9:19 p.m. UTC | #6 
On Fri, Dec 13, 2019 at 10:13 PM Bruce Fields <bfields@fieldses.org> 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@arndb.de> wrote:

> > > On Fri, Dec 13, 2019 at 5:26 PM Chuck Lever <chuck.lever@oracle.com> 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
J. Bruce Fields Dec. 20, 2019, 2:57 a.m. UTC | #7 
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)

>  

>  #ifdef CONFIG_NFSD_V4

>  static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,

> -				  time_t *time, struct nfsd_net *nn)

> +				  time64_t *time, struct nfsd_net *nn)

>  {

>  	char *mesg = buf;

>  	int rv, i;

> @@ -984,11 +984,11 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,

>  		*time = i;

>  	}

>  

> -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);

> +	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%lld\n", *time);

>  }

>  

>  static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,

> -				time_t *time, struct nfsd_net *nn)

> +				time64_t *time, struct nfsd_net *nn)

>  {

>  	ssize_t rv;

>  

> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h

> index 03fc7b4380f9..e426b22b5028 100644

> --- a/fs/nfsd/state.h

> +++ b/fs/nfsd/state.h

> @@ -132,7 +132,7 @@ struct nfs4_delegation {

>  	struct list_head	dl_recall_lru;  /* delegation recalled */

>  	struct nfs4_clnt_odstate *dl_clnt_odstate;

>  	u32			dl_type;

> -	time_t			dl_time;

> +	time64_t		dl_time;

>  /* For recall: */

>  	int			dl_retries;

>  	struct nfsd4_callback	dl_recall;

> @@ -310,7 +310,7 @@ struct nfs4_client {

>  #endif

>  	struct xdr_netobj	cl_name; 	/* id generated by client */

>  	nfs4_verifier		cl_verifier; 	/* generated by client */

> -	time_t                  cl_time;        /* time of last lease renewal */

> +	time64_t		cl_time;	/* time of last lease renewal */

>  	struct sockaddr_storage	cl_addr; 	/* client ipaddress */

>  	bool			cl_mach_cred;	/* SP4_MACH_CRED in force */

>  	struct svc_cred		cl_cred; 	/* setclientid principal */

> @@ -449,7 +449,7 @@ struct nfs4_openowner {

>  	 */

>  	struct list_head	oo_close_lru;

>  	struct nfs4_ol_stateid *oo_last_closed_stid;

> -	time_t			oo_time; /* time of placement on so_close_lru */

> +	time64_t		oo_time; /* time of placement on so_close_lru */

>  #define NFS4_OO_CONFIRMED   1

>  	unsigned char		oo_flags;

>  };

> @@ -606,7 +606,7 @@ static inline bool nfsd4_stateid_generation_after(stateid_t *a, stateid_t *b)

>  struct nfsd4_blocked_lock {

>  	struct list_head	nbl_list;

>  	struct list_head	nbl_lru;

> -	time_t			nbl_time;

> +	time64_t		nbl_time;

>  	struct file_lock	nbl_lock;

>  	struct knfsd_fh		nbl_fh;

>  	struct nfsd4_callback	nbl_cb;

> -- 

> 2.20.0
diff mbox series

Patch

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;
+
+	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 +
+					   2 * nn->nfsd4_lease;
 
 	if (nn->track_reclaim_completes &&
 			atomic_read(&nn->nr_reclaim_complete) ==
@@ -5198,12 +5197,12 @@  static bool clients_still_reclaiming(struct nfsd_net *nn)
 	 * If we've given them *two* lease times to reclaim, and they're
 	 * still not done, give up:
 	 */
-	if (time_after(now, double_grace_period_end))
+	if (ktime_get_boottime_seconds() > double_grace_period_end)
 		return false;
 	return true;
 }
 
-static time_t
+static time64_t
 nfs4_laundromat(struct nfsd_net *nn)
 {
 	struct nfs4_client *clp;
@@ -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;
+	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)
 
 #ifdef CONFIG_NFSD_V4
 static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
-				  time_t *time, struct nfsd_net *nn)
+				  time64_t *time, struct nfsd_net *nn)
 {
 	char *mesg = buf;
 	int rv, i;
@@ -984,11 +984,11 @@  static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
 		*time = i;
 	}
 
-	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
+	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%lld\n", *time);
 }
 
 static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
-				time_t *time, struct nfsd_net *nn)
+				time64_t *time, struct nfsd_net *nn)
 {
 	ssize_t rv;
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 03fc7b4380f9..e426b22b5028 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -132,7 +132,7 @@  struct nfs4_delegation {
 	struct list_head	dl_recall_lru;  /* delegation recalled */
 	struct nfs4_clnt_odstate *dl_clnt_odstate;
 	u32			dl_type;
-	time_t			dl_time;
+	time64_t		dl_time;
 /* For recall: */
 	int			dl_retries;
 	struct nfsd4_callback	dl_recall;
@@ -310,7 +310,7 @@  struct nfs4_client {
 #endif
 	struct xdr_netobj	cl_name; 	/* id generated by client */
 	nfs4_verifier		cl_verifier; 	/* generated by client */
-	time_t                  cl_time;        /* time of last lease renewal */
+	time64_t		cl_time;	/* time of last lease renewal */
 	struct sockaddr_storage	cl_addr; 	/* client ipaddress */
 	bool			cl_mach_cred;	/* SP4_MACH_CRED in force */
 	struct svc_cred		cl_cred; 	/* setclientid principal */
@@ -449,7 +449,7 @@  struct nfs4_openowner {
 	 */
 	struct list_head	oo_close_lru;
 	struct nfs4_ol_stateid *oo_last_closed_stid;
-	time_t			oo_time; /* time of placement on so_close_lru */
+	time64_t		oo_time; /* time of placement on so_close_lru */
 #define NFS4_OO_CONFIRMED   1
 	unsigned char		oo_flags;
 };
@@ -606,7 +606,7 @@  static inline bool nfsd4_stateid_generation_after(stateid_t *a, stateid_t *b)
 struct nfsd4_blocked_lock {
 	struct list_head	nbl_list;
 	struct list_head	nbl_lru;
-	time_t			nbl_time;
+	time64_t		nbl_time;
 	struct file_lock	nbl_lock;
 	struct knfsd_fh		nbl_fh;
 	struct nfsd4_callback	nbl_cb;