NFSD support for asynchronous COPY

Signed-off-by Olga Kornievskaia <[email protected]>

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

fs/nfsd/nfs4callback.c +28 -0

index 49b0a9e..d12d914 100644
@@ -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;

fs/nfsd/nfs4callback.c +59 -0

+	}
+}
+
+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)];

fs/nfsd/state.h +1 -0

index 005c911..f8b0210 100644
@@ -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,
 };

fs/nfsd/xdr4.h +6 -0

index 1e4edbf..4ac2676 100644
@@ -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 {

fs/nfsd/xdr4cb.h +10 -0

index 49b719d..7e39913 100644
@@ -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)

Signature · 1 line

1.8.3.1

Quoted 58 lines · Olga Kornievskaia

Signed-off-by Olga Kornievskaia <[email protected]>

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

fs/nfsd/nfs4callback.c +28 -0

index 49b0a9e..d12d914 100644
@@ -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;

fs/nfsd/nfs4callback.c static int nfs4_xdr_dec_cb_notify_lock(struct rpc_rqst *rqstp, · line 723

Quoted 10 lines

fs/nfsd/nfs4callback.c +10 -0

+     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);

Nit: since we're not using this any more, may as well just make that:

                /* We always return success if bytes were written: */
                p = xdr_encode_hyper(p, 0);

--b.

Quoted 124 lines

fs/nfsd/nfs4callback.c +59 -0

+     }
+}
+
+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)];

fs/nfsd/state.h +1 -0

index 005c911..f8b0210 100644
@@ -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,
 };

fs/nfsd/xdr4.h +6 -0

index 1e4edbf..4ac2676 100644
@@ -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 {

fs/nfsd/xdr4cb.h +10 -1

index 49b719d..7e39913 100644
@@ -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)
--

Quoted 1 line

1.8.3.1

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

fs/nfsd/nfs4proc.c +4 -0

+	if (!copy->cp_synchronous) {
+		status = nfsd4_init_copy_res(copy, 0);
+		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
+		if (!async_copy) {

fs/nfsd/nfs4proc.c +12 -3

+		async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
+				async_copy, "%s", "copy thread");
+		if (IS_ERR(async_copy->copy_task)) {
+			status = PTR_ERR(async_copy->copy_task);
+			goto out_err_dec;
+		}
+		wake_up_process(async_copy->copy_task);
+	} else {
+		status = nfsd4_do_copy(copy, 1);
 	}
-
-	fput(src);
-	fput(dst);
 out:
 	return status;
+out_err_dec:
+	cleanup_async_copy(async_copy);
+	goto out;
 }
 
 static __be32

fs/nfsd/nfs4state.c +2 -0

index 0c04f81..d7767a1 100644
@@ -1774,6 +1774,8 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
 #ifdef CONFIG_NFSD_PNFS
 	INIT_LIST_HEAD(&clp->cl_lo_states);
 #endif
+	INIT_LIST_HEAD(&clp->async_copies);
+	spin_lock_init(&clp->async_lock);
 	spin_lock_init(&clp->cl_lock);
 	rpc_init_wait_queue(&clp->cl_cb_waitq, "Backchannel slot table");
 	return clp;

fs/nfsd/state.h +2 -0

index f8b0210..9189062 100644
@@ -352,6 +352,8 @@ struct nfs4_client {
 	struct rpc_wait_queue	cl_cb_waitq;	/* backchannel callers may */
 						/* wait here for slots */
 	struct net		*net;
+	struct list_head	async_copies;	/* list of async copies */
+	spinlock_t		async_lock;	/* lock for async copies */
 };
 
 /* struct nfs4_client_reset

fs/nfsd/xdr4.h +9 -0

index 9b0c099..0a19954 100644
@@ -529,6 +529,15 @@ struct nfsd4_copy {
 	struct nfsd4_callback	cp_cb;
 	__be32			nfserr;
 	struct knfsd_fh		fh;
+
+	struct nfs4_client      *cp_clp;
+
+	struct file             *fh_src;
+	struct file             *fh_dst;
+	struct net              *net;
+
+	struct list_head	copies;
+	struct task_struct	*copy_task;
 };
 
 struct nfsd4_seek {

Signature · 1 line

1.8.3.1

Nit: this could use a better subject line.

...

Quoted 4 lines

fs/nfsd/nfs4proc.c +4 -0

+     if (!copy->cp_synchronous) {
+             status = nfsd4_init_copy_res(copy, 0);
+             async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
+             if (!async_copy) {

fs/nfsd/nfs4proc.c static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write) · line 1246

Quoted 10 lines

fs/nfsd/nfs4proc.c +10 -0

+                     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));
+             spin_lock(&async_copy->cp_clp->async_lock);
+             list_add(&async_copy->copies,
+                             &async_copy->cp_clp->async_copies);
+             spin_unlock(&async_copy->cp_clp->async_lock);

At this point other threads could in theory look up this async_copy, but its copy_task field is not yet initialized. I don't *think* that's a problem for nfsd4_shutdown_copy, because I don't think the server could be processing rpc's for this client any more at that point. But I think a malicious client might be able to trigger a NULL dereference in nfsd4_offload_cancel.

Is there any reason not to assign copy_task before adding it to this list?

--b.

Quoted 68 lines

fs/nfsd/nfs4proc.c +12 -3

+             async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
+                             async_copy, "%s", "copy thread");
+             if (IS_ERR(async_copy->copy_task)) {
+                     status = PTR_ERR(async_copy->copy_task);
+                     goto out_err_dec;
+             }
+             wake_up_process(async_copy->copy_task);
+     } else {
+             status = nfsd4_do_copy(copy, 1);
      }
-
-     fput(src);
-     fput(dst);
 out:
      return status;
+out_err_dec:
+     cleanup_async_copy(async_copy);
+     goto out;
 }
 
 static __be32

fs/nfsd/nfs4state.c +2 -0

index 0c04f81..d7767a1 100644
@@ -1774,6 +1774,8 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
 #ifdef CONFIG_NFSD_PNFS
      INIT_LIST_HEAD(&clp->cl_lo_states);
 #endif
+     INIT_LIST_HEAD(&clp->async_copies);
+     spin_lock_init(&clp->async_lock);
      spin_lock_init(&clp->cl_lock);
      rpc_init_wait_queue(&clp->cl_cb_waitq, "Backchannel slot table");
      return clp;

fs/nfsd/state.h +2 -0

index f8b0210..9189062 100644
@@ -352,6 +352,8 @@ struct nfs4_client {
      struct rpc_wait_queue   cl_cb_waitq;    /* backchannel callers may */
                                              /* wait here for slots */
      struct net              *net;
+     struct list_head        async_copies;   /* list of async copies */
+     spinlock_t              async_lock;     /* lock for async copies */
 };
 
 /* struct nfs4_client_reset

fs/nfsd/xdr4.h +9 -1

index 9b0c099..0a19954 100644
@@ -529,6 +529,15 @@ struct nfsd4_copy {
      struct nfsd4_callback   cp_cb;
      __be32                  nfserr;
      struct knfsd_fh         fh;
+
+     struct nfs4_client      *cp_clp;
+
+     struct file             *fh_src;
+     struct file             *fh_dst;
+     struct net              *net;
+
+     struct list_head        copies;
+     struct task_struct      *copy_task;
 };
 
 struct nfsd4_seek {
--

Quoted 1 line

1.8.3.1

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

Nit: this could use a better subject line.

On Tue, Oct 24, 2017 at 01:47:47PM -0400, Olga Kornievskaia wrote:
...

Quoted 4 lines

fs/nfsd/nfs4proc.c +4 -0

+     if (!copy->cp_synchronous) {
+             status = nfsd4_init_copy_res(copy, 0);
+             async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
+             if (!async_copy) {

fs/nfsd/nfs4proc.c static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write) · line 1246

Quoted 10 lines

fs/nfsd/nfs4proc.c +10 -0

+                     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));
+             spin_lock(&async_copy->cp_clp->async_lock);
+             list_add(&async_copy->copies,
+                             &async_copy->cp_clp->async_copies);
+             spin_unlock(&async_copy->cp_clp->async_lock);

At this point other threads could in theory look up this async_copy, but its copy_task field is not yet initialized. I don't *think* that's a problem for nfsd4_shutdown_copy, because I don't think the server could be processing rpc's for this client any more at that point. But I think a malicious client might be able to trigger a NULL dereference in nfsd4_offload_cancel.

Is there any reason not to assign copy_task before adding it to this list?

--b.

Quoted 68 lines

fs/nfsd/nfs4proc.c +12 -3

+             async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
+                             async_copy, "%s", "copy thread");
+             if (IS_ERR(async_copy->copy_task)) {
+                     status = PTR_ERR(async_copy->copy_task);
+                     goto out_err_dec;
+             }
+             wake_up_process(async_copy->copy_task);
+     } else {
+             status = nfsd4_do_copy(copy, 1);
      }
-
-     fput(src);
-     fput(dst);
 out:
      return status;
+out_err_dec:
+     cleanup_async_copy(async_copy);
+     goto out;
 }
 
 static __be32

fs/nfsd/nfs4state.c +2 -0

index 0c04f81..d7767a1 100644
@@ -1774,6 +1774,8 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
 #ifdef CONFIG_NFSD_PNFS
      INIT_LIST_HEAD(&clp->cl_lo_states);
 #endif
+     INIT_LIST_HEAD(&clp->async_copies);
+     spin_lock_init(&clp->async_lock);
      spin_lock_init(&clp->cl_lock);
      rpc_init_wait_queue(&clp->cl_cb_waitq, "Backchannel slot table");
      return clp;

fs/nfsd/state.h +2 -0

index f8b0210..9189062 100644
@@ -352,6 +352,8 @@ struct nfs4_client {
      struct rpc_wait_queue   cl_cb_waitq;    /* backchannel callers may */
                                              /* wait here for slots */
      struct net              *net;
+     struct list_head        async_copies;   /* list of async copies */
+     spinlock_t              async_lock;     /* lock for async copies */
 };
 
 /* struct nfs4_client_reset

fs/nfsd/xdr4.h +9 -1

index 9b0c099..0a19954 100644
@@ -529,6 +529,15 @@ struct nfsd4_copy {
      struct nfsd4_callback   cp_cb;
      __be32                  nfserr;
      struct knfsd_fh         fh;
+
+     struct nfs4_client      *cp_clp;
+
+     struct file             *fh_src;
+     struct file             *fh_dst;
+     struct net              *net;
+
+     struct list_head        copies;
+     struct task_struct      *copy_task;
 };
 
 struct nfsd4_seek {
--

Quoted 1 line

1.8.3.1

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

Quoted 26 lines · J. Bruce Fields

Nit: this could use a better subject line.

On Tue, Oct 24, 2017 at 01:47:47PM -0400, Olga Kornievskaia wrote:
...

Quoted 4 lines

fs/nfsd/nfs4proc.c +4 -0

+     if (!copy->cp_synchronous) {
+             status = nfsd4_init_copy_res(copy, 0);
+             async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
+             if (!async_copy) {

fs/nfsd/nfs4proc.c static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write) · line 1246

Quoted 10 lines

fs/nfsd/nfs4proc.c +10 -0

+                     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));
+             spin_lock(&async_copy->cp_clp->async_lock);
+             list_add(&async_copy->copies,
+                             &async_copy->cp_clp->async_copies);
+             spin_unlock(&async_copy->cp_clp->async_lock);

At this point other threads could in theory look up this async_copy, but its copy_task field is not yet initialized. I don't *think* that's a problem for nfsd4_shutdown_copy, because I don't think the server could be processing rpc's for this client any more at that point. But I think a malicious client might be able to trigger a NULL dereference in nfsd4_offload_cancel.

Quoted 2 lines

Is there any reason not to assign copy_task before adding it to this list?

Now that I'm making changes I don't believe this is an issue. A client can't send nfsd4_offload_cancel() because it needs a copy stateid to send it with. And at this point the copy has not been replied to.

fs/nfsd/nfs4proc.c +22 -0

+		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));
+		spin_lock(&async_copy->cp_clp->async_lock);
+		list_add(&async_copy->copies,
+				&async_copy->cp_clp->async_copies);
+		spin_unlock(&async_copy->cp_clp->async_lock);
+		async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
+				async_copy, "%s", "copy thread");
+		if (IS_ERR(async_copy->copy_task)) {
+			status = PTR_ERR(async_copy->copy_task);
+			goto out_err_dec;
+		}
+		wake_up_process(async_copy->copy_task);
+	} else {
+		status = nfsd4_do_copy(copy, 1);

fs/nfsd/nfs4proc.c static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write) · line 1233

Quoted 5 lines · Olga Kornievskaia

fs/nfsd/nfs4proc.c +5 -0

+     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) {

I was thinking we might want to do a synchronous copy anyway in some
cases: e.g. if the copy is relatively small or if the filesystem supports clone.

But I guess that's a premature optimization; better to keep this as you have it for now.

--b.

Quoted 12 lines

fs/nfsd/nfs4proc.c +12 -0

+             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));
+             spin_lock(&async_copy->cp_clp->async_lock);
+             list_add(&async_copy->copies,
+                             &async_copy->cp_clp->async_copies);

fs/nfsd/nfs4proc.c static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write) · line 1255

Quoted 10 lines

fs/nfsd/nfs4proc.c +10 -0

+             spin_unlock(&async_copy->cp_clp->async_lock);
+             async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
+                             async_copy, "%s", "copy thread");
+             if (IS_ERR(async_copy->copy_task)) {
+                     status = PTR_ERR(async_copy->copy_task);
+                     goto out_err_dec;
+             }
+             wake_up_process(async_copy->copy_task);
+     } else {
+             status = nfsd4_do_copy(copy, 1);

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

If I understand correctly (I may be wrong), once this patch is applied a COPY may fail that previously worked--because we're switching over to the asynchronous copy implementation before it's actually complete.

Of course, that's fixed by the end of this series. But we try to avoid that situation, where functionality is temporarily broken in the middle of a patch series and then fixed later.

Options might be to squash this patch together with some of the later patches. Or go ahead and add this code but don't actually enable it till later. (E.g. arrange that the "if (!copy->cp_synchronous)" case won't be taken till the last patch. Maybe it already works that way, I can't tell.) Or maybe there's some slicker way that I don't see right now.

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

Quoted 3 lines · J. Bruce Fields

If I understand correctly (I may be wrong), once this patch is applied a COPY may fail that previously worked--because we're switching over to the asynchronous copy implementation before it's actually complete.

I will have to double check this with testing but I think after this patch the asynchronous copy is functional but doesn't comply with the spec (eg., doesn't generate the unique stateid).

Quoted 4 lines

Of course, that's fixed by the end of this series. But we try to avoid that situation, where functionality is temporarily broken in the middle of a patch series and then fixed later.

Quoted 6 lines

Options might be to squash this patch together with some of the later patches. Or go ahead and add this code but don't actually enable it till later. (E.g. arrange that the "if (!copy->cp_synchronous)" case won't be taken till the last patch. Maybe it already works that way, I can't tell.) Or maybe there's some slicker way that I don't see right now.

I could do if (!copy->cp_synchronous && 0) and then add a patch that removes 0.

fs/nfsd/nfs4state.c +16 -0

+/*
+ * 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);

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 <[email protected]>

---
 fs/nfsd/netns.h     |  8 +++++++
 fs/nfsd/nfs4proc.c  | 32 +++++++++++++++++++-------
 fs/nfsd/nfs4state.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsctl.c    |  1 +
 fs/nfsd/state.h     | 14 ++++++++++++
 fs/nfsd/xdr4.h      |  2 ++
 6 files changed, 115 insertions(+), 8 deletions(-)

fs/nfsd/netns.h +8 -0

index 3714231..2c88a95 100644
@@ -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 */

fs/nfsd/nfs4proc.c +24 -8

index a020533..6876080 100644
@@ -1039,7 +1039,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;
 

@@ -1053,7 +1054,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;

@@ -1084,7 +1085,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;
 

@@ -1117,8 +1118,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;

@@ -1180,10 +1179,15 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
 	atomic_inc(&dst->cp_clp->cl_refcount);
 	dst->fh_dst = get_file(src->fh_dst);
 	dst->fh_src = get_file(src->fh_src);
+	dst->stid = src->stid;
+	dst->cps = src->cps;
 }
 
 static void cleanup_async_copy(struct nfsd4_copy *copy)
 {
+	list_del(&copy->cps->cp_list);
+	nfs4_free_cp_state(copy->cps);
+	nfs4_put_stid(copy->stid);
 	fput(copy->fh_dst);
 	fput(copy->fh_src);
 	spin_lock(&copy->cp_clp->async_lock);

@@ -1225,7 +1229,7 @@ static int nfsd4_do_async_copy(void *data)
 
 	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;
 

@@ -1234,15 +1238,27 @@ static int nfsd4_do_async_copy(void *data)
 		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);
+
 		status = nfsd4_init_copy_res(copy, 0);
 		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
 		if (!async_copy) {
 			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));
 		spin_lock(&async_copy->cp_clp->async_lock);
 		list_add(&async_copy->copies,
 				&async_copy->cp_clp->async_copies);

fs/nfsd/nfs4state.c +61 -0

index af40762..f9151f2 100644
@@ -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;

Quoted 213 lines · 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 <[email protected]>

---
 fs/nfsd/netns.h     |  8 +++++++
 fs/nfsd/nfs4proc.c  | 32 +++++++++++++++++++-------
 fs/nfsd/nfs4state.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsctl.c    |  1 +
 fs/nfsd/state.h     | 14 ++++++++++++
 fs/nfsd/xdr4.h      |  2 ++
 6 files changed, 115 insertions(+), 8 deletions(-)

fs/nfsd/netns.h +8 -0

index 3714231..2c88a95 100644
@@ -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 */

fs/nfsd/nfs4proc.c +24 -8

index a020533..6876080 100644
@@ -1039,7 +1039,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;
 

@@ -1053,7 +1054,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;

@@ -1084,7 +1085,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;
 

@@ -1117,8 +1118,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;

@@ -1180,10 +1179,15 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
      atomic_inc(&dst->cp_clp->cl_refcount);
      dst->fh_dst = get_file(src->fh_dst);
      dst->fh_src = get_file(src->fh_src);
+     dst->stid = src->stid;
+     dst->cps = src->cps;
 }
 
 static void cleanup_async_copy(struct nfsd4_copy *copy)
 {
+     list_del(&copy->cps->cp_list);
+     nfs4_free_cp_state(copy->cps);
+     nfs4_put_stid(copy->stid);
      fput(copy->fh_dst);
      fput(copy->fh_src);
      spin_lock(&copy->cp_clp->async_lock);

@@ -1225,7 +1229,7 @@ static int nfsd4_do_async_copy(void *data)
 
      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;
 

@@ -1234,15 +1238,27 @@ static int nfsd4_do_async_copy(void *data)
              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);
+
              status = nfsd4_init_copy_res(copy, 0);
              async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
              if (!async_copy) {
                      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));
              spin_lock(&async_copy->cp_clp->async_lock);
              list_add(&async_copy->copies,
                              &async_copy->cp_clp->async_copies);

fs/nfsd/nfs4state.c +61 -0

index af40762..f9151f2 100644
@@ -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;

fs/nfsd/nfs4state.c static void block_delegations(struct knfsd_fh *fh) · line 884

Quoted 6 lines

fs/nfsd/nfs4state.c +2 -0

@@ -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);

Is this really safe if there are still ongoing copies?

I think the copies take a reference count on the parent stateid, so I think we should never actually get here: perhaps this could just be a WARN_ON(!list_empty(&s->s_cp_list)). Though nfsd4_copy puts things on this list before bumping the sc_count, so there may be a race there.

What is supposed to happen, by the way, if a close arrives for a stateid with a copy in progress? Do we cancel the copy before returning from the close, or do we fail the close? (Same question for unlock and delegation return, I guess.)

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

Quoted 4 lines · J. Bruce Fields

What is supposed to happen, by the way, if a close arrives for a stateid with a copy in progress? Do we cancel the copy before returning from the close, or do we fail the close? (Same question for unlock and delegation return, I guess.)

Good questions. I think a normal client shouldn't send CLOSE/UNLOCK while there is an on-going copy. I could see how a DELEGRETURN could be coming in the middle of the copy (because the client received a CB_RECALL).

For an easy solution my proposal is to do the following. Upon receiving close, unlock, delegreturn, if the copy list is non-empty, then we send an ERR_DELAY back until the copy is done.

Another proposal is to kill the copy thread but that seems too drastic (specially in the case of a delegreturn). Right now when the copy thread is killed, it does not send a reply back to the client (as it assumes the client sent the OFFLOAD_CANCEL and doesn't need a reply). I could change that to send a reply back. In case the copy used a delegation stateid, then the next copy would choose a different stateid. This could be done for the delegation stateid and ERR_DELAY for the close/unlock.

Is either one acceptable and if so which one sounds better to you?

Quoted 24 lines · Olga Kornievskaia

Quoted 4 lines · J. Bruce Fields

What is supposed to happen, by the way, if a close arrives for a stateid with a copy in progress? Do we cancel the copy before returning from the close, or do we fail the close? (Same question for unlock and delegation return, I guess.)

Good questions. I think a normal client shouldn't send CLOSE/UNLOCK while there is an on-going copy. I could see how a DELEGRETURN could be coming in the middle of the copy (because the client received a CB_RECALL).

For an easy solution my proposal is to do the following. Upon receiving close, unlock, delegreturn, if the copy list is non-empty, then we send an ERR_DELAY back until the copy is done.

Another proposal is to kill the copy thread but that seems too drastic (specially in the case of a delegreturn). Right now when the copy thread is killed, it does not send a reply back to the client (as it assumes the client sent the OFFLOAD_CANCEL and doesn't need a reply). I could change that to send a reply back. In case the copy used a delegation stateid, then the next copy would choose a different stateid. This could be done for the delegation stateid and ERR_DELAY for the close/unlock.

Quoted 1 line

Is either one acceptable and if so which one sounds better to you?

If we're confident this is something that only a misbehaving client would do, then I'm OK with choosing whatever's simplest. I guess we should double-check with the spec to make sure this case hasn't already been covered there. Returning an error on the conflicting operation sounds good to me.

Is it really OK to do a COPY with a delegation stateid? That sounds messy if the delegation is revoked partway through the copy.

Quoted 2 lines · J. Bruce Fields

Is it really OK to do a COPY with a delegation stateid? That sounds messy if the delegation is revoked partway through the copy.

Yes, according to the spec, it is ok to use any of the stateids.

RFC 7862 15.2.3
The ca_dst_stateid MUST refer to a stateid that is valid for a WRITE
operation and follows the rules for stateids in Sections 8.2.5 and 18.32.3 of [RFC5661].... while for an intra-server copy
ca_src_stateid MUST refer
to a stateid that is valid for a READ operation and follows the rules for stateids in Sections 8.2.5 and 18.22.3 of [RFC5661].

8.2.5 is the section that says in the decreasing priority to choose delegation stateid, locking stateid, open stateid.

Trying to deal with the delegation stateid seems to be rather complex. Here's what I could do (which I mentioned before already):
1. possibly change the client to never use a delegation stateid.
2. for CLOSE/UNLOCK send back ERR_DELAY if copy list is not empty.
3. for DELEGRETURN (or when the server initiates CB_RECALL?) kill any on-going copies. Change the nfsd copy to send a reply if it was killed with a signal. What I'm not sure if when it's killed I get any partial bytes from the VFS, I don't think so. I think in that case, the only thing that I might reply with is a 0bytes copy. And whatever client that did sent a copy with a delegation stateid would deal with restarting it with a different stateid.

Having said all that, I'd like to ask why do you think handling CLOSE/LOCKU/DELEGRETURN should be any different for async copy vs what is currently there for the synchronous copy. Right now if the server receives those operations it does nothing to the on-going synchronous copy.

As you noted, the async copy takes a reference on the parent stateid so the state won't go away while the async copy is going on. Why not keep async copy same as the synchronous one?

Quoted 29 lines · Olga Kornievskaia

Quoted 2 lines · J. Bruce Fields

Is it really OK to do a COPY with a delegation stateid? That sounds messy if the delegation is revoked partway through the copy.

Yes, according to the spec, it is ok to use any of the stateids.

RFC 7862 15.2.3
The ca_dst_stateid MUST refer to a stateid that is valid for a WRITE
operation and follows the rules for stateids in Sections 8.2.5 and 18.32.3 of [RFC5661].... while for an intra-server copy
ca_src_stateid MUST refer
to a stateid that is valid for a READ operation and follows the rules for stateids in Sections 8.2.5 and 18.22.3 of [RFC5661].

8.2.5 is the section that says in the decreasing priority to choose delegation stateid, locking stateid, open stateid.

Trying to deal with the delegation stateid seems to be rather complex. Here's what I could do (which I mentioned before already):
1. possibly change the client to never use a delegation stateid.
2. for CLOSE/UNLOCK send back ERR_DELAY if copy list is not empty.
3. for DELEGRETURN (or when the server initiates CB_RECALL?) kill any on-going copies. Change the nfsd copy to send a reply if it was killed with a signal. What I'm not sure if when it's killed I get any partial bytes from the VFS, I don't think so. I think in that case, the only thing that I might reply with is a 0bytes copy. And whatever client that did sent a copy with a delegation stateid would deal with restarting it with a different stateid.

Quoted 5 lines

Having said all that, I'd like to ask why do you think handling CLOSE/LOCKU/DELEGRETURN should be any different for async copy vs what is currently there for the synchronous copy. Right now if the server receives those operations it does nothing to the on-going synchronous copy.

Or for that matter, why should it be different from READ and WRITE--looks like those can also continue executing after a close, too. So, good point.

Quoted 3 lines

As you noted, the async copy takes a reference on the parent stateid so the state won't go away while the async copy is going on. Why not keep async copy same as the synchronous one?

OK, so maybe it's not such a big deal.

I'm still confused about the delegation case, though: a copy can take a very long time, so we can't just wait for the copy to end before revoking the delegation. So do we allow the copy to continue indefinitely even after the delegation stateid it was initiated with has long ago been revoked?

Maybe it's not a problem. I guess I'm having trouble remembering why copies (or IO in general) uses stateids in the first place....

Quoted 52 lines · J. Bruce Fields

Quoted 29 lines · Olga Kornievskaia

Quoted 2 lines · J. Bruce Fields

Is it really OK to do a COPY with a delegation stateid? That sounds messy if the delegation is revoked partway through the copy.

Yes, according to the spec, it is ok to use any of the stateids.

RFC 7862 15.2.3
The ca_dst_stateid MUST refer to a stateid that is valid for a WRITE
operation and follows the rules for stateids in Sections 8.2.5 and 18.32.3 of [RFC5661].... while for an intra-server copy
ca_src_stateid MUST refer
to a stateid that is valid for a READ operation and follows the rules for stateids in Sections 8.2.5 and 18.22.3 of [RFC5661].

8.2.5 is the section that says in the decreasing priority to choose delegation stateid, locking stateid, open stateid.

Trying to deal with the delegation stateid seems to be rather complex. Here's what I could do (which I mentioned before already):
1. possibly change the client to never use a delegation stateid.
2. for CLOSE/UNLOCK send back ERR_DELAY if copy list is not empty.
3. for DELEGRETURN (or when the server initiates CB_RECALL?) kill any on-going copies. Change the nfsd copy to send a reply if it was killed with a signal. What I'm not sure if when it's killed I get any partial bytes from the VFS, I don't think so. I think in that case, the only thing that I might reply with is a 0bytes copy. And whatever client that did sent a copy with a delegation stateid would deal with restarting it with a different stateid.

Quoted 5 lines

Having said all that, I'd like to ask why do you think handling CLOSE/LOCKU/DELEGRETURN should be any different for async copy vs what is currently there for the synchronous copy. Right now if the server receives those operations it does nothing to the on-going synchronous copy.

Or for that matter, why should it be different from READ and WRITE--looks like those can also continue executing after a close, too. So, good point.

Quoted 3 lines

As you noted, the async copy takes a reference on the parent stateid so the state won't go away while the async copy is going on. Why not keep async copy same as the synchronous one?

OK, so maybe it's not such a big deal.

I'm still confused about the delegation case, though: a copy can take a very long time, so we can't just wait for the copy to end before revoking the delegation. So do we allow the copy to continue indefinitely even after the delegation stateid it was initiated with has long ago been revoked?

Quoted 2 lines

Maybe it's not a problem. I guess I'm having trouble remembering why copies (or IO in general) uses stateids in the first place....

Sigh. No you are right, we shouldn't let it run long past the revocation.

With IO operations, once it starts an operation like CLOSE/UNLOCK/DELEGRETURN can arrive at the server and it's ok because IO isn't long, and once it's done the client will notice a change in stateid and will choose a different stateid to do the next IO operation. I guess that's why a synchronous copy is still bordering ok because it's relatively short and the next copy will choose a different stateid.

Unless there are any big objections, I'll make the following changes
1. CLOSE/UNLOCK will check for none-empty list and return ERR_DELAY.
2. change the copy code to once received a signal to return the partial copy back.
3. DELEGRETURN will stop/kill any on-going copies, which would trigger a partial copy reply and the next copy should choose a different stateid.

Quoted 53 lines · Olga Kornievskaia

Quoted 52 lines · J. Bruce Fields

Quoted 29 lines · Olga Kornievskaia

Quoted 2 lines · J. Bruce Fields

Is it really OK to do a COPY with a delegation stateid? That sounds messy if the delegation is revoked partway through the copy.

Yes, according to the spec, it is ok to use any of the stateids.

RFC 7862 15.2.3
The ca_dst_stateid MUST refer to a stateid that is valid for a WRITE
operation and follows the rules for stateids in Sections 8.2.5 and 18.32.3 of [RFC5661].... while for an intra-server copy
ca_src_stateid MUST refer
to a stateid that is valid for a READ operation and follows the rules for stateids in Sections 8.2.5 and 18.22.3 of [RFC5661].

8.2.5 is the section that says in the decreasing priority to choose delegation stateid, locking stateid, open stateid.

Trying to deal with the delegation stateid seems to be rather complex. Here's what I could do (which I mentioned before already):
1. possibly change the client to never use a delegation stateid.
2. for CLOSE/UNLOCK send back ERR_DELAY if copy list is not empty.
3. for DELEGRETURN (or when the server initiates CB_RECALL?) kill any on-going copies. Change the nfsd copy to send a reply if it was killed with a signal. What I'm not sure if when it's killed I get any partial bytes from the VFS, I don't think so. I think in that case, the only thing that I might reply with is a 0bytes copy. And whatever client that did sent a copy with a delegation stateid would deal with restarting it with a different stateid.

Quoted 5 lines

Having said all that, I'd like to ask why do you think handling CLOSE/LOCKU/DELEGRETURN should be any different for async copy vs what is currently there for the synchronous copy. Right now if the server receives those operations it does nothing to the on-going synchronous copy.

Or for that matter, why should it be different from READ and WRITE--looks like those can also continue executing after a close, too. So, good point.

Quoted 3 lines

As you noted, the async copy takes a reference on the parent stateid so the state won't go away while the async copy is going on. Why not keep async copy same as the synchronous one?

OK, so maybe it's not such a big deal.

I'm still confused about the delegation case, though: a copy can take a very long time, so we can't just wait for the copy to end before revoking the delegation. So do we allow the copy to continue indefinitely even after the delegation stateid it was initiated with has long ago been revoked?

Quoted 5 lines

Quoted 2 lines

Maybe it's not a problem. I guess I'm having trouble remembering why copies (or IO in general) uses stateids in the first place....

Sigh. No you are right, we shouldn't let it run long past the revocation.

I'm worried by it running long past the revocation, but I'm also not managing to come with a case where it causes an actual problem.

--b.

Quoted 15 lines

With IO operations, once it starts an operation like CLOSE/UNLOCK/DELEGRETURN can arrive at the server and it's ok because IO isn't long, and once it's done the client will notice a change in stateid and will choose a different stateid to do the next IO operation. I guess that's why a synchronous copy is still bordering ok because it's relatively short and the next copy will choose a different stateid.

Unless there are any big objections, I'll make the following changes
1. CLOSE/UNLOCK will check for none-empty list and return ERR_DELAY.
2. change the copy code to once received a signal to return the partial copy back.
3. DELEGRETURN will stop/kill any on-going copies, which would trigger a partial copy reply and the next copy should choose a different stateid.

It seems wrong but I'm not sure exactly why. I thinking what if the server receives all state closing operations (delegreturn, unlock, close) and the copy is still going. A file is being changed after the file is closed seem weird. But as I mentioned, properly working client should be unlocking/closing the file until after the copy is done (or until offload_cancel is sent after ctrl-c).

Who is responsible for making sure that the file isn't being accessed after it's closed? Is it server's or client's? Like client is making sure all the writes are done before it does the close. Should it be client's responsibility as well to make sure there are no on-going copies before doing the close?

Is the question here about just delegations or are you questioning whether or not CLOSE/UNLOCK need to be delayed until the copy is finished?

Quoted 13 lines · Olga Kornievskaia

It seems wrong but I'm not sure exactly why. I thinking what if the server receives all state closing operations (delegreturn, unlock, close) and the copy is still going. A file is being changed after the file is closed seem weird. But as I mentioned, properly working client should be unlocking/closing the file until after the copy is done (or until offload_cancel is sent after ctrl-c).

Who is responsible for making sure that the file isn't being accessed after it's closed? Is it server's or client's? Like client is making sure all the writes are done before it does the close. Should it be client's responsibility as well to make sure there are no on-going copies before doing the close?

Quoted 3 lines

Is the question here about just delegations or are you questioning whether or not CLOSE/UNLOCK need to be delayed until the copy is finished?

Yeah, I kinda suspect there's a bug in the CLOSE and UNLOCK case too.

I don't know if the client really causes trouble for anyone but itself if it allows IO to go on past close, unlock, or delegreturn.

If it's still going on after another client acquires a conflicting open, lock, or delegation, that could be a problem: a client that holds a mandatory lock, or an open with DENY_WRITE, or a delegation, has a right not to expect the file to change underneath it. We should already be safe against that in the delegation case thanks to the vfs checks in fs/locks.c:check_conflicting_open().

In the DENY_WRITE case I bet there's still bug. Maybe in the mandatory lock case, too. But those cases are rare and already have other problems.

So, I guess I don't care too much unless someone's seeing another problem.

Still, killing off a long-running copy is probably a good precaution?

Quoted 12 lines · J. Bruce Fields

Yeah, I kinda suspect there's a bug in the CLOSE and UNLOCK case too.

I don't know if the client really causes trouble for anyone but itself if it allows IO to go on past close, unlock, or delegreturn.

If it's still going on after another client acquires a conflicting open, lock, or delegation, that could be a problem: a client that holds a mandatory lock, or an open with DENY_WRITE, or a delegation, has a right not to expect the file to change underneath it. We should already be safe against that in the delegation case thanks to the vfs checks in fs/locks.c:check_conflicting_open().

Quoted 8 lines

In the DENY_WRITE case I bet there's still bug. Maybe in the mandatory lock case, too. But those cases are rare and already have other problems.

So, I guess I don't care too much unless someone's seeing another problem.

Still, killing off a long-running copy is probably a good precaution?

Now that I got back into the code and remembered it again, copy stateid is never associated with a delegation stateid. Copy is associated with the destination file's stateid which is opened for write. Since linux server does not give out write delegations, then the stateid is either an open stateid or lock stateid (in my testing I varied not locking and locking the destination file so to get open stateid as well as lock stateid to be used in the copy). If you want me to keep the code for killing off copy in the delegreturn because in the future there might be write delegations, I can but I recall the rule of thumb is to keep it simple, thus I'm inclined to remove it.

Upon receiving OFFLOAD_CANCEL search the list of copy stateids, if found then set the SIGPENDING signal so that do_splice stops copying and also send kthread_stop to the copy thread to stop and wait for it. Take a reference on the copy from the offload_cancel thread so that it won't go away while we are trying to process it. Server won't be sending CB_OFFLOAD to the client since it received a cancel.

Signed-off-by Olga Kornievskaia <[email protected]>

---
 fs/nfsd/nfs4proc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 fs/nfsd/xdr4.h     |  1 +
 2 files changed, 45 insertions(+), 2 deletions(-)

fs/nfsd/nfs4proc.c +44 -2

index 6876080..3b0bb54 100644
@@ -1097,6 +1097,14 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 out:
        return status;
 }
+
+static void nfs4_put_copy(struct nfsd4_copy *copy)
+{
+       if (!atomic_dec_and_test(&copy->refcount))
+               return;
+       kfree(copy);
+}
+
 static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
 {
        struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);

@@ -1134,6 +1142,8 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
        u64 dst_pos = copy->cp_dst_pos;
 
        do {
+               if (signalled() || kthread_should_stop())
+                       return -1;
                bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
                                copy->fh_dst, dst_pos, bytes_total);
                if (bytes_copied <= 0)

@@ -1152,11 +1162,16 @@ static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
        ssize_t bytes;
 
        bytes = _nfsd_copy_file_range(copy);
+       if (signalled() || kthread_should_stop()) {
+               status = -1;
+               goto cleanup;
+       }
        if (bytes < 0 && !copy->cp_res.wr_bytes_written)
                status = nfserrno(bytes);
        else
                status = nfsd4_init_copy_res(copy, sync);
 
+cleanup:
        fput(copy->fh_src);
        fput(copy->fh_dst);
        return status;

@@ -1194,7 +1209,7 @@ static void cleanup_async_copy(struct nfsd4_copy *copy)
        list_del(&copy->copies);
        spin_unlock(&copy->cp_clp->async_lock);
        atomic_dec(&copy->cp_clp->cl_refcount);
-       kfree(copy);
+       nfs4_put_copy(copy);
 }
 
 static int nfsd4_do_async_copy(void *data)

@@ -1203,6 +1218,9 @@ static int nfsd4_do_async_copy(void *data)
        struct nfsd4_copy *cb_copy;
 
        copy->nfserr = nfsd4_do_copy(copy, 0);
+       if (signalled() || kthread_should_stop())
+               goto out;
+
        cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
        if (!cb_copy)
                goto out;

@@ -1259,6 +1277,7 @@ static int nfsd4_do_async_copy(void *data)
                memcpy(&copy->cp_res.cb_stateid, &copy->cps->cp_stateid,
                        sizeof(copy->cps->cp_stateid));
                dup_copy_fields(copy, async_copy);
+               atomic_set(&async_copy->refcount, 1);
                spin_lock(&async_copy->cp_clp->async_lock);
                list_add(&async_copy->copies,
                                &async_copy->cp_clp->async_copies);

@@ -1285,7 +1304,30 @@ static int nfsd4_do_async_copy(void *data)
                     struct nfsd4_compound_state *cstate,
                     union nfsd4_op_u *u)
 {
-       return 0;
+       struct nfsd4_offload_status *os = &u->offload_status;
+       __be32 status = 0;
+       struct nfsd4_copy *copy;
+       bool found = false;
+       struct nfs4_client *clp = cstate->clp;
+
+       spin_lock(&clp->async_lock);
+       list_for_each_entry(copy, &clp->async_copies, copies) {
+               if (memcmp(&copy->cps->cp_stateid, &os->stateid,
+                               NFS4_STATEID_SIZE))
+                       continue;
+               found = true;
+               atomic_inc(&copy->refcount);
+               break;
+       }
+       spin_unlock(&clp->async_lock);
+       if (found) {
+               set_tsk_thread_flag(copy->copy_task, TIF_SIGPENDING);
+               kthread_stop(copy->copy_task);
+               nfs4_put_copy(copy);
+       } else
+               status = nfserr_bad_stateid;
+
+       return status;
 }
 
 static __be32

fs/nfsd/xdr4.h +0 -0

index 27bac6a..3127b58 100644

fs/nfsd/xdr4.h +0 -1

 };
 
 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 124 lines · Olga Kornievskaia

Upon receiving OFFLOAD_CANCEL search the list of copy stateids, if found then set the SIGPENDING signal so that do_splice stops copying and also send kthread_stop to the copy thread to stop and wait for it. Take a reference on the copy from the offload_cancel thread so that it won't go away while we are trying to process it. Server won't be sending CB_OFFLOAD to the client since it received a cancel.

Signed-off-by Olga Kornievskaia <[email protected]>

---
 fs/nfsd/nfs4proc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 fs/nfsd/xdr4.h     |  1 +
 2 files changed, 45 insertions(+), 2 deletions(-)

fs/nfsd/nfs4proc.c +44 -2

index 6876080..3b0bb54 100644
@@ -1097,6 +1097,14 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
 out:
        return status;
 }
+
+static void nfs4_put_copy(struct nfsd4_copy *copy)
+{
+       if (!atomic_dec_and_test(&copy->refcount))
+               return;
+       kfree(copy);
+}
+
 static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
 {
        struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);

@@ -1134,6 +1142,8 @@ static int _nfsd_copy_file_range(struct nfsd4_copy *copy)
        u64 dst_pos = copy->cp_dst_pos;
 
        do {
+               if (signalled() || kthread_should_stop())
+                       return -1;
                bytes_copied = nfsd_copy_file_range(copy->fh_src, src_pos,
                                copy->fh_dst, dst_pos, bytes_total);
                if (bytes_copied <= 0)

@@ -1152,11 +1162,16 @@ static int nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
        ssize_t bytes;
 
        bytes = _nfsd_copy_file_range(copy);
+       if (signalled() || kthread_should_stop()) {
+               status = -1;
+               goto cleanup;
+       }
        if (bytes < 0 && !copy->cp_res.wr_bytes_written)
                status = nfserrno(bytes);
        else
                status = nfsd4_init_copy_res(copy, sync);
 
+cleanup:
        fput(copy->fh_src);
        fput(copy->fh_dst);
        return status;

@@ -1194,7 +1209,7 @@ static void cleanup_async_copy(struct nfsd4_copy *copy)
        list_del(&copy->copies);
        spin_unlock(&copy->cp_clp->async_lock);
        atomic_dec(&copy->cp_clp->cl_refcount);
-       kfree(copy);
+       nfs4_put_copy(copy);
 }
 
 static int nfsd4_do_async_copy(void *data)

@@ -1203,6 +1218,9 @@ static int nfsd4_do_async_copy(void *data)
        struct nfsd4_copy *cb_copy;
 
        copy->nfserr = nfsd4_do_copy(copy, 0);
+       if (signalled() || kthread_should_stop())
+               goto out;
+
        cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
        if (!cb_copy)
                goto out;

@@ -1259,6 +1277,7 @@ static int nfsd4_do_async_copy(void *data)
                memcpy(&copy->cp_res.cb_stateid, &copy->cps->cp_stateid,
                        sizeof(copy->cps->cp_stateid));
                dup_copy_fields(copy, async_copy);
+               atomic_set(&async_copy->refcount, 1);
                spin_lock(&async_copy->cp_clp->async_lock);
                list_add(&async_copy->copies,
                                &async_copy->cp_clp->async_copies);

@@ -1285,7 +1304,30 @@ static int nfsd4_do_async_copy(void *data)
                     struct nfsd4_compound_state *cstate,
                     union nfsd4_op_u *u)
 {
-       return 0;
+       struct nfsd4_offload_status *os = &u->offload_status;
+       __be32 status = 0;
+       struct nfsd4_copy *copy;
+       bool found = false;
+       struct nfs4_client *clp = cstate->clp;
+
+       spin_lock(&clp->async_lock);
+       list_for_each_entry(copy, &clp->async_copies, copies) {
+               if (memcmp(&copy->cps->cp_stateid, &os->stateid,
+                               NFS4_STATEID_SIZE))
+                       continue;
+               found = true;
+               atomic_inc(&copy->refcount);
+               break;
+       }
+       spin_unlock(&clp->async_lock);
+       if (found) {
+               set_tsk_thread_flag(copy->copy_task, TIF_SIGPENDING);
+               kthread_stop(copy->copy_task);
+               nfs4_put_copy(copy);
+       } else
+               status = nfserr_bad_stateid;
+
+       return status;
 }
 
 static __be32

fs/nfsd/xdr4.h +0 -0

index 27bac6a..3127b58 100644
@@ -540,6 +540,7 @@ struct nfsd4_copy {
 

fs/nfsd/xdr4.h struct nfsd4_copy { · line 543

Quoted 3 lines

fs/nfsd/xdr4.h +1 -0

        struct list_head        copies;
        struct task_struct      *copy_task;
+       atomic_t                refcount;

Hi Bruce,

Is the code moving away from using atomic_t and using refcount_t instead. Should I be changing this as well?

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

Signed-off-by Olga Kornievskaia <[email protected]>

---
 fs/nfsd/nfs4proc.c  | 13 +++++++++++++
 fs/nfsd/nfs4state.c |  1 +
 fs/nfsd/state.h     |  1 +
 3 files changed, 15 insertions(+)

fs/nfsd/nfs4proc.c +13 -0

index b2f6549..34042ff6 100644
@@ -1105,6 +1105,19 @@ static void nfs4_put_copy(struct nfsd4_copy *copy)
 	kfree(copy);
 }
 
+void nfsd4_shutdown_copy(struct nfs4_client *clp)
+{
+	struct nfsd4_copy *copy;
+
+	spin_lock(&clp->async_lock);
+	list_for_each_entry(copy, &clp->async_copies, copies) {
+		set_tsk_thread_flag(copy->copy_task, TIF_SIGPENDING);
+		kthread_stop(copy->copy_task);
+		nfs4_put_copy(copy);
+	}
+	spin_unlock(&clp->async_lock);
+}
+
 static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
 {
 	struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);

fs/nfsd/nfs4state.c +0 -0

index f9151f2..efac39d 100644

fs/nfsd/nfs4state.c +0 -0

 	if (clp->cl_cb_conn.cb_xprt)
 		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
 	free_client(clp);

fs/nfsd/state.h +1 -0

index b4de8ae..88869df 100644
@@ -643,6 +643,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);

Signature · 1 line

1.8.3.1

Quoted 40 lines · 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 <[email protected]>

---
 fs/nfsd/nfs4proc.c  | 13 +++++++++++++
 fs/nfsd/nfs4state.c |  1 +
 fs/nfsd/state.h     |  1 +
 3 files changed, 15 insertions(+)

fs/nfsd/nfs4proc.c +13 -0

index b2f6549..34042ff6 100644
@@ -1105,6 +1105,19 @@ static void nfs4_put_copy(struct nfsd4_copy *copy)
      kfree(copy);
 }
 
+void nfsd4_shutdown_copy(struct nfs4_client *clp)
+{
+     struct nfsd4_copy *copy;
+
+     spin_lock(&clp->async_lock);
+     list_for_each_entry(copy, &clp->async_copies, copies) {
+             set_tsk_thread_flag(copy->copy_task, TIF_SIGPENDING);
+             kthread_stop(copy->copy_task);
+             nfs4_put_copy(copy);
+     }
+     spin_unlock(&clp->async_lock);
+}
+
 static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
 {
      struct nfsd4_copy *copy = container_of(cb, struct nfsd4_copy, cp_cb);

fs/nfsd/nfs4state.c +0 -0

index f9151f2..efac39d 100644
@@ -1947,6 +1947,7 @@ static __be32 mark_client_expired_locked(struct nfs4_client *clp)
      }

fs/nfsd/nfs4state.c static __be32 mark_client_expired_locked(struct nfs4_client *clp) · line 1950

Quoted 3 lines

fs/nfsd/nfs4state.c +1 -0

      nfsd4_return_all_client_layouts(clp);
      nfsd4_shutdown_callback(clp);
+     nfsd4_shutdown_copy(clp);

The asynchronous copy threads can produce more callbacks, and I don't think that's safe after we've called nfsd4_shutdown_callback(), so we should move nfsd4_shutdown_copy() above nfsd4_shutdown_callback() if possible.

--b.

Quoted 16 lines

fs/nfsd/nfs4state.c +0 -0

      if (clp->cl_cb_conn.cb_xprt)
              svc_xprt_put(clp->cl_cb_conn.cb_xprt);
      free_client(clp);

fs/nfsd/state.h +1 -1

index b4de8ae..88869df 100644
@@ -643,6 +643,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);
--

Quoted 1 line

1.8.3.1

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

To do asynchronous copies, NFSD creates a new kthread to handle the request. Upon receiving the COPY, it generates a unique copy stateid (stored in a global list for keeping track of state for OFFLOAD_STATUS to be queried by), starts the thread, and replies back to the client. nfsd4_copy arguments that are allocated on the stack are copies for the kthread.

For the async copy handler, copy is done in the loop for the requested number of bytes. If an error is encountered and partial copy has happened, a successful partial copy is returned in the CB_OFFLOAD. vfs_copy_file_range is called with 4MB chunks for both async and sync, allowing for 4MB granularity of OFFLOAD_STATUS queiry. Once copy is done, the results are queued for the callback workqueue and sent via CB_OFFLOAD.

When the server received an OFFLOAD_CANCEL, it will find the kthread running the copy and will send a SIGPENDING and kthread_stop() and it will interrupt the ongoing do_splice() and once vfs returns we are choosing not to send the CB_OFFLOAD back to the client.

When the server receives an OFFLOAD_STATUS, it will find the kthread running the copy and will locate within the copy state the current number of bytes copied so far.

v6:
1. if an error occured and partial bytes were copied, don't return the error and instead return the successful partial copy.
2. OFFLOAD_STATUS locates the on-going copy and looks up the number of bytes copied so far.

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     | 316 ++++++++++++++++++++++++++++++++++++++++++++-----
 fs/nfsd/nfs4state.c    |  77 +++++++++++-
 fs/nfsd/nfs4xdr.c      |  50 ++++++--
 fs/nfsd/nfsctl.c       |   1 +
 fs/nfsd/state.h        |  21 +++-
 fs/nfsd/xdr4.h         |  28 +++++
 fs/nfsd/xdr4cb.h       |  10 ++
 9 files changed, 571 insertions(+), 37 deletions(-)

Quoted 40 lines · Olga Kornievskaia

To do asynchronous copies, NFSD creates a new kthread to handle the request. Upon receiving the COPY, it generates a unique copy stateid (stored in a global list for keeping track of state for OFFLOAD_STATUS to be queried by), starts the thread, and replies back to the client. nfsd4_copy arguments that are allocated on the stack are copies for the kthread.

For the async copy handler, copy is done in the loop for the requested number of bytes. If an error is encountered and partial copy has happened, a successful partial copy is returned in the CB_OFFLOAD. vfs_copy_file_range is called with 4MB chunks for both async and sync, allowing for 4MB granularity of OFFLOAD_STATUS queiry. Once copy is done, the results are queued for the callback workqueue and sent via CB_OFFLOAD.

When the server received an OFFLOAD_CANCEL, it will find the kthread running the copy and will send a SIGPENDING and kthread_stop() and it will interrupt the ongoing do_splice() and once vfs returns we are choosing not to send the CB_OFFLOAD back to the client.

When the server receives an OFFLOAD_STATUS, it will find the kthread running the copy and will locate within the copy state the current number of bytes copied so far.

v6:
1. if an error occured and partial bytes were copied, don't return the error and instead return the successful partial copy.
2. OFFLOAD_STATUS locates the on-going copy and looks up the number of bytes copied so far.

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

Quoted 10 lines

 fs/nfsd/netns.h        |   8 ++
 fs/nfsd/nfs4callback.c |  97 +++++++++++++++
 fs/nfsd/nfs4proc.c     | 316 ++++++++++++++++++++++++++++++++++++++++++++-----
 fs/nfsd/nfs4state.c    |  77 +++++++++++-
 fs/nfsd/nfs4xdr.c      |  50 ++++++--
 fs/nfsd/nfsctl.c       |   1 +
 fs/nfsd/state.h        |  21 +++-
 fs/nfsd/xdr4.h         |  28 +++++
 fs/nfsd/xdr4cb.h       |  10 ++
 9 files changed, 571 insertions(+), 37 deletions(-)

Bruce, any comments on this version?

Quoted 52 lines · Olga Kornievskaia

Quoted 50 lines · Olga Kornievskaia

To do asynchronous copies, NFSD creates a new kthread to handle the request. Upon receiving the COPY, it generates a unique copy stateid (stored in a global list for keeping track of state for OFFLOAD_STATUS to be queried by), starts the thread, and replies back to the client. nfsd4_copy arguments that are allocated on the stack are copies for the kthread.

For the async copy handler, copy is done in the loop for the requested number of bytes. If an error is encountered and partial copy has happened, a successful partial copy is returned in the CB_OFFLOAD. vfs_copy_file_range is called with 4MB chunks for both async and sync, allowing for 4MB granularity of OFFLOAD_STATUS queiry. Once copy is done, the results are queued for the callback workqueue and sent via CB_OFFLOAD.

When the server received an OFFLOAD_CANCEL, it will find the kthread running the copy and will send a SIGPENDING and kthread_stop() and it will interrupt the ongoing do_splice() and once vfs returns we are choosing not to send the CB_OFFLOAD back to the client.

When the server receives an OFFLOAD_STATUS, it will find the kthread running the copy and will locate within the copy state the current number of bytes copied so far.

v6:
1. if an error occured and partial bytes were copied, don't return the error and instead return the successful partial copy.
2. OFFLOAD_STATUS locates the on-going copy and looks up the number of bytes copied so far.

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     | 316 ++++++++++++++++++++++++++++++++++++++++++++-----
 fs/nfsd/nfs4state.c    |  77 +++++++++++-
 fs/nfsd/nfs4xdr.c      |  50 ++++++--
 fs/nfsd/nfsctl.c       |   1 +
 fs/nfsd/state.h        |  21 +++-
 fs/nfsd/xdr4.h         |  28 +++++
 fs/nfsd/xdr4cb.h       |  10 ++
 9 files changed, 571 insertions(+), 37 deletions(-)

Quoted 1 line

Bruce, any comments on this version?

Bruce, do you have any comments on this version?

Quoted 53 lines · Olga Kornievskaia

Quoted 52 lines · Olga Kornievskaia

Quoted 50 lines · Olga Kornievskaia

To do asynchronous copies, NFSD creates a new kthread to handle the request. Upon receiving the COPY, it generates a unique copy stateid (stored in a global list for keeping track of state for OFFLOAD_STATUS to be queried by), starts the thread, and replies back to the client. nfsd4_copy arguments that are allocated on the stack are copies for the kthread.

For the async copy handler, copy is done in the loop for the requested number of bytes. If an error is encountered and partial copy has happened, a successful partial copy is returned in the CB_OFFLOAD. vfs_copy_file_range is called with 4MB chunks for both async and sync, allowing for 4MB granularity of OFFLOAD_STATUS queiry. Once copy is done, the results are queued for the callback workqueue and sent via CB_OFFLOAD.

When the server received an OFFLOAD_CANCEL, it will find the kthread running the copy and will send a SIGPENDING and kthread_stop() and it will interrupt the ongoing do_splice() and once vfs returns we are choosing not to send the CB_OFFLOAD back to the client.

When the server receives an OFFLOAD_STATUS, it will find the kthread running the copy and will locate within the copy state the current number of bytes copied so far.

v6:
1. if an error occured and partial bytes were copied, don't return the error and instead return the successful partial copy.
2. OFFLOAD_STATUS locates the on-going copy and looks up the number of bytes copied so far.

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     | 316 ++++++++++++++++++++++++++++++++++++++++++++-----
 fs/nfsd/nfs4state.c    |  77 +++++++++++-
 fs/nfsd/nfs4xdr.c      |  50 ++++++--
 fs/nfsd/nfsctl.c       |   1 +
 fs/nfsd/state.h        |  21 +++-
 fs/nfsd/xdr4.h         |  28 +++++
 fs/nfsd/xdr4cb.h       |  10 ++
 9 files changed, 571 insertions(+), 37 deletions(-)

Quoted 3 lines

Quoted 1 line

Bruce, any comments on this version?

Bruce, do you have any comments on this version?

I don't yet, apologies. It is on my list to look at.

Quoted 8 lines · J. Bruce Fields

Quoted 4 lines · Olga Kornievskaia

Quoted 1 line · Olga Kornievskaia

Bruce, any comments on this version?

Bruce, do you have any comments on this version?

I don't yet, apologies. It is on my list to look at.

Any ideas as to when that would be?

Quoted 5 lines · Olga Kornievskaia

Quoted 4 lines · J. Bruce Fields

Quoted 3 lines · Olga Kornievskaia

Quoted 1 line · Olga Kornievskaia

Bruce, any comments on this version?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Bruce, do you have any comments on this version?

I don't yet, apologies. It is on my list to look at.

Any ideas as to when that would be?

Sorry for the silence. I'll try to get it reviewed in time for 4.16.

But I'm worrying about async behavior. Just the usual complaint:

- A large copy could take anywhere from milliseconds to hours.

- The only way the protocol gives to measure progress is OFFLOAD_STATUS, but I'm pessimistic that we'll get a corresponding copy_progress syscall interface that applications will get patched to use.

So, currently the server's trying to avoid the problem by returning short COPY results and hoping applications will handle that gracefully (and that readahead and write behind will save performance).

But I guess that won't work in the server-to-server case. Or would it? I *think* you can just send one NOTIFY

and then reuse the same stateid
in multiple COPYs, so maybe it's not any worse than in the single-server case.

That would be a lot simpler if it worked.

--b.

Quoted 6 lines · J. Bruce Fields

Quoted 5 lines · Olga Kornievskaia

Quoted 4 lines · J. Bruce Fields

Quoted 3 lines · Olga Kornievskaia

Quoted 1 line · Olga Kornievskaia

Bruce, any comments on this version?

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Bruce, do you have any comments on this version?

I don't yet, apologies. It is on my list to look at.

Any ideas as to when that would be?

Sorry for the silence. I'll try to get it reviewed in time for 4.16.

I hope I won't forget too much in 3months for this ;)

Quoted 3 lines

But I'm worrying about async behavior. Just the usual complaint:

- A large copy could take anywhere from milliseconds to hours.

With the sync copy your worry was with tying up the nfsd thread. Here each copy gets its own thread and doesn't interfere with other operations. So why worry?

Quoted 4 lines

- The only way the protocol gives to measure progress is OFFLOAD_STATUS, but I'm pessimistic that we'll get a corresponding copy_progress syscall interface that applications will get patched to use.

Doing a cp, never had a status info so what's different now. Interested folks can do as before ls -l dst_file to monitor progress.

Quoted 3 lines

So, currently the server's trying to avoid the problem by returning short COPY results and hoping applications will handle that gracefully (and that readahead and write behind will save performance).

We have demonstrated that doing async copy performs significantly better (given large enough file size which was different between intra and inter copy but something like 16MB). Really there is no point in introducing an asynchronous copy unless you are doing all of it.

Quoted 2 lines

But I guess that won't work in the server-to-server case. Or would it? I *think* you can just send one NOTIFY

NOTIFY is only for "inter" server-to-server copy and we are currently reviewing "asynchronous intra" copy?

Quoted 3 lines

and then reuse the same stateid
in multiple COPYs, so maybe it's not any worse than in the single-server case.

No you can not reuse the same stateid in multiple COPY's (spec doesn't allow it). It has to uniquely identity the copy. Otherwise, when client receives a reply from the async COPY it doesn't know which out of those multiple copies to match it to.

Quoted 47 lines · J. Bruce Fields

Quoted 18 lines · Olga Kornievskaia

Quoted 6 lines · J. Bruce Fields

Quoted 5 lines · Olga Kornievskaia

Quoted 4 lines · J. Bruce Fields

Quoted 3 lines · Olga Kornievskaia

Quoted 1 line · Olga Kornievskaia

Bruce, any comments on this version?

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Bruce, do you have any comments on this version?

I don't yet, apologies. It is on my list to look at.

Any ideas as to when that would be?

Sorry for the silence. I'll try to get it reviewed in time for 4.16.

I hope I won't forget too much in 3months for this ;)

Quoted 7 lines

Quoted 3 lines

But I'm worrying about async behavior. Just the usual complaint:

- A large copy could take anywhere from milliseconds to hours.

With the sync copy your worry was with tying up the nfsd thread. Here each copy gets its own thread and doesn't interfere with other operations. So why worry?

I'm mainly just worried about the lack of feedback to the user. "cp" doesn't traditionally give any, but other programs that copy data do, and they'll have to decide what to do instead.

Quoted 7 lines

Quoted 4 lines

- The only way the protocol gives to measure progress is OFFLOAD_STATUS, but I'm pessimistic that we'll get a corresponding copy_progress syscall interface that applications will get patched to use.

Doing a cp, never had a status info so what's different now. Interested folks can do as before ls -l dst_file to monitor progress.

Maybe it'll be no big deal. I'm not completely opposed to trying and finding out, partly because it shouldn't be that hard to back out later if necessary.

Quoted 4 lines

Quoted 3 lines

So, currently the server's trying to avoid the problem by returning short COPY results and hoping applications will handle that gracefully (and that readahead and write behind will save performance).

Quoted 6 lines

Quoted 4 lines

We have demonstrated that doing async copy performs significantly better (given large enough file size which was different between intra and inter copy but something like 16MB). Really there is no point in introducing an asynchronous copy unless you are doing all of it.

Sorry, I can't find that right now, do you have a URL or a message id?

I re-sent it to you.

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

But I guess that won't work in the server-to-server case. Or would it? I *think* you can just send one NOTIFY

NOTIFY is only for "inter" server-to-server copy and we are currently reviewing "asynchronous intra" copy?

Quoted 3 lines

and then reuse the same stateid
in multiple COPYs, so maybe it's not any worse than in the single-server case.

Quoted 6 lines

Quoted 4 lines

No you can not reuse the same stateid in multiple COPY's (spec doesn't allow it). It has to uniquely identity the copy. Otherwise, when client receives a reply from the async COPY it doesn't know which out of those multiple copies to match it to.

Why couldn't a client reuse the stateid after a synchronous COPY?