Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals
@ 2019-12-30 15:20 Robert Milkowski
  2019-12-30 15:36 ` Chuck Lever
  0 siblings, 1 reply; 5+ messages in thread
From: Robert Milkowski @ 2019-12-30 15:20 UTC (permalink / raw)
  To: linux-nfs; +Cc: Chuck Lever, Trond Myklebust, Anna Schumaker, linux-kernel

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals
  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
  2020-01-20 17:55   ` Robert Milkowski
  0 siblings, 1 reply; 5+ messages in thread
From: Chuck Lever @ 2019-12-30 15:36 UTC (permalink / raw)
  To: Robert Milkowski
  Cc: Linux NFS Mailing List, Trond Myklebust, Anna Schumaker, linux-kernel



> 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




^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals
  2019-12-30 15:36 ` Chuck Lever
@ 2020-01-20 17:55   ` Robert Milkowski
  2020-01-22 19:10     ` Schumaker, Anna
  0 siblings, 1 reply; 5+ messages in thread
From: Robert Milkowski @ 2020-01-20 17:55 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust
  Cc: Linux NFS Mailing List, Trond Myklebust, Anna Schumaker, linux-kernel



> -----Original Message-----
> From: Chuck Lever <chuck.lever@oracle.com>
> Sent: 30 December 2019 15:37
> 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
> 
> 
> 
> > 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>
> 
> 

How do we progress it further?

-- 
Robert Milkowski




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals
  2020-01-20 17:55   ` Robert Milkowski
@ 2020-01-22 19:10     ` Schumaker, Anna
  2020-01-22 19:55       ` Robert Milkowski
  0 siblings, 1 reply; 5+ messages in thread
From: Schumaker, Anna @ 2020-01-22 19:10 UTC (permalink / raw)
  To: rmilkowski, chuck.lever, trondmy; +Cc: linux-nfs, linux-kernel, trond.myklebust

Hi Robert,

On Mon, 2020-01-20 at 17:55 +0000, Robert Milkowski wrote:
> > -----Original Message-----
> > From: Chuck Lever <chuck.lever@oracle.com>
> > Sent: 30 December 2019 15:37
> > 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
> > 
> > 
> > 
> > > 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>
> > 
> > 
> 
> How do we progress it further?

Thanks for following up! I have the patch included in my linux-next branch for
the next merge window.

Anna

> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals
  2020-01-22 19:10     ` Schumaker, Anna
@ 2020-01-22 19:55       ` Robert Milkowski
  0 siblings, 0 replies; 5+ messages in thread
From: Robert Milkowski @ 2020-01-22 19:55 UTC (permalink / raw)
  To: Schumaker, Anna, chuck.lever, trondmy
  Cc: linux-nfs, linux-kernel, trond.myklebust



> -----Original Message-----
> From: Schumaker, Anna <Anna.Schumaker@netapp.com>
> Sent: 22 January 2020 19:11
> To: rmilkowski@gmail.com; chuck.lever@oracle.com; trondmy@hammerspace.com
> Cc: linux-nfs@vger.kernel.org; linux-kernel@vger.kernel.org;
> trond.myklebust@hammerspace.com
> Subject: Re: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit
> lease renewals
> 
> Hi Robert,
> 
> On Mon, 2020-01-20 at 17:55 +0000, Robert Milkowski wrote:
> > > -----Original Message-----
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > Sent: 30 December 2019 15:37
> > > 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
> > >
> > >
> > >
> > > > 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>
> > >
> > >
> >
> > How do we progress it further?
> 
> Thanks for following up! I have the patch included in my linux-next branch
> for the next merge window.
> 
> Anna

Nice. Thanks!


-- 
Robert Milkowski



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2020-01-20 17:55   ` Robert Milkowski
2020-01-22 19:10     ` Schumaker, Anna
2020-01-22 19:55       ` Robert Milkowski

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git