From: Chuck Lever <chuck.lever@oracle.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Bruce Fields <bfields@fieldses.org>,
Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
linux-kernel@vger.kernel.org, y2038@lists.linaro.org
Subject: Re: [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation
Date: Fri, 13 Dec 2019 11:26:13 -0500 [thread overview]
Message-ID: <CBC9899C-12BE-466E-8809-EA928AAE1F11@oracle.com> (raw)
In-Reply-To: <20191213141046.1770441-11-arnd@arndb.de>
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
next prev parent reply other threads:[~2019-12-13 20:39 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-13 14:10 [PATCH v2 00/12] nfsd: avoid 32-bit time_t Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 01/12] nfsd: use ktime_get_seconds() for timestamps Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 02/12] nfsd: print 64-bit timestamps in client_info_show Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 03/12] nfsd: handle nfs3 timestamps as unsigned Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 04/12] nfsd: use timespec64 in encode_time_delta Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 05/12] nfsd: make 'boot_time' 64-bit wide Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 06/12] nfsd: pass a 64-bit guardtime to nfsd_setattr() Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 07/12] nfsd: use time64_t in nfsd_proc_setattr() check Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 08/12] nfsd: fix delay timer on 32-bit architectures Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 09/12] nfsd: fix jiffies/time_t mixup in LRU list Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 10/12] nfsd: use boottime for lease expiry alculation Arnd Bergmann
2019-12-13 16:26 ` Chuck Lever [this message]
2019-12-13 16:40 ` Arnd Bergmann
2019-12-13 18:23 ` Chuck Lever
2019-12-13 21:09 ` Arnd Bergmann
2019-12-13 21:13 ` Bruce Fields
2019-12-13 21:19 ` Arnd Bergmann
2019-12-20 2:57 ` J. Bruce Fields
2019-12-13 14:10 ` [PATCH v2 11/12] nfsd: use ktime_get_real_seconds() in nfs4_verifier Arnd Bergmann
2019-12-13 14:10 ` [PATCH v2 12/12] nfsd: remove nfs4_reset_lease() declarations Arnd Bergmann
2019-12-18 17:21 ` [PATCH v2 00/12] nfsd: avoid 32-bit time_t Arnd Bergmann
2019-12-18 17:24 ` J. Bruce Fields
2019-12-18 18:05 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CBC9899C-12BE-466E-8809-EA928AAE1F11@oracle.com \
--to=chuck.lever@oracle.com \
--cc=arnd@arndb.de \
--cc=bfields@fieldses.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=y2038@lists.linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).