From: Chuck Lever <chuck.lever@oracle.com>
To: Robert Milkowski <rmilkowski@gmail.com>
Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
Trond Myklebust <trond.myklebust@hammerspace.com>,
Anna Schumaker <anna.schumaker@netapp.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals
Date: Mon, 30 Dec 2019 10:36:32 -0500 [thread overview]
Message-ID: <D82A1590-FAA3-47C5-B198-937ED88EF71C@oracle.com> (raw)
In-Reply-To: <025801d5bf24$aa242100$fe6c6300$@gmail.com>
> On Dec 30, 2019, at 10:20 AM, Robert Milkowski <rmilkowski@gmail.com> wrote:
>
> From: Robert Milkowski <rmilkowski@gmail.com>
>
> Currently, each time nfs4_do_fsinfo() is called it will do an implicit
> NFS4 lease renewal, which is not compliant with the NFS4 specification.
> This can result in a lease being expired by an NFS server.
>
> Commit 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing leases")
> introduced implicit client lease renewal in nfs4_do_fsinfo(),
> which can result in the NFSv4.0 lease to expire on a server side,
> and servers returning NFS4ERR_EXPIRED or NFS4ERR_STALE_CLIENTID.
>
> This can easily be reproduced by frequently unmounting a sub-mount,
> then stat'ing it to get it mounted again, which will delay or even
> completely prevent client from sending RENEW operations if no other
> NFS operations are issued. Eventually nfs server will expire client's
> lease and return an error on file access or next RENEW.
>
> This can also happen when a sub-mount is automatically unmounted
> due to inactivity (after nfs_mountpoint_expiry_timeout), then it is
> mounted again via stat(). This can result in a short window during
> which client's lease will expire on a server but not on a client.
> This specific case was observed on production systems.
>
> This patch makes an explicit lease renewal instead of an implicit one,
> by adding RENEW to a compound operation issued by nfs4_do_fsinfo(),
> similarly to NFSv4.1 which adds SEQUENCE operation.
>
> Fixes: 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing leases")
> Signed-off-by: Robert Milkowski <rmilkowski@gmail.com>
Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> fs/nfs/nfs4proc.c | 4 ++++
> fs/nfs/nfs4xdr.c | 13 +++++++++++--
> include/linux/nfs_xdr.h | 3 +++
> 3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 76d3716..6d075f0 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4998,12 +4998,16 @@ static int nfs4_proc_statfs(struct nfs_server *server, struct nfs_fh *fhandle, s
> static int _nfs4_do_fsinfo(struct nfs_server *server, struct nfs_fh *fhandle,
> struct nfs_fsinfo *fsinfo)
> {
> + struct nfs_client *clp = server->nfs_client;
> struct nfs4_fsinfo_arg args = {
> .fh = fhandle,
> .bitmask = server->attr_bitmask,
> + .clientid = clp->cl_clientid,
> + .renew = nfs4_has_session(clp) ? 0 : 1, /* append RENEW */
> };
> struct nfs4_fsinfo_res res = {
> .fsinfo = fsinfo,
> + .renew = nfs4_has_session(clp) ? 0 : 1,
> };
> struct rpc_message msg = {
> .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_FSINFO],
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 936c577..0ce9a10 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -555,11 +555,13 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
> #define NFS4_enc_fsinfo_sz (compound_encode_hdr_maxsz + \
> encode_sequence_maxsz + \
> encode_putfh_maxsz + \
> - encode_fsinfo_maxsz)
> + encode_fsinfo_maxsz + \
> + encode_renew_maxsz)
> #define NFS4_dec_fsinfo_sz (compound_decode_hdr_maxsz + \
> decode_sequence_maxsz + \
> decode_putfh_maxsz + \
> - decode_fsinfo_maxsz)
> + decode_fsinfo_maxsz + \
> + decode_renew_maxsz)
> #define NFS4_enc_renew_sz (compound_encode_hdr_maxsz + \
> encode_renew_maxsz)
> #define NFS4_dec_renew_sz (compound_decode_hdr_maxsz + \
> @@ -2646,6 +2648,8 @@ static void nfs4_xdr_enc_fsinfo(struct rpc_rqst *req, struct xdr_stream *xdr,
> encode_sequence(xdr, &args->seq_args, &hdr);
> encode_putfh(xdr, args->fh, &hdr);
> encode_fsinfo(xdr, args->bitmask, &hdr);
> + if (args->renew)
> + encode_renew(xdr, args->clientid, &hdr);
> encode_nops(&hdr);
> }
>
> @@ -6778,6 +6782,11 @@ static int nfs4_xdr_dec_fsinfo(struct rpc_rqst *req, struct xdr_stream *xdr,
> status = decode_putfh(xdr);
> if (!status)
> status = decode_fsinfo(xdr, res->fsinfo);
> + if (status)
> + goto out;
> + if (res->renew)
> + status = decode_renew(xdr);
> +out:
> return status;
> }
>
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 72d5695..49bd673 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1025,11 +1025,14 @@ struct nfs4_fsinfo_arg {
> struct nfs4_sequence_args seq_args;
> const struct nfs_fh * fh;
> const u32 * bitmask;
> + clientid4 clientid;
> + unsigned char renew:1;
> };
>
> struct nfs4_fsinfo_res {
> struct nfs4_sequence_res seq_res;
> struct nfs_fsinfo *fsinfo;
> + unsigned char renew:1;
> };
>
> struct nfs4_getattr_arg {
> --
> 1.8.3.1
>
>
--
Chuck Lever
next prev parent reply other threads:[~2019-12-30 15:36 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-30 15:20 [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals Robert Milkowski
2019-12-30 15:36 ` Chuck Lever [this message]
2020-01-20 17:55 ` Robert Milkowski
2020-01-22 19:10 ` Schumaker, Anna
2020-01-22 19:55 ` Robert Milkowski
2020-01-23 19:08 ` Trond Myklebust
2020-01-27 14:45 ` Robert Milkowski
2020-01-27 15:05 ` Chuck Lever
2020-01-27 15:34 ` Robert Milkowski
2020-01-27 15:54 ` Trond Myklebust
2020-01-27 17:26 ` Robert Milkowski
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=D82A1590-FAA3-47C5-B198-937ED88EF71C@oracle.com \
--to=chuck.lever@oracle.com \
--cc=anna.schumaker@netapp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=rmilkowski@gmail.com \
--cc=trond.myklebust@hammerspace.com \
/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).