Message ID | 1741120693-2517-2-git-send-email-dai.ngo@oracle.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Chuck Lever |
Headers | show |
Series | NFSD: offer write delegation for OPEN with OPEN4_SHARE_ACCESS only | expand |
On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote: > RFC8881, section 9.1.2 says: > > "In the case of READ, the server may perform the corresponding > check on the access mode, or it may choose to allow READ for > OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE > implementation may unavoidably do (e.g., due to buffer cache > constraints)." > > and in section 10.4.1: > "Similarly, when closing a file opened for OPEN4_SHARE_ACCESS_WRITE/ > OPEN4_SHARE_ACCESS_BOTH and if an OPEN_DELEGATE_WRITE delegation > is in effect" > > This patch offers write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE > only. Also deleted no longer use find_rw_file(). > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > fs/nfsd/nfs4state.c | 34 +++++++++++++--------------------- > 1 file changed, 13 insertions(+), 21 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 0f97f2c62b3a..b533225e57cf 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -633,18 +633,6 @@ find_readable_file(struct nfs4_file *f) > return ret; > } > > -static struct nfsd_file * > -find_rw_file(struct nfs4_file *f) > -{ > - struct nfsd_file *ret; > - > - spin_lock(&f->fi_lock); > - ret = nfsd_file_get(f->fi_fds[O_RDWR]); > - spin_unlock(&f->fi_lock); > - > - return ret; > -} > - > struct nfsd_file * > find_any_file(struct nfs4_file *f) > { > @@ -5382,7 +5370,6 @@ static int nfsd4_cb_recall_done(struct nfsd4_callback *cb, > if (dp->dl_stid.sc_status) > /* CLOSED or REVOKED */ > return 1; > - > switch (task->tk_status) { > case 0: > return 1; > @@ -5987,14 +5974,19 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, > * "An OPEN_DELEGATE_WRITE delegation allows the client to handle, > * on its own, all opens." > * > - * Furthermore the client can use a write delegation for most READ > - * operations as well, so we require a O_RDWR file here. > + * Furthermore, section 9.1.2 says: > + * > + * "In the case of READ, the server may perform the corresponding > + * check on the access mode, or it may choose to allow READ for > + * OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE > + * implementation may unavoidably do (e.g., due to buffer cache > + * constraints)." > * > - * Offer a write delegation in the case of a BOTH open, and ensure > - * we get the O_RDWR descriptor. > + * We choose to offer a write delegation for OPEN with the > + * OPEN4_SHARE_ACCESS_WRITE access mode to accommodate such clients. > */ > - if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) { > - nf = find_rw_file(fp); > + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { > + nf = find_writeable_file(fp); > dl_type = deleg_ts ? OPEN_DELEGATE_WRITE_ATTRS_DELEG : OPEN_DELEGATE_WRITE; > } > > @@ -6116,7 +6108,7 @@ static bool > nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, > struct kstat *stat) > { > - struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); > + struct nfsd_file *nf = find_writeable_file(dp->dl_stid.sc_file); > struct path path; > int rc; > > @@ -7063,7 +7055,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate, > return_revoked = true; > if (typemask & SC_TYPE_DELEG) > /* Always allow REVOKED for DELEG so we can > - * retturn the appropriate error. > + * return the appropriate error. > */ > statusmask |= SC_STATUS_REVOKED; > This patch also looks good. The only other issue I have with this is the patch ordering. If a bisect lands between these two patches then delegations won't work quite right. Is there a reason to order the patches this way? Reviewed-by: Jeff Layton <jlayton@kernel.org>
On 3/5/25 9:36 AM, Jeff Layton wrote: > On Tue, 2025-03-04 at 12:38 -0800, Dai Ngo wrote: >> RFC8881, section 9.1.2 says: >> >> "In the case of READ, the server may perform the corresponding >> check on the access mode, or it may choose to allow READ for >> OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE >> implementation may unavoidably do (e.g., due to buffer cache >> constraints)." >> >> and in section 10.4.1: >> "Similarly, when closing a file opened for OPEN4_SHARE_ACCESS_WRITE/ >> OPEN4_SHARE_ACCESS_BOTH and if an OPEN_DELEGATE_WRITE delegation >> is in effect" >> >> This patch offers write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE >> only. Also deleted no longer use find_rw_file(). >> >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> fs/nfsd/nfs4state.c | 34 +++++++++++++--------------------- >> 1 file changed, 13 insertions(+), 21 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index 0f97f2c62b3a..b533225e57cf 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -633,18 +633,6 @@ find_readable_file(struct nfs4_file *f) >> return ret; >> } >> >> -static struct nfsd_file * >> -find_rw_file(struct nfs4_file *f) >> -{ >> - struct nfsd_file *ret; >> - >> - spin_lock(&f->fi_lock); >> - ret = nfsd_file_get(f->fi_fds[O_RDWR]); >> - spin_unlock(&f->fi_lock); >> - >> - return ret; >> -} >> - >> struct nfsd_file * >> find_any_file(struct nfs4_file *f) >> { >> @@ -5382,7 +5370,6 @@ static int nfsd4_cb_recall_done(struct nfsd4_callback *cb, >> if (dp->dl_stid.sc_status) >> /* CLOSED or REVOKED */ >> return 1; >> - >> switch (task->tk_status) { >> case 0: >> return 1; >> @@ -5987,14 +5974,19 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, >> * "An OPEN_DELEGATE_WRITE delegation allows the client to handle, >> * on its own, all opens." >> * >> - * Furthermore the client can use a write delegation for most READ >> - * operations as well, so we require a O_RDWR file here. >> + * Furthermore, section 9.1.2 says: >> + * >> + * "In the case of READ, the server may perform the corresponding >> + * check on the access mode, or it may choose to allow READ for >> + * OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE >> + * implementation may unavoidably do (e.g., due to buffer cache >> + * constraints)." >> * >> - * Offer a write delegation in the case of a BOTH open, and ensure >> - * we get the O_RDWR descriptor. >> + * We choose to offer a write delegation for OPEN with the >> + * OPEN4_SHARE_ACCESS_WRITE access mode to accommodate such clients. >> */ >> - if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) { >> - nf = find_rw_file(fp); >> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { >> + nf = find_writeable_file(fp); >> dl_type = deleg_ts ? OPEN_DELEGATE_WRITE_ATTRS_DELEG : OPEN_DELEGATE_WRITE; >> } >> >> @@ -6116,7 +6108,7 @@ static bool >> nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, >> struct kstat *stat) >> { >> - struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); >> + struct nfsd_file *nf = find_writeable_file(dp->dl_stid.sc_file); >> struct path path; >> int rc; >> >> @@ -7063,7 +7055,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate, >> return_revoked = true; >> if (typemask & SC_TYPE_DELEG) >> /* Always allow REVOKED for DELEG so we can >> - * retturn the appropriate error. >> + * return the appropriate error. >> */ >> statusmask |= SC_STATUS_REVOKED; >> > > This patch also looks good. > > The only other issue I have with this is the patch ordering. If a > bisect lands between these two patches then delegations won't work > quite right. Is there a reason to order the patches this way? I also wondered about the patch order for the same reason. > Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 0f97f2c62b3a..b533225e57cf 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -633,18 +633,6 @@ find_readable_file(struct nfs4_file *f) return ret; } -static struct nfsd_file * -find_rw_file(struct nfs4_file *f) -{ - struct nfsd_file *ret; - - spin_lock(&f->fi_lock); - ret = nfsd_file_get(f->fi_fds[O_RDWR]); - spin_unlock(&f->fi_lock); - - return ret; -} - struct nfsd_file * find_any_file(struct nfs4_file *f) { @@ -5382,7 +5370,6 @@ static int nfsd4_cb_recall_done(struct nfsd4_callback *cb, if (dp->dl_stid.sc_status) /* CLOSED or REVOKED */ return 1; - switch (task->tk_status) { case 0: return 1; @@ -5987,14 +5974,19 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp, * "An OPEN_DELEGATE_WRITE delegation allows the client to handle, * on its own, all opens." * - * Furthermore the client can use a write delegation for most READ - * operations as well, so we require a O_RDWR file here. + * Furthermore, section 9.1.2 says: + * + * "In the case of READ, the server may perform the corresponding + * check on the access mode, or it may choose to allow READ for + * OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE + * implementation may unavoidably do (e.g., due to buffer cache + * constraints)." * - * Offer a write delegation in the case of a BOTH open, and ensure - * we get the O_RDWR descriptor. + * We choose to offer a write delegation for OPEN with the + * OPEN4_SHARE_ACCESS_WRITE access mode to accommodate such clients. */ - if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) { - nf = find_rw_file(fp); + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) { + nf = find_writeable_file(fp); dl_type = deleg_ts ? OPEN_DELEGATE_WRITE_ATTRS_DELEG : OPEN_DELEGATE_WRITE; } @@ -6116,7 +6108,7 @@ static bool nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh, struct kstat *stat) { - struct nfsd_file *nf = find_rw_file(dp->dl_stid.sc_file); + struct nfsd_file *nf = find_writeable_file(dp->dl_stid.sc_file); struct path path; int rc; @@ -7063,7 +7055,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate, return_revoked = true; if (typemask & SC_TYPE_DELEG) /* Always allow REVOKED for DELEG so we can - * retturn the appropriate error. + * return the appropriate error. */ statusmask |= SC_STATUS_REVOKED;
RFC8881, section 9.1.2 says: "In the case of READ, the server may perform the corresponding check on the access mode, or it may choose to allow READ for OPEN4_SHARE_ACCESS_WRITE, to accommodate clients whose WRITE implementation may unavoidably do (e.g., due to buffer cache constraints)." and in section 10.4.1: "Similarly, when closing a file opened for OPEN4_SHARE_ACCESS_WRITE/ OPEN4_SHARE_ACCESS_BOTH and if an OPEN_DELEGATE_WRITE delegation is in effect" This patch offers write delegation for OPEN with OPEN4_SHARE_ACCESS_WRITE only. Also deleted no longer use find_rw_file(). Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/nfsd/nfs4state.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)