nvmf: use spdk_nvmf_request_exec to submit zcopy_start

Since this path now supports sending zero-copy, use it for zcopy_start.
Additionally, it makes it possible make zcopy_start void, as it reports all errors
asynchronously via request_complete(), and remove some of the duplicated
error checks.

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
Change-Id: I41f43ce1651432d9a7d74e3680d4a3f780128a1d
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10789
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>

include/spdk/nvmf_transport.h

@@ -448,7 +448,7 @@ void spdk_nvmf_request_exec(struct spdk_nvmf_request *req);
 void spdk_nvmf_request_exec_fabrics(struct spdk_nvmf_request *req);
 int spdk_nvmf_request_free(struct spdk_nvmf_request *req);
 int spdk_nvmf_request_complete(struct spdk_nvmf_request *req);
-int spdk_nvmf_request_zcopy_start(struct spdk_nvmf_request *req);
+void spdk_nvmf_request_zcopy_start(struct spdk_nvmf_request *req);
 int spdk_nvmf_request_zcopy_end(struct spdk_nvmf_request *req, bool commit);
 
 static inline bool

lib/nvmf/ctrlr.c

@@ -3720,68 +3720,15 @@ nvmf_ctrlr_use_zcopy(struct spdk_nvmf_request *req)
 	return true;
 }
 
-/* If this function returns a non-zero value the request
+void
- * reverts to using SPDK buffers
- */
-int
 spdk_nvmf_request_zcopy_start(struct spdk_nvmf_request *req)
 {
-	struct spdk_nvmf_qpair *qpair = req->qpair;
+	assert(req->zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
-	struct spdk_nvmf_subsystem_poll_group *sgroup = NULL;
-	struct spdk_nvmf_subsystem_pg_ns_info *ns_info;
-	uint32_t nsid;
-	struct spdk_bdev *bdev;
-	struct spdk_bdev_desc *desc;
-	struct spdk_io_channel *ch;
-	int rc;
 
-	if (!qpair->ctrlr) {
+	/* Set iovcnt to be the maximum number of iovs that the ZCOPY can use */
-		goto end;
-	}
-
-	if (qpair->group->sgroups == NULL) {
-		goto end;
-	}
-
-	rc = spdk_nvmf_request_get_bdev(req->cmd->nvme_cmd.nsid, req,
-					&bdev, &desc, &ch);
-	if (rc != 0) {
-		goto end;
-	}
-
-	if (ch == NULL) {
-		goto end;
-	}
-
-	nsid = req->cmd->nvme_cmd.nsid;
-	sgroup = &qpair->group->sgroups[qpair->ctrlr->subsys->id];
-	ns_info = &sgroup->ns_info[nsid - 1];
-	if (ns_info->state != SPDK_NVMF_SUBSYSTEM_ACTIVE) {
-		goto end;
-	}
-
-	if (qpair->state != SPDK_NVMF_QPAIR_ACTIVE) {
-		goto end;
-	}
-
-	/* Set iovcnt to be the maximum number of
-	 * iovs that the ZCOPY can use
-	 */
 	req->iovcnt = NVMF_REQ_MAX_BUFFERS;
-	TAILQ_INSERT_TAIL(&qpair->outstanding, req, link);
-	rc = nvmf_bdev_ctrlr_zcopy_start(bdev, desc, ch, req);
-	if (rc == 0) {
-		ns_info->io_outstanding++;
-		return 0;
-	}
-	TAILQ_REMOVE(&qpair->outstanding, req, link);
 
-end:
+	spdk_nvmf_request_exec(req);
-	/* An error occurred, the subsystem is paused, or the qpair is not active.
-	 * Revert to using SPDK buffers
-	 */
-	req->zcopy_phase = NVMF_ZCOPY_PHASE_NONE;
-	return -1;
 }
 
 int

test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c

@@ -269,6 +269,7 @@ nvmf_bdev_ctrlr_zcopy_start(struct spdk_bdev *bdev,
 			    struct spdk_io_channel *ch,
 			    struct spdk_nvmf_request *req)
 {
+	struct spdk_nvme_cpl *rsp = &req->rsp->nvme_cpl;
 	uint64_t start_lba;
 	uint64_t num_blocks;
 
@@ -276,7 +277,9 @@ nvmf_bdev_ctrlr_zcopy_start(struct spdk_bdev *bdev,
 	num_blocks = (from_le32(&req->cmd->nvme_cmd.cdw12) & 0xFFFFu) + 1;
 
 	if ((start_lba + num_blocks) > bdev->blockcnt) {
-		return -ENXIO;
+		rsp->status.sct = SPDK_NVME_SCT_GENERIC;
+		rsp->status.sc = SPDK_NVME_SC_DATA_SGL_LENGTH_INVALID;
+		return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;
 	}
 
 	if (req->cmd->nvme_cmd.opc == SPDK_NVME_OPC_WRITE) {
@@ -288,8 +291,7 @@ nvmf_bdev_ctrlr_zcopy_start(struct spdk_bdev *bdev,
 	}
 
 
-	spdk_nvmf_request_complete(req);
+	return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;
-	return 0;
 }
 
 int
@@ -297,7 +299,7 @@ nvmf_bdev_ctrlr_zcopy_end(struct spdk_nvmf_request *req, bool commit)
 {
 	req->zcopy_bdev_io = NULL;
 	spdk_nvmf_request_complete(req);
-	return 0;
+	return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;
 }
 
 static void
@@ -2375,6 +2377,11 @@ test_nvmf_ctrlr_use_zcopy(void)
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
 }
 
+static void
+qpair_state_change_done(void *cb_arg, int status)
+{
+}
+
 static void
 test_spdk_nvmf_request_zcopy_start(void)
 {
@@ -2442,49 +2449,51 @@ test_spdk_nvmf_request_zcopy_start(void)
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
 	qpair.ctrlr = NULL;
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) < 0);
+	spdk_nvmf_request_zcopy_start(&req);
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);
+	CU_ASSERT_EQUAL(req.zcopy_phase, NVMF_ZCOPY_PHASE_INIT_FAILED);
+	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sct, SPDK_NVME_SCT_GENERIC);
+	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sc, SPDK_NVME_SC_COMMAND_SEQUENCE_ERROR);
 	qpair.ctrlr = &ctrlr;
 
-	/* Fail because no sgroup */
-	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
-	group.sgroups = NULL;
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) < 0);
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);
-	group.sgroups = &sgroups;
-
 	/* Fail because bad NSID */
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
 	cmd.nsid = 0;
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) < 0);
+	spdk_nvmf_request_zcopy_start(&req);
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);
+	CU_ASSERT_EQUAL(req.zcopy_phase, NVMF_ZCOPY_PHASE_INIT_FAILED);
+	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sct, SPDK_NVME_SCT_GENERIC);
+	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sc, SPDK_NVME_SC_INVALID_NAMESPACE_OR_FORMAT);
 	cmd.nsid = 1;
 
 	/* Fail because bad Channel */
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
 	ns_info.channel = NULL;
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) < 0);
+	spdk_nvmf_request_zcopy_start(&req);
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);
+	CU_ASSERT_EQUAL(req.zcopy_phase, NVMF_ZCOPY_PHASE_INIT_FAILED);
+	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sct, SPDK_NVME_SCT_GENERIC);
+	CU_ASSERT_EQUAL(rsp.nvme_cpl.status.sc, SPDK_NVME_SC_INVALID_NAMESPACE_OR_FORMAT);
 	ns_info.channel = &io_ch;
 
-	/* Fail because NSID is not active */
+	/* Queue the requet because NSID is not active */
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
 	ns_info.state = SPDK_NVMF_SUBSYSTEM_PAUSING;
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) < 0);
+	spdk_nvmf_request_zcopy_start(&req);
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);
+	CU_ASSERT_EQUAL(req.zcopy_phase, NVMF_ZCOPY_PHASE_INIT);
+	CU_ASSERT_EQUAL(TAILQ_FIRST(&sgroups.queued), &req);
 	ns_info.state = SPDK_NVMF_SUBSYSTEM_ACTIVE;
+	TAILQ_REMOVE(&sgroups.queued, &req, link);
 
 	/* Fail because QPair is not active */
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
 	qpair.state = SPDK_NVMF_QPAIR_DEACTIVATING;
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) < 0);
+	qpair.state_cb = qpair_state_change_done;
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);
+	spdk_nvmf_request_zcopy_start(&req);
+	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT_FAILED);
 	qpair.state = SPDK_NVMF_QPAIR_ACTIVE;
+	qpair.state_cb = NULL;
 
 	/* Fail because nvmf_bdev_ctrlr_zcopy_start fails */
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
@@ -2492,15 +2501,15 @@ test_spdk_nvmf_request_zcopy_start(void)
 	cmd.cdw10 = bdev.blockcnt;	/* SLBA: CDW10 and CDW11 */
 	cmd.cdw12 = 100;	/* NLB: CDW12 bits 15:00, 0's based */
 	req.length = (cmd.cdw12 + 1) * bdev.blocklen;
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) < 0);
+	spdk_nvmf_request_zcopy_start(&req);
-	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_NONE);
+	CU_ASSERT_EQUAL(req.zcopy_phase, NVMF_ZCOPY_PHASE_INIT_FAILED);
 	cmd.cdw10 = 0;
 	cmd.cdw12 = 0;
 
 	/* Success */
 	CU_ASSERT(nvmf_ctrlr_use_zcopy(&req));
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_INIT);
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) == 0);
+	spdk_nvmf_request_zcopy_start(&req);
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_EXECUTE);
 }
 
@@ -2574,7 +2583,7 @@ test_zcopy_read(void)
 	CU_ASSERT(ns_info.io_outstanding == 0);
 
 	/* Perform the zcopy start */
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) == 0);
+	spdk_nvmf_request_zcopy_start(&req);
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_EXECUTE);
 	CU_ASSERT(req.zcopy_bdev_io == zcopy_start_bdev_io_read);
 	CU_ASSERT(qpair.outstanding.tqh_first == &req);
@@ -2660,7 +2669,7 @@ test_zcopy_write(void)
 	CU_ASSERT(ns_info.io_outstanding == 0);
 
 	/* Perform the zcopy start */
-	CU_ASSERT(spdk_nvmf_request_zcopy_start(&req) == 0);
+	spdk_nvmf_request_zcopy_start(&req);
 	CU_ASSERT(req.zcopy_phase == NVMF_ZCOPY_PHASE_EXECUTE);
 	CU_ASSERT(req.zcopy_bdev_io == zcopy_start_bdev_io_write);
 	CU_ASSERT(qpair.outstanding.tqh_first == &req);