| Message ID | 001901d5ba67$8954ede0$9bfec9a0$@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease | expand |
Hi Robert- > On Dec 24, 2019, at 9:36 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. In addition to stating the bug symptoms, IMO you need Fixes: 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing leases") And this description needs to explain how 83ca7f5ab31f broke things. > Signed-off-by: Robert Milkowski <rmilkowski@gmail.com> > --- > fs/nfs/nfs4proc.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 76d3716..b6cad9a 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -5016,19 +5016,23 @@ static int _nfs4_do_fsinfo(struct nfs_server *server, struct nfs_fh *fhandle, > > 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_exception exception = { > .interruptible = true, > }; > - unsigned long now = jiffies; > + unsigned long last_renewal = jiffies; > int err; > > do { > err = _nfs4_do_fsinfo(server, fhandle, fsinfo); > trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err); There are two usual practices to introduce behavior that diverges amongst NFSv4 minor versions. Neither practice is followed here. - The difference is added to the XDR encoder and decoder. You could do that by adding a RENEW to the COMPOUND in the NFSv4.0 case. - The function is converted to a virtual function which is added to struct nfs4_minor_version_ops. Prior to 83ca7f5ab31f, IIRC this function was part of a code path that did actually renew the lease. It seems to me that the proper fix here is to make NFSv4.0 renew the lease, not the other way around. Is there a reason not to add a RENEW to the NFSv4.0 version of the fsinfo COMPOUND? > if (err == 0) { > - nfs4_set_lease_period(server->nfs_client, > + /* no implicit lease renewal allowed here for v4.0 */ > + if (clp->cl_minorversion == 0 && clp->cl_last_renewal != 0) > + last_renewal = clp->cl_last_renewal; > + nfs4_set_lease_period(clp, > fsinfo->lease_time * HZ, > - now); > + last_renewal); > break; > } > err = nfs4_handle_exception(server, err, &exception); > -- > 1.8.3.1 -- Chuck Lever
Hi Chuck, > > On Dec 24, 2019, at 9:36 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. > > In addition to stating the bug symptoms, IMO you need > > Fixes: 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing leases") > Right, makes sense. I will include it in the next patch revision. Thans. > And this description needs to explain how 83ca7f5ab31f broke things. Is adding the below to the previous description enough? The 83ca7f5ab31f introduced implicit renew operation when calling nfs4_do_fsinfo(), which is not done by NFS servers which under some circumstances results in nfsv4.0 lease to expire on a server side which then will return NFS4ERR_EXPIRED or NFS4ERR_STALE_CLIENTID. This can be easily reproduced by frequently unmounting a sub-mount over nfsv4.0, which after a grace period will result in an error returned by a server when accessing a file. This can also happen after a sub-mount is automatically unmounted due to inactivity (after nfs_mountpoint_expiry_timeout), then the sub-mount is stat'ed causing it to be mounted again, and a file is accessed shortly after (this all depends on specific grace time, last RENEW, etc.). This specific case was observed on a production systems. > > There are two usual practices to introduce behavior that diverges > amongst NFSv4 minor versions. Neither practice is followed here. > > - The difference is added to the XDR encoder and decoder. You could > do that by adding a RENEW to the COMPOUND in the NFSv4.0 case. > > - The function is converted to a virtual function which is added to > struct nfs4_minor_version_ops. > Thanks for the explanation, I'm learning here and really do appreciate any help. So given the above advise I ended up with the below: 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 { Let me know if that's what you had it mind and if no further comments I will finish testing and submit new patch. > Prior to 83ca7f5ab31f, IIRC this function was part of a code path > that did actually renew the lease. It seems to me that the proper > fix here is to make NFSv4.0 renew the lease, not the other way > around. Is there a reason not to add a RENEW to the NFSv4.0 version > of the fsinfo COMPOUND? > Strictly speaking I don't think renew is required here, but adding it as part of the compound operation is harmless and more in-line with how it is currently done for v4.1. Also, before the 83ca7f5ab31f, implicit lease renewal was only done in nfs4_proc_setclientid_confirm(), but the function is not called when mounting a sub-mount, and it was not done in nfs4_do_fsinfo() either. The implicit renewal in nfs4_do_fsinfo() when mounting each submount was introduced by the commit, before it only happened on root mount. So this particular issue I'm trying to fix here did not occur before the change, I think. (btw: according to RFC7530 section 9.5. the implicit renewal in setclientid_confirm() wasn't correct either but I think it was harmless).
> On Dec 27, 2019, at 2:07 PM, Robert Milkowski <rmilkowski@gmail.com> wrote: > > Hi Chuck, > >>> On Dec 24, 2019, at 9:36 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. >> >> In addition to stating the bug symptoms, IMO you need >> >> Fixes: 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing leases") >> > > Right, makes sense. I will include it in the next patch revision. > Thans. > > >> And this description needs to explain how 83ca7f5ab31f broke things. > > Is adding the below to the previous description enough? > > The 83ca7f5ab31f introduced implicit renew operation when calling nfs4_do_fsinfo(), > which is not done by NFS servers which under some circumstances results in nfsv4.0 lease > to expire on a server side which then will return NFS4ERR_EXPIRED or NFS4ERR_STALE_CLIENTID. > This can be easily reproduced by frequently unmounting a sub-mount over nfsv4.0, > which after a grace period will result in an error returned by a server when accessing a file. > This can also happen after a sub-mount is automatically unmounted due to inactivity > (after nfs_mountpoint_expiry_timeout), then the sub-mount is stat'ed causing it to be mounted again, > and a file is accessed shortly after (this all depends on specific grace time, last RENEW, etc.). > This specific case was observed on a production systems. Wrap at 72, but OK. Some prefer short descriptions, but I like to have enough bread crumbs to find my way back to the purpose of a commit when I've forgotten it 6 months from now. A timing-related failure is obnoxious, so this explanation is going to also help sustaining engineers locate this fix quickly if needed. >> There are two usual practices to introduce behavior that diverges >> amongst NFSv4 minor versions. Neither practice is followed here. >> >> - The difference is added to the XDR encoder and decoder. You could >> do that by adding a RENEW to the COMPOUND in the NFSv4.0 case. >> >> - The function is converted to a virtual function which is added to >> struct nfs4_minor_version_ops. >> > > Thanks for the explanation, I'm learning here and really do appreciate any help. > So given the above advise I ended up with the below: > > > 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, Urgh. I wish there was a better way to do this, but it will do for now. > }; > 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 { > > > > Let me know if that's what you had it mind and if no further comments I will finish testing and submit new patch. Interesting to test what happens if: a) the server fails the COMPOUND before getting to the RENEW? b) the RENEW itself fails; does the client correctly perform state recovery in that case? >> Prior to 83ca7f5ab31f, IIRC this function was part of a code path >> that did actually renew the lease. It seems to me that the proper >> fix here is to make NFSv4.0 renew the lease, not the other way >> around. Is there a reason not to add a RENEW to the NFSv4.0 version >> of the fsinfo COMPOUND? >> > > Strictly speaking I don't think renew is required here, That's correct, a RENEW is not required by the protocol. Our client implementation assumes that the lease is renewed, however, and appending a RENEW is a valid way to ensure that happens with NFSv4.0. > but adding it as part of the compound > operation is harmless and more in-line with how it is currently done for v4.1. > > Also, before the 83ca7f5ab31f, implicit lease renewal was only done in nfs4_proc_setclientid_confirm(), > but the function is not called when mounting a sub-mount, and it was not done in nfs4_do_fsinfo() either. > The implicit renewal in nfs4_do_fsinfo() when mounting each submount was introduced by the commit, before it only happened on root > mount. > So this particular issue I'm trying to fix here did not occur before the change, I think. Does that alter your explanation in the patch description? Does 83ca7f5 make things worse? > (btw: according to RFC7530 section 9.5. the implicit renewal in setclientid_confirm() wasn't correct either but I think it was > harmless). Indeed, the last paragraph of 16.34.5 clearly states: > SETCLIENTID_CONFIRM does not establish or renew a lease. -- Chuck Lever
> > Wrap at 72, but OK. Some prefer short descriptions, but I like to > have enough bread crumbs to find my way back to the purpose of a > commit when I've forgotten it 6 months from now. > > A timing-related failure is obnoxious, so this explanation is going > to also help sustaining engineers locate this fix quickly if needed. Agree. > Interesting to test what happens if: > > a) the server fails the COMPOUND before getting to the RENEW? There is no change in behaviour in this case. The nfs4_xdr_dec_fsinfo() returns first error without decoding any further operations in the compound. Then depending on the error we might end up retrying in nfs4_do_fsinfo(): ... do { err = _nfs4_do_fsinfo(server, fhandle, fsinfo); trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err); if (err == 0) { nfs4_set_lease_period(server->nfs_client, fsinfo->lease_time * HZ, now); break; } err = nfs4_handle_exception(server, err, &exception); } while (exception.retry); return err; ... For example, currently it will retry with RENEW if it got NFS4ERR_STALE_CLIENTID from any operations in the compound op here. However, it won't do so for NFS4ERR_EXPIRED currently - this is another bug I'm addressing via a different patch (it affects not just RENEW but other operations like open(), etc.), see email: [PATCH] NFSv4: open() should try lease recovery on NFS4ERR_EXPIRED I will be sending updated patch there in coming days as well. In this specific case when mounting a sub-mount and NFS4ERR_EXPIRED is returned by server we fail to mount, with or without my patch. > > b) the RENEW itself fails; does the client correctly perform state > recovery in that case? > Same as above really, as we don't care in this case which operation in the compound returned an error - we start error handling on first operation which errored out without checking the others. I did check that we handle correctly NFS4ERR_STALE_CLIENTID for example (we re-issue RENEW), but we do not NFS4ERR_EXPIRED correctly here. I don't think it matters in regards to the patch discussed here, as if NFS4ERR_EXPIRERED is returned, without this patch we would end up mounting a sub-mount but erroring out on any file access (like open()), while with the patch we won't mount a sub-mount and still return same error to open(). To fix it, we need the other mentioned patch, which is orthogonal here. However, there is an improvement here by having the patch discussed here for the nfs4_do_fsinfo() - without the patch we definitely do end up with expired lease situation as described which does result in NFS4ERR_EXPIRED returned by some nfs servers which currently translates to EIO returned by OPEN(), while with the patch this specific condition will not happen in the first place, therefor NFS4ERR_EXPIRED won't be returned here and we won't hit the other issue. > > Also, before the 83ca7f5ab31f, implicit lease renewal was only done in > nfs4_proc_setclientid_confirm(), > > but the function is not called when mounting a sub-mount, and it was not done in nfs4_do_fsinfo() > either. > > The implicit renewal in nfs4_do_fsinfo() when mounting each submount was introduced by the commit, > before it only happened on root > > mount. > > So this particular issue I'm trying to fix here did not occur before the change, I think. > > Does that alter your explanation in the patch description? No, I don't think so. > Does 83ca7f5 make things worse? What I meant was that it is the 83ca7f5 commit which introduced the issue by assuming implicit renewal in a different code path which is called on every mount (sub-mount or not). Before the commit although there was an implicit renewal in nfs4_proc_setclientid_confirm(), I believe it was harmless as this one is essentially only called if clientid is not set yet, in which case a server would just set last_renewal as well. What the 83ca7f5 commit introduced is an implicit renew in do_fsinfo() which is called on every mount of a sub-mount, in which case both sides (client and server) already have clientid established, so what it does is it delays RENEW op being send by client which in turn means there is a short window during which lease actually expires from server but not client point of view. In extreme case when one mounts/unmounts a sub-mount RENEW operation is not being send at all. Let me describe the issue again with an example timeline. At some point /mnt/fs1 is mounted over nfsv4.0, then RENEW is issued every 60s by a client (assuming server has grace period set to 90s and no other nfs operations are being send, in which case Linux sends RENEW every 2/3 of 90 = 60s). Let's pick up one such RENEW and assume time 0: 0 RENEW 60 RENEW 120 RENEW 178 stat /mnt/fs1/submount Results in /mnt/fs1/submount being mounted Linux client wrongly assumes implicit lease renewal in nfs4_do_fsinfo() (introduced by 83ca7f5), but server does not 210 lease expires from server perspective (120 + 90) 215 open(/mnt/fs1/submount/f1) results in NFS4ERR_EXPIRED returned by server and EIO returned to an application (due to the other bug mentioned above) 238 next RENEW scheduled by client Server returns NFS4ERR_EXPIRED Client tries again and then recovers via SETCLIENTID So in the above case, there is a window of 28s (238-210) where all open() calls (and many other) will fail. The discussed patch here makes the implicit RENEW at 178 into an explicit one (or originally in v1 version of the patch I removed the implicit renewal here altogether), so now both server and client renew the lease and there won't be a window where we let the lease expire. At 215 the open() fails to recover due to bug in nfs4_do_handle_exception() as briefly discussed above and in more detail in the separate email/patch, while at 238 RENEW recovers from NFS4ERR_EXPIRED as error handling is done in nfs4_renew_done() which does the right thing by calling nfs4_schedule_lease_recovery().
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 76d3716..b6cad9a 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -5016,19 +5016,23 @@ static int _nfs4_do_fsinfo(struct nfs_server *server, struct nfs_fh *fhandle, 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_exception exception = { .interruptible = true, }; - unsigned long now = jiffies; + unsigned long last_renewal = jiffies; int err; do { err = _nfs4_do_fsinfo(server, fhandle, fsinfo); trace_nfs4_fsinfo(server, fhandle, fsinfo->fattr, err); if (err == 0) { - nfs4_set_lease_period(server->nfs_client, + /* no implicit lease renewal allowed here for v4.0 */ + if (clp->cl_minorversion == 0 && clp->cl_last_renewal != 0) + last_renewal = clp->cl_last_renewal; + nfs4_set_lease_period(clp, fsinfo->lease_time * HZ, - now); + last_renewal); break; } err = nfs4_handle_exception(server, err, &exception);