[PATCH v4 00/10] NFSD support for asynchronous COPY

[PATCH v4 00/10] NFSD support for asynchronous COPY

[Olga Kornievskaia]

To do asynchronous copies, NFSD creates a single threaded workqueue
and does not tie up an NFSD thread to complete the copy. Upon receiving
the COPY, it generates a unique copy stateid (stores a global list
for keeping track of state for OFFLOAD_STATUS to be queried by),
queues up a workqueue for the copy, and replies back to the client.
nfsd4_copy arguments that are allocated on the stack are copied for
the work item.

In the async copy handler, it calls into VFS copy_file_range() with
4MB chunks and loops until it completes the requested copy size. If
error is encountered it's saved but also we save the amount of data
copied so far. Once done, the results are queued for the callback
workqueue and sent via CB_OFFLOAD. Also currently, choosing to clean
up the copy state information stored in the global list when cope is
done and not doing it when callback's release function (it could be
done there alternatively if needed it?).

When the source server received an OFFLOAD_CANCEL, it will remove the
stateid from the global list and mark the copy cancelled. If its
cancelled we are choosing not to send the CB_OFFLOAD back to the client.

Olga Kornievskaia (10):
  NFSD CB_OFFLOAD xdr
  NFSD OFFLOAD_STATUS xdr
  NFSD OFFLOAD_CANCEL xdr
  NFSD xdr callback stateid in async COPY reply
  NFSD first draft of async copy
  NFSD return nfs4_stid in nfs4_preprocess_stateid_op
  NFSD create new stateid for async copy
  NFSD handle OFFLOAD_CANCEL op
  NFSD support OFFLOAD_STATUS
  NFSD stop queued async copies on client shutdown

 fs/nfsd/netns.h        |   8 ++
 fs/nfsd/nfs4callback.c |  97 +++++++++++++++++
 fs/nfsd/nfs4proc.c     | 276 ++++++++++++++++++++++++++++++++++++++++++++-----
 fs/nfsd/nfs4state.c    | 100 +++++++++++++++++-
 fs/nfsd/nfs4xdr.c      |  53 ++++++++--
 fs/nfsd/nfsctl.c       |   1 +
 fs/nfsd/state.h        |  27 ++++-
 fs/nfsd/xdr4.h         |  25 +++++
 fs/nfsd/xdr4cb.h       |  10 ++
 9 files changed, 559 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[PATCH v4 01/10] NFSD CB_OFFLOAD xdr

[Olga Kornievskaia]

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4callback.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/state.h        |  1 +
 fs/nfsd/xdr4.h         |  6 ++++
 fs/nfsd/xdr4cb.h       | 10 ++++++
 4 files changed, 114 insertions(+)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 49b0a9e..d12d914 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -39,6 +39,7 @@
 #include "state.h"
 #include "netns.h"
 #include "xdr4cb.h"
+#include "xdr4.h"
 
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
@@ -105,6 +106,7 @@ enum nfs_cb_opnum4 {
 	OP_CB_WANTS_CANCELLED		= 12,
 	OP_CB_NOTIFY_LOCK		= 13,
 	OP_CB_NOTIFY_DEVICEID		= 14,
+	OP_CB_OFFLOAD			= 15,
 	OP_CB_ILLEGAL			= 10044
 };
 
@@ -683,6 +685,100 @@ static int nfs4_xdr_dec_cb_notify_lock(struct rpc_rqst *rqstp,
 }
 
 /*
+ * struct write_response4 {
+ *	stateid4	wr_callback_id<1>;
+ *	length4		wr_count;
+ *	stable_how4	wr_committed;
+ *	verifier4	wr_writeverf;
+ * };
+ * union offload_info4 switch (nfsstat4 coa_status) {
+ *	case NFS4_OK:
+ *		write_response4	coa_resok4;
+ *	default:
+ *	length4		coa_bytes_copied;
+ * };
+ * struct CB_OFFLOAD4args {
+ *	nfs_fh4		coa_fh;
+ *	stateid4	coa_stateid;
+ *	offload_info4	coa_offload_info;
+ * };
+ */
+static void encode_offload_info4(struct xdr_stream *xdr,
+				 __be32 nfserr,
+				 const struct nfsd4_copy *cp)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, 4);
+	*p++ = nfserr;
+	if (!nfserr) {
+		p = xdr_reserve_space(xdr, 4 + 8 + 4 + NFS4_VERIFIER_SIZE);
+		p = xdr_encode_empty_array(p);
+		p = xdr_encode_hyper(p, cp->cp_res.wr_bytes_written);
+		*p++ = cpu_to_be32(cp->cp_res.wr_stable_how);
+		p = xdr_encode_opaque_fixed(p, cp->cp_res.wr_verifier.data,
+					    NFS4_VERIFIER_SIZE);
+	} else {
+		p = xdr_reserve_space(xdr, 8);
+		p = xdr_encode_hyper(p, cp->cp_res.wr_bytes_written);
+	}
+}
+
+static void encode_cb_offload4args(struct xdr_stream *xdr,
+				   __be32 nfserr,
+				   const struct knfsd_fh *fh,
+				   const struct nfsd4_copy *cp,
+				   struct nfs4_cb_compound_hdr *hdr)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, 4);
+	*p++ = cpu_to_be32(OP_CB_OFFLOAD);
+	encode_nfs_fh4(xdr, fh);
+	encode_stateid4(xdr, &cp->cp_res.cb_stateid);
+	encode_offload_info4(xdr, nfserr, cp);
+
+	hdr->nops++;
+}
+
+static void nfs4_xdr_enc_cb_offload(struct rpc_rqst *req,
+				    struct xdr_stream *xdr,
+				    const void *data)
+{
+	const struct nfsd4_callback *cb = data;
+	const struct nfsd4_copy *cp =
+		container_of(cb, struct nfsd4_copy, cp_cb);
+	struct nfs4_cb_compound_hdr hdr = {
+		.ident = 0,
+		.minorversion = cb->cb_clp->cl_minorversion,
+	};
+
+	encode_cb_compound4args(xdr, &hdr);
+	encode_cb_sequence4args(xdr, cb, &hdr);
+	encode_cb_offload4args(xdr, cp->nfserr, &cp->fh, cp, &hdr);
+	encode_cb_nops(&hdr);
+}
+
+static int nfs4_xdr_dec_cb_offload(struct rpc_rqst *rqstp,
+				   struct xdr_stream *xdr,
+				   void *data)
+{
+	struct nfsd4_callback *cb = data;
+	struct nfs4_cb_compound_hdr hdr;
+	int status;
+
+	status = decode_cb_compound4res(xdr, &hdr);
+	if (unlikely(status))
+		return status;
+
+	if (cb) {
+		status = decode_cb_sequence4res(xdr, cb);
+		if (unlikely(status || cb->cb_seq_status))
+			return status;
+	}
+	return decode_cb_op_status(xdr, OP_CB_OFFLOAD, &cb->cb_status);
+}
+/*
  * RPC procedure tables
  */
 #define PROC(proc, call, argtype, restype)				\
@@ -703,6 +799,7 @@ static int nfs4_xdr_dec_cb_notify_lock(struct rpc_rqst *rqstp,
 	PROC(CB_LAYOUT,	COMPOUND,	cb_layout,	cb_layout),
 #endif
 	PROC(CB_NOTIFY_LOCK,	COMPOUND,	cb_notify_lock,	cb_notify_lock),
+	PROC(CB_OFFLOAD,	COMPOUND,	cb_offload,	cb_offload),
 };
 
 static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 005c911..f8b0210 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -570,6 +570,7 @@ enum nfsd4_cb_op {
 	NFSPROC4_CLNT_CB_NULL = 0,
 	NFSPROC4_CLNT_CB_RECALL,
 	NFSPROC4_CLNT_CB_LAYOUT,
+	NFSPROC4_CLNT_CB_OFFLOAD,
 	NFSPROC4_CLNT_CB_SEQUENCE,
 	NFSPROC4_CLNT_CB_NOTIFY_LOCK,
 };
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 1e4edbf..4ac2676 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -507,6 +507,7 @@ struct nfsd42_write_res {
 	u64			wr_bytes_written;
 	u32			wr_stable_how;
 	nfs4_verifier		wr_verifier;
+	stateid_t		cb_stateid;
 };
 
 struct nfsd4_copy {
@@ -523,6 +524,11 @@ struct nfsd4_copy {
 
 	/* response */
 	struct nfsd42_write_res	cp_res;
+
+	/* for cb_offload */
+	struct nfsd4_callback	cp_cb;
+	__be32			nfserr;
+	struct knfsd_fh		fh;
 };
 
 struct nfsd4_seek {
diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
index 49b719d..7e39913 100644
--- a/fs/nfsd/xdr4cb.h
+++ b/fs/nfsd/xdr4cb.h
@@ -37,3 +37,13 @@
 #define NFS4_dec_cb_notify_lock_sz	(cb_compound_dec_hdr_sz  +      \
 					cb_sequence_dec_sz +            \
 					op_dec_sz)
+#define enc_cb_offload_info_sz		(1 + 1 + 2 + 1 +		\
+					XDR_QUADLEN(NFS4_VERIFIER_SIZE))
+#define NFS4_enc_cb_offload_sz		(cb_compound_enc_hdr_sz +       \
+					cb_sequence_enc_sz +            \
+					enc_nfs4_fh_sz +		\
+					enc_stateid_sz +		\
+					enc_cb_offload_info_sz)
+#define NFS4_dec_cb_offload_sz		(cb_compound_dec_hdr_sz  +      \
+					cb_sequence_dec_sz +            \
+					op_dec_sz)
-- 
1.8.3.1

[PATCH v4 02/10] NFSD OFFLOAD_STATUS xdr

[Olga Kornievskaia]

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c | 20 ++++++++++++++++++++
 fs/nfsd/nfs4xdr.c  | 30 ++++++++++++++++++++++++++++--
 fs/nfsd/xdr4.h     | 10 ++++++++++
 3 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 3c69db7..8601fc4 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1142,6 +1142,13 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 	fput(file);
 	return status;
 }
+static __be32
+nfsd4_offload_status(struct svc_rqst *rqstp,
+		     struct nfsd4_compound_state *cstate,
+		     union nfsd4_op_u *u)
+{
+	return nfserr_notsupp;
+}
 
 static __be32
 nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
@@ -2039,6 +2046,14 @@ static inline u32 nfsd4_copy_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 		1 /* cr_synchronous */) * sizeof(__be32);
 }
 
+static inline u32 nfsd4_offload_status_rsize(struct svc_rqst *rqstp,
+					     struct nfsd4_op *op)
+{
+	return (op_encode_hdr_size +
+		2 /* osr_count */ +
+		1 /* osr_complete<1> optional 0 for now */) * sizeof(__be32);
+}
+
 #ifdef CONFIG_NFSD_PNFS
 static inline u32 nfsd4_getdeviceinfo_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 {
@@ -2452,6 +2467,11 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 		.op_name = "OP_SEEK",
 		.op_rsize_bop = nfsd4_seek_rsize,
 	},
+	[OP_OFFLOAD_STATUS] = {
+		.op_func = nfsd4_offload_status,
+		.op_name = "OP_OFFLOAD_STATUS",
+		.op_rsize_bop = nfsd4_offload_status_rsize,
+	},
 };
 
 /**
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 2c61c6b..ed8b61f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1768,6 +1768,13 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
 }
 
 static __be32
+nfsd4_decode_offload_status(struct nfsd4_compoundargs *argp,
+			    struct nfsd4_offload_status *os)
+{
+	return nfsd4_decode_stateid(argp, &os->stateid);
+}
+
+static __be32
 nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
 {
 	DECODE_HEAD;
@@ -1874,7 +1881,7 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
 	[OP_LAYOUTERROR]	= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_LAYOUTSTATS]	= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_OFFLOAD_CANCEL]	= (nfsd4_dec)nfsd4_decode_notsupp,
-	[OP_OFFLOAD_STATUS]	= (nfsd4_dec)nfsd4_decode_notsupp,
+	[OP_OFFLOAD_STATUS]	= (nfsd4_dec)nfsd4_decode_offload_status,
 	[OP_READ_PLUS]		= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_SEEK]		= (nfsd4_dec)nfsd4_decode_seek,
 	[OP_WRITE_SAME]		= (nfsd4_dec)nfsd4_decode_notsupp,
@@ -4216,6 +4223,25 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 }
 
 static __be32
+nfsd4_encode_offload_status(struct nfsd4_compoundres *resp, __be32 nfserr,
+			    struct nfsd4_offload_status *os)
+{
+	struct xdr_stream *xdr = &resp->xdr;
+	__be32 *p;
+
+	if (nfserr)
+		return nfserr;
+
+	p = xdr_reserve_space(xdr, 8 + 4);
+	if (!p)
+		return nfserr_resource;
+	p = xdr_encode_hyper(p, os->count);
+	*p++ = cpu_to_be32(0);
+
+	return nfserr;
+}
+
+static __be32
 nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
 		  struct nfsd4_seek *seek)
 {
@@ -4318,7 +4344,7 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 	[OP_LAYOUTERROR]	= (nfsd4_enc)nfsd4_encode_noop,
 	[OP_LAYOUTSTATS]	= (nfsd4_enc)nfsd4_encode_noop,
 	[OP_OFFLOAD_CANCEL]	= (nfsd4_enc)nfsd4_encode_noop,
-	[OP_OFFLOAD_STATUS]	= (nfsd4_enc)nfsd4_encode_noop,
+	[OP_OFFLOAD_STATUS]	= (nfsd4_enc)nfsd4_encode_offload_status,
 	[OP_READ_PLUS]		= (nfsd4_enc)nfsd4_encode_noop,
 	[OP_SEEK]		= (nfsd4_enc)nfsd4_encode_seek,
 	[OP_WRITE_SAME]		= (nfsd4_enc)nfsd4_encode_noop,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 4ac2676..9b0c099 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -542,6 +542,15 @@ struct nfsd4_seek {
 	loff_t		seek_pos;
 };
 
+struct nfsd4_offload_status {
+	/* request */
+	stateid_t	stateid;
+
+	/* response */
+	u64		count;
+	u32		status;
+};
+
 struct nfsd4_op {
 	int					opnum;
 	const struct nfsd4_operation *		opdesc;
@@ -600,6 +609,7 @@ struct nfsd4_op {
 		struct nfsd4_fallocate		deallocate;
 		struct nfsd4_clone		clone;
 		struct nfsd4_copy		copy;
+		struct nfsd4_offload_status	offload_status;
 		struct nfsd4_seek		seek;
 	} u;
 	struct nfs4_replay *			replay;
-- 
1.8.3.1

[PATCH v4 03/10] NFSD OFFLOAD_CANCEL xdr

[Olga Kornievskaia]

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c | 13 +++++++++++++
 fs/nfsd/nfs4xdr.c  |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 8601fc4..11ade90 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1121,6 +1121,14 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 }
 
 static __be32
+nfsd4_offload_cancel(struct svc_rqst *rqstp,
+		     struct nfsd4_compound_state *cstate,
+		     union nfsd4_op_u *u)
+{
+	return 0;
+}
+
+static __be32
 nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		struct nfsd4_fallocate *fallocate, int flags)
 {
@@ -2472,6 +2480,11 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 		.op_name = "OP_OFFLOAD_STATUS",
 		.op_rsize_bop = nfsd4_offload_status_rsize,
 	},
+	[OP_OFFLOAD_CANCEL] = {
+		.op_func = nfsd4_offload_cancel,
+		.op_name = "OP_OFFLOAD_CANCEL",
+		.op_rsize_bop = nfsd4_only_status_rsize,
+	},
 };
 
 /**
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ed8b61f..31ee5e1 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1880,7 +1880,7 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
 	[OP_IO_ADVISE]		= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_LAYOUTERROR]	= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_LAYOUTSTATS]	= (nfsd4_dec)nfsd4_decode_notsupp,
-	[OP_OFFLOAD_CANCEL]	= (nfsd4_dec)nfsd4_decode_notsupp,
+	[OP_OFFLOAD_CANCEL]	= (nfsd4_dec)nfsd4_decode_offload_status,
 	[OP_OFFLOAD_STATUS]	= (nfsd4_dec)nfsd4_decode_offload_status,
 	[OP_READ_PLUS]		= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_SEEK]		= (nfsd4_dec)nfsd4_decode_seek,
-- 
1.8.3.1

[PATCH v4 04/10] NFSD xdr callback stateid in async COPY reply

[Olga Kornievskaia]

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4xdr.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 31ee5e1..5f5058e 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4190,15 +4190,27 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 #endif /* CONFIG_NFSD_PNFS */
 
 static __be32
-nfsd42_encode_write_res(struct nfsd4_compoundres *resp, struct nfsd42_write_res *write)
+nfsd42_encode_write_res(struct nfsd4_compoundres *resp,
+		struct nfsd42_write_res *write, bool sync)
 {
 	__be32 *p;
+	p = xdr_reserve_space(&resp->xdr, 4);
+	if (!p)
+		return nfserr_resource;
 
-	p = xdr_reserve_space(&resp->xdr, 4 + 8 + 4 + NFS4_VERIFIER_SIZE);
+	if (sync)
+		*p++ = cpu_to_be32(0);
+	else {
+		__be32 nfserr;
+		*p++ = cpu_to_be32(1);
+		nfserr = nfsd4_encode_stateid(&resp->xdr, &write->cb_stateid);
+		if (nfserr)
+			return nfserr;
+	}
+	p = xdr_reserve_space(&resp->xdr, 8 + 4 + NFS4_VERIFIER_SIZE);
 	if (!p)
 		return nfserr_resource;
 
-	*p++ = cpu_to_be32(0);
 	p = xdr_encode_hyper(p, write->wr_bytes_written);
 	*p++ = cpu_to_be32(write->wr_stable_how);
 	p = xdr_encode_opaque_fixed(p, write->wr_verifier.data,
@@ -4212,7 +4224,8 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 {
 	__be32 *p;
 
-	nfserr = nfsd42_encode_write_res(resp, &copy->cp_res);
+	nfserr = nfsd42_encode_write_res(resp, &copy->cp_res,
+			copy->cp_synchronous);
 	if (nfserr)
 		return nfserr;
 
-- 
1.8.3.1

[PATCH v4 05/10] NFSD first draft of async copy

[Olga Kornievskaia]

Asynchronous copies are handled by a single threaded workqueue.
If we get asynchronous request, make sure to copy the needed
arguments/state from the stack before starting the copy. Then
queue work and reply back to the client indicating copy is
asynchronous.

nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
the total number of bytes need to copy. In case a failure
happens in the middle, we can return an error as well as how
much we copied so far. Once done creating a workitem for the
callback workqueue and send CB_OFFLOAD with the results.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c  | 164 ++++++++++++++++++++++++++++++++++++++++++++++------
 fs/nfsd/nfs4state.c |   9 ++-
 fs/nfsd/state.h     |   2 +
 fs/nfsd/xdr4.h      |   7 +++
 4 files changed, 162 insertions(+), 20 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 11ade90..de21b1a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -76,6 +76,21 @@
 { }
 #endif
 
+static struct workqueue_struct *copy_wq;
+
+int nfsd4_create_copy_queue(void)
+{
+	copy_wq = create_singlethread_workqueue("nfsd4_copy");
+	if (!copy_wq)
+		return -ENOMEM;
+	return 0;
+}
+
+void nfsd4_destroy_copy_queue(void)
+{
+	destroy_workqueue(copy_wq);
+}
+
 #define NFSDDBG_FACILITY		NFSDDBG_PROC
 
 static u32 nfsd_attrmask[] = {
@@ -1085,37 +1100,148 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 out:
 	return status;
 }
+static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
+{
+	struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);
+
+	kfree(copy);
+}
+
+static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
+				 struct rpc_task *task)
+{
+	return 1;
+}
+
+static const struct nfsd4_callback_ops nfsd4_cb_offload_ops = {
+	.release = nfsd4_cb_offload_release,
+	.done = nfsd4_cb_offload_done
+};
+
+static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
+{
+	memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
+		sizeof(copy->cp_dst_stateid));
+	copy->cp_res.wr_stable_how = NFS_UNSTABLE;
+	copy->cp_consecutive = 1;
+	copy->cp_synchronous = sync;
+	gen_boot_verifier(&copy->cp_res.wr_verifier, copy->net);
+
+	return nfs_ok;
+}
+
+static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
+{
+	ssize_t bytes_copied = 0;
+	size_t bytes_total = copy->cp_count;
+	size_t bytes_to_copy;
+	u64 src_pos = copy->cp_src_pos;
+	u64 dst_pos = copy->cp_dst_pos;
+
+	do {
+		bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
+		bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
+					copy->fh_dst, dst_pos, bytes_to_copy);
+		if (bytes_copied <= 0)
+			break;
+		bytes_total -= bytes_copied;
+		copy->cp_res.wr_bytes_written += bytes_copied;
+		src_pos += bytes_copied;
+		dst_pos += bytes_copied;
+	} while (bytes_total > 0 && !copy->cp_synchronous);
+	return bytes_copied;
+}
+
+static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
+{
+	__be32 status;
+	ssize_t bytes;
+
+	bytes = _nfsd_copy_file_range(copy);
+	if (bytes < 0)
+		status = nfserrno(bytes);
+	else
+		status = nfsd4_init_copy_res(copy, sync);
+
+	fput(copy->fh_src);
+	fput(copy->fh_dst);
+	return status;
+}
+
+static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
+{
+	memcpy(&dst->cp_src_stateid, &src->cp_src_stateid, sizeof(stateid_t));
+	memcpy(&dst->cp_dst_stateid, &src->cp_dst_stateid, sizeof(stateid_t));
+	dst->cp_src_pos = src->cp_src_pos;
+	dst->cp_dst_pos = src->cp_dst_pos;
+	dst->cp_count = src->cp_count;
+	dst->cp_consecutive = src->cp_consecutive;
+	dst->cp_synchronous = src->cp_synchronous;
+	memcpy(&dst->cp_res, &src->cp_res, sizeof(src->cp_res));
+	/* skipping nfsd4_callback */
+	memcpy(&dst->fh, &src->fh, sizeof(src->fh));
+	dst->fh = src->fh;
+	dst->cp_clp = src->cp_clp;
+	dst->fh_src = src->fh_src;
+	dst->fh_dst = src->fh_dst;
+	dst->net = src->net;
+}
+
+static void nfsd4_do_async_copy(struct work_struct *work)
+{
+	struct nfsd4_copy *copy =
+		container_of(work, struct nfsd4_copy, cp_work);
+	struct nfsd4_copy *cb_copy;
+
+	copy->nfserr = nfsd4_do_copy(copy, 0);
+	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
+	if (!cb_copy)
+		goto out;
+	memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
+	cb_copy->cp_clp = copy->cp_clp;
+	cb_copy->nfserr = copy->nfserr;
+	memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
+	nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
+			&nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
+	nfsd4_run_cb(&cb_copy->cp_cb);
+out:
+	kfree(copy);
+}
 
 static __be32
 nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		union nfsd4_op_u *u)
 {
 	struct nfsd4_copy *copy = &u->copy;
-	struct file *src, *dst;
 	__be32 status;
-	ssize_t bytes;
 
-	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid, &src,
-				   &copy->cp_dst_stateid, &dst);
+	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
+				   &copy->fh_src, &copy->cp_dst_stateid,
+				   &copy->fh_dst);
 	if (status)
 		goto out;
 
-	bytes = nfsd_copy_file_range(src, copy->cp_src_pos,
-			dst, copy->cp_dst_pos, copy->cp_count);
-
-	if (bytes < 0)
-		status = nfserrno(bytes);
-	else {
-		copy->cp_res.wr_bytes_written = bytes;
-		copy->cp_res.wr_stable_how = NFS_UNSTABLE;
-		copy->cp_consecutive = 1;
-		copy->cp_synchronous = 1;
-		gen_boot_verifier(&copy->cp_res.wr_verifier, SVC_NET(rqstp));
-		status = nfs_ok;
+	copy->cp_clp = cstate->clp;
+	memcpy(&copy->fh, &cstate->current_fh.fh_handle,
+		sizeof(struct knfsd_fh));
+	copy->net = SVC_NET(rqstp);
+	if (!copy->cp_synchronous) {
+		struct nfsd4_copy *async_copy;
+
+		status = nfsd4_init_copy_res(copy, 0);
+		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
+		if (!async_copy) {
+			status = nfserrno(-ENOMEM);
+			goto out;
+		}
+		dup_copy_fields(copy, async_copy);
+		memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
+			sizeof(copy->cp_dst_stateid));
+		INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
+		queue_work(copy_wq, &async_copy->cp_work);
+	} else {
+		status = nfsd4_do_copy(copy, 1);
 	}
-
-	fput(src);
-	fput(dst);
 out:
 	return status;
 }
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0c04f81..fe6f275 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7033,8 +7033,14 @@ static int nfs4_state_create_net(struct net *net)
 		goto out_free_laundry;
 
 	set_max_delegations();
-	return 0;
 
+	ret = nfsd4_create_copy_queue();
+	if (ret)
+		goto out_free_callback;
+
+	return 0;
+out_free_callback:
+	nfsd4_destroy_callback_queue();
 out_free_laundry:
 	destroy_workqueue(laundry_wq);
 out_cleanup_cred:
@@ -7097,6 +7103,7 @@ static int nfs4_state_create_net(struct net *net)
 	destroy_workqueue(laundry_wq);
 	nfsd4_destroy_callback_queue();
 	cleanup_callback_cred();
+	nfsd4_destroy_copy_queue();
 }
 
 static void
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index f8b0210..b5358cf 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -630,6 +630,8 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);
 extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
+extern int nfsd4_create_copy_queue(void);
+extern void nfsd4_destroy_copy_queue(void);
 
 struct nfs4_file *find_file(struct knfsd_fh *fh);
 void put_nfs4_file(struct nfs4_file *fi);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 9b0c099..f7a94d3 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -529,6 +529,13 @@ struct nfsd4_copy {
 	struct nfsd4_callback	cp_cb;
 	__be32			nfserr;
 	struct knfsd_fh		fh;
+
+	struct work_struct      cp_work;
+	struct nfs4_client      *cp_clp;
+
+	struct file             *fh_src;
+	struct file             *fh_dst;
+	struct net              *net;
 };
 
 struct nfsd4_seek {
-- 
1.8.3.1

[PATCH v4 06/10] NFSD return nfs4_stid in nfs4_preprocess_stateid_op

[Olga Kornievskaia]

Needed for copy to add nfs4_cp_state to the nfs4_stid.

Signed-off-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c  | 17 ++++++++++-------
 fs/nfsd/nfs4state.c |  8 ++++++--
 fs/nfsd/state.h     |  3 ++-
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index de21b1a..044af6b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -787,7 +787,8 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 	/* check stateid */
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					&read->rd_stateid, RD_STATE,
-					&read->rd_filp, &read->rd_tmp_file);
+					&read->rd_filp, &read->rd_tmp_file,
+					NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_read: couldn't process stateid!\n");
 		goto out;
@@ -953,7 +954,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
 		status = nfs4_preprocess_stateid_op(rqstp, cstate,
 				&cstate->current_fh, &setattr->sa_stateid,
-				WR_STATE, NULL, NULL);
+				WR_STATE, NULL, NULL, NULL);
 		if (status) {
 			dprintk("NFSD: nfsd4_setattr: couldn't process stateid!\n");
 			return status;
@@ -1019,7 +1020,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 		return nfserr_inval;
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
-						stateid, WR_STATE, &filp, NULL);
+					stateid, WR_STATE, &filp, NULL, NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_write: couldn't process stateid!\n");
 		return status;
@@ -1050,14 +1051,16 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 	__be32 status;
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
-					    src_stateid, RD_STATE, src, NULL);
+					    src_stateid, RD_STATE, src, NULL,
+					    NULL);
 	if (status) {
 		dprintk("NFSD: %s: couldn't process src stateid!\n", __func__);
 		goto out;
 	}
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
-					    dst_stateid, WR_STATE, dst, NULL);
+					    dst_stateid, WR_STATE, dst, NULL,
+					    NULL);
 	if (status) {
 		dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
 		goto out_put_src;
@@ -1263,7 +1266,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					    &fallocate->falloc_stateid,
-					    WR_STATE, &file, NULL);
+					    WR_STATE, &file, NULL, NULL);
 	if (status != nfs_ok) {
 		dprintk("NFSD: nfsd4_fallocate: couldn't process stateid!\n");
 		return status;
@@ -1310,7 +1313,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					    &seek->seek_stateid,
-					    RD_STATE, &file, NULL);
+					    RD_STATE, &file, NULL, NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
 		return status;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fe6f275..2c89dae 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4945,7 +4945,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 __be32
 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
-		stateid_t *stateid, int flags, struct file **filpp, bool *tmp_file)
+		stateid_t *stateid, int flags, struct file **filpp,
+		bool *tmp_file, struct nfs4_stid **cstid)
 {
 	struct inode *ino = d_inode(fhp->fh_dentry);
 	struct net *net = SVC_NET(rqstp);
@@ -4996,8 +4997,11 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 	if (!status && filpp)
 		status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
 out:
-	if (s)
+	if (s) {
+		if (!status && cstid)
+			*cstid = s;
 		nfs4_put_stid(s);
+	}
 	return status;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index b5358cf..cfd13f0 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -600,7 +600,8 @@ struct nfsd4_blocked_lock {
 
 extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
-		stateid_t *stateid, int flags, struct file **filp, bool *tmp_file);
+		stateid_t *stateid, int flags, struct file **filp,
+		bool *tmp_file, struct nfs4_stid **cstid);
 __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn);
-- 
1.8.3.1

[PATCH v4 07/10] NFSD create new stateid for async copy

[Olga Kornievskaia]

Generate a new stateid to be used for reply to the asynchronous
COPY (this would also be used later by COPY_NOTIFY as well).
Associate the stateid with the parent OPEN/LOCK/DELEG stateid
that can be freed during the free of the parent stateid. However,
right now deciding to bind the lifetime to when the vfs copy
is done. This way don't need to keep the nfsd_net structure for
the callback. The drawback is that time copy state information
is available for query by OFFLOAD_STATUS is slightly less.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/netns.h     |  8 +++++++
 fs/nfsd/nfs4proc.c  | 31 ++++++++++++++++++-------
 fs/nfsd/nfs4state.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsctl.c    |  1 +
 fs/nfsd/state.h     | 14 ++++++++++++
 fs/nfsd/xdr4.h      |  2 ++
 6 files changed, 114 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 3714231..2c88a95 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -119,6 +119,14 @@ struct nfsd_net {
 	u32 clverifier_counter;
 
 	struct svc_serv *nfsd_serv;
+
+	/*
+	 * clientid and stateid data for construction of net unique COPY
+	 * stateids.
+	 */
+	u32		s2s_cp_cl_id;
+	struct idr	s2s_cp_stateids;
+	spinlock_t	s2s_cp_lock;
 };
 
 /* Simple check to find out if a given net was properly initialized */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 044af6b..3cddebb 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1046,7 +1046,8 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 static __be32
 nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		  stateid_t *src_stateid, struct file **src,
-		  stateid_t *dst_stateid, struct file **dst)
+		  stateid_t *dst_stateid, struct file **dst,
+		  struct nfs4_stid **stid)
 {
 	__be32 status;
 
@@ -1060,7 +1061,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					    dst_stateid, WR_STATE, dst, NULL,
-					    NULL);
+					    stid);
 	if (status) {
 		dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
 		goto out_put_src;
@@ -1091,7 +1092,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 	__be32 status;
 
 	status = nfsd4_verify_copy(rqstp, cstate, &clone->cl_src_stateid, &src,
-				   &clone->cl_dst_stateid, &dst);
+				   &clone->cl_dst_stateid, &dst, NULL);
 	if (status)
 		goto out;
 
@@ -1123,8 +1124,6 @@ static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
 
 static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
 {
-	memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
-		sizeof(copy->cp_dst_stateid));
 	copy->cp_res.wr_stable_how = NFS_UNSTABLE;
 	copy->cp_consecutive = 1;
 	copy->cp_synchronous = sync;
@@ -1188,6 +1187,8 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
 	dst->fh_src = src->fh_src;
 	dst->fh_dst = src->fh_dst;
 	dst->net = src->net;
+	dst->stid = src->stid;
+	dst->cps = src->cps;
 }
 
 static void nfsd4_do_async_copy(struct work_struct *work)
@@ -1208,6 +1209,9 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 			&nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
 	nfsd4_run_cb(&cb_copy->cp_cb);
 out:
+	list_del(&copy->cps->cp_list);
+	nfs4_free_cp_state(copy->cps);
+	nfs4_put_stid(copy->stid);
 	kfree(copy);
 }
 
@@ -1220,7 +1224,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 
 	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
 				   &copy->fh_src, &copy->cp_dst_stateid,
-				   &copy->fh_dst);
+				   &copy->fh_dst, &copy->stid);
 	if (status)
 		goto out;
 
@@ -1229,6 +1233,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 		sizeof(struct knfsd_fh));
 	copy->net = SVC_NET(rqstp);
 	if (!copy->cp_synchronous) {
+		struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 		struct nfsd4_copy *async_copy;
 
 		status = nfsd4_init_copy_res(copy, 0);
@@ -1237,9 +1242,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 			status = nfserrno(-ENOMEM);
 			goto out;
 		}
+		copy->cps = nfs4_alloc_init_cp_state(nn, copy->stid);
+		if (!copy->cps) {
+			status = nfserrno(-ENOMEM);
+			kfree(async_copy);
+			goto out;
+		}
+		/* take a reference on the parent stateid so it's not
+		 * not freed by the copy compound
+		 */
+		atomic_inc(&copy->stid->sc_count);
+		memcpy(&copy->cp_res.cb_stateid, &copy->cps->cp_stateid,
+			sizeof(copy->cps->cp_stateid));
 		dup_copy_fields(copy, async_copy);
-		memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
-			sizeof(copy->cp_dst_stateid));
 		INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
 		queue_work(copy_wq, &async_copy->cp_work);
 	} else {
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2c89dae..be59baf 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -658,6 +658,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
 	/* Will be incremented before return to client: */
 	atomic_set(&stid->sc_count, 1);
 	spin_lock_init(&stid->sc_lock);
+	INIT_LIST_HEAD(&stid->sc_cp_list);
 
 	/*
 	 * It shouldn't be a problem to reuse an opaque stateid value.
@@ -674,6 +675,66 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
 	return NULL;
 }
 
+/*
+ * Create a unique stateid_t to represent each COPY. Hang the copy
+ * stateids off the OPEN/LOCK/DELEG stateid from the client open
+ * the source file.
+ */
+struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn,
+					       struct nfs4_stid *p_stid)
+{
+	struct nfs4_cp_state *cps;
+	int new_id;
+
+	cps = kzalloc(sizeof(struct nfs4_cp_state), GFP_KERNEL);
+	if (!cps)
+		return NULL;
+	idr_preload(GFP_KERNEL);
+	spin_lock(&nn->s2s_cp_lock);
+	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, cps, 0, 0, GFP_NOWAIT);
+	spin_unlock(&nn->s2s_cp_lock);
+	idr_preload_end();
+	if (new_id < 0)
+		goto out_free;
+	cps->cp_stateid.si_opaque.so_id = new_id;
+	cps->cp_stateid.si_opaque.so_clid.cl_boot = nn->boot_time;
+	cps->cp_stateid.si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
+	cps->cp_p_stid = p_stid;
+	INIT_LIST_HEAD(&cps->cp_list);
+	list_add(&cps->cp_list, &p_stid->sc_cp_list);
+
+	return cps;
+out_free:
+	kfree(cps);
+	return NULL;
+}
+
+void nfs4_free_cp_state(struct nfs4_cp_state *cps)
+{
+	struct nfsd_net *nn;
+
+	nn = net_generic(cps->cp_p_stid->sc_client->net, nfsd_net_id);
+	spin_lock(&nn->s2s_cp_lock);
+	idr_remove(&nn->s2s_cp_stateids, cps->cp_stateid.si_opaque.so_id);
+	spin_unlock(&nn->s2s_cp_lock);
+
+	kfree(cps);
+}
+
+static void nfs4_free_cp_statelist(struct nfs4_stid *stid)
+{
+	struct nfs4_cp_state *cps;
+
+	might_sleep();
+
+	while (!list_empty(&stid->sc_cp_list)) {
+		cps = list_first_entry(&stid->sc_cp_list, struct nfs4_cp_state,
+				       cp_list);
+		list_del(&cps->cp_list);
+		nfs4_free_cp_state(cps);
+	}
+}
+
 static struct nfs4_ol_stateid * nfs4_alloc_open_stateid(struct nfs4_client *clp)
 {
 	struct nfs4_stid *stid;
@@ -819,6 +880,9 @@ static void block_delegations(struct knfsd_fh *fh)
 	}
 	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
 	spin_unlock(&clp->cl_lock);
+
+	nfs4_free_cp_statelist(s);
+
 	s->sc_free(s);
 	if (fp)
 		put_nfs4_file(fp);
@@ -6952,6 +7016,8 @@ static int nfs4_state_create_net(struct net *net)
 	INIT_LIST_HEAD(&nn->close_lru);
 	INIT_LIST_HEAD(&nn->del_recall_lru);
 	spin_lock_init(&nn->client_lock);
+	spin_lock_init(&nn->s2s_cp_lock);
+	idr_init(&nn->s2s_cp_stateids);
 
 	spin_lock_init(&nn->blocked_locks_lock);
 	INIT_LIST_HEAD(&nn->blocked_locks_lru);
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 6493df6..232270d 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1241,6 +1241,7 @@ static __net_init int nfsd_init_net(struct net *net)
 	nn->nfsd4_grace = 90;
 	nn->clverifier_counter = prandom_u32();
 	nn->clientid_counter = prandom_u32();
+	nn->s2s_cp_cl_id = nn->clientid_counter++;
 	return 0;
 
 out_idmap_error:
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index cfd13f0..8724955 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -93,6 +93,7 @@ struct nfs4_stid {
 #define NFS4_REVOKED_DELEG_STID 16
 #define NFS4_CLOSED_DELEG_STID 32
 #define NFS4_LAYOUT_STID 64
+	struct list_head	sc_cp_list;
 	unsigned char		sc_type;
 	stateid_t		sc_stateid;
 	spinlock_t		sc_lock;
@@ -102,6 +103,17 @@ struct nfs4_stid {
 };
 
 /*
+ * Keep a list of stateids issued by the COPY, associate it with the
+ * parent OPEN/LOCK/DELEG stateid. Used for lookup by
+ * OFFLOAD_CANCEL and OFFLOAD_STATUS (as well as COPY_NOTIFY)
+ */
+struct nfs4_cp_state {
+	stateid_t		cp_stateid;
+	struct list_head	cp_list;	/* per parent nfs4_stid */
+	struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
+};
+
+/*
  * Represents a delegation stateid. The nfs4_client holds references to these
  * and they are put when it is being destroyed or when the delegation is
  * returned by the client:
@@ -607,6 +619,8 @@ __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     struct nfs4_stid **s, struct nfsd_net *nn);
 struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *slab,
 				  void (*sc_free)(struct nfs4_stid *));
+struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn, struct nfs4_stid *p_stid);
+void nfs4_free_cp_state(struct nfs4_cp_state *cps);
 void nfs4_unhash_stid(struct nfs4_stid *s);
 void nfs4_put_stid(struct nfs4_stid *s);
 void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index f7a94d3..4ccd43b 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -536,6 +536,8 @@ struct nfsd4_copy {
 	struct file             *fh_src;
 	struct file             *fh_dst;
 	struct net              *net;
+	struct nfs4_stid        *stid;
+	struct nfs4_cp_state	*cps;
 };
 
 struct nfsd4_seek {
-- 
1.8.3.1

[PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[Olga Kornievskaia]

Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
if found mark it cancelled. If copy has more interations to
call vfs_copy_file_range, it'll stop it. Server won't be sending
CB_OFFLOAD to the client since it received a cancel.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
 fs/nfsd/nfs4state.c | 16 ++++++++++++++++
 fs/nfsd/state.h     |  4 ++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 3cddebb..f4f3d93 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
 	size_t bytes_to_copy;
 	u64 src_pos = copy->cp_src_pos;
 	u64 dst_pos = copy->cp_dst_pos;
+	bool cancelled = false;
 
 	do {
 		bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
@@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
 		copy->cp_res.wr_bytes_written += bytes_copied;
 		src_pos += bytes_copied;
 		dst_pos += bytes_copied;
-	} while (bytes_total > 0 && !copy->cp_synchronous);
+		if (!copy->cp_synchronous) {
+			spin_lock(&copy->cps->cp_lock);
+			cancelled = copy->cps->cp_cancelled;
+			spin_unlock(&copy->cps->cp_lock);
+		}
+	} while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
 	return bytes_copied;
 }
 
@@ -1198,6 +1204,10 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 	struct nfsd4_copy *cb_copy;
 
 	copy->nfserr = nfsd4_do_copy(copy, 0);
+
+	if (copy->cps->cp_cancelled)
+		goto out;
+
 	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
 	if (!cb_copy)
 		goto out;
@@ -1269,7 +1279,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 		     struct nfsd4_compound_state *cstate,
 		     union nfsd4_op_u *u)
 {
-	return 0;
+	struct nfsd4_offload_status *os = &u->offload_status;
+	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
+	__be32 status;
+	struct nfs4_cp_state *state = NULL;
+
+	status = find_cp_state(nn, &os->stateid, &state);
+	if (state) {
+		spin_lock(&state->cp_lock);
+		state->cp_cancelled = true;
+		spin_unlock(&state->cp_lock);
+	}
+
+	return status;
 }
 
 static __be32
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index be59baf..97ab3f8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -752,6 +752,22 @@ static void nfs4_free_deleg(struct nfs4_stid *stid)
 	atomic_long_dec(&num_delegations);
 }
 
+__be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
+			    struct nfs4_cp_state **cps)
+{
+	struct nfs4_cp_state *state = NULL;
+
+	if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
+		return nfserr_bad_stateid;
+	spin_lock(&nn->s2s_cp_lock);
+	state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
+	spin_unlock(&nn->s2s_cp_lock);
+	if (!state)
+		return nfserr_bad_stateid;
+	*cps = state;
+	return 0;
+}
+
 /*
  * When we recall a delegation, we should be careful not to hand it
  * out again straight away.
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 8724955..7a070d5 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -111,6 +111,8 @@ struct nfs4_cp_state {
 	stateid_t		cp_stateid;
 	struct list_head	cp_list;	/* per parent nfs4_stid */
 	struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
+	bool                    cp_cancelled;   /* copy cancelled */
+	spinlock_t              cp_lock;
 };
 
 /*
@@ -647,6 +649,8 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
 extern int nfsd4_create_copy_queue(void);
 extern void nfsd4_destroy_copy_queue(void);
+extern __be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
+			struct nfs4_cp_state **cps);
 
 struct nfs4_file *find_file(struct knfsd_fh *fh);
 void put_nfs4_file(struct nfs4_file *fi);
-- 
1.8.3.1

[PATCH v4 09/10] NFSD support OFFLOAD_STATUS

[Olga Kornievskaia]

Update number of bytes copied in the copy state and query that
value under lock if OFFLOAD_STATUS operation received.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c | 16 +++++++++++++++-
 fs/nfsd/state.h    |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f4f3d93..74bfa4b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1154,6 +1154,8 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
 		if (!copy->cp_synchronous) {
 			spin_lock(&copy->cps->cp_lock);
 			cancelled = copy->cps->cp_cancelled;
+			copy->cps->cp_bytes_copied =
+				copy->cp_res.wr_bytes_written;
 			spin_unlock(&copy->cps->cp_lock);
 		}
 	} while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
@@ -1321,7 +1323,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 		     struct nfsd4_compound_state *cstate,
 		     union nfsd4_op_u *u)
 {
-	return nfserr_notsupp;
+	struct nfsd4_offload_status *os = &u->offload_status;
+	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
+	__be32 status;
+	struct nfs4_cp_state *state = NULL;
+
+	status = find_cp_state(nn, &os->stateid, &state);
+
+	if (state) {
+		spin_lock(&state->cp_lock);
+		os->count = state->cp_bytes_copied;
+		spin_unlock(&state->cp_lock);
+	}
+	return status;
 }
 
 static __be32
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 7a070d5..c31025c 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -113,6 +113,7 @@ struct nfs4_cp_state {
 	struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
 	bool                    cp_cancelled;   /* copy cancelled */
 	spinlock_t              cp_lock;
+	ssize_t                 cp_bytes_copied;/* copy progress */
 };
 
 /*
-- 
1.8.3.1

[PATCH v4 10/10] NFSD stop queued async copies on client shutdown

[Olga Kornievskaia]

If client is shutting down and there are still async copies going
on, then stop queued async copies.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c  | 9 +++++++++
 fs/nfsd/nfs4state.c | 1 +
 fs/nfsd/state.h     | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 74bfa4b..8d0c87f 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -91,6 +91,12 @@ void nfsd4_destroy_copy_queue(void)
 	destroy_workqueue(copy_wq);
 }
 
+void nfsd4_shutdown_copy(struct nfs4_client *clp)
+{
+	set_bit(NFSD4_CLIENT_COPY_KILL, &clp->cl_flags);
+	flush_workqueue(copy_wq);
+}
+
 #define NFSDDBG_FACILITY		NFSDDBG_PROC
 
 static u32 nfsd_attrmask[] = {
@@ -1205,6 +1211,9 @@ static void nfsd4_do_async_copy(struct work_struct *work)
 		container_of(work, struct nfsd4_copy, cp_work);
 	struct nfsd4_copy *cb_copy;
 
+	if (test_bit(NFSD4_CLIENT_COPY_KILL, &copy->cp_clp->cl_flags))
+		goto out;
+
 	copy->nfserr = nfsd4_do_copy(copy, 0);
 
 	if (copy->cps->cp_cancelled)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 97ab3f8..f4b193e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1961,6 +1961,7 @@ static __be32 mark_client_expired_locked(struct nfs4_client *clp)
 	}
 	nfsd4_return_all_client_layouts(clp);
 	nfsd4_shutdown_callback(clp);
+	nfsd4_shutdown_copy(clp);
 	if (clp->cl_cb_conn.cb_xprt)
 		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
 	free_client(clp);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index c31025c..55f09a9 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -336,6 +336,7 @@ struct nfs4_client {
 #define NFSD4_CLIENT_RECLAIM_COMPLETE	(3)	/* reclaim_complete done */
 #define NFSD4_CLIENT_CONFIRMED		(4)	/* client is confirmed */
 #define NFSD4_CLIENT_UPCALL_LOCK	(5)	/* upcall serialization */
+#define NFSD4_CLIENT_COPY_KILL		(6)	/* stop copy workqueue */
 #define NFSD4_CLIENT_CB_FLAG_MASK	(1 << NFSD4_CLIENT_CB_UPDATE | \
 					 1 << NFSD4_CLIENT_CB_KILL)
 	unsigned long		cl_flags;
@@ -644,6 +645,7 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
 extern int nfsd4_create_callback_queue(void);
 extern void nfsd4_destroy_callback_queue(void);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
+extern void nfsd4_shutdown_copy(struct nfs4_client *clp);
 extern void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp);
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);
-- 
1.8.3.1

Re: [PATCH v4 05/10] NFSD first draft of async copy

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
> Asynchronous copies are handled by a single threaded workqueue.
> If we get asynchronous request, make sure to copy the needed
> arguments/state from the stack before starting the copy. Then
> queue work and reply back to the client indicating copy is
> asynchronous.
> 
> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
> the total number of bytes need to copy.

I don't think you need to do this--just call vfs_copy_file_range()
directly, as nfsd_copy_file_range does.

The 4MB chunking was only there to prevent a synchronous copy from
taking too long, which isn't an issue in the async case.

--b.

> In case a failure
> happens in the middle, we can return an error as well as how
> much we copied so far. Once done creating a workitem for the
> callback workqueue and send CB_OFFLOAD with the results.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4proc.c  | 164 ++++++++++++++++++++++++++++++++++++++++++++++------
>  fs/nfsd/nfs4state.c |   9 ++-
>  fs/nfsd/state.h     |   2 +
>  fs/nfsd/xdr4.h      |   7 +++
>  4 files changed, 162 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 11ade90..de21b1a 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -76,6 +76,21 @@
>  { }
>  #endif
>  
> +static struct workqueue_struct *copy_wq;
> +
> +int nfsd4_create_copy_queue(void)
> +{
> +	copy_wq = create_singlethread_workqueue("nfsd4_copy");
> +	if (!copy_wq)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +void nfsd4_destroy_copy_queue(void)
> +{
> +	destroy_workqueue(copy_wq);
> +}
> +
>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>  
>  static u32 nfsd_attrmask[] = {
> @@ -1085,37 +1100,148 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>  out:
>  	return status;
>  }
> +static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> +{
> +	struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);
> +
> +	kfree(copy);
> +}
> +
> +static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
> +				 struct rpc_task *task)
> +{
> +	return 1;
> +}
> +
> +static const struct nfsd4_callback_ops nfsd4_cb_offload_ops = {
> +	.release = nfsd4_cb_offload_release,
> +	.done = nfsd4_cb_offload_done
> +};
> +
> +static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
> +{
> +	memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> +		sizeof(copy->cp_dst_stateid));
> +	copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> +	copy->cp_consecutive = 1;
> +	copy->cp_synchronous = sync;
> +	gen_boot_verifier(&copy->cp_res.wr_verifier, copy->net);
> +
> +	return nfs_ok;
> +}
> +
> +static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> +{
> +	ssize_t bytes_copied = 0;
> +	size_t bytes_total = copy->cp_count;
> +	size_t bytes_to_copy;
> +	u64 src_pos = copy->cp_src_pos;
> +	u64 dst_pos = copy->cp_dst_pos;
> +
> +	do {
> +		bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> +		bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
> +					copy->fh_dst, dst_pos, bytes_to_copy);
> +		if (bytes_copied <= 0)
> +			break;
> +		bytes_total -= bytes_copied;
> +		copy->cp_res.wr_bytes_written += bytes_copied;
> +		src_pos += bytes_copied;
> +		dst_pos += bytes_copied;
> +	} while (bytes_total > 0 && !copy->cp_synchronous);
> +	return bytes_copied;
> +}
> +
> +static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
> +{
> +	__be32 status;
> +	ssize_t bytes;
> +
> +	bytes = _nfsd_copy_file_range(copy);
> +	if (bytes < 0)
> +		status = nfserrno(bytes);
> +	else
> +		status = nfsd4_init_copy_res(copy, sync);
> +
> +	fput(copy->fh_src);
> +	fput(copy->fh_dst);
> +	return status;
> +}
> +
> +static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> +{
> +	memcpy(&dst->cp_src_stateid, &src->cp_src_stateid, sizeof(stateid_t));
> +	memcpy(&dst->cp_dst_stateid, &src->cp_dst_stateid, sizeof(stateid_t));
> +	dst->cp_src_pos = src->cp_src_pos;
> +	dst->cp_dst_pos = src->cp_dst_pos;
> +	dst->cp_count = src->cp_count;
> +	dst->cp_consecutive = src->cp_consecutive;
> +	dst->cp_synchronous = src->cp_synchronous;
> +	memcpy(&dst->cp_res, &src->cp_res, sizeof(src->cp_res));
> +	/* skipping nfsd4_callback */
> +	memcpy(&dst->fh, &src->fh, sizeof(src->fh));
> +	dst->fh = src->fh;
> +	dst->cp_clp = src->cp_clp;
> +	dst->fh_src = src->fh_src;
> +	dst->fh_dst = src->fh_dst;
> +	dst->net = src->net;
> +}
> +
> +static void nfsd4_do_async_copy(struct work_struct *work)
> +{
> +	struct nfsd4_copy *copy =
> +		container_of(work, struct nfsd4_copy, cp_work);
> +	struct nfsd4_copy *cb_copy;
> +
> +	copy->nfserr = nfsd4_do_copy(copy, 0);
> +	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> +	if (!cb_copy)
> +		goto out;
> +	memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
> +	cb_copy->cp_clp = copy->cp_clp;
> +	cb_copy->nfserr = copy->nfserr;
> +	memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
> +	nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
> +			&nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
> +	nfsd4_run_cb(&cb_copy->cp_cb);
> +out:
> +	kfree(copy);
> +}
>  
>  static __be32
>  nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		union nfsd4_op_u *u)
>  {
>  	struct nfsd4_copy *copy = &u->copy;
> -	struct file *src, *dst;
>  	__be32 status;
> -	ssize_t bytes;
>  
> -	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid, &src,
> -				   &copy->cp_dst_stateid, &dst);
> +	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> +				   &copy->fh_src, &copy->cp_dst_stateid,
> +				   &copy->fh_dst);
>  	if (status)
>  		goto out;
>  
> -	bytes = nfsd_copy_file_range(src, copy->cp_src_pos,
> -			dst, copy->cp_dst_pos, copy->cp_count);
> -
> -	if (bytes < 0)
> -		status = nfserrno(bytes);
> -	else {
> -		copy->cp_res.wr_bytes_written = bytes;
> -		copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> -		copy->cp_consecutive = 1;
> -		copy->cp_synchronous = 1;
> -		gen_boot_verifier(&copy->cp_res.wr_verifier, SVC_NET(rqstp));
> -		status = nfs_ok;
> +	copy->cp_clp = cstate->clp;
> +	memcpy(&copy->fh, &cstate->current_fh.fh_handle,
> +		sizeof(struct knfsd_fh));
> +	copy->net = SVC_NET(rqstp);
> +	if (!copy->cp_synchronous) {
> +		struct nfsd4_copy *async_copy;
> +
> +		status = nfsd4_init_copy_res(copy, 0);
> +		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> +		if (!async_copy) {
> +			status = nfserrno(-ENOMEM);
> +			goto out;
> +		}
> +		dup_copy_fields(copy, async_copy);
> +		memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> +			sizeof(copy->cp_dst_stateid));
> +		INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
> +		queue_work(copy_wq, &async_copy->cp_work);
> +	} else {
> +		status = nfsd4_do_copy(copy, 1);
>  	}
> -
> -	fput(src);
> -	fput(dst);
>  out:
>  	return status;
>  }
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0c04f81..fe6f275 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7033,8 +7033,14 @@ static int nfs4_state_create_net(struct net *net)
>  		goto out_free_laundry;
>  
>  	set_max_delegations();
> -	return 0;
>  
> +	ret = nfsd4_create_copy_queue();
> +	if (ret)
> +		goto out_free_callback;
> +
> +	return 0;
> +out_free_callback:
> +	nfsd4_destroy_callback_queue();
>  out_free_laundry:
>  	destroy_workqueue(laundry_wq);
>  out_cleanup_cred:
> @@ -7097,6 +7103,7 @@ static int nfs4_state_create_net(struct net *net)
>  	destroy_workqueue(laundry_wq);
>  	nfsd4_destroy_callback_queue();
>  	cleanup_callback_cred();
> +	nfsd4_destroy_copy_queue();
>  }
>  
>  static void
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index f8b0210..b5358cf 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -630,6 +630,8 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
>  extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>  							struct nfsd_net *nn);
>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
> +extern int nfsd4_create_copy_queue(void);
> +extern void nfsd4_destroy_copy_queue(void);
>  
>  struct nfs4_file *find_file(struct knfsd_fh *fh);
>  void put_nfs4_file(struct nfs4_file *fi);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 9b0c099..f7a94d3 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -529,6 +529,13 @@ struct nfsd4_copy {
>  	struct nfsd4_callback	cp_cb;
>  	__be32			nfserr;
>  	struct knfsd_fh		fh;
> +
> +	struct work_struct      cp_work;
> +	struct nfs4_client      *cp_clp;
> +
> +	struct file             *fh_src;
> +	struct file             *fh_dst;
> +	struct net              *net;
>  };
>  
>  struct nfsd4_seek {
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 05/10] NFSD first draft of async copy

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
> Asynchronous copies are handled by a single threaded workqueue.
> If we get asynchronous request, make sure to copy the needed
> arguments/state from the stack before starting the copy.

I'm worried by those copies--I suspect you want to be taking actual
references (so, bumping reference counts) to the clp and the
filehandles, not just copying pointers.

--b.

> Then
> queue work and reply back to the client indicating copy is
> asynchronous.
> 
> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
> the total number of bytes need to copy. In case a failure
> happens in the middle, we can return an error as well as how
> much we copied so far. Once done creating a workitem for the
> callback workqueue and send CB_OFFLOAD with the results.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4proc.c  | 164 ++++++++++++++++++++++++++++++++++++++++++++++------
>  fs/nfsd/nfs4state.c |   9 ++-
>  fs/nfsd/state.h     |   2 +
>  fs/nfsd/xdr4.h      |   7 +++
>  4 files changed, 162 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 11ade90..de21b1a 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -76,6 +76,21 @@
>  { }
>  #endif
>  
> +static struct workqueue_struct *copy_wq;
> +
> +int nfsd4_create_copy_queue(void)
> +{
> +	copy_wq = create_singlethread_workqueue("nfsd4_copy");
> +	if (!copy_wq)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +void nfsd4_destroy_copy_queue(void)
> +{
> +	destroy_workqueue(copy_wq);
> +}
> +
>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>  
>  static u32 nfsd_attrmask[] = {
> @@ -1085,37 +1100,148 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>  out:
>  	return status;
>  }
> +static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> +{
> +	struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);
> +
> +	kfree(copy);
> +}
> +
> +static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
> +				 struct rpc_task *task)
> +{
> +	return 1;
> +}
> +
> +static const struct nfsd4_callback_ops nfsd4_cb_offload_ops = {
> +	.release = nfsd4_cb_offload_release,
> +	.done = nfsd4_cb_offload_done
> +};
> +
> +static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
> +{
> +	memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> +		sizeof(copy->cp_dst_stateid));
> +	copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> +	copy->cp_consecutive = 1;
> +	copy->cp_synchronous = sync;
> +	gen_boot_verifier(&copy->cp_res.wr_verifier, copy->net);
> +
> +	return nfs_ok;
> +}
> +
> +static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> +{
> +	ssize_t bytes_copied = 0;
> +	size_t bytes_total = copy->cp_count;
> +	size_t bytes_to_copy;
> +	u64 src_pos = copy->cp_src_pos;
> +	u64 dst_pos = copy->cp_dst_pos;
> +
> +	do {
> +		bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> +		bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
> +					copy->fh_dst, dst_pos, bytes_to_copy);
> +		if (bytes_copied <= 0)
> +			break;
> +		bytes_total -= bytes_copied;
> +		copy->cp_res.wr_bytes_written += bytes_copied;
> +		src_pos += bytes_copied;
> +		dst_pos += bytes_copied;
> +	} while (bytes_total > 0 && !copy->cp_synchronous);
> +	return bytes_copied;
> +}
> +
> +static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
> +{
> +	__be32 status;
> +	ssize_t bytes;
> +
> +	bytes = _nfsd_copy_file_range(copy);
> +	if (bytes < 0)
> +		status = nfserrno(bytes);
> +	else
> +		status = nfsd4_init_copy_res(copy, sync);
> +
> +	fput(copy->fh_src);
> +	fput(copy->fh_dst);
> +	return status;
> +}
> +
> +static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> +{
> +	memcpy(&dst->cp_src_stateid, &src->cp_src_stateid, sizeof(stateid_t));
> +	memcpy(&dst->cp_dst_stateid, &src->cp_dst_stateid, sizeof(stateid_t));
> +	dst->cp_src_pos = src->cp_src_pos;
> +	dst->cp_dst_pos = src->cp_dst_pos;
> +	dst->cp_count = src->cp_count;
> +	dst->cp_consecutive = src->cp_consecutive;
> +	dst->cp_synchronous = src->cp_synchronous;
> +	memcpy(&dst->cp_res, &src->cp_res, sizeof(src->cp_res));
> +	/* skipping nfsd4_callback */
> +	memcpy(&dst->fh, &src->fh, sizeof(src->fh));
> +	dst->fh = src->fh;
> +	dst->cp_clp = src->cp_clp;
> +	dst->fh_src = src->fh_src;
> +	dst->fh_dst = src->fh_dst;
> +	dst->net = src->net;
> +}
> +
> +static void nfsd4_do_async_copy(struct work_struct *work)
> +{
> +	struct nfsd4_copy *copy =
> +		container_of(work, struct nfsd4_copy, cp_work);
> +	struct nfsd4_copy *cb_copy;
> +
> +	copy->nfserr = nfsd4_do_copy(copy, 0);
> +	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> +	if (!cb_copy)
> +		goto out;
> +	memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
> +	cb_copy->cp_clp = copy->cp_clp;
> +	cb_copy->nfserr = copy->nfserr;
> +	memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
> +	nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
> +			&nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
> +	nfsd4_run_cb(&cb_copy->cp_cb);
> +out:
> +	kfree(copy);
> +}
>  
>  static __be32
>  nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		union nfsd4_op_u *u)
>  {
>  	struct nfsd4_copy *copy = &u->copy;
> -	struct file *src, *dst;
>  	__be32 status;
> -	ssize_t bytes;
>  
> -	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid, &src,
> -				   &copy->cp_dst_stateid, &dst);
> +	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> +				   &copy->fh_src, &copy->cp_dst_stateid,
> +				   &copy->fh_dst);
>  	if (status)
>  		goto out;
>  
> -	bytes = nfsd_copy_file_range(src, copy->cp_src_pos,
> -			dst, copy->cp_dst_pos, copy->cp_count);
> -
> -	if (bytes < 0)
> -		status = nfserrno(bytes);
> -	else {
> -		copy->cp_res.wr_bytes_written = bytes;
> -		copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> -		copy->cp_consecutive = 1;
> -		copy->cp_synchronous = 1;
> -		gen_boot_verifier(&copy->cp_res.wr_verifier, SVC_NET(rqstp));
> -		status = nfs_ok;
> +	copy->cp_clp = cstate->clp;
> +	memcpy(&copy->fh, &cstate->current_fh.fh_handle,
> +		sizeof(struct knfsd_fh));
> +	copy->net = SVC_NET(rqstp);
> +	if (!copy->cp_synchronous) {
> +		struct nfsd4_copy *async_copy;
> +
> +		status = nfsd4_init_copy_res(copy, 0);
> +		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> +		if (!async_copy) {
> +			status = nfserrno(-ENOMEM);
> +			goto out;
> +		}
> +		dup_copy_fields(copy, async_copy);
> +		memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> +			sizeof(copy->cp_dst_stateid));
> +		INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
> +		queue_work(copy_wq, &async_copy->cp_work);
> +	} else {
> +		status = nfsd4_do_copy(copy, 1);
>  	}
> -
> -	fput(src);
> -	fput(dst);
>  out:
>  	return status;
>  }
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0c04f81..fe6f275 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7033,8 +7033,14 @@ static int nfs4_state_create_net(struct net *net)
>  		goto out_free_laundry;
>  
>  	set_max_delegations();
> -	return 0;
>  
> +	ret = nfsd4_create_copy_queue();
> +	if (ret)
> +		goto out_free_callback;
> +
> +	return 0;
> +out_free_callback:
> +	nfsd4_destroy_callback_queue();
>  out_free_laundry:
>  	destroy_workqueue(laundry_wq);
>  out_cleanup_cred:
> @@ -7097,6 +7103,7 @@ static int nfs4_state_create_net(struct net *net)
>  	destroy_workqueue(laundry_wq);
>  	nfsd4_destroy_callback_queue();
>  	cleanup_callback_cred();
> +	nfsd4_destroy_copy_queue();
>  }
>  
>  static void
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index f8b0210..b5358cf 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -630,6 +630,8 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
>  extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>  							struct nfsd_net *nn);
>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
> +extern int nfsd4_create_copy_queue(void);
> +extern void nfsd4_destroy_copy_queue(void);
>  
>  struct nfs4_file *find_file(struct knfsd_fh *fh);
>  void put_nfs4_file(struct nfs4_file *fi);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 9b0c099..f7a94d3 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -529,6 +529,13 @@ struct nfsd4_copy {
>  	struct nfsd4_callback	cp_cb;
>  	__be32			nfserr;
>  	struct knfsd_fh		fh;
> +
> +	struct work_struct      cp_work;
> +	struct nfs4_client      *cp_clp;
> +
> +	struct file             *fh_src;
> +	struct file             *fh_dst;
> +	struct net              *net;
>  };
>  
>  struct nfsd4_seek {
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
> if found mark it cancelled. If copy has more interations to
> call vfs_copy_file_range, it'll stop it. Server won't be sending
> CB_OFFLOAD to the client since it received a cancel.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
>  fs/nfsd/state.h     |  4 ++++
>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3cddebb..f4f3d93 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>  	size_t bytes_to_copy;
>  	u64 src_pos = copy->cp_src_pos;
>  	u64 dst_pos = copy->cp_dst_pos;
> +	bool cancelled = false;
>  
>  	do {
>  		bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>  		copy->cp_res.wr_bytes_written += bytes_copied;
>  		src_pos += bytes_copied;
>  		dst_pos += bytes_copied;
> -	} while (bytes_total > 0 && !copy->cp_synchronous);
> +		if (!copy->cp_synchronous) {
> +			spin_lock(&copy->cps->cp_lock);
> +			cancelled = copy->cps->cp_cancelled;
> +			spin_unlock(&copy->cps->cp_lock);
> +		}
> +	} while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
>  	return bytes_copied;

I'd rather we sent a signal, and then we won't need this
logic--vfs_copy_range() will just return EINTR or something.

That will help us get rid of the 4MB-at-a-time loop.  And will mean we
don't need to wait for the next 4MB copy to finish before stopping the
loop.  Normally I wouldn't expect that to take too long, but it might.
And a situation where a cancel is sent is a situation where we're
probably more likely to have some problem slowing down the copy.

Also: don't we want OFFLOAD_CANCEL to wait until the cancel has actually
taken effect before returning?

I can't see any language in the spec to that affect, but it would seem
surprising to me if I got back a succesful response to OFFLOAD_CANCEL
and then noticed that the target file was still changing.

--b.

>  }
>  
> @@ -1198,6 +1204,10 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>  	struct nfsd4_copy *cb_copy;
>  
>  	copy->nfserr = nfsd4_do_copy(copy, 0);
> +
> +	if (copy->cps->cp_cancelled)
> +		goto out;
> +
>  	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>  	if (!cb_copy)
>  		goto out;
> @@ -1269,7 +1279,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>  		     struct nfsd4_compound_state *cstate,
>  		     union nfsd4_op_u *u)
>  {
> -	return 0;
> +	struct nfsd4_offload_status *os = &u->offload_status;
> +	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> +	__be32 status;
> +	struct nfs4_cp_state *state = NULL;
> +
> +	status = find_cp_state(nn, &os->stateid, &state);
> +	if (state) {
> +		spin_lock(&state->cp_lock);
> +		state->cp_cancelled = true;
> +		spin_unlock(&state->cp_lock);
> +	}
> +
> +	return status;
>  }
>  
>  static __be32
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index be59baf..97ab3f8 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -752,6 +752,22 @@ static void nfs4_free_deleg(struct nfs4_stid *stid)
>  	atomic_long_dec(&num_delegations);
>  }
>  
> +__be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
> +			    struct nfs4_cp_state **cps)
> +{
> +	struct nfs4_cp_state *state = NULL;
> +
> +	if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
> +		return nfserr_bad_stateid;
> +	spin_lock(&nn->s2s_cp_lock);
> +	state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
> +	spin_unlock(&nn->s2s_cp_lock);
> +	if (!state)
> +		return nfserr_bad_stateid;
> +	*cps = state;
> +	return 0;
> +}
> +
>  /*
>   * When we recall a delegation, we should be careful not to hand it
>   * out again straight away.
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 8724955..7a070d5 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -111,6 +111,8 @@ struct nfs4_cp_state {
>  	stateid_t		cp_stateid;
>  	struct list_head	cp_list;	/* per parent nfs4_stid */
>  	struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
> +	bool                    cp_cancelled;   /* copy cancelled */
> +	spinlock_t              cp_lock;
>  };
>  
>  /*
> @@ -647,6 +649,8 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
>  extern int nfsd4_create_copy_queue(void);
>  extern void nfsd4_destroy_copy_queue(void);
> +extern __be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
> +			struct nfs4_cp_state **cps);
>  
>  struct nfs4_file *find_file(struct knfsd_fh *fh);
>  void put_nfs4_file(struct nfs4_file *fi);
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 05/10] NFSD first draft of async copy

[Olga Kornievskaia]

> On Sep 28, 2017, at 2:07 PM, J. Bruce Fields <bfields@redhat.com> =
wrote:
>=20
> On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
>> Asynchronous copies are handled by a single threaded workqueue.
>> If we get asynchronous request, make sure to copy the needed
>> arguments/state from the stack before starting the copy. Then
>> queue work and reply back to the client indicating copy is
>> asynchronous.
>>=20
>> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
>> the total number of bytes need to copy.
>=20
> I don't think you need to do this--just call vfs_copy_file_range()
> directly, as nfsd_copy_file_range does.
>=20
> The 4MB chunking was only there to prevent a synchronous copy from
> taking too long, which isn't an issue in the async case.
>=20

One reason is to allow for the copy to be cancelled in the =E2=80=9Cmiddle=
=E2=80=9D.
Otherwise, we have no control over once we call into the =
vfs_copy_file_range
to stop the on-going copy. At least this way we check every 4MB chunks.

> --b.
>=20
>> In case a failure
>> happens in the middle, we can return an error as well as how
>> much we copied so far. Once done creating a workitem for the
>> callback workqueue and send CB_OFFLOAD with the results.
>>=20
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>> fs/nfsd/nfs4proc.c  | 164 =
++++++++++++++++++++++++++++++++++++++++++++++------
>> fs/nfsd/nfs4state.c |   9 ++-
>> fs/nfsd/state.h     |   2 +
>> fs/nfsd/xdr4.h      |   7 +++
>> 4 files changed, 162 insertions(+), 20 deletions(-)
>>=20
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 11ade90..de21b1a 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -76,6 +76,21 @@
>> { }
>> #endif
>>=20
>> +static struct workqueue_struct *copy_wq;
>> +
>> +int nfsd4_create_copy_queue(void)
>> +{
>> +	copy_wq =3D create_singlethread_workqueue("nfsd4_copy");
>> +	if (!copy_wq)
>> +		return -ENOMEM;
>> +	return 0;
>> +}
>> +
>> +void nfsd4_destroy_copy_queue(void)
>> +{
>> +	destroy_workqueue(copy_wq);
>> +}
>> +
>> #define NFSDDBG_FACILITY		NFSDDBG_PROC
>>=20
>> static u32 nfsd_attrmask[] =3D {
>> @@ -1085,37 +1100,148 @@ static int fill_in_write_vector(struct kvec =
*vec, struct nfsd4_write *write)
>> out:
>> 	return status;
>> }
>> +static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>> +{
>> +	struct nfsd4_copy *copy =3D container_of(cb, struct nfsd4_copy, =
cp_cb);
>> +
>> +	kfree(copy);
>> +}
>> +
>> +static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
>> +				 struct rpc_task *task)
>> +{
>> +	return 1;
>> +}
>> +
>> +static const struct nfsd4_callback_ops nfsd4_cb_offload_ops =3D {
>> +	.release =3D nfsd4_cb_offload_release,
>> +	.done =3D nfsd4_cb_offload_done
>> +};
>> +
>> +static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
>> +{
>> +	memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
>> +		sizeof(copy->cp_dst_stateid));
>> +	copy->cp_res.wr_stable_how =3D NFS_UNSTABLE;
>> +	copy->cp_consecutive =3D 1;
>> +	copy->cp_synchronous =3D sync;
>> +	gen_boot_verifier(&copy->cp_res.wr_verifier, copy->net);
>> +
>> +	return nfs_ok;
>> +}
>> +
>> +static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> +{
>> +	ssize_t bytes_copied =3D 0;
>> +	size_t bytes_total =3D copy->cp_count;
>> +	size_t bytes_to_copy;
>> +	u64 src_pos =3D copy->cp_src_pos;
>> +	u64 dst_pos =3D copy->cp_dst_pos;
>> +
>> +	do {
>> +		bytes_to_copy =3D min_t(u64, bytes_total, MAX_RW_COUNT);
>> +		bytes_copied =3D nfsd_copy_file_range(copy->fh_src, =
src_pos,
>> +					copy->fh_dst, dst_pos, =
bytes_to_copy);
>> +		if (bytes_copied <=3D 0)
>> +			break;
>> +		bytes_total -=3D bytes_copied;
>> +		copy->cp_res.wr_bytes_written +=3D bytes_copied;
>> +		src_pos +=3D bytes_copied;
>> +		dst_pos +=3D bytes_copied;
>> +	} while (bytes_total > 0 && !copy->cp_synchronous);
>> +	return bytes_copied;
>> +}
>> +
>> +static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
>> +{
>> +	__be32 status;
>> +	ssize_t bytes;
>> +
>> +	bytes =3D _nfsd_copy_file_range(copy);
>> +	if (bytes < 0)
>> +		status =3D nfserrno(bytes);
>> +	else
>> +		status =3D nfsd4_init_copy_res(copy, sync);
>> +
>> +	fput(copy->fh_src);
>> +	fput(copy->fh_dst);
>> +	return status;
>> +}
>> +
>> +static void dup_copy_fields(struct nfsd4_copy *src, struct =
nfsd4_copy *dst)
>> +{
>> +	memcpy(&dst->cp_src_stateid, &src->cp_src_stateid, =
sizeof(stateid_t));
>> +	memcpy(&dst->cp_dst_stateid, &src->cp_dst_stateid, =
sizeof(stateid_t));
>> +	dst->cp_src_pos =3D src->cp_src_pos;
>> +	dst->cp_dst_pos =3D src->cp_dst_pos;
>> +	dst->cp_count =3D src->cp_count;
>> +	dst->cp_consecutive =3D src->cp_consecutive;
>> +	dst->cp_synchronous =3D src->cp_synchronous;
>> +	memcpy(&dst->cp_res, &src->cp_res, sizeof(src->cp_res));
>> +	/* skipping nfsd4_callback */
>> +	memcpy(&dst->fh, &src->fh, sizeof(src->fh));
>> +	dst->fh =3D src->fh;
>> +	dst->cp_clp =3D src->cp_clp;
>> +	dst->fh_src =3D src->fh_src;
>> +	dst->fh_dst =3D src->fh_dst;
>> +	dst->net =3D src->net;
>> +}
>> +
>> +static void nfsd4_do_async_copy(struct work_struct *work)
>> +{
>> +	struct nfsd4_copy *copy =3D
>> +		container_of(work, struct nfsd4_copy, cp_work);
>> +	struct nfsd4_copy *cb_copy;
>> +
>> +	copy->nfserr =3D nfsd4_do_copy(copy, 0);
>> +	cb_copy =3D kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>> +	if (!cb_copy)
>> +		goto out;
>> +	memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
>> +	cb_copy->cp_clp =3D copy->cp_clp;
>> +	cb_copy->nfserr =3D copy->nfserr;
>> +	memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
>> +	nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
>> +			&nfsd4_cb_offload_ops, =
NFSPROC4_CLNT_CB_OFFLOAD);
>> +	nfsd4_run_cb(&cb_copy->cp_cb);
>> +out:
>> +	kfree(copy);
>> +}
>>=20
>> static __be32
>> nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state =
*cstate,
>> 		union nfsd4_op_u *u)
>> {
>> 	struct nfsd4_copy *copy =3D &u->copy;
>> -	struct file *src, *dst;
>> 	__be32 status;
>> -	ssize_t bytes;
>>=20
>> -	status =3D nfsd4_verify_copy(rqstp, cstate, =
&copy->cp_src_stateid, &src,
>> -				   &copy->cp_dst_stateid, &dst);
>> +	status =3D nfsd4_verify_copy(rqstp, cstate, =
&copy->cp_src_stateid,
>> +				   &copy->fh_src, &copy->cp_dst_stateid,
>> +				   &copy->fh_dst);
>> 	if (status)
>> 		goto out;
>>=20
>> -	bytes =3D nfsd_copy_file_range(src, copy->cp_src_pos,
>> -			dst, copy->cp_dst_pos, copy->cp_count);
>> -
>> -	if (bytes < 0)
>> -		status =3D nfserrno(bytes);
>> -	else {
>> -		copy->cp_res.wr_bytes_written =3D bytes;
>> -		copy->cp_res.wr_stable_how =3D NFS_UNSTABLE;
>> -		copy->cp_consecutive =3D 1;
>> -		copy->cp_synchronous =3D 1;
>> -		gen_boot_verifier(&copy->cp_res.wr_verifier, =
SVC_NET(rqstp));
>> -		status =3D nfs_ok;
>> +	copy->cp_clp =3D cstate->clp;
>> +	memcpy(&copy->fh, &cstate->current_fh.fh_handle,
>> +		sizeof(struct knfsd_fh));
>> +	copy->net =3D SVC_NET(rqstp);
>> +	if (!copy->cp_synchronous) {
>> +		struct nfsd4_copy *async_copy;
>> +
>> +		status =3D nfsd4_init_copy_res(copy, 0);
>> +		async_copy =3D kzalloc(sizeof(struct nfsd4_copy), =
GFP_KERNEL);
>> +		if (!async_copy) {
>> +			status =3D nfserrno(-ENOMEM);
>> +			goto out;
>> +		}
>> +		dup_copy_fields(copy, async_copy);
>> +		memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
>> +			sizeof(copy->cp_dst_stateid));
>> +		INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
>> +		queue_work(copy_wq, &async_copy->cp_work);
>> +	} else {
>> +		status =3D nfsd4_do_copy(copy, 1);
>> 	}
>> -
>> -	fput(src);
>> -	fput(dst);
>> out:
>> 	return status;
>> }
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 0c04f81..fe6f275 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -7033,8 +7033,14 @@ static int nfs4_state_create_net(struct net =
*net)
>> 		goto out_free_laundry;
>>=20
>> 	set_max_delegations();
>> -	return 0;
>>=20
>> +	ret =3D nfsd4_create_copy_queue();
>> +	if (ret)
>> +		goto out_free_callback;
>> +
>> +	return 0;
>> +out_free_callback:
>> +	nfsd4_destroy_callback_queue();
>> out_free_laundry:
>> 	destroy_workqueue(laundry_wq);
>> out_cleanup_cred:
>> @@ -7097,6 +7103,7 @@ static int nfs4_state_create_net(struct net =
*net)
>> 	destroy_workqueue(laundry_wq);
>> 	nfsd4_destroy_callback_queue();
>> 	cleanup_callback_cred();
>> +	nfsd4_destroy_copy_queue();
>> }
>>=20
>> static void
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index f8b0210..b5358cf 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -630,6 +630,8 @@ extern void nfsd4_init_cb(struct nfsd4_callback =
*cb, struct nfs4_client *clp,
>> extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char =
*name,
>> 							struct nfsd_net =
*nn);
>> extern bool nfs4_has_reclaimed_state(const char *name, struct =
nfsd_net *nn);
>> +extern int nfsd4_create_copy_queue(void);
>> +extern void nfsd4_destroy_copy_queue(void);
>>=20
>> struct nfs4_file *find_file(struct knfsd_fh *fh);
>> void put_nfs4_file(struct nfs4_file *fi);
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index 9b0c099..f7a94d3 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -529,6 +529,13 @@ struct nfsd4_copy {
>> 	struct nfsd4_callback	cp_cb;
>> 	__be32			nfserr;
>> 	struct knfsd_fh		fh;
>> +
>> +	struct work_struct      cp_work;
>> +	struct nfs4_client      *cp_clp;
>> +
>> +	struct file             *fh_src;
>> +	struct file             *fh_dst;
>> +	struct net              *net;
>> };
>>=20
>> struct nfsd4_seek {
>> --=20
>> 1.8.3.1
>>=20

Re: [PATCH v4 05/10] NFSD first draft of async copy

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 02:44:42PM -0400, Olga Kornievskaia wrote:
> 
> > On Sep 28, 2017, at 2:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
> >> Asynchronous copies are handled by a single threaded workqueue.
> >> If we get asynchronous request, make sure to copy the needed
> >> arguments/state from the stack before starting the copy. Then
> >> queue work and reply back to the client indicating copy is
> >> asynchronous.
> >> 
> >> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
> >> the total number of bytes need to copy.
> > 
> > I don't think you need to do this--just call vfs_copy_file_range()
> > directly, as nfsd_copy_file_range does.
> > 
> > The 4MB chunking was only there to prevent a synchronous copy from
> > taking too long, which isn't an issue in the async case.
> > 
> 
> One reason is to allow for the copy to be cancelled in the “middle”.
> Otherwise, we have no control over once we call into the vfs_copy_file_range
> to stop the on-going copy. At least this way we check every 4MB chunks.

Yes, see my other email, I think we should signal the thread to stop it.

Even with copying in chunks--we'd like to be able to cancel the copy
mid-chunk.

Talking this over here with Jeff and Trond.... I don't *think* there's
an easy way to cancel a long-running work item.

So probably you want to create your own thread for the copy instead.  It
looks like kthread_create/kthread_stop/kthread_should_stop are what you
want?  You can see some examples if you "git grep kthread net/sunrpc".

--b.

> 
> > --b.
> > 
> >> In case a failure
> >> happens in the middle, we can return an error as well as how
> >> much we copied so far. Once done creating a workitem for the
> >> callback workqueue and send CB_OFFLOAD with the results.
> >> 
> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >> ---
> >> fs/nfsd/nfs4proc.c  | 164 ++++++++++++++++++++++++++++++++++++++++++++++------
> >> fs/nfsd/nfs4state.c |   9 ++-
> >> fs/nfsd/state.h     |   2 +
> >> fs/nfsd/xdr4.h      |   7 +++
> >> 4 files changed, 162 insertions(+), 20 deletions(-)
> >> 
> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> index 11ade90..de21b1a 100644
> >> --- a/fs/nfsd/nfs4proc.c
> >> +++ b/fs/nfsd/nfs4proc.c
> >> @@ -76,6 +76,21 @@
> >> { }
> >> #endif
> >> 
> >> +static struct workqueue_struct *copy_wq;
> >> +
> >> +int nfsd4_create_copy_queue(void)
> >> +{
> >> +	copy_wq = create_singlethread_workqueue("nfsd4_copy");
> >> +	if (!copy_wq)
> >> +		return -ENOMEM;
> >> +	return 0;
> >> +}
> >> +
> >> +void nfsd4_destroy_copy_queue(void)
> >> +{
> >> +	destroy_workqueue(copy_wq);
> >> +}
> >> +
> >> #define NFSDDBG_FACILITY		NFSDDBG_PROC
> >> 
> >> static u32 nfsd_attrmask[] = {
> >> @@ -1085,37 +1100,148 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
> >> out:
> >> 	return status;
> >> }
> >> +static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> >> +{
> >> +	struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);
> >> +
> >> +	kfree(copy);
> >> +}
> >> +
> >> +static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
> >> +				 struct rpc_task *task)
> >> +{
> >> +	return 1;
> >> +}
> >> +
> >> +static const struct nfsd4_callback_ops nfsd4_cb_offload_ops = {
> >> +	.release = nfsd4_cb_offload_release,
> >> +	.done = nfsd4_cb_offload_done
> >> +};
> >> +
> >> +static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
> >> +{
> >> +	memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> >> +		sizeof(copy->cp_dst_stateid));
> >> +	copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> >> +	copy->cp_consecutive = 1;
> >> +	copy->cp_synchronous = sync;
> >> +	gen_boot_verifier(&copy->cp_res.wr_verifier, copy->net);
> >> +
> >> +	return nfs_ok;
> >> +}
> >> +
> >> +static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >> +{
> >> +	ssize_t bytes_copied = 0;
> >> +	size_t bytes_total = copy->cp_count;
> >> +	size_t bytes_to_copy;
> >> +	u64 src_pos = copy->cp_src_pos;
> >> +	u64 dst_pos = copy->cp_dst_pos;
> >> +
> >> +	do {
> >> +		bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> >> +		bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
> >> +					copy->fh_dst, dst_pos, bytes_to_copy);
> >> +		if (bytes_copied <= 0)
> >> +			break;
> >> +		bytes_total -= bytes_copied;
> >> +		copy->cp_res.wr_bytes_written += bytes_copied;
> >> +		src_pos += bytes_copied;
> >> +		dst_pos += bytes_copied;
> >> +	} while (bytes_total > 0 && !copy->cp_synchronous);
> >> +	return bytes_copied;
> >> +}
> >> +
> >> +static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
> >> +{
> >> +	__be32 status;
> >> +	ssize_t bytes;
> >> +
> >> +	bytes = _nfsd_copy_file_range(copy);
> >> +	if (bytes < 0)
> >> +		status = nfserrno(bytes);
> >> +	else
> >> +		status = nfsd4_init_copy_res(copy, sync);
> >> +
> >> +	fput(copy->fh_src);
> >> +	fput(copy->fh_dst);
> >> +	return status;
> >> +}
> >> +
> >> +static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> >> +{
> >> +	memcpy(&dst->cp_src_stateid, &src->cp_src_stateid, sizeof(stateid_t));
> >> +	memcpy(&dst->cp_dst_stateid, &src->cp_dst_stateid, sizeof(stateid_t));
> >> +	dst->cp_src_pos = src->cp_src_pos;
> >> +	dst->cp_dst_pos = src->cp_dst_pos;
> >> +	dst->cp_count = src->cp_count;
> >> +	dst->cp_consecutive = src->cp_consecutive;
> >> +	dst->cp_synchronous = src->cp_synchronous;
> >> +	memcpy(&dst->cp_res, &src->cp_res, sizeof(src->cp_res));
> >> +	/* skipping nfsd4_callback */
> >> +	memcpy(&dst->fh, &src->fh, sizeof(src->fh));
> >> +	dst->fh = src->fh;
> >> +	dst->cp_clp = src->cp_clp;
> >> +	dst->fh_src = src->fh_src;
> >> +	dst->fh_dst = src->fh_dst;
> >> +	dst->net = src->net;
> >> +}
> >> +
> >> +static void nfsd4_do_async_copy(struct work_struct *work)
> >> +{
> >> +	struct nfsd4_copy *copy =
> >> +		container_of(work, struct nfsd4_copy, cp_work);
> >> +	struct nfsd4_copy *cb_copy;
> >> +
> >> +	copy->nfserr = nfsd4_do_copy(copy, 0);
> >> +	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >> +	if (!cb_copy)
> >> +		goto out;
> >> +	memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
> >> +	cb_copy->cp_clp = copy->cp_clp;
> >> +	cb_copy->nfserr = copy->nfserr;
> >> +	memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
> >> +	nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
> >> +			&nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
> >> +	nfsd4_run_cb(&cb_copy->cp_cb);
> >> +out:
> >> +	kfree(copy);
> >> +}
> >> 
> >> static __be32
> >> nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >> 		union nfsd4_op_u *u)
> >> {
> >> 	struct nfsd4_copy *copy = &u->copy;
> >> -	struct file *src, *dst;
> >> 	__be32 status;
> >> -	ssize_t bytes;
> >> 
> >> -	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid, &src,
> >> -				   &copy->cp_dst_stateid, &dst);
> >> +	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> >> +				   &copy->fh_src, &copy->cp_dst_stateid,
> >> +				   &copy->fh_dst);
> >> 	if (status)
> >> 		goto out;
> >> 
> >> -	bytes = nfsd_copy_file_range(src, copy->cp_src_pos,
> >> -			dst, copy->cp_dst_pos, copy->cp_count);
> >> -
> >> -	if (bytes < 0)
> >> -		status = nfserrno(bytes);
> >> -	else {
> >> -		copy->cp_res.wr_bytes_written = bytes;
> >> -		copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> >> -		copy->cp_consecutive = 1;
> >> -		copy->cp_synchronous = 1;
> >> -		gen_boot_verifier(&copy->cp_res.wr_verifier, SVC_NET(rqstp));
> >> -		status = nfs_ok;
> >> +	copy->cp_clp = cstate->clp;
> >> +	memcpy(&copy->fh, &cstate->current_fh.fh_handle,
> >> +		sizeof(struct knfsd_fh));
> >> +	copy->net = SVC_NET(rqstp);
> >> +	if (!copy->cp_synchronous) {
> >> +		struct nfsd4_copy *async_copy;
> >> +
> >> +		status = nfsd4_init_copy_res(copy, 0);
> >> +		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >> +		if (!async_copy) {
> >> +			status = nfserrno(-ENOMEM);
> >> +			goto out;
> >> +		}
> >> +		dup_copy_fields(copy, async_copy);
> >> +		memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> >> +			sizeof(copy->cp_dst_stateid));
> >> +		INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
> >> +		queue_work(copy_wq, &async_copy->cp_work);
> >> +	} else {
> >> +		status = nfsd4_do_copy(copy, 1);
> >> 	}
> >> -
> >> -	fput(src);
> >> -	fput(dst);
> >> out:
> >> 	return status;
> >> }
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 0c04f81..fe6f275 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -7033,8 +7033,14 @@ static int nfs4_state_create_net(struct net *net)
> >> 		goto out_free_laundry;
> >> 
> >> 	set_max_delegations();
> >> -	return 0;
> >> 
> >> +	ret = nfsd4_create_copy_queue();
> >> +	if (ret)
> >> +		goto out_free_callback;
> >> +
> >> +	return 0;
> >> +out_free_callback:
> >> +	nfsd4_destroy_callback_queue();
> >> out_free_laundry:
> >> 	destroy_workqueue(laundry_wq);
> >> out_cleanup_cred:
> >> @@ -7097,6 +7103,7 @@ static int nfs4_state_create_net(struct net *net)
> >> 	destroy_workqueue(laundry_wq);
> >> 	nfsd4_destroy_callback_queue();
> >> 	cleanup_callback_cred();
> >> +	nfsd4_destroy_copy_queue();
> >> }
> >> 
> >> static void
> >> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >> index f8b0210..b5358cf 100644
> >> --- a/fs/nfsd/state.h
> >> +++ b/fs/nfsd/state.h
> >> @@ -630,6 +630,8 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
> >> extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
> >> 							struct nfsd_net *nn);
> >> extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
> >> +extern int nfsd4_create_copy_queue(void);
> >> +extern void nfsd4_destroy_copy_queue(void);
> >> 
> >> struct nfs4_file *find_file(struct knfsd_fh *fh);
> >> void put_nfs4_file(struct nfs4_file *fi);
> >> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> >> index 9b0c099..f7a94d3 100644
> >> --- a/fs/nfsd/xdr4.h
> >> +++ b/fs/nfsd/xdr4.h
> >> @@ -529,6 +529,13 @@ struct nfsd4_copy {
> >> 	struct nfsd4_callback	cp_cb;
> >> 	__be32			nfserr;
> >> 	struct knfsd_fh		fh;
> >> +
> >> +	struct work_struct      cp_work;
> >> +	struct nfs4_client      *cp_clp;
> >> +
> >> +	struct file             *fh_src;
> >> +	struct file             *fh_dst;
> >> +	struct net              *net;
> >> };
> >> 
> >> struct nfsd4_seek {
> >> -- 
> >> 1.8.3.1
> >> 
> 

Re: [PATCH v4 05/10] NFSD first draft of async copy

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 03:04:13PM -0400, Olga Kornievskaia wrote:
> 
> > On Sep 28, 2017, at 2:55 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > On Thu, Sep 28, 2017 at 02:44:42PM -0400, Olga Kornievskaia wrote:
> >> 
> >>> On Sep 28, 2017, at 2:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> >>> 
> >>> On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
> >>>> Asynchronous copies are handled by a single threaded workqueue.
> >>>> If we get asynchronous request, make sure to copy the needed
> >>>> arguments/state from the stack before starting the copy. Then
> >>>> queue work and reply back to the client indicating copy is
> >>>> asynchronous.
> >>>> 
> >>>> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
> >>>> the total number of bytes need to copy.
> >>> 
> >>> I don't think you need to do this--just call vfs_copy_file_range()
> >>> directly, as nfsd_copy_file_range does.
> >>> 
> >>> The 4MB chunking was only there to prevent a synchronous copy from
> >>> taking too long, which isn't an issue in the async case.
> >>> 
> >> 
> >> One reason is to allow for the copy to be cancelled in the “middle”.
> >> Otherwise, we have no control over once we call into the vfs_copy_file_range
> >> to stop the on-going copy. At least this way we check every 4MB chunks.
> > 
> > Yes, see my other email, I think we should signal the thread to stop it.
> > 
> > Even with copying in chunks--we'd like to be able to cancel the copy
> > mid-chunk.
> > 
> > Talking this over here with Jeff and Trond.... I don't *think* there's
> > an easy way to cancel a long-running work item.
> 
> Yes I believe I was looking into signaling but couldn’t figure out how to do it
> with the current code...
> 
> > So probably you want to create your own thread for the copy instead.  It
> > looks like kthread_create/kthread_stop/kthread_should_stop are what you
> > want?  You can see some examples if you "git grep kthread net/sunrpc”.
> 
> Ok so should I instead create a dedicated thread to do all copies and have 
> a way to give this thread work? Or are you ok with creating a thread for
> each individual copy which would be much simpler?

I'd be in favor of a thread per copy.  That should make the cancelling
either as well.

--b.

Re: [PATCH v4 05/10] NFSD first draft of async copy

[Olga Kornievskaia]

On Thu, Sep 28, 2017 at 3:07 PM, J. Bruce Fields <bfields@redhat.com> wrote=
:
> On Thu, Sep 28, 2017 at 03:04:13PM -0400, Olga Kornievskaia wrote:
>>
>> > On Sep 28, 2017, at 2:55 PM, J. Bruce Fields <bfields@redhat.com> wrot=
e:
>> >
>> > On Thu, Sep 28, 2017 at 02:44:42PM -0400, Olga Kornievskaia wrote:
>> >>
>> >>> On Sep 28, 2017, at 2:07 PM, J. Bruce Fields <bfields@redhat.com> wr=
ote:
>> >>>
>> >>> On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
>> >>>> Asynchronous copies are handled by a single threaded workqueue.
>> >>>> If we get asynchronous request, make sure to copy the needed
>> >>>> arguments/state from the stack before starting the copy. Then
>> >>>> queue work and reply back to the client indicating copy is
>> >>>> asynchronous.
>> >>>>
>> >>>> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
>> >>>> the total number of bytes need to copy.
>> >>>
>> >>> I don't think you need to do this--just call vfs_copy_file_range()
>> >>> directly, as nfsd_copy_file_range does.
>> >>>
>> >>> The 4MB chunking was only there to prevent a synchronous copy from
>> >>> taking too long, which isn't an issue in the async case.
>> >>>
>> >>
>> >> One reason is to allow for the copy to be cancelled in the =E2=80=9Cm=
iddle=E2=80=9D.
>> >> Otherwise, we have no control over once we call into the vfs_copy_fil=
e_range
>> >> to stop the on-going copy. At least this way we check every 4MB chunk=
s.
>> >
>> > Yes, see my other email, I think we should signal the thread to stop i=
t.
>> >
>> > Even with copying in chunks--we'd like to be able to cancel the copy
>> > mid-chunk.
>> >
>> > Talking this over here with Jeff and Trond.... I don't *think* there's
>> > an easy way to cancel a long-running work item.
>>
>> Yes I believe I was looking into signaling but couldn=E2=80=99t figure o=
ut how to do it
>> with the current code...
>>
>> > So probably you want to create your own thread for the copy instead.  =
It
>> > looks like kthread_create/kthread_stop/kthread_should_stop are what yo=
u
>> > want?  You can see some examples if you "git grep kthread net/sunrpc=
=E2=80=9D.
>>
>> Ok so should I instead create a dedicated thread to do all copies and ha=
ve
>> a way to give this thread work? Or are you ok with creating a thread for
>> each individual copy which would be much simpler?
>
> I'd be in favor of a thread per copy.  That should make the cancelling
> either as well.

ok thanks. will work on that.

Re: [PATCH v4 07/10] NFSD create new stateid for async copy

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 01:29:42PM -0400, Olga Kornievskaia wrote:
> Generate a new stateid to be used for reply to the asynchronous
> COPY (this would also be used later by COPY_NOTIFY as well).
> Associate the stateid with the parent OPEN/LOCK/DELEG stateid
> that can be freed during the free of the parent stateid. However,
> right now deciding to bind the lifetime to when the vfs copy
> is done. This way don't need to keep the nfsd_net structure for
> the callback. The drawback is that time copy state information
> is available for query by OFFLOAD_STATUS is slightly less.

I don't think we're under any obligation to handle OFFLOAD_STATUS after
the copy is done, are we?

There's a possibility that OFFLOAD_STATUS might arrive just as the copy
finishes, and the client might even received the failed response to
OFFLOAD_STATUS before it receives the final CB_OFFLOAD.  I think it's up
to the client to handle that situation.

--b.

> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/netns.h     |  8 +++++++
>  fs/nfsd/nfs4proc.c  | 31 ++++++++++++++++++-------
>  fs/nfsd/nfs4state.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/nfsd/nfsctl.c    |  1 +
>  fs/nfsd/state.h     | 14 ++++++++++++
>  fs/nfsd/xdr4.h      |  2 ++
>  6 files changed, 114 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 3714231..2c88a95 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -119,6 +119,14 @@ struct nfsd_net {
>  	u32 clverifier_counter;
>  
>  	struct svc_serv *nfsd_serv;
> +
> +	/*
> +	 * clientid and stateid data for construction of net unique COPY
> +	 * stateids.
> +	 */
> +	u32		s2s_cp_cl_id;
> +	struct idr	s2s_cp_stateids;
> +	spinlock_t	s2s_cp_lock;
>  };
>  
>  /* Simple check to find out if a given net was properly initialized */
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 044af6b..3cddebb 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1046,7 +1046,8 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>  static __be32
>  nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		  stateid_t *src_stateid, struct file **src,
> -		  stateid_t *dst_stateid, struct file **dst)
> +		  stateid_t *dst_stateid, struct file **dst,
> +		  struct nfs4_stid **stid)
>  {
>  	__be32 status;
>  
> @@ -1060,7 +1061,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					    dst_stateid, WR_STATE, dst, NULL,
> -					    NULL);
> +					    stid);
>  	if (status) {
>  		dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
>  		goto out_put_src;
> @@ -1091,7 +1092,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>  	__be32 status;
>  
>  	status = nfsd4_verify_copy(rqstp, cstate, &clone->cl_src_stateid, &src,
> -				   &clone->cl_dst_stateid, &dst);
> +				   &clone->cl_dst_stateid, &dst, NULL);
>  	if (status)
>  		goto out;
>  
> @@ -1123,8 +1124,6 @@ static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
>  
>  static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
>  {
> -	memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> -		sizeof(copy->cp_dst_stateid));
>  	copy->cp_res.wr_stable_how = NFS_UNSTABLE;
>  	copy->cp_consecutive = 1;
>  	copy->cp_synchronous = sync;
> @@ -1188,6 +1187,8 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>  	dst->fh_src = src->fh_src;
>  	dst->fh_dst = src->fh_dst;
>  	dst->net = src->net;
> +	dst->stid = src->stid;
> +	dst->cps = src->cps;
>  }
>  
>  static void nfsd4_do_async_copy(struct work_struct *work)
> @@ -1208,6 +1209,9 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>  			&nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
>  	nfsd4_run_cb(&cb_copy->cp_cb);
>  out:
> +	list_del(&copy->cps->cp_list);
> +	nfs4_free_cp_state(copy->cps);
> +	nfs4_put_stid(copy->stid);
>  	kfree(copy);
>  }
>  
> @@ -1220,7 +1224,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>  
>  	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
>  				   &copy->fh_src, &copy->cp_dst_stateid,
> -				   &copy->fh_dst);
> +				   &copy->fh_dst, &copy->stid);
>  	if (status)
>  		goto out;
>  
> @@ -1229,6 +1233,7 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>  		sizeof(struct knfsd_fh));
>  	copy->net = SVC_NET(rqstp);
>  	if (!copy->cp_synchronous) {
> +		struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  		struct nfsd4_copy *async_copy;
>  
>  		status = nfsd4_init_copy_res(copy, 0);
> @@ -1237,9 +1242,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>  			status = nfserrno(-ENOMEM);
>  			goto out;
>  		}
> +		copy->cps = nfs4_alloc_init_cp_state(nn, copy->stid);
> +		if (!copy->cps) {
> +			status = nfserrno(-ENOMEM);
> +			kfree(async_copy);
> +			goto out;
> +		}
> +		/* take a reference on the parent stateid so it's not
> +		 * not freed by the copy compound
> +		 */
> +		atomic_inc(&copy->stid->sc_count);
> +		memcpy(&copy->cp_res.cb_stateid, &copy->cps->cp_stateid,
> +			sizeof(copy->cps->cp_stateid));
>  		dup_copy_fields(copy, async_copy);
> -		memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> -			sizeof(copy->cp_dst_stateid));
>  		INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
>  		queue_work(copy_wq, &async_copy->cp_work);
>  	} else {
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2c89dae..be59baf 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -658,6 +658,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
>  	/* Will be incremented before return to client: */
>  	atomic_set(&stid->sc_count, 1);
>  	spin_lock_init(&stid->sc_lock);
> +	INIT_LIST_HEAD(&stid->sc_cp_list);
>  
>  	/*
>  	 * It shouldn't be a problem to reuse an opaque stateid value.
> @@ -674,6 +675,66 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
>  	return NULL;
>  }
>  
> +/*
> + * Create a unique stateid_t to represent each COPY. Hang the copy
> + * stateids off the OPEN/LOCK/DELEG stateid from the client open
> + * the source file.
> + */
> +struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn,
> +					       struct nfs4_stid *p_stid)
> +{
> +	struct nfs4_cp_state *cps;
> +	int new_id;
> +
> +	cps = kzalloc(sizeof(struct nfs4_cp_state), GFP_KERNEL);
> +	if (!cps)
> +		return NULL;
> +	idr_preload(GFP_KERNEL);
> +	spin_lock(&nn->s2s_cp_lock);
> +	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, cps, 0, 0, GFP_NOWAIT);
> +	spin_unlock(&nn->s2s_cp_lock);
> +	idr_preload_end();
> +	if (new_id < 0)
> +		goto out_free;
> +	cps->cp_stateid.si_opaque.so_id = new_id;
> +	cps->cp_stateid.si_opaque.so_clid.cl_boot = nn->boot_time;
> +	cps->cp_stateid.si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
> +	cps->cp_p_stid = p_stid;
> +	INIT_LIST_HEAD(&cps->cp_list);
> +	list_add(&cps->cp_list, &p_stid->sc_cp_list);
> +
> +	return cps;
> +out_free:
> +	kfree(cps);
> +	return NULL;
> +}
> +
> +void nfs4_free_cp_state(struct nfs4_cp_state *cps)
> +{
> +	struct nfsd_net *nn;
> +
> +	nn = net_generic(cps->cp_p_stid->sc_client->net, nfsd_net_id);
> +	spin_lock(&nn->s2s_cp_lock);
> +	idr_remove(&nn->s2s_cp_stateids, cps->cp_stateid.si_opaque.so_id);
> +	spin_unlock(&nn->s2s_cp_lock);
> +
> +	kfree(cps);
> +}
> +
> +static void nfs4_free_cp_statelist(struct nfs4_stid *stid)
> +{
> +	struct nfs4_cp_state *cps;
> +
> +	might_sleep();
> +
> +	while (!list_empty(&stid->sc_cp_list)) {
> +		cps = list_first_entry(&stid->sc_cp_list, struct nfs4_cp_state,
> +				       cp_list);
> +		list_del(&cps->cp_list);
> +		nfs4_free_cp_state(cps);
> +	}
> +}
> +
>  static struct nfs4_ol_stateid * nfs4_alloc_open_stateid(struct nfs4_client *clp)
>  {
>  	struct nfs4_stid *stid;
> @@ -819,6 +880,9 @@ static void block_delegations(struct knfsd_fh *fh)
>  	}
>  	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
>  	spin_unlock(&clp->cl_lock);
> +
> +	nfs4_free_cp_statelist(s);
> +
>  	s->sc_free(s);
>  	if (fp)
>  		put_nfs4_file(fp);
> @@ -6952,6 +7016,8 @@ static int nfs4_state_create_net(struct net *net)
>  	INIT_LIST_HEAD(&nn->close_lru);
>  	INIT_LIST_HEAD(&nn->del_recall_lru);
>  	spin_lock_init(&nn->client_lock);
> +	spin_lock_init(&nn->s2s_cp_lock);
> +	idr_init(&nn->s2s_cp_stateids);
>  
>  	spin_lock_init(&nn->blocked_locks_lock);
>  	INIT_LIST_HEAD(&nn->blocked_locks_lru);
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 6493df6..232270d 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1241,6 +1241,7 @@ static __net_init int nfsd_init_net(struct net *net)
>  	nn->nfsd4_grace = 90;
>  	nn->clverifier_counter = prandom_u32();
>  	nn->clientid_counter = prandom_u32();
> +	nn->s2s_cp_cl_id = nn->clientid_counter++;
>  	return 0;
>  
>  out_idmap_error:
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index cfd13f0..8724955 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -93,6 +93,7 @@ struct nfs4_stid {
>  #define NFS4_REVOKED_DELEG_STID 16
>  #define NFS4_CLOSED_DELEG_STID 32
>  #define NFS4_LAYOUT_STID 64
> +	struct list_head	sc_cp_list;
>  	unsigned char		sc_type;
>  	stateid_t		sc_stateid;
>  	spinlock_t		sc_lock;
> @@ -102,6 +103,17 @@ struct nfs4_stid {
>  };
>  
>  /*
> + * Keep a list of stateids issued by the COPY, associate it with the
> + * parent OPEN/LOCK/DELEG stateid. Used for lookup by
> + * OFFLOAD_CANCEL and OFFLOAD_STATUS (as well as COPY_NOTIFY)
> + */
> +struct nfs4_cp_state {
> +	stateid_t		cp_stateid;
> +	struct list_head	cp_list;	/* per parent nfs4_stid */
> +	struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
> +};
> +
> +/*
>   * Represents a delegation stateid. The nfs4_client holds references to these
>   * and they are put when it is being destroyed or when the delegation is
>   * returned by the client:
> @@ -607,6 +619,8 @@ __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		     struct nfs4_stid **s, struct nfsd_net *nn);
>  struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *slab,
>  				  void (*sc_free)(struct nfs4_stid *));
> +struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn, struct nfs4_stid *p_stid);
> +void nfs4_free_cp_state(struct nfs4_cp_state *cps);
>  void nfs4_unhash_stid(struct nfs4_stid *s);
>  void nfs4_put_stid(struct nfs4_stid *s);
>  void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index f7a94d3..4ccd43b 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -536,6 +536,8 @@ struct nfsd4_copy {
>  	struct file             *fh_src;
>  	struct file             *fh_dst;
>  	struct net              *net;
> +	struct nfs4_stid        *stid;
> +	struct nfs4_cp_state	*cps;
>  };
>  
>  struct nfsd4_seek {
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 10/10] NFSD stop queued async copies on client shutdown

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 01:29:45PM -0400, Olga Kornievskaia wrote:
> If client is shutting down and there are still async copies going
> on, then stop queued async copies.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4proc.c  | 9 +++++++++
>  fs/nfsd/nfs4state.c | 1 +
>  fs/nfsd/state.h     | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 74bfa4b..8d0c87f 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -91,6 +91,12 @@ void nfsd4_destroy_copy_queue(void)
>  	destroy_workqueue(copy_wq);
>  }
>  
> +void nfsd4_shutdown_copy(struct nfs4_client *clp)
> +{
> +	set_bit(NFSD4_CLIENT_COPY_KILL, &clp->cl_flags);
> +	flush_workqueue(copy_wq);

Looks like that will just stop new copies and wait on any ongoing ones.
That could be a long wait.

A signal should help there too.

If you still need a "this client is dying" bit in cl_flags, you could
probably just use CB_KILL instead of this new one.

--b.

> +}
> +
>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>  
>  static u32 nfsd_attrmask[] = {
> @@ -1205,6 +1211,9 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>  		container_of(work, struct nfsd4_copy, cp_work);
>  	struct nfsd4_copy *cb_copy;
>  
> +	if (test_bit(NFSD4_CLIENT_COPY_KILL, &copy->cp_clp->cl_flags))
> +		goto out;
> +
>  	copy->nfserr = nfsd4_do_copy(copy, 0);
>  
>  	if (copy->cps->cp_cancelled)
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 97ab3f8..f4b193e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1961,6 +1961,7 @@ static __be32 mark_client_expired_locked(struct nfs4_client *clp)
>  	}
>  	nfsd4_return_all_client_layouts(clp);
>  	nfsd4_shutdown_callback(clp);
> +	nfsd4_shutdown_copy(clp);
>  	if (clp->cl_cb_conn.cb_xprt)
>  		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
>  	free_client(clp);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index c31025c..55f09a9 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -336,6 +336,7 @@ struct nfs4_client {
>  #define NFSD4_CLIENT_RECLAIM_COMPLETE	(3)	/* reclaim_complete done */
>  #define NFSD4_CLIENT_CONFIRMED		(4)	/* client is confirmed */
>  #define NFSD4_CLIENT_UPCALL_LOCK	(5)	/* upcall serialization */
> +#define NFSD4_CLIENT_COPY_KILL		(6)	/* stop copy workqueue */
>  #define NFSD4_CLIENT_CB_FLAG_MASK	(1 << NFSD4_CLIENT_CB_UPDATE | \
>  					 1 << NFSD4_CLIENT_CB_KILL)
>  	unsigned long		cl_flags;
> @@ -644,6 +645,7 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
>  extern int nfsd4_create_callback_queue(void);
>  extern void nfsd4_destroy_callback_queue(void);
>  extern void nfsd4_shutdown_callback(struct nfs4_client *);
> +extern void nfsd4_shutdown_copy(struct nfs4_client *clp);
>  extern void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp);
>  extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>  							struct nfsd_net *nn);
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 07/10] NFSD create new stateid for async copy

[Olga Kornievskaia]

On Thu, Sep 28, 2017 at 3:12 PM, J. Bruce Fields <bfields@redhat.com> wrote=
:
> On Thu, Sep 28, 2017 at 01:29:42PM -0400, Olga Kornievskaia wrote:
>> Generate a new stateid to be used for reply to the asynchronous
>> COPY (this would also be used later by COPY_NOTIFY as well).
>> Associate the stateid with the parent OPEN/LOCK/DELEG stateid
>> that can be freed during the free of the parent stateid. However,
>> right now deciding to bind the lifetime to when the vfs copy
>> is done. This way don't need to keep the nfsd_net structure for
>> the callback. The drawback is that time copy state information
>> is available for query by OFFLOAD_STATUS is slightly less.
>
> I don't think we're under any obligation to handle OFFLOAD_STATUS after
> the copy is done, are we?

It depends on how you define =E2=80=9Ccopy is done=E2=80=9D. I=E2=80=99d sa=
y copy is done once the
server sent the CB_OFFLOAD. But I=E2=80=99m choosing to clear the state onc=
e the
vfs_copy_file_range() is done. Then the callback is queued up (what if
there is wait until the callback can be processed) and there is a period
where if the client were to send a OFFLOAD_STATUS it should get the
bytes representing the full copy but instead the client will get an error b=
ack.

I was worried about what if some implementation depend on querying for
the status and stopping to poll only when they received that all bytes were
copied and they might not get that with how the server is coded right now?

Thus I thought I=E2=80=99d bring it up to decide if the freeing should be m=
oved into
the callback process (but then more stuff needed to be copied)=E2=80=A6.

> There's a possibility that OFFLOAD_STATUS might arrive just as the copy
> finishes, and the client might even received the failed response to
> OFFLOAD_STATUS before it receives the final CB_OFFLOAD.  I think it's up
> to the client to handle that situation.
>
> --b.
>
>>
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfsd/netns.h     |  8 +++++++
>>  fs/nfsd/nfs4proc.c  | 31 ++++++++++++++++++-------
>>  fs/nfsd/nfs4state.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++=
+++++++
>>  fs/nfsd/nfsctl.c    |  1 +
>>  fs/nfsd/state.h     | 14 ++++++++++++
>>  fs/nfsd/xdr4.h      |  2 ++
>>  6 files changed, 114 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> index 3714231..2c88a95 100644
>> --- a/fs/nfsd/netns.h
>> +++ b/fs/nfsd/netns.h
>> @@ -119,6 +119,14 @@ struct nfsd_net {
>>       u32 clverifier_counter;
>>
>>       struct svc_serv *nfsd_serv;
>> +
>> +     /*
>> +      * clientid and stateid data for construction of net unique COPY
>> +      * stateids.
>> +      */
>> +     u32             s2s_cp_cl_id;
>> +     struct idr      s2s_cp_stateids;
>> +     spinlock_t      s2s_cp_lock;
>>  };
>>
>>  /* Simple check to find out if a given net was properly initialized */
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 044af6b..3cddebb 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1046,7 +1046,8 @@ static int fill_in_write_vector(struct kvec *vec, =
struct nfsd4_write *write)
>>  static __be32
>>  nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *=
cstate,
>>                 stateid_t *src_stateid, struct file **src,
>> -               stateid_t *dst_stateid, struct file **dst)
>> +               stateid_t *dst_stateid, struct file **dst,
>> +               struct nfs4_stid **stid)
>>  {
>>       __be32 status;
>>
>> @@ -1060,7 +1061,7 @@ static int fill_in_write_vector(struct kvec *vec, =
struct nfsd4_write *write)
>>
>>       status =3D nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->curr=
ent_fh,
>>                                           dst_stateid, WR_STATE, dst, NU=
LL,
>> -                                         NULL);
>> +                                         stid);
>>       if (status) {
>>               dprintk("NFSD: %s: couldn't process dst stateid!\n", __fun=
c__);
>>               goto out_put_src;
>> @@ -1091,7 +1092,7 @@ static int fill_in_write_vector(struct kvec *vec, =
struct nfsd4_write *write)
>>       __be32 status;
>>
>>       status =3D nfsd4_verify_copy(rqstp, cstate, &clone->cl_src_stateid=
, &src,
>> -                                &clone->cl_dst_stateid, &dst);
>> +                                &clone->cl_dst_stateid, &dst, NULL);
>>       if (status)
>>               goto out;
>>
>> @@ -1123,8 +1124,6 @@ static int nfsd4_cb_offload_done(struct nfsd4_call=
back *cb,
>>
>>  static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
>>  {
>> -     memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
>> -             sizeof(copy->cp_dst_stateid));
>>       copy->cp_res.wr_stable_how =3D NFS_UNSTABLE;
>>       copy->cp_consecutive =3D 1;
>>       copy->cp_synchronous =3D sync;
>> @@ -1188,6 +1187,8 @@ static void dup_copy_fields(struct nfsd4_copy *src=
, struct nfsd4_copy *dst)
>>       dst->fh_src =3D src->fh_src;
>>       dst->fh_dst =3D src->fh_dst;
>>       dst->net =3D src->net;
>> +     dst->stid =3D src->stid;
>> +     dst->cps =3D src->cps;
>>  }
>>
>>  static void nfsd4_do_async_copy(struct work_struct *work)
>> @@ -1208,6 +1209,9 @@ static void nfsd4_do_async_copy(struct work_struct=
 *work)
>>                       &nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
>>       nfsd4_run_cb(&cb_copy->cp_cb);
>>  out:
>> +     list_del(&copy->cps->cp_list);
>> +     nfs4_free_cp_state(copy->cps);
>> +     nfs4_put_stid(copy->stid);
>>       kfree(copy);
>>  }
>>
>> @@ -1220,7 +1224,7 @@ static void nfsd4_do_async_copy(struct work_struct=
 *work)
>>
>>       status =3D nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
>>                                  &copy->fh_src, &copy->cp_dst_stateid,
>> -                                &copy->fh_dst);
>> +                                &copy->fh_dst, &copy->stid);
>>       if (status)
>>               goto out;
>>
>> @@ -1229,6 +1233,7 @@ static void nfsd4_do_async_copy(struct work_struct=
 *work)
>>               sizeof(struct knfsd_fh));
>>       copy->net =3D SVC_NET(rqstp);
>>       if (!copy->cp_synchronous) {
>> +             struct nfsd_net *nn =3D net_generic(SVC_NET(rqstp), nfsd_n=
et_id);
>>               struct nfsd4_copy *async_copy;
>>
>>               status =3D nfsd4_init_copy_res(copy, 0);
>> @@ -1237,9 +1242,19 @@ static void nfsd4_do_async_copy(struct work_struc=
t *work)
>>                       status =3D nfserrno(-ENOMEM);
>>                       goto out;
>>               }
>> +             copy->cps =3D nfs4_alloc_init_cp_state(nn, copy->stid);
>> +             if (!copy->cps) {
>> +                     status =3D nfserrno(-ENOMEM);
>> +                     kfree(async_copy);
>> +                     goto out;
>> +             }
>> +             /* take a reference on the parent stateid so it's not
>> +              * not freed by the copy compound
>> +              */
>> +             atomic_inc(&copy->stid->sc_count);
>> +             memcpy(&copy->cp_res.cb_stateid, &copy->cps->cp_stateid,
>> +                     sizeof(copy->cps->cp_stateid));
>>               dup_copy_fields(copy, async_copy);
>> -             memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
>> -                     sizeof(copy->cp_dst_stateid));
>>               INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
>>               queue_work(copy_wq, &async_copy->cp_work);
>>       } else {
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 2c89dae..be59baf 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -658,6 +658,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client=
 *cl, struct kmem_cache *sla
>>       /* Will be incremented before return to client: */
>>       atomic_set(&stid->sc_count, 1);
>>       spin_lock_init(&stid->sc_lock);
>> +     INIT_LIST_HEAD(&stid->sc_cp_list);
>>
>>       /*
>>        * It shouldn't be a problem to reuse an opaque stateid value.
>> @@ -674,6 +675,66 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_clien=
t *cl, struct kmem_cache *sla
>>       return NULL;
>>  }
>>
>> +/*
>> + * Create a unique stateid_t to represent each COPY. Hang the copy
>> + * stateids off the OPEN/LOCK/DELEG stateid from the client open
>> + * the source file.
>> + */
>> +struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn,
>> +                                            struct nfs4_stid *p_stid)
>> +{
>> +     struct nfs4_cp_state *cps;
>> +     int new_id;
>> +
>> +     cps =3D kzalloc(sizeof(struct nfs4_cp_state), GFP_KERNEL);
>> +     if (!cps)
>> +             return NULL;
>> +     idr_preload(GFP_KERNEL);
>> +     spin_lock(&nn->s2s_cp_lock);
>> +     new_id =3D idr_alloc_cyclic(&nn->s2s_cp_stateids, cps, 0, 0, GFP_N=
OWAIT);
>> +     spin_unlock(&nn->s2s_cp_lock);
>> +     idr_preload_end();
>> +     if (new_id < 0)
>> +             goto out_free;
>> +     cps->cp_stateid.si_opaque.so_id =3D new_id;
>> +     cps->cp_stateid.si_opaque.so_clid.cl_boot =3D nn->boot_time;
>> +     cps->cp_stateid.si_opaque.so_clid.cl_id =3D nn->s2s_cp_cl_id;
>> +     cps->cp_p_stid =3D p_stid;
>> +     INIT_LIST_HEAD(&cps->cp_list);
>> +     list_add(&cps->cp_list, &p_stid->sc_cp_list);
>> +
>> +     return cps;
>> +out_free:
>> +     kfree(cps);
>> +     return NULL;
>> +}
>> +
>> +void nfs4_free_cp_state(struct nfs4_cp_state *cps)
>> +{
>> +     struct nfsd_net *nn;
>> +
>> +     nn =3D net_generic(cps->cp_p_stid->sc_client->net, nfsd_net_id);
>> +     spin_lock(&nn->s2s_cp_lock);
>> +     idr_remove(&nn->s2s_cp_stateids, cps->cp_stateid.si_opaque.so_id);
>> +     spin_unlock(&nn->s2s_cp_lock);
>> +
>> +     kfree(cps);
>> +}
>> +
>> +static void nfs4_free_cp_statelist(struct nfs4_stid *stid)
>> +{
>> +     struct nfs4_cp_state *cps;
>> +
>> +     might_sleep();
>> +
>> +     while (!list_empty(&stid->sc_cp_list)) {
>> +             cps =3D list_first_entry(&stid->sc_cp_list, struct nfs4_cp=
_state,
>> +                                    cp_list);
>> +             list_del(&cps->cp_list);
>> +             nfs4_free_cp_state(cps);
>> +     }
>> +}
>> +
>>  static struct nfs4_ol_stateid * nfs4_alloc_open_stateid(struct nfs4_cli=
ent *clp)
>>  {
>>       struct nfs4_stid *stid;
>> @@ -819,6 +880,9 @@ static void block_delegations(struct knfsd_fh *fh)
>>       }
>>       idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
>>       spin_unlock(&clp->cl_lock);
>> +
>> +     nfs4_free_cp_statelist(s);
>> +
>>       s->sc_free(s);
>>       if (fp)
>>               put_nfs4_file(fp);
>> @@ -6952,6 +7016,8 @@ static int nfs4_state_create_net(struct net *net)
>>       INIT_LIST_HEAD(&nn->close_lru);
>>       INIT_LIST_HEAD(&nn->del_recall_lru);
>>       spin_lock_init(&nn->client_lock);
>> +     spin_lock_init(&nn->s2s_cp_lock);
>> +     idr_init(&nn->s2s_cp_stateids);
>>
>>       spin_lock_init(&nn->blocked_locks_lock);
>>       INIT_LIST_HEAD(&nn->blocked_locks_lru);
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index 6493df6..232270d 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -1241,6 +1241,7 @@ static __net_init int nfsd_init_net(struct net *ne=
t)
>>       nn->nfsd4_grace =3D 90;
>>       nn->clverifier_counter =3D prandom_u32();
>>       nn->clientid_counter =3D prandom_u32();
>> +     nn->s2s_cp_cl_id =3D nn->clientid_counter++;
>>       return 0;
>>
>>  out_idmap_error:
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index cfd13f0..8724955 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -93,6 +93,7 @@ struct nfs4_stid {
>>  #define NFS4_REVOKED_DELEG_STID 16
>>  #define NFS4_CLOSED_DELEG_STID 32
>>  #define NFS4_LAYOUT_STID 64
>> +     struct list_head        sc_cp_list;
>>       unsigned char           sc_type;
>>       stateid_t               sc_stateid;
>>       spinlock_t              sc_lock;
>> @@ -102,6 +103,17 @@ struct nfs4_stid {
>>  };
>>
>>  /*
>> + * Keep a list of stateids issued by the COPY, associate it with the
>> + * parent OPEN/LOCK/DELEG stateid. Used for lookup by
>> + * OFFLOAD_CANCEL and OFFLOAD_STATUS (as well as COPY_NOTIFY)
>> + */
>> +struct nfs4_cp_state {
>> +     stateid_t               cp_stateid;
>> +     struct list_head        cp_list;        /* per parent nfs4_stid */
>> +     struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
>> +};
>> +
>> +/*
>>   * Represents a delegation stateid. The nfs4_client holds references to=
 these
>>   * and they are put when it is being destroyed or when the delegation i=
s
>>   * returned by the client:
>> @@ -607,6 +619,8 @@ __be32 nfsd4_lookup_stateid(struct nfsd4_compound_st=
ate *cstate,
>>                    struct nfs4_stid **s, struct nfsd_net *nn);
>>  struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_c=
ache *slab,
>>                                 void (*sc_free)(struct nfs4_stid *));
>> +struct nfs4_cp_state *nfs4_alloc_init_cp_state(struct nfsd_net *nn, str=
uct nfs4_stid *p_stid);
>> +void nfs4_free_cp_state(struct nfs4_cp_state *cps);
>>  void nfs4_unhash_stid(struct nfs4_stid *s);
>>  void nfs4_put_stid(struct nfs4_stid *s);
>>  void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index f7a94d3..4ccd43b 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -536,6 +536,8 @@ struct nfsd4_copy {
>>       struct file             *fh_src;
>>       struct file             *fh_dst;
>>       struct net              *net;
>> +     struct nfs4_stid        *stid;
>> +     struct nfs4_cp_state    *cps;
>>  };
>>
>>  struct nfsd4_seek {
>> --
>> 1.8.3.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 07/10] NFSD create new stateid for async copy

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 03:21:54PM -0400, Olga Kornievskaia wrote:
> On Thu, Sep 28, 2017 at 3:12 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > On Thu, Sep 28, 2017 at 01:29:42PM -0400, Olga Kornievskaia wrote:
> >> Generate a new stateid to be used for reply to the asynchronous
> >> COPY (this would also be used later by COPY_NOTIFY as well).
> >> Associate the stateid with the parent OPEN/LOCK/DELEG stateid
> >> that can be freed during the free of the parent stateid. However,
> >> right now deciding to bind the lifetime to when the vfs copy
> >> is done. This way don't need to keep the nfsd_net structure for
> >> the callback. The drawback is that time copy state information
> >> is available for query by OFFLOAD_STATUS is slightly less.
> >
> > I don't think we're under any obligation to handle OFFLOAD_STATUS after
> > the copy is done, are we?
> 
> It depends on how you define “copy is done”. I’d say copy is done once the
> server sent the CB_OFFLOAD. But I’m choosing to clear the state once the
> vfs_copy_file_range() is done. Then the callback is queued up (what if
> there is wait until the callback can be processed) and there is a period
> where if the client were to send a OFFLOAD_STATUS it should get the
> bytes representing the full copy but instead the client will get an error back.
> 
> I was worried about what if some implementation depend on querying for
> the status and stopping to poll only when they received that all bytes were
> copied and they might not get that with how the server is coded right now?

I don't see the spec providing any guarantee that the copy status be
kept around any minimum length of time, so I don't think a client would
be correct to do that.

--b.

Re: [PATCH v4 02/10] NFSD OFFLOAD_STATUS xdr

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 01:29:37PM -0400, Olga Kornievskaia wrote:
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4proc.c | 20 ++++++++++++++++++++
>  fs/nfsd/nfs4xdr.c  | 30 ++++++++++++++++++++++++++++--
>  fs/nfsd/xdr4.h     | 10 ++++++++++
>  3 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3c69db7..8601fc4 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1142,6 +1142,13 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>  	fput(file);
>  	return status;
>  }
> +static __be32
> +nfsd4_offload_status(struct svc_rqst *rqstp,
> +		     struct nfsd4_compound_state *cstate,
> +		     union nfsd4_op_u *u)
> +{
> +	return nfserr_notsupp;
> +}
>  
>  static __be32
>  nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> @@ -2039,6 +2046,14 @@ static inline u32 nfsd4_copy_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
>  		1 /* cr_synchronous */) * sizeof(__be32);
>  }
>  
> +static inline u32 nfsd4_offload_status_rsize(struct svc_rqst *rqstp,
> +					     struct nfsd4_op *op)
> +{
> +	return (op_encode_hdr_size +
> +		2 /* osr_count */ +
> +		1 /* osr_complete<1> optional 0 for now */) * sizeof(__be32);
> +}
> +
>  #ifdef CONFIG_NFSD_PNFS
>  static inline u32 nfsd4_getdeviceinfo_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
>  {
> @@ -2452,6 +2467,11 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
>  		.op_name = "OP_SEEK",
>  		.op_rsize_bop = nfsd4_seek_rsize,
>  	},
> +	[OP_OFFLOAD_STATUS] = {
> +		.op_func = nfsd4_offload_status,
> +		.op_name = "OP_OFFLOAD_STATUS",
> +		.op_rsize_bop = nfsd4_offload_status_rsize,
> +	},
>  };
>  
>  /**
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 2c61c6b..ed8b61f 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1768,6 +1768,13 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>  }
>  
>  static __be32
> +nfsd4_decode_offload_status(struct nfsd4_compoundargs *argp,
> +			    struct nfsd4_offload_status *os)
> +{
> +	return nfsd4_decode_stateid(argp, &os->stateid);
> +}
> +
> +static __be32
>  nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
>  {
>  	DECODE_HEAD;
> @@ -1874,7 +1881,7 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>  	[OP_LAYOUTERROR]	= (nfsd4_dec)nfsd4_decode_notsupp,
>  	[OP_LAYOUTSTATS]	= (nfsd4_dec)nfsd4_decode_notsupp,
>  	[OP_OFFLOAD_CANCEL]	= (nfsd4_dec)nfsd4_decode_notsupp,
> -	[OP_OFFLOAD_STATUS]	= (nfsd4_dec)nfsd4_decode_notsupp,
> +	[OP_OFFLOAD_STATUS]	= (nfsd4_dec)nfsd4_decode_offload_status,
>  	[OP_READ_PLUS]		= (nfsd4_dec)nfsd4_decode_notsupp,
>  	[OP_SEEK]		= (nfsd4_dec)nfsd4_decode_seek,
>  	[OP_WRITE_SAME]		= (nfsd4_dec)nfsd4_decode_notsupp,
> @@ -4216,6 +4223,25 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
>  }
>  
>  static __be32
> +nfsd4_encode_offload_status(struct nfsd4_compoundres *resp, __be32 nfserr,
> +			    struct nfsd4_offload_status *os)
> +{
> +	struct xdr_stream *xdr = &resp->xdr;
> +	__be32 *p;
> +
> +	if (nfserr)
> +		return nfserr;

Note you can skip this--see upstream bac966d60652 and b7571e4cd39a.

--b.

> +
> +	p = xdr_reserve_space(xdr, 8 + 4);
> +	if (!p)
> +		return nfserr_resource;
> +	p = xdr_encode_hyper(p, os->count);
> +	*p++ = cpu_to_be32(0);
> +
> +	return nfserr;
> +}
> +
> +static __be32
>  nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
>  		  struct nfsd4_seek *seek)
>  {
> @@ -4318,7 +4344,7 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
>  	[OP_LAYOUTERROR]	= (nfsd4_enc)nfsd4_encode_noop,
>  	[OP_LAYOUTSTATS]	= (nfsd4_enc)nfsd4_encode_noop,
>  	[OP_OFFLOAD_CANCEL]	= (nfsd4_enc)nfsd4_encode_noop,
> -	[OP_OFFLOAD_STATUS]	= (nfsd4_enc)nfsd4_encode_noop,
> +	[OP_OFFLOAD_STATUS]	= (nfsd4_enc)nfsd4_encode_offload_status,
>  	[OP_READ_PLUS]		= (nfsd4_enc)nfsd4_encode_noop,
>  	[OP_SEEK]		= (nfsd4_enc)nfsd4_encode_seek,
>  	[OP_WRITE_SAME]		= (nfsd4_enc)nfsd4_encode_noop,
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 4ac2676..9b0c099 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -542,6 +542,15 @@ struct nfsd4_seek {
>  	loff_t		seek_pos;
>  };
>  
> +struct nfsd4_offload_status {
> +	/* request */
> +	stateid_t	stateid;
> +
> +	/* response */
> +	u64		count;
> +	u32		status;
> +};
> +
>  struct nfsd4_op {
>  	int					opnum;
>  	const struct nfsd4_operation *		opdesc;
> @@ -600,6 +609,7 @@ struct nfsd4_op {
>  		struct nfsd4_fallocate		deallocate;
>  		struct nfsd4_clone		clone;
>  		struct nfsd4_copy		copy;
> +		struct nfsd4_offload_status	offload_status;
>  		struct nfsd4_seek		seek;
>  	} u;
>  	struct nfs4_replay *			replay;
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 03/10] NFSD OFFLOAD_CANCEL xdr

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 01:29:38PM -0400, Olga Kornievskaia wrote:
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4proc.c | 13 +++++++++++++
>  fs/nfsd/nfs4xdr.c  |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 8601fc4..11ade90 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1121,6 +1121,14 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>  }
>  
>  static __be32
> +nfsd4_offload_cancel(struct svc_rqst *rqstp,
> +		     struct nfsd4_compound_state *cstate,
> +		     union nfsd4_op_u *u)
> +{
> +	return 0;
> +}
> +
> +static __be32
>  nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		struct nfsd4_fallocate *fallocate, int flags)
>  {
> @@ -2472,6 +2480,11 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
>  		.op_name = "OP_OFFLOAD_STATUS",
>  		.op_rsize_bop = nfsd4_offload_status_rsize,
>  	},
> +	[OP_OFFLOAD_CANCEL] = {

I think you want .op_flags = OP_MODIFIES_SOMETHING as well.

> +		.op_func = nfsd4_offload_cancel,
> +		.op_name = "OP_OFFLOAD_CANCEL",
> +		.op_rsize_bop = nfsd4_only_status_rsize,
> +	},
>  };
>  
>  /**
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index ed8b61f..31ee5e1 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1880,7 +1880,7 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>  	[OP_IO_ADVISE]		= (nfsd4_dec)nfsd4_decode_notsupp,
>  	[OP_LAYOUTERROR]	= (nfsd4_dec)nfsd4_decode_notsupp,
>  	[OP_LAYOUTSTATS]	= (nfsd4_dec)nfsd4_decode_notsupp,
> -	[OP_OFFLOAD_CANCEL]	= (nfsd4_dec)nfsd4_decode_notsupp,
> +	[OP_OFFLOAD_CANCEL]	= (nfsd4_dec)nfsd4_decode_offload_status,
>  	[OP_OFFLOAD_STATUS]	= (nfsd4_dec)nfsd4_decode_offload_status,
>  	[OP_READ_PLUS]		= (nfsd4_dec)nfsd4_decode_notsupp,
>  	[OP_SEEK]		= (nfsd4_dec)nfsd4_decode_seek,
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 03/10] NFSD OFFLOAD_CANCEL xdr

[Olga Kornievskaia]

On Thu, Sep 28, 2017 at 3:34 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> On Thu, Sep 28, 2017 at 01:29:38PM -0400, Olga Kornievskaia wrote:
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfsd/nfs4proc.c | 13 +++++++++++++
>>  fs/nfsd/nfs4xdr.c  |  2 +-
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 8601fc4..11ade90 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1121,6 +1121,14 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>>  }
>>
>>  static __be32
>> +nfsd4_offload_cancel(struct svc_rqst *rqstp,
>> +                  struct nfsd4_compound_state *cstate,
>> +                  union nfsd4_op_u *u)
>> +{
>> +     return 0;
>> +}
>> +
>> +static __be32
>>  nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>               struct nfsd4_fallocate *fallocate, int flags)
>>  {
>> @@ -2472,6 +2480,11 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
>>               .op_name = "OP_OFFLOAD_STATUS",
>>               .op_rsize_bop = nfsd4_offload_status_rsize,
>>       },
>> +     [OP_OFFLOAD_CANCEL] = {
>
> I think you want .op_flags = OP_MODIFIES_SOMETHING as well.

Got it. Is it because it modifies state of the server? I guess I'm not
clear what OP_MODIFIES_SOMETHING represents.


>> +             .op_func = nfsd4_offload_cancel,
>> +             .op_name = "OP_OFFLOAD_CANCEL",
>> +             .op_rsize_bop = nfsd4_only_status_rsize,
>> +     },
>>  };
>>
>>  /**
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index ed8b61f..31ee5e1 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1880,7 +1880,7 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>>       [OP_IO_ADVISE]          = (nfsd4_dec)nfsd4_decode_notsupp,
>>       [OP_LAYOUTERROR]        = (nfsd4_dec)nfsd4_decode_notsupp,
>>       [OP_LAYOUTSTATS]        = (nfsd4_dec)nfsd4_decode_notsupp,
>> -     [OP_OFFLOAD_CANCEL]     = (nfsd4_dec)nfsd4_decode_notsupp,
>> +     [OP_OFFLOAD_CANCEL]     = (nfsd4_dec)nfsd4_decode_offload_status,
>>       [OP_OFFLOAD_STATUS]     = (nfsd4_dec)nfsd4_decode_offload_status,
>>       [OP_READ_PLUS]          = (nfsd4_dec)nfsd4_decode_notsupp,
>>       [OP_SEEK]               = (nfsd4_dec)nfsd4_decode_seek,
>> --
>> 1.8.3.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 03/10] NFSD OFFLOAD_CANCEL xdr

[J. Bruce Fields]

On Thu, Sep 28, 2017 at 03:40:47PM -0400, Olga Kornievskaia wrote:
> On Thu, Sep 28, 2017 at 3:34 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > On Thu, Sep 28, 2017 at 01:29:38PM -0400, Olga Kornievskaia wrote:
> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >> ---
> >>  fs/nfsd/nfs4proc.c | 13 +++++++++++++
> >>  fs/nfsd/nfs4xdr.c  |  2 +-
> >>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> index 8601fc4..11ade90 100644
> >> --- a/fs/nfsd/nfs4proc.c
> >> +++ b/fs/nfsd/nfs4proc.c
> >> @@ -1121,6 +1121,14 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
> >>  }
> >>
> >>  static __be32
> >> +nfsd4_offload_cancel(struct svc_rqst *rqstp,
> >> +                  struct nfsd4_compound_state *cstate,
> >> +                  union nfsd4_op_u *u)
> >> +{
> >> +     return 0;
> >> +}
> >> +
> >> +static __be32
> >>  nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>               struct nfsd4_fallocate *fallocate, int flags)
> >>  {
> >> @@ -2472,6 +2480,11 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
> >>               .op_name = "OP_OFFLOAD_STATUS",
> >>               .op_rsize_bop = nfsd4_offload_status_rsize,
> >>       },
> >> +     [OP_OFFLOAD_CANCEL] = {
> >
> > I think you want .op_flags = OP_MODIFIES_SOMETHING as well.
> 
> Got it. Is it because it modifies state of the server? I guess I'm not
> clear what OP_MODIFIES_SOMETHING represents.

If OP_MODIFIES_SOMETHING is set then we will refuse to perform the
operation if it looks like we might run out of space to encode the
result.

Eh, now that I think about it I'm not actually sure if it matters for an
operation like this.  Probably doesn't hurt.

--b.

> 
> 
> >> +             .op_func = nfsd4_offload_cancel,
> >> +             .op_name = "OP_OFFLOAD_CANCEL",
> >> +             .op_rsize_bop = nfsd4_only_status_rsize,
> >> +     },
> >>  };
> >>
> >>  /**
> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >> index ed8b61f..31ee5e1 100644
> >> --- a/fs/nfsd/nfs4xdr.c
> >> +++ b/fs/nfsd/nfs4xdr.c
> >> @@ -1880,7 +1880,7 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
> >>       [OP_IO_ADVISE]          = (nfsd4_dec)nfsd4_decode_notsupp,
> >>       [OP_LAYOUTERROR]        = (nfsd4_dec)nfsd4_decode_notsupp,
> >>       [OP_LAYOUTSTATS]        = (nfsd4_dec)nfsd4_decode_notsupp,
> >> -     [OP_OFFLOAD_CANCEL]     = (nfsd4_dec)nfsd4_decode_notsupp,
> >> +     [OP_OFFLOAD_CANCEL]     = (nfsd4_dec)nfsd4_decode_offload_status,
> >>       [OP_OFFLOAD_STATUS]     = (nfsd4_dec)nfsd4_decode_offload_status,
> >>       [OP_READ_PLUS]          = (nfsd4_dec)nfsd4_decode_notsupp,
> >>       [OP_SEEK]               = (nfsd4_dec)nfsd4_decode_seek,
> >> --
> >> 1.8.3.1
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 05/10] NFSD first draft of async copy

[Olga Kornievskaia]

On Thu, Sep 28, 2017 at 2:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
>> Asynchronous copies are handled by a single threaded workqueue.
>> If we get asynchronous request, make sure to copy the needed
>> arguments/state from the stack before starting the copy. Then
>> queue work and reply back to the client indicating copy is
>> asynchronous.
>>
>> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
>> the total number of bytes need to copy.
>
> I don't think you need to do this--just call vfs_copy_file_range()
> directly, as nfsd_copy_file_range does.
>
> The 4MB chunking was only there to prevent a synchronous copy from
> taking too long, which isn't an issue in the async case.

Before I remove the loop, the other reason I kept the loop was for a
meaningful OFFLOAD_STATUS. Without the loop, any query for the
OFFLOAD_STATUS would either return 0 or if it's really really really
really lucky then it might return full-bytes copy value. There will
never be an intermediate reply. I kept the loop and kept track of
bytes copied so far for the status query.

>
> --b.
>
>> In case a failure
>> happens in the middle, we can return an error as well as how
>> much we copied so far. Once done creating a workitem for the
>> callback workqueue and send CB_OFFLOAD with the results.
>>
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfsd/nfs4proc.c  | 164 ++++++++++++++++++++++++++++++++++++++++++++++------
>>  fs/nfsd/nfs4state.c |   9 ++-
>>  fs/nfsd/state.h     |   2 +
>>  fs/nfsd/xdr4.h      |   7 +++
>>  4 files changed, 162 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 11ade90..de21b1a 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -76,6 +76,21 @@
>>  { }
>>  #endif
>>
>> +static struct workqueue_struct *copy_wq;
>> +
>> +int nfsd4_create_copy_queue(void)
>> +{
>> +     copy_wq = create_singlethread_workqueue("nfsd4_copy");
>> +     if (!copy_wq)
>> +             return -ENOMEM;
>> +     return 0;
>> +}
>> +
>> +void nfsd4_destroy_copy_queue(void)
>> +{
>> +     destroy_workqueue(copy_wq);
>> +}
>> +
>>  #define NFSDDBG_FACILITY             NFSDDBG_PROC
>>
>>  static u32 nfsd_attrmask[] = {
>> @@ -1085,37 +1100,148 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>>  out:
>>       return status;
>>  }
>> +static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>> +{
>> +     struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);
>> +
>> +     kfree(copy);
>> +}
>> +
>> +static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
>> +                              struct rpc_task *task)
>> +{
>> +     return 1;
>> +}
>> +
>> +static const struct nfsd4_callback_ops nfsd4_cb_offload_ops = {
>> +     .release = nfsd4_cb_offload_release,
>> +     .done = nfsd4_cb_offload_done
>> +};
>> +
>> +static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
>> +{
>> +     memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
>> +             sizeof(copy->cp_dst_stateid));
>> +     copy->cp_res.wr_stable_how = NFS_UNSTABLE;
>> +     copy->cp_consecutive = 1;
>> +     copy->cp_synchronous = sync;
>> +     gen_boot_verifier(&copy->cp_res.wr_verifier, copy->net);
>> +
>> +     return nfs_ok;
>> +}
>> +
>> +static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> +{
>> +     ssize_t bytes_copied = 0;
>> +     size_t bytes_total = copy->cp_count;
>> +     size_t bytes_to_copy;
>> +     u64 src_pos = copy->cp_src_pos;
>> +     u64 dst_pos = copy->cp_dst_pos;
>> +
>> +     do {
>> +             bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
>> +             bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
>> +                                     copy->fh_dst, dst_pos, bytes_to_copy);
>> +             if (bytes_copied <= 0)
>> +                     break;
>> +             bytes_total -= bytes_copied;
>> +             copy->cp_res.wr_bytes_written += bytes_copied;
>> +             src_pos += bytes_copied;
>> +             dst_pos += bytes_copied;
>> +     } while (bytes_total > 0 && !copy->cp_synchronous);
>> +     return bytes_copied;
>> +}
>> +
>> +static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
>> +{
>> +     __be32 status;
>> +     ssize_t bytes;
>> +
>> +     bytes = _nfsd_copy_file_range(copy);
>> +     if (bytes < 0)
>> +             status = nfserrno(bytes);
>> +     else
>> +             status = nfsd4_init_copy_res(copy, sync);
>> +
>> +     fput(copy->fh_src);
>> +     fput(copy->fh_dst);
>> +     return status;
>> +}
>> +
>> +static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>> +{
>> +     memcpy(&dst->cp_src_stateid, &src->cp_src_stateid, sizeof(stateid_t));
>> +     memcpy(&dst->cp_dst_stateid, &src->cp_dst_stateid, sizeof(stateid_t));
>> +     dst->cp_src_pos = src->cp_src_pos;
>> +     dst->cp_dst_pos = src->cp_dst_pos;
>> +     dst->cp_count = src->cp_count;
>> +     dst->cp_consecutive = src->cp_consecutive;
>> +     dst->cp_synchronous = src->cp_synchronous;
>> +     memcpy(&dst->cp_res, &src->cp_res, sizeof(src->cp_res));
>> +     /* skipping nfsd4_callback */
>> +     memcpy(&dst->fh, &src->fh, sizeof(src->fh));
>> +     dst->fh = src->fh;
>> +     dst->cp_clp = src->cp_clp;
>> +     dst->fh_src = src->fh_src;
>> +     dst->fh_dst = src->fh_dst;
>> +     dst->net = src->net;
>> +}
>> +
>> +static void nfsd4_do_async_copy(struct work_struct *work)
>> +{
>> +     struct nfsd4_copy *copy =
>> +             container_of(work, struct nfsd4_copy, cp_work);
>> +     struct nfsd4_copy *cb_copy;
>> +
>> +     copy->nfserr = nfsd4_do_copy(copy, 0);
>> +     cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>> +     if (!cb_copy)
>> +             goto out;
>> +     memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
>> +     cb_copy->cp_clp = copy->cp_clp;
>> +     cb_copy->nfserr = copy->nfserr;
>> +     memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
>> +     nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
>> +                     &nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
>> +     nfsd4_run_cb(&cb_copy->cp_cb);
>> +out:
>> +     kfree(copy);
>> +}
>>
>>  static __be32
>>  nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>               union nfsd4_op_u *u)
>>  {
>>       struct nfsd4_copy *copy = &u->copy;
>> -     struct file *src, *dst;
>>       __be32 status;
>> -     ssize_t bytes;
>>
>> -     status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid, &src,
>> -                                &copy->cp_dst_stateid, &dst);
>> +     status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
>> +                                &copy->fh_src, &copy->cp_dst_stateid,
>> +                                &copy->fh_dst);
>>       if (status)
>>               goto out;
>>
>> -     bytes = nfsd_copy_file_range(src, copy->cp_src_pos,
>> -                     dst, copy->cp_dst_pos, copy->cp_count);
>> -
>> -     if (bytes < 0)
>> -             status = nfserrno(bytes);
>> -     else {
>> -             copy->cp_res.wr_bytes_written = bytes;
>> -             copy->cp_res.wr_stable_how = NFS_UNSTABLE;
>> -             copy->cp_consecutive = 1;
>> -             copy->cp_synchronous = 1;
>> -             gen_boot_verifier(&copy->cp_res.wr_verifier, SVC_NET(rqstp));
>> -             status = nfs_ok;
>> +     copy->cp_clp = cstate->clp;
>> +     memcpy(&copy->fh, &cstate->current_fh.fh_handle,
>> +             sizeof(struct knfsd_fh));
>> +     copy->net = SVC_NET(rqstp);
>> +     if (!copy->cp_synchronous) {
>> +             struct nfsd4_copy *async_copy;
>> +
>> +             status = nfsd4_init_copy_res(copy, 0);
>> +             async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>> +             if (!async_copy) {
>> +                     status = nfserrno(-ENOMEM);
>> +                     goto out;
>> +             }
>> +             dup_copy_fields(copy, async_copy);
>> +             memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
>> +                     sizeof(copy->cp_dst_stateid));
>> +             INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
>> +             queue_work(copy_wq, &async_copy->cp_work);
>> +     } else {
>> +             status = nfsd4_do_copy(copy, 1);
>>       }
>> -
>> -     fput(src);
>> -     fput(dst);
>>  out:
>>       return status;
>>  }
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 0c04f81..fe6f275 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -7033,8 +7033,14 @@ static int nfs4_state_create_net(struct net *net)
>>               goto out_free_laundry;
>>
>>       set_max_delegations();
>> -     return 0;
>>
>> +     ret = nfsd4_create_copy_queue();
>> +     if (ret)
>> +             goto out_free_callback;
>> +
>> +     return 0;
>> +out_free_callback:
>> +     nfsd4_destroy_callback_queue();
>>  out_free_laundry:
>>       destroy_workqueue(laundry_wq);
>>  out_cleanup_cred:
>> @@ -7097,6 +7103,7 @@ static int nfs4_state_create_net(struct net *net)
>>       destroy_workqueue(laundry_wq);
>>       nfsd4_destroy_callback_queue();
>>       cleanup_callback_cred();
>> +     nfsd4_destroy_copy_queue();
>>  }
>>
>>  static void
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index f8b0210..b5358cf 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -630,6 +630,8 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
>>  extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>>                                                       struct nfsd_net *nn);
>>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
>> +extern int nfsd4_create_copy_queue(void);
>> +extern void nfsd4_destroy_copy_queue(void);
>>
>>  struct nfs4_file *find_file(struct knfsd_fh *fh);
>>  void put_nfs4_file(struct nfs4_file *fi);
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index 9b0c099..f7a94d3 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -529,6 +529,13 @@ struct nfsd4_copy {
>>       struct nfsd4_callback   cp_cb;
>>       __be32                  nfserr;
>>       struct knfsd_fh         fh;
>> +
>> +     struct work_struct      cp_work;
>> +     struct nfs4_client      *cp_clp;
>> +
>> +     struct file             *fh_src;
>> +     struct file             *fh_dst;
>> +     struct net              *net;
>>  };
>>
>>  struct nfsd4_seek {
>> --
>> 1.8.3.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 05/10] NFSD first draft of async copy

[J. Bruce Fields]

On Fri, Sep 29, 2017 at 05:51:23PM -0400, Olga Kornievskaia wrote:
> On Thu, Sep 28, 2017 at 2:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > On Thu, Sep 28, 2017 at 01:29:40PM -0400, Olga Kornievskaia wrote:
> >> Asynchronous copies are handled by a single threaded workqueue.
> >> If we get asynchronous request, make sure to copy the needed
> >> arguments/state from the stack before starting the copy. Then
> >> queue work and reply back to the client indicating copy is
> >> asynchronous.
> >>
> >> nfsd_copy_file_range() will copy in 4MBchunk so do a loop over
> >> the total number of bytes need to copy.
> >
> > I don't think you need to do this--just call vfs_copy_file_range()
> > directly, as nfsd_copy_file_range does.
> >
> > The 4MB chunking was only there to prevent a synchronous copy from
> > taking too long, which isn't an issue in the async case.
> 
> Before I remove the loop, the other reason I kept the loop was for a
> meaningful OFFLOAD_STATUS. Without the loop, any query for the
> OFFLOAD_STATUS would either return 0 or if it's really really really
> really lucky then it might return full-bytes copy value. There will
> never be an intermediate reply. I kept the loop and kept track of
> bytes copied so far for the status query.

Ugh, I hadn't thought about OFFLOAD_STATUS.

Maybe I'm being ornery, but: would returning 0 be OK for a first pass?
The client doesn't use OFFLOAD_STATUS anyway, does it?  It'd require new
userspace API.

One advantage of the current synchronous copy is that existing
applications should still be able to track progress, since we return all
the way back to the client after each 4MB chunk.  We'll lose that with
asynchronous copy, whether or not the server supports OFFLOAD_STATUS.

Anyway, if there's actually a chance that someone might use
OFFLOAD_STATUS, then maybe we do want to keep that loop.  I don't have a
better suggestion.

--b.

> 
> >
> > --b.
> >
> >> In case a failure
> >> happens in the middle, we can return an error as well as how
> >> much we copied so far. Once done creating a workitem for the
> >> callback workqueue and send CB_OFFLOAD with the results.
> >>
> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >> ---
> >>  fs/nfsd/nfs4proc.c  | 164 ++++++++++++++++++++++++++++++++++++++++++++++------
> >>  fs/nfsd/nfs4state.c |   9 ++-
> >>  fs/nfsd/state.h     |   2 +
> >>  fs/nfsd/xdr4.h      |   7 +++
> >>  4 files changed, 162 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> index 11ade90..de21b1a 100644
> >> --- a/fs/nfsd/nfs4proc.c
> >> +++ b/fs/nfsd/nfs4proc.c
> >> @@ -76,6 +76,21 @@
> >>  { }
> >>  #endif
> >>
> >> +static struct workqueue_struct *copy_wq;
> >> +
> >> +int nfsd4_create_copy_queue(void)
> >> +{
> >> +     copy_wq = create_singlethread_workqueue("nfsd4_copy");
> >> +     if (!copy_wq)
> >> +             return -ENOMEM;
> >> +     return 0;
> >> +}
> >> +
> >> +void nfsd4_destroy_copy_queue(void)
> >> +{
> >> +     destroy_workqueue(copy_wq);
> >> +}
> >> +
> >>  #define NFSDDBG_FACILITY             NFSDDBG_PROC
> >>
> >>  static u32 nfsd_attrmask[] = {
> >> @@ -1085,37 +1100,148 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
> >>  out:
> >>       return status;
> >>  }
> >> +static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> >> +{
> >> +     struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);
> >> +
> >> +     kfree(copy);
> >> +}
> >> +
> >> +static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
> >> +                              struct rpc_task *task)
> >> +{
> >> +     return 1;
> >> +}
> >> +
> >> +static const struct nfsd4_callback_ops nfsd4_cb_offload_ops = {
> >> +     .release = nfsd4_cb_offload_release,
> >> +     .done = nfsd4_cb_offload_done
> >> +};
> >> +
> >> +static int nfsd4_init_copy_res(struct nfsd4_copy *copy, bool sync)
> >> +{
> >> +     memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> >> +             sizeof(copy->cp_dst_stateid));
> >> +     copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> >> +     copy->cp_consecutive = 1;
> >> +     copy->cp_synchronous = sync;
> >> +     gen_boot_verifier(&copy->cp_res.wr_verifier, copy->net);
> >> +
> >> +     return nfs_ok;
> >> +}
> >> +
> >> +static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >> +{
> >> +     ssize_t bytes_copied = 0;
> >> +     size_t bytes_total = copy->cp_count;
> >> +     size_t bytes_to_copy;
> >> +     u64 src_pos = copy->cp_src_pos;
> >> +     u64 dst_pos = copy->cp_dst_pos;
> >> +
> >> +     do {
> >> +             bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> >> +             bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
> >> +                                     copy->fh_dst, dst_pos, bytes_to_copy);
> >> +             if (bytes_copied <= 0)
> >> +                     break;
> >> +             bytes_total -= bytes_copied;
> >> +             copy->cp_res.wr_bytes_written += bytes_copied;
> >> +             src_pos += bytes_copied;
> >> +             dst_pos += bytes_copied;
> >> +     } while (bytes_total > 0 && !copy->cp_synchronous);
> >> +     return bytes_copied;
> >> +}
> >> +
> >> +static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
> >> +{
> >> +     __be32 status;
> >> +     ssize_t bytes;
> >> +
> >> +     bytes = _nfsd_copy_file_range(copy);
> >> +     if (bytes < 0)
> >> +             status = nfserrno(bytes);
> >> +     else
> >> +             status = nfsd4_init_copy_res(copy, sync);
> >> +
> >> +     fput(copy->fh_src);
> >> +     fput(copy->fh_dst);
> >> +     return status;
> >> +}
> >> +
> >> +static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> >> +{
> >> +     memcpy(&dst->cp_src_stateid, &src->cp_src_stateid, sizeof(stateid_t));
> >> +     memcpy(&dst->cp_dst_stateid, &src->cp_dst_stateid, sizeof(stateid_t));
> >> +     dst->cp_src_pos = src->cp_src_pos;
> >> +     dst->cp_dst_pos = src->cp_dst_pos;
> >> +     dst->cp_count = src->cp_count;
> >> +     dst->cp_consecutive = src->cp_consecutive;
> >> +     dst->cp_synchronous = src->cp_synchronous;
> >> +     memcpy(&dst->cp_res, &src->cp_res, sizeof(src->cp_res));
> >> +     /* skipping nfsd4_callback */
> >> +     memcpy(&dst->fh, &src->fh, sizeof(src->fh));
> >> +     dst->fh = src->fh;
> >> +     dst->cp_clp = src->cp_clp;
> >> +     dst->fh_src = src->fh_src;
> >> +     dst->fh_dst = src->fh_dst;
> >> +     dst->net = src->net;
> >> +}
> >> +
> >> +static void nfsd4_do_async_copy(struct work_struct *work)
> >> +{
> >> +     struct nfsd4_copy *copy =
> >> +             container_of(work, struct nfsd4_copy, cp_work);
> >> +     struct nfsd4_copy *cb_copy;
> >> +
> >> +     copy->nfserr = nfsd4_do_copy(copy, 0);
> >> +     cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >> +     if (!cb_copy)
> >> +             goto out;
> >> +     memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
> >> +     cb_copy->cp_clp = copy->cp_clp;
> >> +     cb_copy->nfserr = copy->nfserr;
> >> +     memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
> >> +     nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
> >> +                     &nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
> >> +     nfsd4_run_cb(&cb_copy->cp_cb);
> >> +out:
> >> +     kfree(copy);
> >> +}
> >>
> >>  static __be32
> >>  nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>               union nfsd4_op_u *u)
> >>  {
> >>       struct nfsd4_copy *copy = &u->copy;
> >> -     struct file *src, *dst;
> >>       __be32 status;
> >> -     ssize_t bytes;
> >>
> >> -     status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid, &src,
> >> -                                &copy->cp_dst_stateid, &dst);
> >> +     status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> >> +                                &copy->fh_src, &copy->cp_dst_stateid,
> >> +                                &copy->fh_dst);
> >>       if (status)
> >>               goto out;
> >>
> >> -     bytes = nfsd_copy_file_range(src, copy->cp_src_pos,
> >> -                     dst, copy->cp_dst_pos, copy->cp_count);
> >> -
> >> -     if (bytes < 0)
> >> -             status = nfserrno(bytes);
> >> -     else {
> >> -             copy->cp_res.wr_bytes_written = bytes;
> >> -             copy->cp_res.wr_stable_how = NFS_UNSTABLE;
> >> -             copy->cp_consecutive = 1;
> >> -             copy->cp_synchronous = 1;
> >> -             gen_boot_verifier(&copy->cp_res.wr_verifier, SVC_NET(rqstp));
> >> -             status = nfs_ok;
> >> +     copy->cp_clp = cstate->clp;
> >> +     memcpy(&copy->fh, &cstate->current_fh.fh_handle,
> >> +             sizeof(struct knfsd_fh));
> >> +     copy->net = SVC_NET(rqstp);
> >> +     if (!copy->cp_synchronous) {
> >> +             struct nfsd4_copy *async_copy;
> >> +
> >> +             status = nfsd4_init_copy_res(copy, 0);
> >> +             async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >> +             if (!async_copy) {
> >> +                     status = nfserrno(-ENOMEM);
> >> +                     goto out;
> >> +             }
> >> +             dup_copy_fields(copy, async_copy);
> >> +             memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> >> +                     sizeof(copy->cp_dst_stateid));
> >> +             INIT_WORK(&async_copy->cp_work, nfsd4_do_async_copy);
> >> +             queue_work(copy_wq, &async_copy->cp_work);
> >> +     } else {
> >> +             status = nfsd4_do_copy(copy, 1);
> >>       }
> >> -
> >> -     fput(src);
> >> -     fput(dst);
> >>  out:
> >>       return status;
> >>  }
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 0c04f81..fe6f275 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -7033,8 +7033,14 @@ static int nfs4_state_create_net(struct net *net)
> >>               goto out_free_laundry;
> >>
> >>       set_max_delegations();
> >> -     return 0;
> >>
> >> +     ret = nfsd4_create_copy_queue();
> >> +     if (ret)
> >> +             goto out_free_callback;
> >> +
> >> +     return 0;
> >> +out_free_callback:
> >> +     nfsd4_destroy_callback_queue();
> >>  out_free_laundry:
> >>       destroy_workqueue(laundry_wq);
> >>  out_cleanup_cred:
> >> @@ -7097,6 +7103,7 @@ static int nfs4_state_create_net(struct net *net)
> >>       destroy_workqueue(laundry_wq);
> >>       nfsd4_destroy_callback_queue();
> >>       cleanup_callback_cred();
> >> +     nfsd4_destroy_copy_queue();
> >>  }
> >>
> >>  static void
> >> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >> index f8b0210..b5358cf 100644
> >> --- a/fs/nfsd/state.h
> >> +++ b/fs/nfsd/state.h
> >> @@ -630,6 +630,8 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
> >>  extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
> >>                                                       struct nfsd_net *nn);
> >>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
> >> +extern int nfsd4_create_copy_queue(void);
> >> +extern void nfsd4_destroy_copy_queue(void);
> >>
> >>  struct nfs4_file *find_file(struct knfsd_fh *fh);
> >>  void put_nfs4_file(struct nfs4_file *fi);
> >> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> >> index 9b0c099..f7a94d3 100644
> >> --- a/fs/nfsd/xdr4.h
> >> +++ b/fs/nfsd/xdr4.h
> >> @@ -529,6 +529,13 @@ struct nfsd4_copy {
> >>       struct nfsd4_callback   cp_cb;
> >>       __be32                  nfserr;
> >>       struct knfsd_fh         fh;
> >> +
> >> +     struct work_struct      cp_work;
> >> +     struct nfs4_client      *cp_clp;
> >> +
> >> +     struct file             *fh_src;
> >> +     struct file             *fh_dst;
> >> +     struct net              *net;
> >>  };
> >>
> >>  struct nfsd4_seek {
> >> --
> >> 1.8.3.1
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[Olga Kornievskaia]

On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
>> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
>> if found mark it cancelled. If copy has more interations to
>> call vfs_copy_file_range, it'll stop it. Server won't be sending
>> CB_OFFLOAD to the client since it received a cancel.
>>
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
>>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
>>  fs/nfsd/state.h     |  4 ++++
>>  3 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 3cddebb..f4f3d93 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>>       size_t bytes_to_copy;
>>       u64 src_pos = copy->cp_src_pos;
>>       u64 dst_pos = copy->cp_dst_pos;
>> +     bool cancelled = false;
>>
>>       do {
>>               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
>> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>>               copy->cp_res.wr_bytes_written += bytes_copied;
>>               src_pos += bytes_copied;
>>               dst_pos += bytes_copied;
>> -     } while (bytes_total > 0 && !copy->cp_synchronous);
>> +             if (!copy->cp_synchronous) {
>> +                     spin_lock(&copy->cps->cp_lock);
>> +                     cancelled = copy->cps->cp_cancelled;
>> +                     spin_unlock(&copy->cps->cp_lock);
>> +             }
>> +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
>>       return bytes_copied;
>
> I'd rather we sent a signal, and then we won't need this
> logic--vfs_copy_range() will just return EINTR or something.

Hi Bruce,

Now that I've implemented using the kthread instead of the workqueue,
I don't see that it can provide any better  guarantee than the work
queue. vfs_copy_range() is not interrupted in the middle and returning
the EINTR. The function that runs the kthread, it has to at some point
call signalled()/kthread_should_stop() function to see if it was
signaled and use it to 'stop working instead of continuing on'.

If I were to remove the loop and check (if signaled() ||
kthread_should_stop()) before and after calling the
vfs_copy_file_range(), the copy will either not start if the
OFFLOAD_CANCEL was received before copy started or the whole copy
would happen.

Even with the loop, I'd be checking after every call for
vfs_copy_file_range() just like it was in the current version with the
workqueue.

Please advise if you still want the kthread-based implementation or
keep the workqueue.

> That will help us get rid of the 4MB-at-a-time loop.  And will mean we
> don't need to wait for the next 4MB copy to finish before stopping the
> loop.  Normally I wouldn't expect that to take too long, but it might.
> And a situation where a cancel is sent is a situation where we're
> probably more likely to have some problem slowing down the copy.
>
> Also: don't we want OFFLOAD_CANCEL to wait until the cancel has actually
> taken effect before returning?
>
> I can't see any language in the spec to that affect, but it would seem
> surprising to me if I got back a succesful response to OFFLOAD_CANCEL
> and then noticed that the target file was still changing.
>
> --b.
>
>>  }
>>
>> @@ -1198,6 +1204,10 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>>       struct nfsd4_copy *cb_copy;
>>
>>       copy->nfserr = nfsd4_do_copy(copy, 0);
>> +
>> +     if (copy->cps->cp_cancelled)
>> +             goto out;
>> +
>>       cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>>       if (!cb_copy)
>>               goto out;
>> @@ -1269,7 +1279,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>>                    struct nfsd4_compound_state *cstate,
>>                    union nfsd4_op_u *u)
>>  {
>> -     return 0;
>> +     struct nfsd4_offload_status *os = &u->offload_status;
>> +     struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>> +     __be32 status;
>> +     struct nfs4_cp_state *state = NULL;
>> +
>> +     status = find_cp_state(nn, &os->stateid, &state);
>> +     if (state) {
>> +             spin_lock(&state->cp_lock);
>> +             state->cp_cancelled = true;
>> +             spin_unlock(&state->cp_lock);
>> +     }
>> +
>> +     return status;
>>  }
>>
>>  static __be32
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index be59baf..97ab3f8 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -752,6 +752,22 @@ static void nfs4_free_deleg(struct nfs4_stid *stid)
>>       atomic_long_dec(&num_delegations);
>>  }
>>
>> +__be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
>> +                         struct nfs4_cp_state **cps)
>> +{
>> +     struct nfs4_cp_state *state = NULL;
>> +
>> +     if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
>> +             return nfserr_bad_stateid;
>> +     spin_lock(&nn->s2s_cp_lock);
>> +     state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
>> +     spin_unlock(&nn->s2s_cp_lock);
>> +     if (!state)
>> +             return nfserr_bad_stateid;
>> +     *cps = state;
>> +     return 0;
>> +}
>> +
>>  /*
>>   * When we recall a delegation, we should be careful not to hand it
>>   * out again straight away.
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 8724955..7a070d5 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -111,6 +111,8 @@ struct nfs4_cp_state {
>>       stateid_t               cp_stateid;
>>       struct list_head        cp_list;        /* per parent nfs4_stid */
>>       struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
>> +     bool                    cp_cancelled;   /* copy cancelled */
>> +     spinlock_t              cp_lock;
>>  };
>>
>>  /*
>> @@ -647,6 +649,8 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
>>  extern int nfsd4_create_copy_queue(void);
>>  extern void nfsd4_destroy_copy_queue(void);
>> +extern __be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
>> +                     struct nfs4_cp_state **cps);
>>
>>  struct nfs4_file *find_file(struct knfsd_fh *fh);
>>  void put_nfs4_file(struct nfs4_file *fi);
>> --
>> 1.8.3.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[J. Bruce Fields]

On Mon, Oct 09, 2017 at 10:53:13AM -0400, Olga Kornievskaia wrote:
> On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
> >> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
> >> if found mark it cancelled. If copy has more interations to
> >> call vfs_copy_file_range, it'll stop it. Server won't be sending
> >> CB_OFFLOAD to the client since it received a cancel.
> >>
> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >> ---
> >>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
> >>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
> >>  fs/nfsd/state.h     |  4 ++++
> >>  3 files changed, 44 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> index 3cddebb..f4f3d93 100644
> >> --- a/fs/nfsd/nfs4proc.c
> >> +++ b/fs/nfsd/nfs4proc.c
> >> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >>       size_t bytes_to_copy;
> >>       u64 src_pos = copy->cp_src_pos;
> >>       u64 dst_pos = copy->cp_dst_pos;
> >> +     bool cancelled = false;
> >>
> >>       do {
> >>               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> >> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >>               copy->cp_res.wr_bytes_written += bytes_copied;
> >>               src_pos += bytes_copied;
> >>               dst_pos += bytes_copied;
> >> -     } while (bytes_total > 0 && !copy->cp_synchronous);
> >> +             if (!copy->cp_synchronous) {
> >> +                     spin_lock(&copy->cps->cp_lock);
> >> +                     cancelled = copy->cps->cp_cancelled;
> >> +                     spin_unlock(&copy->cps->cp_lock);
> >> +             }
> >> +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
> >>       return bytes_copied;
> >
> > I'd rather we sent a signal, and then we won't need this
> > logic--vfs_copy_range() will just return EINTR or something.
> 
> Hi Bruce,
> 
> Now that I've implemented using the kthread instead of the workqueue,
> I don't see that it can provide any better  guarantee than the work
> queue. vfs_copy_range() is not interrupted in the middle and returning
> the EINTR. The function that runs the kthread, it has to at some point
> call signalled()/kthread_should_stop() function to see if it was
> signaled and use it to 'stop working instead of continuing on'.
> 
> If I were to remove the loop and check (if signaled() ||
> kthread_should_stop()) before and after calling the
> vfs_copy_file_range(), the copy will either not start if the
> OFFLOAD_CANCEL was received before copy started or the whole copy
> would happen.
> 
> Even with the loop, I'd be checking after every call for
> vfs_copy_file_range() just like it was in the current version with the
> workqueue.
> 
> Please advise if you still want the kthread-based implementation or
> keep the workqueue.

That's interesting.

To me that sounds like a bug somewhere under vfs_copy_file_range().
splice_direct_to_actor() can do long-running copies, so it should be
interruptible, shouldn't it?

--b.

> 
> > That will help us get rid of the 4MB-at-a-time loop.  And will mean we
> > don't need to wait for the next 4MB copy to finish before stopping the
> > loop.  Normally I wouldn't expect that to take too long, but it might.
> > And a situation where a cancel is sent is a situation where we're
> > probably more likely to have some problem slowing down the copy.
> >
> > Also: don't we want OFFLOAD_CANCEL to wait until the cancel has actually
> > taken effect before returning?
> >
> > I can't see any language in the spec to that affect, but it would seem
> > surprising to me if I got back a succesful response to OFFLOAD_CANCEL
> > and then noticed that the target file was still changing.
> >
> > --b.
> >
> >>  }
> >>
> >> @@ -1198,6 +1204,10 @@ static void nfsd4_do_async_copy(struct work_struct *work)
> >>       struct nfsd4_copy *cb_copy;
> >>
> >>       copy->nfserr = nfsd4_do_copy(copy, 0);
> >> +
> >> +     if (copy->cps->cp_cancelled)
> >> +             goto out;
> >> +
> >>       cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >>       if (!cb_copy)
> >>               goto out;
> >> @@ -1269,7 +1279,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
> >>                    struct nfsd4_compound_state *cstate,
> >>                    union nfsd4_op_u *u)
> >>  {
> >> -     return 0;
> >> +     struct nfsd4_offload_status *os = &u->offload_status;
> >> +     struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> >> +     __be32 status;
> >> +     struct nfs4_cp_state *state = NULL;
> >> +
> >> +     status = find_cp_state(nn, &os->stateid, &state);
> >> +     if (state) {
> >> +             spin_lock(&state->cp_lock);
> >> +             state->cp_cancelled = true;
> >> +             spin_unlock(&state->cp_lock);
> >> +     }
> >> +
> >> +     return status;
> >>  }
> >>
> >>  static __be32
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index be59baf..97ab3f8 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -752,6 +752,22 @@ static void nfs4_free_deleg(struct nfs4_stid *stid)
> >>       atomic_long_dec(&num_delegations);
> >>  }
> >>
> >> +__be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
> >> +                         struct nfs4_cp_state **cps)
> >> +{
> >> +     struct nfs4_cp_state *state = NULL;
> >> +
> >> +     if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
> >> +             return nfserr_bad_stateid;
> >> +     spin_lock(&nn->s2s_cp_lock);
> >> +     state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
> >> +     spin_unlock(&nn->s2s_cp_lock);
> >> +     if (!state)
> >> +             return nfserr_bad_stateid;
> >> +     *cps = state;
> >> +     return 0;
> >> +}
> >> +
> >>  /*
> >>   * When we recall a delegation, we should be careful not to hand it
> >>   * out again straight away.
> >> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >> index 8724955..7a070d5 100644
> >> --- a/fs/nfsd/state.h
> >> +++ b/fs/nfsd/state.h
> >> @@ -111,6 +111,8 @@ struct nfs4_cp_state {
> >>       stateid_t               cp_stateid;
> >>       struct list_head        cp_list;        /* per parent nfs4_stid */
> >>       struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
> >> +     bool                    cp_cancelled;   /* copy cancelled */
> >> +     spinlock_t              cp_lock;
> >>  };
> >>
> >>  /*
> >> @@ -647,6 +649,8 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
> >>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
> >>  extern int nfsd4_create_copy_queue(void);
> >>  extern void nfsd4_destroy_copy_queue(void);
> >> +extern __be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
> >> +                     struct nfs4_cp_state **cps);
> >>
> >>  struct nfs4_file *find_file(struct knfsd_fh *fh);
> >>  void put_nfs4_file(struct nfs4_file *fi);
> >> --
> >> 1.8.3.1
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[Olga Kornievskaia]

On Mon, Oct 9, 2017 at 11:58 AM, J. Bruce Fields <bfields@redhat.com> wrote:
> On Mon, Oct 09, 2017 at 10:53:13AM -0400, Olga Kornievskaia wrote:
>> On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
>> > On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
>> >> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
>> >> if found mark it cancelled. If copy has more interations to
>> >> call vfs_copy_file_range, it'll stop it. Server won't be sending
>> >> CB_OFFLOAD to the client since it received a cancel.
>> >>
>> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> >> ---
>> >>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
>> >>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
>> >>  fs/nfsd/state.h     |  4 ++++
>> >>  3 files changed, 44 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> >> index 3cddebb..f4f3d93 100644
>> >> --- a/fs/nfsd/nfs4proc.c
>> >> +++ b/fs/nfsd/nfs4proc.c
>> >> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> >>       size_t bytes_to_copy;
>> >>       u64 src_pos = copy->cp_src_pos;
>> >>       u64 dst_pos = copy->cp_dst_pos;
>> >> +     bool cancelled = false;
>> >>
>> >>       do {
>> >>               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
>> >> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> >>               copy->cp_res.wr_bytes_written += bytes_copied;
>> >>               src_pos += bytes_copied;
>> >>               dst_pos += bytes_copied;
>> >> -     } while (bytes_total > 0 && !copy->cp_synchronous);
>> >> +             if (!copy->cp_synchronous) {
>> >> +                     spin_lock(&copy->cps->cp_lock);
>> >> +                     cancelled = copy->cps->cp_cancelled;
>> >> +                     spin_unlock(&copy->cps->cp_lock);
>> >> +             }
>> >> +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
>> >>       return bytes_copied;
>> >
>> > I'd rather we sent a signal, and then we won't need this
>> > logic--vfs_copy_range() will just return EINTR or something.
>>
>> Hi Bruce,
>>
>> Now that I've implemented using the kthread instead of the workqueue,
>> I don't see that it can provide any better  guarantee than the work
>> queue. vfs_copy_range() is not interrupted in the middle and returning
>> the EINTR. The function that runs the kthread, it has to at some point
>> call signalled()/kthread_should_stop() function to see if it was
>> signaled and use it to 'stop working instead of continuing on'.
>>
>> If I were to remove the loop and check (if signaled() ||
>> kthread_should_stop()) before and after calling the
>> vfs_copy_file_range(), the copy will either not start if the
>> OFFLOAD_CANCEL was received before copy started or the whole copy
>> would happen.
>>
>> Even with the loop, I'd be checking after every call for
>> vfs_copy_file_range() just like it was in the current version with the
>> workqueue.
>>
>> Please advise if you still want the kthread-based implementation or
>> keep the workqueue.
>
> That's interesting.
>
> To me that sounds like a bug somewhere under vfs_copy_file_range().
> splice_direct_to_actor() can do long-running copies, so it should be
> interruptible, shouldn't it?

So I found it. Yes do_splice_direct() will react to somebody sending a
ctrl-c and will stop. It calls signal_pendning(). However, in our
case, I'm calling kthread_stop() and that sets a different flag and
one needs to also check for kthread_should_stop() as a stopping
condition. splice.c lacks that.

I hope they can agree that it's a bug. I don't have any luck with VFS...



>
> --b.
>
>>
>> > That will help us get rid of the 4MB-at-a-time loop.  And will mean we
>> > don't need to wait for the next 4MB copy to finish before stopping the
>> > loop.  Normally I wouldn't expect that to take too long, but it might.
>> > And a situation where a cancel is sent is a situation where we're
>> > probably more likely to have some problem slowing down the copy.
>> >
>> > Also: don't we want OFFLOAD_CANCEL to wait until the cancel has actually
>> > taken effect before returning?
>> >
>> > I can't see any language in the spec to that affect, but it would seem
>> > surprising to me if I got back a succesful response to OFFLOAD_CANCEL
>> > and then noticed that the target file was still changing.
>> >
>> > --b.
>> >
>> >>  }
>> >>
>> >> @@ -1198,6 +1204,10 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>> >>       struct nfsd4_copy *cb_copy;
>> >>
>> >>       copy->nfserr = nfsd4_do_copy(copy, 0);
>> >> +
>> >> +     if (copy->cps->cp_cancelled)
>> >> +             goto out;
>> >> +
>> >>       cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>> >>       if (!cb_copy)
>> >>               goto out;
>> >> @@ -1269,7 +1279,19 @@ static void nfsd4_do_async_copy(struct work_struct *work)
>> >>                    struct nfsd4_compound_state *cstate,
>> >>                    union nfsd4_op_u *u)
>> >>  {
>> >> -     return 0;
>> >> +     struct nfsd4_offload_status *os = &u->offload_status;
>> >> +     struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>> >> +     __be32 status;
>> >> +     struct nfs4_cp_state *state = NULL;
>> >> +
>> >> +     status = find_cp_state(nn, &os->stateid, &state);
>> >> +     if (state) {
>> >> +             spin_lock(&state->cp_lock);
>> >> +             state->cp_cancelled = true;
>> >> +             spin_unlock(&state->cp_lock);
>> >> +     }
>> >> +
>> >> +     return status;
>> >>  }
>> >>
>> >>  static __be32
>> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> >> index be59baf..97ab3f8 100644
>> >> --- a/fs/nfsd/nfs4state.c
>> >> +++ b/fs/nfsd/nfs4state.c
>> >> @@ -752,6 +752,22 @@ static void nfs4_free_deleg(struct nfs4_stid *stid)
>> >>       atomic_long_dec(&num_delegations);
>> >>  }
>> >>
>> >> +__be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
>> >> +                         struct nfs4_cp_state **cps)
>> >> +{
>> >> +     struct nfs4_cp_state *state = NULL;
>> >> +
>> >> +     if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
>> >> +             return nfserr_bad_stateid;
>> >> +     spin_lock(&nn->s2s_cp_lock);
>> >> +     state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
>> >> +     spin_unlock(&nn->s2s_cp_lock);
>> >> +     if (!state)
>> >> +             return nfserr_bad_stateid;
>> >> +     *cps = state;
>> >> +     return 0;
>> >> +}
>> >> +
>> >>  /*
>> >>   * When we recall a delegation, we should be careful not to hand it
>> >>   * out again straight away.
>> >> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> >> index 8724955..7a070d5 100644
>> >> --- a/fs/nfsd/state.h
>> >> +++ b/fs/nfsd/state.h
>> >> @@ -111,6 +111,8 @@ struct nfs4_cp_state {
>> >>       stateid_t               cp_stateid;
>> >>       struct list_head        cp_list;        /* per parent nfs4_stid */
>> >>       struct nfs4_stid        *cp_p_stid;     /* pointer to parent */
>> >> +     bool                    cp_cancelled;   /* copy cancelled */
>> >> +     spinlock_t              cp_lock;
>> >>  };
>> >>
>> >>  /*
>> >> @@ -647,6 +649,8 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>> >>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
>> >>  extern int nfsd4_create_copy_queue(void);
>> >>  extern void nfsd4_destroy_copy_queue(void);
>> >> +extern __be32 find_cp_state(struct nfsd_net *nn, stateid_t *st,
>> >> +                     struct nfs4_cp_state **cps);
>> >>
>> >>  struct nfs4_file *find_file(struct knfsd_fh *fh);
>> >>  void put_nfs4_file(struct nfs4_file *fi);
>> >> --
>> >> 1.8.3.1
>> >>
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[J. Bruce Fields]

On Tue, Oct 10, 2017 at 05:14:29PM -0400, Olga Kornievskaia wrote:
> On Mon, Oct 9, 2017 at 11:58 AM, J. Bruce Fields <bfields@redhat.com> wrote:
> > On Mon, Oct 09, 2017 at 10:53:13AM -0400, Olga Kornievskaia wrote:
> >> On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> >> > On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
> >> >> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
> >> >> if found mark it cancelled. If copy has more interations to
> >> >> call vfs_copy_file_range, it'll stop it. Server won't be sending
> >> >> CB_OFFLOAD to the client since it received a cancel.
> >> >>
> >> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >> >> ---
> >> >>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
> >> >>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
> >> >>  fs/nfsd/state.h     |  4 ++++
> >> >>  3 files changed, 44 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> >> index 3cddebb..f4f3d93 100644
> >> >> --- a/fs/nfsd/nfs4proc.c
> >> >> +++ b/fs/nfsd/nfs4proc.c
> >> >> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >> >>       size_t bytes_to_copy;
> >> >>       u64 src_pos = copy->cp_src_pos;
> >> >>       u64 dst_pos = copy->cp_dst_pos;
> >> >> +     bool cancelled = false;
> >> >>
> >> >>       do {
> >> >>               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> >> >> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >> >>               copy->cp_res.wr_bytes_written += bytes_copied;
> >> >>               src_pos += bytes_copied;
> >> >>               dst_pos += bytes_copied;
> >> >> -     } while (bytes_total > 0 && !copy->cp_synchronous);
> >> >> +             if (!copy->cp_synchronous) {
> >> >> +                     spin_lock(&copy->cps->cp_lock);
> >> >> +                     cancelled = copy->cps->cp_cancelled;
> >> >> +                     spin_unlock(&copy->cps->cp_lock);
> >> >> +             }
> >> >> +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
> >> >>       return bytes_copied;
> >> >
> >> > I'd rather we sent a signal, and then we won't need this
> >> > logic--vfs_copy_range() will just return EINTR or something.
> >>
> >> Hi Bruce,
> >>
> >> Now that I've implemented using the kthread instead of the workqueue,
> >> I don't see that it can provide any better  guarantee than the work
> >> queue. vfs_copy_range() is not interrupted in the middle and returning
> >> the EINTR. The function that runs the kthread, it has to at some point
> >> call signalled()/kthread_should_stop() function to see if it was
> >> signaled and use it to 'stop working instead of continuing on'.
> >>
> >> If I were to remove the loop and check (if signaled() ||
> >> kthread_should_stop()) before and after calling the
> >> vfs_copy_file_range(), the copy will either not start if the
> >> OFFLOAD_CANCEL was received before copy started or the whole copy
> >> would happen.
> >>
> >> Even with the loop, I'd be checking after every call for
> >> vfs_copy_file_range() just like it was in the current version with the
> >> workqueue.
> >>
> >> Please advise if you still want the kthread-based implementation or
> >> keep the workqueue.
> >
> > That's interesting.
> >
> > To me that sounds like a bug somewhere under vfs_copy_file_range().
> > splice_direct_to_actor() can do long-running copies, so it should be
> > interruptible, shouldn't it?
> 
> So I found it. Yes do_splice_direct() will react to somebody sending a
> ctrl-c and will stop. It calls signal_pendning(). However, in our
> case, I'm calling kthread_stop() and that sets a different flag and
> one needs to also check for kthread_should_stop() as a stopping
> condition. splice.c lacks that.
> 
> I hope they can agree that it's a bug. I don't have any luck with VFS...

Argh.  No, it's probably not their bug, I guess kthreads just ignore
signals.  OK, I can't immediately see what the right thing to do is
here....

I do think we need to do something as we want to be able to interrupt
and clean up copy threads when we can.

--b.

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[Olga Kornievskaia]

On Wed, Oct 11, 2017 at 10:07 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Tue, Oct 10, 2017 at 05:14:29PM -0400, Olga Kornievskaia wrote:
>> On Mon, Oct 9, 2017 at 11:58 AM, J. Bruce Fields <bfields@redhat.com> wrote:
>> > On Mon, Oct 09, 2017 at 10:53:13AM -0400, Olga Kornievskaia wrote:
>> >> On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
>> >> > On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
>> >> >> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
>> >> >> if found mark it cancelled. If copy has more interations to
>> >> >> call vfs_copy_file_range, it'll stop it. Server won't be sending
>> >> >> CB_OFFLOAD to the client since it received a cancel.
>> >> >>
>> >> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> >> >> ---
>> >> >>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
>> >> >>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
>> >> >>  fs/nfsd/state.h     |  4 ++++
>> >> >>  3 files changed, 44 insertions(+), 2 deletions(-)
>> >> >>
>> >> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> >> >> index 3cddebb..f4f3d93 100644
>> >> >> --- a/fs/nfsd/nfs4proc.c
>> >> >> +++ b/fs/nfsd/nfs4proc.c
>> >> >> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> >> >>       size_t bytes_to_copy;
>> >> >>       u64 src_pos = copy->cp_src_pos;
>> >> >>       u64 dst_pos = copy->cp_dst_pos;
>> >> >> +     bool cancelled = false;
>> >> >>
>> >> >>       do {
>> >> >>               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
>> >> >> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> >> >>               copy->cp_res.wr_bytes_written += bytes_copied;
>> >> >>               src_pos += bytes_copied;
>> >> >>               dst_pos += bytes_copied;
>> >> >> -     } while (bytes_total > 0 && !copy->cp_synchronous);
>> >> >> +             if (!copy->cp_synchronous) {
>> >> >> +                     spin_lock(&copy->cps->cp_lock);
>> >> >> +                     cancelled = copy->cps->cp_cancelled;
>> >> >> +                     spin_unlock(&copy->cps->cp_lock);
>> >> >> +             }
>> >> >> +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
>> >> >>       return bytes_copied;
>> >> >
>> >> > I'd rather we sent a signal, and then we won't need this
>> >> > logic--vfs_copy_range() will just return EINTR or something.
>> >>
>> >> Hi Bruce,
>> >>
>> >> Now that I've implemented using the kthread instead of the workqueue,
>> >> I don't see that it can provide any better  guarantee than the work
>> >> queue. vfs_copy_range() is not interrupted in the middle and returning
>> >> the EINTR. The function that runs the kthread, it has to at some point
>> >> call signalled()/kthread_should_stop() function to see if it was
>> >> signaled and use it to 'stop working instead of continuing on'.
>> >>
>> >> If I were to remove the loop and check (if signaled() ||
>> >> kthread_should_stop()) before and after calling the
>> >> vfs_copy_file_range(), the copy will either not start if the
>> >> OFFLOAD_CANCEL was received before copy started or the whole copy
>> >> would happen.
>> >>
>> >> Even with the loop, I'd be checking after every call for
>> >> vfs_copy_file_range() just like it was in the current version with the
>> >> workqueue.
>> >>
>> >> Please advise if you still want the kthread-based implementation or
>> >> keep the workqueue.
>> >
>> > That's interesting.
>> >
>> > To me that sounds like a bug somewhere under vfs_copy_file_range().
>> > splice_direct_to_actor() can do long-running copies, so it should be
>> > interruptible, shouldn't it?
>>
>> So I found it. Yes do_splice_direct() will react to somebody sending a
>> ctrl-c and will stop. It calls signal_pendning(). However, in our
>> case, I'm calling kthread_stop() and that sets a different flag and
>> one needs to also check for kthread_should_stop() as a stopping
>> condition. splice.c lacks that.
>>
>> I hope they can agree that it's a bug. I don't have any luck with VFS...
>
> Argh.  No, it's probably not their bug, I guess kthreads just ignore
> signals.  OK, I can't immediately see what the right thing to do is
> here....
>
> I do think we need to do something as we want to be able to interrupt
> and clean up copy threads when we can.

A bug is not the right word. It would be asking them to accommodate
stopping to include kthread_stop condition. Why do you say kthreads
ignore signals? You can say that kthread_stop doesn't send a signal.

Also another note, I still can't remove the loop around the call to
the vfs_copy_file_range() because it's not guaranteed to copy all the
bytes that the call asks for. The implementation of
vfs_copy_file_range will do_splice_direct only MAX_RW_COUNT at a time.
So the upper layer needs to loop to make sure it copies all the bytes.

If VFS will decide to reject the request to add kthread_should_stop to
their conditions, then the loop could be a way to stop every 4MB.
Copying 4MB would be the equivalent of what the current synchronous
copy does now anyway?

>
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[J. Bruce Fields]

On Wed, Oct 11, 2017 at 11:02:56AM -0400, Olga Kornievskaia wrote:
> On Wed, Oct 11, 2017 at 10:07 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Tue, Oct 10, 2017 at 05:14:29PM -0400, Olga Kornievskaia wrote:
> >> On Mon, Oct 9, 2017 at 11:58 AM, J. Bruce Fields <bfields@redhat.com> wrote:
> >> > On Mon, Oct 09, 2017 at 10:53:13AM -0400, Olga Kornievskaia wrote:
> >> >> On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> >> >> > On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
> >> >> >> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
> >> >> >> if found mark it cancelled. If copy has more interations to
> >> >> >> call vfs_copy_file_range, it'll stop it. Server won't be sending
> >> >> >> CB_OFFLOAD to the client since it received a cancel.
> >> >> >>
> >> >> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >> >> >> ---
> >> >> >>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
> >> >> >>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
> >> >> >>  fs/nfsd/state.h     |  4 ++++
> >> >> >>  3 files changed, 44 insertions(+), 2 deletions(-)
> >> >> >>
> >> >> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> >> >> index 3cddebb..f4f3d93 100644
> >> >> >> --- a/fs/nfsd/nfs4proc.c
> >> >> >> +++ b/fs/nfsd/nfs4proc.c
> >> >> >> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >> >> >>       size_t bytes_to_copy;
> >> >> >>       u64 src_pos = copy->cp_src_pos;
> >> >> >>       u64 dst_pos = copy->cp_dst_pos;
> >> >> >> +     bool cancelled = false;
> >> >> >>
> >> >> >>       do {
> >> >> >>               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> >> >> >> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> >> >> >>               copy->cp_res.wr_bytes_written += bytes_copied;
> >> >> >>               src_pos += bytes_copied;
> >> >> >>               dst_pos += bytes_copied;
> >> >> >> -     } while (bytes_total > 0 && !copy->cp_synchronous);
> >> >> >> +             if (!copy->cp_synchronous) {
> >> >> >> +                     spin_lock(&copy->cps->cp_lock);
> >> >> >> +                     cancelled = copy->cps->cp_cancelled;
> >> >> >> +                     spin_unlock(&copy->cps->cp_lock);
> >> >> >> +             }
> >> >> >> +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
> >> >> >>       return bytes_copied;
> >> >> >
> >> >> > I'd rather we sent a signal, and then we won't need this
> >> >> > logic--vfs_copy_range() will just return EINTR or something.
> >> >>
> >> >> Hi Bruce,
> >> >>
> >> >> Now that I've implemented using the kthread instead of the workqueue,
> >> >> I don't see that it can provide any better  guarantee than the work
> >> >> queue. vfs_copy_range() is not interrupted in the middle and returning
> >> >> the EINTR. The function that runs the kthread, it has to at some point
> >> >> call signalled()/kthread_should_stop() function to see if it was
> >> >> signaled and use it to 'stop working instead of continuing on'.
> >> >>
> >> >> If I were to remove the loop and check (if signaled() ||
> >> >> kthread_should_stop()) before and after calling the
> >> >> vfs_copy_file_range(), the copy will either not start if the
> >> >> OFFLOAD_CANCEL was received before copy started or the whole copy
> >> >> would happen.
> >> >>
> >> >> Even with the loop, I'd be checking after every call for
> >> >> vfs_copy_file_range() just like it was in the current version with the
> >> >> workqueue.
> >> >>
> >> >> Please advise if you still want the kthread-based implementation or
> >> >> keep the workqueue.
> >> >
> >> > That's interesting.
> >> >
> >> > To me that sounds like a bug somewhere under vfs_copy_file_range().
> >> > splice_direct_to_actor() can do long-running copies, so it should be
> >> > interruptible, shouldn't it?
> >>
> >> So I found it. Yes do_splice_direct() will react to somebody sending a
> >> ctrl-c and will stop. It calls signal_pendning(). However, in our
> >> case, I'm calling kthread_stop() and that sets a different flag and
> >> one needs to also check for kthread_should_stop() as a stopping
> >> condition. splice.c lacks that.
> >>
> >> I hope they can agree that it's a bug. I don't have any luck with VFS...
> >
> > Argh.  No, it's probably not their bug, I guess kthreads just ignore
> > signals.  OK, I can't immediately see what the right thing to do is
> > here....
> >
> > I do think we need to do something as we want to be able to interrupt
> > and clean up copy threads when we can.
> 
> A bug is not the right word. It would be asking them to accommodate
> stopping to include kthread_stop condition. Why do you say kthreads
> ignore signals? You can say that kthread_stop doesn't send a signal.

I think both are true.

I doubt it's reasonable to add kthread_should_stop everywhere that
there are currently checks for signals.

> Also another note, I still can't remove the loop around the call to
> the vfs_copy_file_range() because it's not guaranteed to copy all the
> bytes that the call asks for. The implementation of
> vfs_copy_file_range will do_splice_direct only MAX_RW_COUNT at a time.
> So the upper layer needs to loop to make sure it copies all the bytes.

MAX_RW_COUNT is about 4 gigs.  I'm not sure if it's really a problem to
copy only 4 gigs at a time?  But, yes, maybe the loop is still worth it.

> If VFS will decide to reject the request to add kthread_should_stop to
> their conditions, then the loop could be a way to stop every 4MB.
> Copying 4MB would be the equivalent of what the current synchronous
> copy does now anyway?

I'm still a little worried about copy threads hanging indefinitely if
the peer goes away mid-copy.  The ability to signal the copy thread
would help.

--b.

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[Olga Kornievskaia]

On Wed, Oct 11, 2017 at 11:19 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Wed, Oct 11, 2017 at 11:02:56AM -0400, Olga Kornievskaia wrote:
>> On Wed, Oct 11, 2017 at 10:07 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Tue, Oct 10, 2017 at 05:14:29PM -0400, Olga Kornievskaia wrote:
>> >> On Mon, Oct 9, 2017 at 11:58 AM, J. Bruce Fields <bfields@redhat.com> wrote:
>> >> > On Mon, Oct 09, 2017 at 10:53:13AM -0400, Olga Kornievskaia wrote:
>> >> >> On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
>> >> >> > On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
>> >> >> >> Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
>> >> >> >> if found mark it cancelled. If copy has more interations to
>> >> >> >> call vfs_copy_file_range, it'll stop it. Server won't be sending
>> >> >> >> CB_OFFLOAD to the client since it received a cancel.
>> >> >> >>
>> >> >> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> >> >> >> ---
>> >> >> >>  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
>> >> >> >>  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
>> >> >> >>  fs/nfsd/state.h     |  4 ++++
>> >> >> >>  3 files changed, 44 insertions(+), 2 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> >> >> >> index 3cddebb..f4f3d93 100644
>> >> >> >> --- a/fs/nfsd/nfs4proc.c
>> >> >> >> +++ b/fs/nfsd/nfs4proc.c
>> >> >> >> @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> >> >> >>       size_t bytes_to_copy;
>> >> >> >>       u64 src_pos = copy->cp_src_pos;
>> >> >> >>       u64 dst_pos = copy->cp_dst_pos;
>> >> >> >> +     bool cancelled = false;
>> >> >> >>
>> >> >> >>       do {
>> >> >> >>               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
>> >> >> >> @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
>> >> >> >>               copy->cp_res.wr_bytes_written += bytes_copied;
>> >> >> >>               src_pos += bytes_copied;
>> >> >> >>               dst_pos += bytes_copied;
>> >> >> >> -     } while (bytes_total > 0 && !copy->cp_synchronous);
>> >> >> >> +             if (!copy->cp_synchronous) {
>> >> >> >> +                     spin_lock(&copy->cps->cp_lock);
>> >> >> >> +                     cancelled = copy->cps->cp_cancelled;
>> >> >> >> +                     spin_unlock(&copy->cps->cp_lock);
>> >> >> >> +             }
>> >> >> >> +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
>> >> >> >>       return bytes_copied;
>> >> >> >
>> >> >> > I'd rather we sent a signal, and then we won't need this
>> >> >> > logic--vfs_copy_range() will just return EINTR or something.
>> >> >>
>> >> >> Hi Bruce,
>> >> >>
>> >> >> Now that I've implemented using the kthread instead of the workqueue,
>> >> >> I don't see that it can provide any better  guarantee than the work
>> >> >> queue. vfs_copy_range() is not interrupted in the middle and returning
>> >> >> the EINTR. The function that runs the kthread, it has to at some point
>> >> >> call signalled()/kthread_should_stop() function to see if it was
>> >> >> signaled and use it to 'stop working instead of continuing on'.
>> >> >>
>> >> >> If I were to remove the loop and check (if signaled() ||
>> >> >> kthread_should_stop()) before and after calling the
>> >> >> vfs_copy_file_range(), the copy will either not start if the
>> >> >> OFFLOAD_CANCEL was received before copy started or the whole copy
>> >> >> would happen.
>> >> >>
>> >> >> Even with the loop, I'd be checking after every call for
>> >> >> vfs_copy_file_range() just like it was in the current version with the
>> >> >> workqueue.
>> >> >>
>> >> >> Please advise if you still want the kthread-based implementation or
>> >> >> keep the workqueue.
>> >> >
>> >> > That's interesting.
>> >> >
>> >> > To me that sounds like a bug somewhere under vfs_copy_file_range().
>> >> > splice_direct_to_actor() can do long-running copies, so it should be
>> >> > interruptible, shouldn't it?
>> >>
>> >> So I found it. Yes do_splice_direct() will react to somebody sending a
>> >> ctrl-c and will stop. It calls signal_pendning(). However, in our
>> >> case, I'm calling kthread_stop() and that sets a different flag and
>> >> one needs to also check for kthread_should_stop() as a stopping
>> >> condition. splice.c lacks that.
>> >>
>> >> I hope they can agree that it's a bug. I don't have any luck with VFS...
>> >
>> > Argh.  No, it's probably not their bug, I guess kthreads just ignore
>> > signals.  OK, I can't immediately see what the right thing to do is
>> > here....
>> >
>> > I do think we need to do something as we want to be able to interrupt
>> > and clean up copy threads when we can.
>>
>> A bug is not the right word. It would be asking them to accommodate
>> stopping to include kthread_stop condition. Why do you say kthreads
>> ignore signals? You can say that kthread_stop doesn't send a signal.
>
> I think both are true.
>
> I doubt it's reasonable to add kthread_should_stop everywhere that
> there are currently checks for signals.
>
>> Also another note, I still can't remove the loop around the call to
>> the vfs_copy_file_range() because it's not guaranteed to copy all the
>> bytes that the call asks for. The implementation of
>> vfs_copy_file_range will do_splice_direct only MAX_RW_COUNT at a time.
>> So the upper layer needs to loop to make sure it copies all the bytes.
>
> MAX_RW_COUNT is about 4 gigs.  I'm not sure if it's really a problem to
> copy only 4 gigs at a time?  But, yes, maybe the loop is still worth it.
>
>> If VFS will decide to reject the request to add kthread_should_stop to
>> their conditions, then the loop could be a way to stop every 4MB.
>> Copying 4MB would be the equivalent of what the current synchronous
>> copy does now anyway?
>
> I'm still a little worried about copy threads hanging indefinitely if
> the peer goes away mid-copy.  The ability to signal the copy thread
> would help.

So I don't know if this is ok or not but I can directly set the
SIGPENDING bit in the copy task structure from the OFFLOAD_CANCEL
thread and that stops the splice copy too.

>
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 08/10] NFSD handle OFFLOAD_CANCEL op

[Jeff Layton]

On Wed, 2017-10-11 at 12:08 -0400, Olga Kornievskaia wrote:
> On Wed, Oct 11, 2017 at 11:19 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Wed, Oct 11, 2017 at 11:02:56AM -0400, Olga Kornievskaia wrote:
> > > On Wed, Oct 11, 2017 at 10:07 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > > > On Tue, Oct 10, 2017 at 05:14:29PM -0400, Olga Kornievskaia wrote:
> > > > > On Mon, Oct 9, 2017 at 11:58 AM, J. Bruce Fields <bfields@redhat.com> wrote:
> > > > > > On Mon, Oct 09, 2017 at 10:53:13AM -0400, Olga Kornievskaia wrote:
> > > > > > > On Thu, Sep 28, 2017 at 2:38 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > > > > > > > On Thu, Sep 28, 2017 at 01:29:43PM -0400, Olga Kornievskaia wrote:
> > > > > > > > > Upon receiving OFFLOAD_CANCEL search the list of copy stateids,
> > > > > > > > > if found mark it cancelled. If copy has more interations to
> > > > > > > > > call vfs_copy_file_range, it'll stop it. Server won't be sending
> > > > > > > > > CB_OFFLOAD to the client since it received a cancel.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > > > > > > > ---
> > > > > > > > >  fs/nfsd/nfs4proc.c  | 26 ++++++++++++++++++++++++--
> > > > > > > > >  fs/nfsd/nfs4state.c | 16 ++++++++++++++++
> > > > > > > > >  fs/nfsd/state.h     |  4 ++++
> > > > > > > > >  3 files changed, 44 insertions(+), 2 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > > > index 3cddebb..f4f3d93 100644
> > > > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > > > @@ -1139,6 +1139,7 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> > > > > > > > >       size_t bytes_to_copy;
> > > > > > > > >       u64 src_pos = copy->cp_src_pos;
> > > > > > > > >       u64 dst_pos = copy->cp_dst_pos;
> > > > > > > > > +     bool cancelled = false;
> > > > > > > > > 
> > > > > > > > >       do {
> > > > > > > > >               bytes_to_copy = min_t(u64, bytes_total, MAX_RW_COUNT);
> > > > > > > > > @@ -1150,7 +1151,12 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
> > > > > > > > >               copy->cp_res.wr_bytes_written += bytes_copied;
> > > > > > > > >               src_pos += bytes_copied;
> > > > > > > > >               dst_pos += bytes_copied;
> > > > > > > > > -     } while (bytes_total > 0 && !copy->cp_synchronous);
> > > > > > > > > +             if (!copy->cp_synchronous) {
> > > > > > > > > +                     spin_lock(&copy->cps->cp_lock);
> > > > > > > > > +                     cancelled = copy->cps->cp_cancelled;
> > > > > > > > > +                     spin_unlock(&copy->cps->cp_lock);
> > > > > > > > > +             }
> > > > > > > > > +     } while (bytes_total > 0 && !copy->cp_synchronous && !cancelled);
> > > > > > > > >       return bytes_copied;
> > > > > > > > 
> > > > > > > > I'd rather we sent a signal, and then we won't need this
> > > > > > > > logic--vfs_copy_range() will just return EINTR or something.
> > > > > > > 
> > > > > > > Hi Bruce,
> > > > > > > 
> > > > > > > Now that I've implemented using the kthread instead of the workqueue,
> > > > > > > I don't see that it can provide any better  guarantee than the work
> > > > > > > queue. vfs_copy_range() is not interrupted in the middle and returning
> > > > > > > the EINTR. The function that runs the kthread, it has to at some point
> > > > > > > call signalled()/kthread_should_stop() function to see if it was
> > > > > > > signaled and use it to 'stop working instead of continuing on'.
> > > > > > > 
> > > > > > > If I were to remove the loop and check (if signaled() ||
> > > > > > > kthread_should_stop()) before and after calling the
> > > > > > > vfs_copy_file_range(), the copy will either not start if the
> > > > > > > OFFLOAD_CANCEL was received before copy started or the whole copy
> > > > > > > would happen.
> > > > > > > 
> > > > > > > Even with the loop, I'd be checking after every call for
> > > > > > > vfs_copy_file_range() just like it was in the current version with the
> > > > > > > workqueue.
> > > > > > > 
> > > > > > > Please advise if you still want the kthread-based implementation or
> > > > > > > keep the workqueue.
> > > > > > 
> > > > > > That's interesting.
> > > > > > 
> > > > > > To me that sounds like a bug somewhere under vfs_copy_file_range().
> > > > > > splice_direct_to_actor() can do long-running copies, so it should be
> > > > > > interruptible, shouldn't it?
> > > > > 
> > > > > So I found it. Yes do_splice_direct() will react to somebody sending a
> > > > > ctrl-c and will stop. It calls signal_pendning(). However, in our
> > > > > case, I'm calling kthread_stop() and that sets a different flag and
> > > > > one needs to also check for kthread_should_stop() as a stopping
> > > > > condition. splice.c lacks that.
> > > > > 
> > > > > I hope they can agree that it's a bug. I don't have any luck with VFS...
> > > > 
> > > > Argh.  No, it's probably not their bug, I guess kthreads just ignore
> > > > signals.  OK, I can't immediately see what the right thing to do is
> > > > here....
> > > > 
> > > > I do think we need to do something as we want to be able to interrupt
> > > > and clean up copy threads when we can.
> > > 
> > > A bug is not the right word. It would be asking them to accommodate
> > > stopping to include kthread_stop condition. Why do you say kthreads
> > > ignore signals? You can say that kthread_stop doesn't send a signal.
> > 
> > I think both are true.
> > 
> > I doubt it's reasonable to add kthread_should_stop everywhere that
> > there are currently checks for signals.
> > 
> > > Also another note, I still can't remove the loop around the call to
> > > the vfs_copy_file_range() because it's not guaranteed to copy all the
> > > bytes that the call asks for. The implementation of
> > > vfs_copy_file_range will do_splice_direct only MAX_RW_COUNT at a time.
> > > So the upper layer needs to loop to make sure it copies all the bytes.
> > 
> > MAX_RW_COUNT is about 4 gigs.  I'm not sure if it's really a problem to
> > copy only 4 gigs at a time?  But, yes, maybe the loop is still worth it.
> > 
> > > If VFS will decide to reject the request to add kthread_should_stop to
> > > their conditions, then the loop could be a way to stop every 4MB.
> > > Copying 4MB would be the equivalent of what the current synchronous
> > > copy does now anyway?
> > 
> > I'm still a little worried about copy threads hanging indefinitely if
> > the peer goes away mid-copy.  The ability to signal the copy thread
> > would help.
> 
> So I don't know if this is ok or not but I can directly set the
> SIGPENDING bit in the copy task structure from the OFFLOAD_CANCEL
> thread and that stops the splice copy too.

Sorry I'm a bit late here...

The main reason (IMO) to use your own thread here is to avoid squatting
on top of workqueue threads for too long. Workqueue threads are shared
by all workqueues in the system, so when you use one to run your long-
running in-kernel copy task, you're potentially depriving other (quite
possibly more immediate) work from getting done. Spawning a thread in
the context of copy offload seems like negligible overhead and is
"nicer" to the rest of the system.

There's also the problem of how to interrupt a long-running copy. Early
on, kthreadd will ignore_signals(), so kthreads (including workqueue
threads) by default will always ignore them because they inherit that.

If you decide to use workqueues, you _must_ reinstate the signal handler
state for the thread when you're done. Otherwise you'll leave other work
receptive to signals (which may be problematic). There's also the
problem of figuring out which kthread to send a signal when you want to
interrupt it. Not trivial once you've handed it off to the workqueue.

So, I think a kthread is also just simpler here. When you spawn a
thread, you need to make it be receptive to signals to allow
vfs_copy_file_range to see them and stop what it's doing. It's your
choice whether to use that signal as a key to shut down (like nfsd does)
or whether you want to just use kthread_stop. nfsd() does this, so you
may be able to use it to guide you.

The simplest approach (IMO) is to just check signal_pending when the
copy returns, and cleanup/exit the thread when it returns true. You need
to be able to deal with signals anyway to interrupt a copy, so using the
kthread_stop mechanism doesn't really add anything there.

Hope this helps!
-- 
Jeff Layton <jlayton@redhat.com>