[PATCH 0/7] nvmet: use centralize req completion for log

[PATCH 0/7] nvmet: use centralize req completion for log

[Chaitanya Kulkarni]

Hi,

There is a repeated call for nvmet_req_complete() in :-
nvmet_execute_get_log_page_error()
nvmet_execute_get_log_page_smart()
nvmet_execute_get_log_page_noop()
nvmet_execute_get_log_changed_ns(),
nvmet_execute_get_log_cmd_effects_ns() 
nvmet_execute_get_log_page_ana()

Instead of repeating the call in the current log pages (6) use
the nvmet_req_completion() call in the caller function
nvmet_execute_get_log_page().

Note that this is not the new pattern it is similar to what we have done
it for nvmet_set_features(), nvmet_get_features() and their respective
feature handlers.

This series is passing basic get-log page and blktests as below.

I've one more series in this area, please wait for any other cleanup
in this area.

-ck

Chaitanya Kulkarni (7):
  nvmet: prep to callers req completion for log
  nvmet: centralize req completion for err log
  nvmet: centralize req completion for smart log
  nvmet: centralize req completion for noop log
  nvmet: centralize req completion for change ns
  nvmet: centralize req completion for effects log
  nvmet: centralize req completion for ana log

 drivers/nvme/target/admin-cmd.c | 73 ++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 34 deletions(-)

1. Manual execution of the log pages err, smart, fw-log,
   changed-ns, effects-ns, ana :- 
[16817.567503] nvmet: creating nvm controller 1 for subsystem testnqn for NQN nqn.2014-08.org.nvmexpress:uuid:e78b2b4f-b1dc-4266-95ea-854fcec1738b.
[16817.567608] nvmet: nvmet_execute_get_log_cmd_effects_ns 203
[16817.567611] nvmet: nvmet_execute_get_log_cmd_effects_ns 228
[16817.567621] nvmet: nvmet_execute_get_log_page_ana 290
[16817.567625] nvmet: nvmet_execute_get_log_page_ana 322
[16817.567632] nvmet: nvmet_execute_get_log_page_smart 142
[16817.567638] nvmet: nvmet_execute_get_log_page_smart 165
[16817.567666] nvme nvme1: creating 48 I/O queues.
[16817.571704] nvme nvme1: new ctrl: "testnqn"
nvme (nvme-6.4) # echo 0 > /sys/kernel/config/nvmet/subsystems/testnqn/namespaces/1/enable 
nvme (nvme-6.4) # echo 1 > /sys/kernel/config/nvmet/subsystems/testnqn/namespaces/1/enable 
nvme (nvme-6.4) # nvme fw-log /dev/nvme1n1 
Firmware Log for device:nvme1n1
afi  : 0
nvme (nvme-6.4) # echo $?
0
nvme (nvme-6.4) # nvme error-log /dev/nvme1n1 > /dev/null 
nvme (nvme-6.4) # echo $?
0
nvme (nvme-6.4) # dmesg  -c 
[16832.390334] nvme nvme1: rescanning namespaces.
[16832.390346] nvmet: nvmet_execute_get_log_changed_ns 238
[16832.390351] nvmet: nvmet_execute_get_log_changed_ns 254
[16836.279322] nvme nvme1: rescanning namespaces.
[16836.279350] nvmet: nvmet_execute_get_log_changed_ns 238
[16836.279355] nvmet: nvmet_execute_get_log_changed_ns 254
[16849.107716] nvmet: nvmet_execute_get_log_page_noop 45
[16859.728750] nvmet: nvmet_execute_get_log_page_error 51
[16859.728763] nvmet: nvmet_execute_get_log_page_error 73

2. 
blktests (master) # ./check nvme
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime    ...  20.017s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.083s  ...  10.110s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.450s  ...  1.434s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.794s  ...  1.776s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.070s  ...  0.067s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.028s  ...  0.028s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.469s  ...  1.480s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.452s  ...  1.497s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  86.116s  ...  85.324s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  70.376s  ...  78.223s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  73.964s  ...  73.261s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  67.237s  ...  67.202s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.229s  ...  4.178s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.749s  ...  3.711s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime  13.430s  ...  13.294s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime  13.388s  ...  13.481s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.434s  ...  1.434s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.446s  ...  1.455s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.429s  ...  1.419s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.428s  ...  1.446s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.790s  ...  1.762s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.468s  ...  1.445s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.433s  ...  1.438s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.411s  ...  1.423s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.424s  ...  1.428s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.476s  ...  1.426s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.427s  ...  1.425s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.550s  ...  1.568s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.225s  ...  0.222s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  4.058s  ...  3.986s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.012s  ...  0.011s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  8.056s  ...  7.861s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.713s  ...  0.711s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  4.688s  ...  4.685s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  3.203s  ...  3.147s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.657s  ...  1.712s
nvme/045 (Test re-authentication)                            [passed]
    runtime  3.953s  ...  3.980s
blktests (master) # nvme_trtype=tcp ./check nvme
nvme/002 (create many subsystems and test discovery)         [not run]
    runtime  20.017s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.110s  ...  10.123s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.434s  ...  1.138s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.776s  ...  1.202s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.067s  ...  0.064s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.028s  ...  0.038s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.480s  ...  1.154s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.497s  ...  1.129s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  85.324s  ...  90.112s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  78.223s  ...  79.330s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  73.261s  ...  77.092s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  67.202s  ...  63.555s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.178s  ...  3.822s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.711s  ...  3.433s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [not run]
    runtime  13.294s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/017 (create/delete many file-ns and test discovery)     [not run]
    runtime  13.481s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.434s  ...  1.119s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.455s  ...  1.139s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.419s  ...  1.115s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.446s  ...  1.117s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.762s  ...  1.160s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.445s  ...  1.147s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.438s  ...  1.107s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.423s  ...  1.108s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.428s  ...  1.113s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.426s  ...  1.122s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.425s  ...  1.126s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.568s  ...  1.258s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.222s  ...  0.118s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  3.986s  ...  0.820s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.011s  ...  0.014s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.861s  ...  7.334s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.711s  ...  0.418s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  4.685s  ...  2.533s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  3.147s  ...  0.695s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.712s  ...  1.068s
nvme/045 (Test re-authentication)                            [passed]
    runtime  3.980s  ...  3.739s
blktests (master) # 


-- 
2.29.0

[PATCH 1/7] nvmet: prep to callers req completion for log

[Chaitanya Kulkarni]

In order to use request completion in nvmet_execute_get_log_page()
and remove repeated calls for nvmet_req_complete() in :-
nvmet_execute_get_log_page_error()
nvmet_execute_get_log_page_smart()
nvmet_execute_get_log_page_noop()
nvmet_execute_get_log_changed_ns(),
nvmet_execute_get_log_cmd_effects_ns() 
nvmet_execute_get_log_page_ana()

add default case for swicth so we will report appropriate error and
keep the backward compatibility.

Please note that this is not a pattern and is present in the
nvmet_execute_set_feature() that save current and future repeated
calls for nvmet_req_completion().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 39cb570f833d..45e4ba2a498e 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -319,6 +319,8 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 
 static void nvmet_execute_get_log_page(struct nvmet_req *req)
 {
+	u16 status;
+
 	if (!nvmet_check_transfer_len(req, nvmet_get_log_page_len(req->cmd)))
 		return;
 
@@ -340,11 +342,13 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_cmd_effects_ns(req);
 	case NVME_LOG_ANA:
 		return nvmet_execute_get_log_page_ana(req);
+	default:
+		pr_debug("unhandled lid %d on qid %d\n",
+			 req->cmd->get_log_page.lid, req->sq->qid);
+		req->error_loc = offsetof(struct nvme_get_log_page_command, lid);
+		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 	}
-	pr_debug("unhandled lid %d on qid %d\n",
-	       req->cmd->get_log_page.lid, req->sq->qid);
-	req->error_loc = offsetof(struct nvme_get_log_page_command, lid);
-	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
+	nvmet_req_complete(req, status);
 }
 
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
-- 
2.29.0

[PATCH 2/7] nvmet: centralize req completion for err log

[Chaitanya Kulkarni]

Remove nvmet_req_completion() call from
nvmet_execute_get_log_page_error() and use the one that is in the caller
nvmet_execute_get_log_page().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 45e4ba2a498e..44ad1c945938 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -45,7 +45,7 @@ static void nvmet_execute_get_log_page_noop(struct nvmet_req *req)
 	nvmet_req_complete(req, nvmet_zero_sgl(req, 0, req->transfer_len));
 }
 
-static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
+static u16 nvmet_execute_get_log_page_error(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	unsigned long flags;
@@ -68,7 +68,7 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
 		offset += sizeof(struct nvme_error_slot);
 	}
 	spin_unlock_irqrestore(&ctrl->error_lock, flags);
-	nvmet_req_complete(req, 0);
+	return NVME_SC_SUCCESS;
 }
 
 static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
@@ -326,7 +326,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 
 	switch (req->cmd->get_log_page.lid) {
 	case NVME_LOG_ERROR:
-		return nvmet_execute_get_log_page_error(req);
+		status = nvmet_execute_get_log_page_error(req);
+		break;
 	case NVME_LOG_SMART:
 		return nvmet_execute_get_log_page_smart(req);
 	case NVME_LOG_FW_SLOT:
-- 
2.29.0

[PATCH 3/7] nvmet: centralize req completion for smart log

[Chaitanya Kulkarni]

Remove nvmet_req_completion() call from
nvmet_execute_get_log_page_smart() and use the one that is in the caller
nvmet_execute_get_log_page().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 44ad1c945938..c2f510c80bbf 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -130,18 +130,18 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 	return NVME_SC_SUCCESS;
 }
 
-static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
+static u16 nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 {
 	struct nvme_smart_log *log;
 	u16 status = NVME_SC_INTERNAL;
 	unsigned long flags;
 
 	if (req->transfer_len != sizeof(*log))
-		goto out;
+		return status;
 
 	log = kzalloc(sizeof(*log), GFP_KERNEL);
 	if (!log)
-		goto out;
+		return status;
 
 	if (req->cmd->get_log_page.nsid == cpu_to_le32(NVME_NSID_ALL))
 		status = nvmet_get_smart_log_all(req, log);
@@ -158,8 +158,7 @@ static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
 out_free_log:
 	kfree(log);
-out:
-	nvmet_req_complete(req, status);
+	return status;
 }
 
 static void nvmet_get_cmd_effects_nvm(struct nvme_effects_log *log)
@@ -329,7 +328,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		status = nvmet_execute_get_log_page_error(req);
 		break;
 	case NVME_LOG_SMART:
-		return nvmet_execute_get_log_page_smart(req);
+		status = nvmet_execute_get_log_page_smart(req);
+		break;
 	case NVME_LOG_FW_SLOT:
 		/*
 		 * We only support a single firmware slot which always is
-- 
2.29.0

[PATCH 4/7] nvmet: centralize req completion for noop log

[Chaitanya Kulkarni]

Remove nvmet_req_completion() call from
nvmet_execute_get_log_page_noop() and use the one that is in the caller
nvmet_execute_get_log_page().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index c2f510c80bbf..84cacc93bf65 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -40,9 +40,9 @@ u64 nvmet_get_log_page_offset(struct nvme_command *cmd)
 	return le64_to_cpu(cmd->get_log_page.lpo);
 }
 
-static void nvmet_execute_get_log_page_noop(struct nvmet_req *req)
+static u16 nvmet_execute_get_log_page_noop(struct nvmet_req *req)
 {
-	nvmet_req_complete(req, nvmet_zero_sgl(req, 0, req->transfer_len));
+	return nvmet_zero_sgl(req, 0, req->transfer_len);
 }
 
 static u16 nvmet_execute_get_log_page_error(struct nvmet_req *req)
@@ -336,7 +336,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		 * active, so we can zero out the whole firmware slot log and
 		 * still claim to fully implement this mandatory log page.
 		 */
-		return nvmet_execute_get_log_page_noop(req);
+		status = nvmet_execute_get_log_page_noop(req);
+		break;
 	case NVME_LOG_CHANGED_NS:
 		return nvmet_execute_get_log_changed_ns(req);
 	case NVME_LOG_CMD_EFFECTS:
-- 
2.29.0

[PATCH 5/7] nvmet: centralize req completion for change ns

[Chaitanya Kulkarni]

Remove nvmet_req_completion() call from
nvmet_execute_get_log_changed_ns() and use the one that is in the caller
nvmet_execute_get_log_page().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 84cacc93bf65..5e6ac85b1843 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -225,14 +225,14 @@ static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
-static void nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
+static u16 nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	u16 status = NVME_SC_INTERNAL;
 	size_t len;
 
 	if (req->transfer_len != NVME_MAX_CHANGED_NAMESPACES * sizeof(__le32))
-		goto out;
+		return status;
 
 	mutex_lock(&ctrl->lock);
 	if (ctrl->nr_changed_ns == U32_MAX)
@@ -245,8 +245,8 @@ static void nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
 	ctrl->nr_changed_ns = 0;
 	nvmet_clear_aen_bit(req, NVME_AEN_BIT_NS_ATTR);
 	mutex_unlock(&ctrl->lock);
-out:
-	nvmet_req_complete(req, status);
+
+	return status;
 }
 
 static u32 nvmet_format_ana_group(struct nvmet_req *req, u32 grpid,
@@ -339,7 +339,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		status = nvmet_execute_get_log_page_noop(req);
 		break;
 	case NVME_LOG_CHANGED_NS:
-		return nvmet_execute_get_log_changed_ns(req);
+		status = nvmet_execute_get_log_changed_ns(req);
+		break;
 	case NVME_LOG_CMD_EFFECTS:
 		return nvmet_execute_get_log_cmd_effects_ns(req);
 	case NVME_LOG_ANA:
-- 
2.29.0

[PATCH 6/7] nvmet: centralize req completion for effects log

[Chaitanya Kulkarni]

Remove nvmet_req_completion() call
nvmet_execute_get_log_cmd_effects_ns() and use the one that is in the 
caller nvmet_execute_get_log_page().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 5e6ac85b1843..3fc187580c8d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -190,16 +190,14 @@ static void nvmet_get_cmd_effects_zns(struct nvme_effects_log *log)
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
 
-static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
+static u16 nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
 {
 	struct nvme_effects_log *log;
 	u16 status = NVME_SC_SUCCESS;
 
 	log = kzalloc(sizeof(*log), GFP_KERNEL);
-	if (!log) {
-		status = NVME_SC_INTERNAL;
-		goto out;
-	}
+	if (!log)
+		return NVME_SC_INTERNAL;
 
 	switch (req->cmd->get_log_page.csi) {
 	case NVME_CSI_NVM:
@@ -221,8 +219,7 @@ static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
 	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
 free:
 	kfree(log);
-out:
-	nvmet_req_complete(req, status);
+	return status;
 }
 
 static u16 nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
@@ -342,7 +339,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		status = nvmet_execute_get_log_changed_ns(req);
 		break;
 	case NVME_LOG_CMD_EFFECTS:
-		return nvmet_execute_get_log_cmd_effects_ns(req);
+		status = nvmet_execute_get_log_cmd_effects_ns(req);
+		break;
 	case NVME_LOG_ANA:
 		return nvmet_execute_get_log_page_ana(req);
 	default:
-- 
2.29.0

[PATCH 7/7] nvmet: centralize req completion for ana log

[Chaitanya Kulkarni]

Remove nvmet_req_completion() call from nvmet_execute_get_log_page_ana()
and use the one that is in the caller nvmet_execute_get_log_page().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/admin-cmd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 3fc187580c8d..7bbb211ab1a1 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -268,7 +268,7 @@ static u32 nvmet_format_ana_group(struct nvmet_req *req, u32 grpid,
 	return struct_size(desc, nsids, count);
 }
 
-static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
+static u16 nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 {
 	struct nvme_ana_rsp_hdr hdr = { 0, };
 	struct nvme_ana_group_desc *desc;
@@ -282,7 +282,7 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	desc = kmalloc(struct_size(desc, nsids, NVMET_MAX_NAMESPACES),
 		       GFP_KERNEL);
 	if (!desc)
-		goto out;
+		return status;
 
 	down_read(&nvmet_ana_sem);
 	for (grpid = 1; grpid <= NVMET_MAX_ANAGRPS; grpid++) {
@@ -309,8 +309,7 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 
 	/* copy the header last once we know the number of groups */
 	status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
-out:
-	nvmet_req_complete(req, status);
+	return status;
 }
 
 static void nvmet_execute_get_log_page(struct nvmet_req *req)
@@ -342,7 +341,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		status = nvmet_execute_get_log_cmd_effects_ns(req);
 		break;
 	case NVME_LOG_ANA:
-		return nvmet_execute_get_log_page_ana(req);
+		status = nvmet_execute_get_log_page_ana(req);
+		break;
 	default:
 		pr_debug("unhandled lid %d on qid %d\n",
 			 req->cmd->get_log_page.lid, req->sq->qid);
-- 
2.29.0

Re: [PATCH 0/7] nvmet: use centralize req completion for log

[Sagi Grimberg]

Series looks good to me,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 0/7] nvmet: use centralize req completion for log

[Christoph Hellwig]

I don't get the point of this series.  It creates a lot of churn,
adds more code, but doesn't seem to actually have any real benefit.

What am I missing?

Re: [PATCH 0/7] nvmet: use centralize req completion for log

[Chaitanya Kulkarni]

On 3/27/23 17:59, Christoph Hellwig wrote:
> I don't get the point of this series.  It creates a lot of churn,
> adds more code, but doesn't seem to actually have any real benefit.
>
> What am I missing?

Original nvmet_execute_get_log_page() [1], implemented as no-op
pretty much. It is perfectly fine to have a one
nvmet_req_complete(). Over the period it has grown to have:-
NVME_LOG_ERROR
NVME_LOG_SMART
NVME_LOG_FW_SLOT
NVME_LOG_CHANGED_NS
NVME_LOG_CMD_EFFECTS
NVME_LOG_ANA

which also adds 6 additional calls to nvmet_req_complete().

What is the point of having many function call repeated when
it should be called from parent function similar to what we
have in the nvmet_execute_[set|get]_fetures() ?

Also, about the churn I didn't do a good job of reviewing and
adding some of this, so I think I should fix it now.

-ck

[1]

+static void nvmet_execute_get_log_page(struct nvmet_req *req)
+{
+       size_t data_len = nvmet_get_log_page_len(req->cmd);
+       void *buf;
+       u16 status = 0;
+
+       buf = kzalloc(data_len, GFP_KERNEL);
+       if (!buf) {
+               status = NVME_SC_INTERNAL;
+               goto out;
+       }
+
+       switch (req->cmd->get_log_page.lid) {
+       case 0x01:
+               /*
+                * We currently never set the More bit in the status field,
+                * so all error log entries are invalid and can be 
zeroed out.
+                * This is called a minum viable implementation (TM) of this
+                * mandatory log page.
+                */
+               break;
+       case 0x02:
+               /*
+                * XXX: fill out actual smart log
+                *
+                * We might have a hard time coming up with useful 
values for
+                * many of the fields, and even when we have useful data
+                * available (e.g. units or commands read/written) those 
aren't
+                * persistent over power loss.
+                */
+               break;
+       case 0x03
+               /*
+                * We only support a single firmware slot which always is
+                * active, so we can zero out the whole firmware slot 
log and
+                * still claim to fully implement this mandatory log page.
+                */
+               break;
+       default:
+               BUG();
+       }
+
+       status = nvmet_copy_to_sgl(req, 0, buf, data_len);
+
+       kfree(buf);
+out:
+       nvmet_req_complete(req, status);
+}
+