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