diff mbox series

[v7,4/4] NFSD: Enable write delegation support for NFSv4.1+ client

Message ID 1688089960-24568-5-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

Commit Message

Dai Ngo June 30, 2023, 1:52 a.m. UTC 
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 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.

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

Comments

Chuck Lever June 30, 2023, 3:40 p.m. UTC | #1 
> On Jun 29, 2023, at 9:52 PM, Dai Ngo <dai.ngo@oracle.com> 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 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.
> 
> 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 4c96cfe19531..15b5043eeca5 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.

I'll take these 4 for v6.6, and update this comment to match the patch
description as I apply the patches.


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

--
Chuck Lever
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4c96cfe19531..15b5043eeca5 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);