| Message ID | 1688006176-32597-3-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: add support for NFSv4.1+ write delegation | expand |
On Wed, Jun 28, 2023 at 07:36:13PM -0700, Dai Ngo wrote: > This patch grants write delegations for OPEN with NFS4_SHARE_ACCESS_WRITE > if there is no conflict with other OPENs. > > Write delegation conflicts with another OPEN, REMOVE, RENAME and SETATTR > are handled the same as read delegation using notify_change, > try_break_deleg. > > The write delegation support is for NFSv4.1+ client only since the NFSv4.0 > Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It > expects the server to look ahead in the compound to find a stateid in order > to determine whether the client that sends the GETATTR is the same client > that holds the write delegation. RFC 7530 spec does not call for the server > to look ahead in order to service the GETATTR op. Here (and the comment below) I would rather state this issue in terms of protocol constraints. "The NFSv4.0 protocol does not enable a server to determine that a conflicting GETATTR originated from the client holding the delegation versus coming from some other client. With NFSv4.1 and later, the SEQUENCE operation that begins each COMPOUND contains a client ID, so delegation recall can be safely squelched in this case. With NFSv4.0, therefore, the server must recall or send a CB_GETATTR (per RFC 7530 Section 16.7.5) even when the GETATTR originates from the client holding that delegation. An NFSv4.0 client can trigger a pathological situation if it always sends a DELEGRETURN preceded by a conflicting GETATTR in the same COMPOUND. COMPOUND execution will always stop at the GETATTR and the DELEGRETURN will never get executed. The server eventually revokes the delegation, which can result in loss of open or lock state." Comments and further edits welcome! > Tracepoint added to track whether read or write delegation is granted. > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++----------- > fs/nfsd/trace.h | 1 + > 2 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 6e61fa3acaf1..f971919b04c7 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh) > > static struct nfs4_delegation * > alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, > - struct nfs4_clnt_odstate *odstate) > + struct nfs4_clnt_odstate *odstate, u32 dl_type) > { > struct nfs4_delegation *dp; > long n; > @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, > INIT_LIST_HEAD(&dp->dl_recall_lru); > dp->dl_clnt_odstate = odstate; > get_clnt_odstate(odstate); > - dp->dl_type = NFS4_OPEN_DELEGATE_READ; > + dp->dl_type = dl_type; > dp->dl_retries = 1; > dp->dl_recalled = false; > nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client, > @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > struct nfs4_delegation *dp; > struct nfsd_file *nf; > struct file_lock *fl; > + u32 dl_type; > > /* > * The fi_had_conflict and nfs_get_existing_delegation checks > @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > if (fp->fi_had_conflict) > return ERR_PTR(-EAGAIN); > > - nf = find_readable_file(fp); > + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > + nf = find_writeable_file(fp); > + dl_type = NFS4_OPEN_DELEGATE_WRITE; > + } else { > + nf = find_readable_file(fp); > + dl_type = NFS4_OPEN_DELEGATE_READ; > + } > if (!nf) { > /* > * We probably could attempt another open and get a read > @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > return ERR_PTR(status); > > status = -ENOMEM; > - dp = alloc_init_deleg(clp, fp, odstate); > + dp = alloc_init_deleg(clp, fp, odstate, dl_type); > if (!dp) > goto out_delegees; > > - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); > + fl = nfs4_alloc_init_lease(dp, dl_type); > if (!fl) > goto out_clnt_odstate; > > @@ -5570,8 +5577,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) > /* > * Attempt to hand out a delegation. > * > - * Note we don't support write delegations, and won't until the vfs has > - * proper support for them. > + * Note we don't support write delegations for NFSv4.0 client since the Linux > + * client behavior is not compliant with RFC 7530 Section 16.7.5 with regard > + * to handle the conflict GETATTR. It expects the server to look ahead in the > + * compound (PUTFH, GETATTR, DELEGRETURN) to find a stateid in order to > + * determine whether the client that sends the GETATTR is the same with the > + * client that holds the write delegation. RFC 7530 spec does not call for > + * the server to look ahead in order to service the conflict GETATTR op. > */ > static void > nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > @@ -5590,8 +5602,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > case NFS4_OPEN_CLAIM_PREVIOUS: > if (!cb_up) > open->op_recall = 1; > - if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ) > - goto out_no_deleg; > break; > case NFS4_OPEN_CLAIM_NULL: > parent = currentfh; > @@ -5606,6 +5616,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > goto out_no_deleg; > if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED)) > goto out_no_deleg; > + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE && > + !clp->cl_minorversion) > + goto out_no_deleg; > break; > default: > goto out_no_deleg; > @@ -5616,8 +5629,13 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > > memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); > > - trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); > - open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; > + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; > + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); > + } else { > + open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; > + trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); > + } > nfs4_put_stid(&dp->dl_stid); > return; > out_no_deleg: > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h > index 72a906a053dc..56f28364cc6b 100644 > --- a/fs/nfsd/trace.h > +++ b/fs/nfsd/trace.h > @@ -607,6 +607,7 @@ DEFINE_STATEID_EVENT(layout_recall_release); > > DEFINE_STATEID_EVENT(open); > DEFINE_STATEID_EVENT(deleg_read); > +DEFINE_STATEID_EVENT(deleg_write); > DEFINE_STATEID_EVENT(deleg_return); > DEFINE_STATEID_EVENT(deleg_recall); > > -- > 2.39.3 >
Thank you Chuck and Jeff for reviewing the patch. On 6/29/23 7:54 AM, Chuck Lever wrote: > On Wed, Jun 28, 2023 at 07:36:13PM -0700, Dai Ngo wrote: >> This patch grants write delegations for OPEN with NFS4_SHARE_ACCESS_WRITE >> if there is no conflict with other OPENs. >> >> Write delegation conflicts with another OPEN, REMOVE, RENAME and SETATTR >> are handled the same as read delegation using notify_change, >> try_break_deleg. >> >> The write delegation support is for NFSv4.1+ client only since the NFSv4.0 >> Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It >> expects the server to look ahead in the compound to find a stateid in order >> to determine whether the client that sends the GETATTR is the same client >> that holds the write delegation. RFC 7530 spec does not call for the server >> to look ahead in order to service the GETATTR op. > Here (and the comment below) I would rather state this issue in > terms of protocol constraints. > > "The NFSv4.0 protocol does not enable a server to determine that a > conflicting GETATTR originated from the client holding the > delegation versus coming from some other client. With NFSv4.1 and > later, the SEQUENCE operation that begins each COMPOUND contains a > client ID, so delegation recall can be safely squelched in this case. > > With NFSv4.0, therefore, the server must recall or send a CB_GETATTR > (per RFC 7530 Section 16.7.5) even when the GETATTR originates from > the client holding that delegation. > > An NFSv4.0 client can trigger a pathological situation if it always > sends a DELEGRETURN preceded by a conflicting GETATTR in the same > COMPOUND. COMPOUND execution will always stop at the GETATTR and the > DELEGRETURN will never get executed. The server eventually revokes > the delegation, which can result in loss of open or lock state." > > Comments and further edits welcome! I will update the comment with this explanation in v7. -Dai > > >> Tracepoint added to track whether read or write delegation is granted. >> >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++----------- >> fs/nfsd/trace.h | 1 + >> 2 files changed, 30 insertions(+), 11 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index 6e61fa3acaf1..f971919b04c7 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh) >> >> static struct nfs4_delegation * >> alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, >> - struct nfs4_clnt_odstate *odstate) >> + struct nfs4_clnt_odstate *odstate, u32 dl_type) >> { >> struct nfs4_delegation *dp; >> long n; >> @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, >> INIT_LIST_HEAD(&dp->dl_recall_lru); >> dp->dl_clnt_odstate = odstate; >> get_clnt_odstate(odstate); >> - dp->dl_type = NFS4_OPEN_DELEGATE_READ; >> + dp->dl_type = dl_type; >> dp->dl_retries = 1; >> dp->dl_recalled = false; >> nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client, >> @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> struct nfs4_delegation *dp; >> struct nfsd_file *nf; >> struct file_lock *fl; >> + u32 dl_type; >> >> /* >> * The fi_had_conflict and nfs_get_existing_delegation checks >> @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> if (fp->fi_had_conflict) >> return ERR_PTR(-EAGAIN); >> >> - nf = find_readable_file(fp); >> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { >> + nf = find_writeable_file(fp); >> + dl_type = NFS4_OPEN_DELEGATE_WRITE; >> + } else { >> + nf = find_readable_file(fp); >> + dl_type = NFS4_OPEN_DELEGATE_READ; >> + } >> if (!nf) { >> /* >> * We probably could attempt another open and get a read >> @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> return ERR_PTR(status); >> >> status = -ENOMEM; >> - dp = alloc_init_deleg(clp, fp, odstate); >> + dp = alloc_init_deleg(clp, fp, odstate, dl_type); >> if (!dp) >> goto out_delegees; >> >> - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); >> + fl = nfs4_alloc_init_lease(dp, dl_type); >> if (!fl) >> goto out_clnt_odstate; >> >> @@ -5570,8 +5577,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) >> /* >> * Attempt to hand out a delegation. >> * >> - * Note we don't support write delegations, and won't until the vfs has >> - * proper support for them. >> + * Note we don't support write delegations for NFSv4.0 client since the Linux >> + * client behavior is not compliant with RFC 7530 Section 16.7.5 with regard >> + * to handle the conflict GETATTR. It expects the server to look ahead in the >> + * compound (PUTFH, GETATTR, DELEGRETURN) to find a stateid in order to >> + * determine whether the client that sends the GETATTR is the same with the >> + * client that holds the write delegation. RFC 7530 spec does not call for >> + * the server to look ahead in order to service the conflict GETATTR op. >> */ >> static void >> nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> @@ -5590,8 +5602,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> case NFS4_OPEN_CLAIM_PREVIOUS: >> if (!cb_up) >> open->op_recall = 1; >> - if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ) >> - goto out_no_deleg; >> break; >> case NFS4_OPEN_CLAIM_NULL: >> parent = currentfh; >> @@ -5606,6 +5616,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> goto out_no_deleg; >> if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED)) >> goto out_no_deleg; >> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE && >> + !clp->cl_minorversion) >> + goto out_no_deleg; >> break; >> default: >> goto out_no_deleg; >> @@ -5616,8 +5629,13 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> >> memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); >> >> - trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); >> - open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; >> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { >> + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; >> + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); >> + } else { >> + open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; >> + trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); >> + } >> nfs4_put_stid(&dp->dl_stid); >> return; >> out_no_deleg: >> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h >> index 72a906a053dc..56f28364cc6b 100644 >> --- a/fs/nfsd/trace.h >> +++ b/fs/nfsd/trace.h >> @@ -607,6 +607,7 @@ DEFINE_STATEID_EVENT(layout_recall_release); >> >> DEFINE_STATEID_EVENT(open); >> DEFINE_STATEID_EVENT(deleg_read); >> +DEFINE_STATEID_EVENT(deleg_write); >> DEFINE_STATEID_EVENT(deleg_return); >> DEFINE_STATEID_EVENT(deleg_recall); >> >> -- >> 2.39.3 >>
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 6e61fa3acaf1..f971919b04c7 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh) static struct nfs4_delegation * alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, - struct nfs4_clnt_odstate *odstate) + struct nfs4_clnt_odstate *odstate, u32 dl_type) { struct nfs4_delegation *dp; long n; @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, INIT_LIST_HEAD(&dp->dl_recall_lru); dp->dl_clnt_odstate = odstate; get_clnt_odstate(odstate); - dp->dl_type = NFS4_OPEN_DELEGATE_READ; + dp->dl_type = dl_type; dp->dl_retries = 1; dp->dl_recalled = false; nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client, @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, struct nfs4_delegation *dp; struct nfsd_file *nf; struct file_lock *fl; + u32 dl_type; /* * The fi_had_conflict and nfs_get_existing_delegation checks @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, if (fp->fi_had_conflict) return ERR_PTR(-EAGAIN); - nf = find_readable_file(fp); + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { + nf = find_writeable_file(fp); + dl_type = NFS4_OPEN_DELEGATE_WRITE; + } else { + nf = find_readable_file(fp); + dl_type = NFS4_OPEN_DELEGATE_READ; + } if (!nf) { /* * We probably could attempt another open and get a read @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, return ERR_PTR(status); status = -ENOMEM; - dp = alloc_init_deleg(clp, fp, odstate); + dp = alloc_init_deleg(clp, fp, odstate, dl_type); if (!dp) goto out_delegees; - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); + fl = nfs4_alloc_init_lease(dp, dl_type); if (!fl) goto out_clnt_odstate; @@ -5570,8 +5577,13 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) /* * Attempt to hand out a delegation. * - * Note we don't support write delegations, and won't until the vfs has - * proper support for them. + * Note we don't support write delegations for NFSv4.0 client since the Linux + * client behavior is not compliant with RFC 7530 Section 16.7.5 with regard + * to handle the conflict GETATTR. It expects the server to look ahead in the + * compound (PUTFH, GETATTR, DELEGRETURN) to find a stateid in order to + * determine whether the client that sends the GETATTR is the same with the + * client that holds the write delegation. RFC 7530 spec does not call for + * the server to look ahead in order to service the conflict GETATTR op. */ static void nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, @@ -5590,8 +5602,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, case NFS4_OPEN_CLAIM_PREVIOUS: if (!cb_up) open->op_recall = 1; - if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ) - goto out_no_deleg; break; case NFS4_OPEN_CLAIM_NULL: parent = currentfh; @@ -5606,6 +5616,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, goto out_no_deleg; if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED)) goto out_no_deleg; + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE && + !clp->cl_minorversion) + goto out_no_deleg; break; default: goto out_no_deleg; @@ -5616,8 +5629,13 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid)); - trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); - open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { + open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE; + trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid); + } else { + open->op_delegate_type = NFS4_OPEN_DELEGATE_READ; + trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid); + } nfs4_put_stid(&dp->dl_stid); return; out_no_deleg: diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h index 72a906a053dc..56f28364cc6b 100644 --- a/fs/nfsd/trace.h +++ b/fs/nfsd/trace.h @@ -607,6 +607,7 @@ DEFINE_STATEID_EVENT(layout_recall_release); DEFINE_STATEID_EVENT(open); DEFINE_STATEID_EVENT(deleg_read); +DEFINE_STATEID_EVENT(deleg_write); DEFINE_STATEID_EVENT(deleg_return); DEFINE_STATEID_EVENT(deleg_recall);
This patch grants write delegations for OPEN with NFS4_SHARE_ACCESS_WRITE if there is no conflict with other OPENs. Write delegation conflicts with another OPEN, REMOVE, RENAME and SETATTR are handled the same as read delegation using notify_change, try_break_deleg. The write delegation support is for NFSv4.1+ client only since the NFSv4.0 Linux client behavior is not compliant with RFC 7530 Section 16.7.5. It expects the server to look ahead in the compound to find a stateid in order to determine whether the client that sends the GETATTR is the same client that holds the write delegation. RFC 7530 spec does not call for the server to look ahead in order to service the GETATTR op. Tracepoint added to track whether read or write delegation is granted. Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++----------- fs/nfsd/trace.h | 1 + 2 files changed, 30 insertions(+), 11 deletions(-)