Re: [PATCH 6/7] nvme: also return I/O command effects from nvme_command_effects

Keith Busch
2022-12-13T19:18:57Z

On Tue, Dec 13, 2022 at 07:54:15PM +0100, Christoph Hellwig wrote:
> On Tue, Dec 13, 2022 at 10:53:07AM -0700, Keith Busch wrote:
> > Also nvme_cmd_write_uncorr.
>
> Indeed.
>
> > I think you'd need to know the namespace's command set to assume these
> > opcodes, though.
>
> ctrl->effects always is NVM command set if supported, or otherwise
> empty for I/O commands.
>
> > I was thinking instead of appending effects per-IO, we could always set
> > ns->head->effects even if the device doesn't support the log page, then
> > overwrite the log page's opcodes with known LBCC effects during
> > initialization. That way we don't need a per-io check. Does that sound
> > reasonable?
>
> I'd prefer to keep OR ingin the known worst cases. But we can do
> that at setup time.

This is what I was thinking (but please ignore that I missed the
endian conversion in this example):

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d11e0d5b231df..b8672dc28beee 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1060,39 +1060,19 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
}
EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);

-static u32 nvme_known_admin_effects(u8 opcode)
-{
- switch (opcode) {
- case nvme_admin_format_nvm:
- return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_NCC |
- NVME_CMD_EFFECTS_CSE_MASK;
- case nvme_admin_sanitize_nvm:
- return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK;
- default:
- break;
- }
- return 0;
-}
-
u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
{
- u32 effects = 0;
-
if (ns) {
- if (ns->head->effects)
- effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+ u32 effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+
if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
dev_warn_once(ctrl->device,
"IO command:%02x has unhandled effects:%08x\n",
opcode, effects);
- return 0;
+ return effects;
}

- if (ctrl->effects)
- effects = le32_to_cpu(ctrl->effects->acs[opcode]);
- effects |= nvme_known_admin_effects(opcode);
-
- return effects;
+ return le32_to_cpu(ctrl->effects->acs[opcode]);
}
EXPORT_SYMBOL_NS_GPL(nvme_command_effects, NVME_TARGET_PASSTHRU);

@@ -3100,6 +3080,21 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
return ret;
}

+static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
+{
+ struct nvme_effects_log *log = ctrl->effects;
+
+ log->acs[nvme_admin_format_nvm] |= NVME_CMD_EFFECTS_LBCC |
+ NVME_CMD_EFFECTS_NCC |
+ NVME_CMD_EFFECTS_CSE_MASK;
+ log->acs[nvme_admin_sanitize_nvm] |= NVME_CMD_EFFECTS_LBCC |
+ NVME_CMD_EFFECTS_CSE_MASK;
+
+ log->iocs[nvme_cmd_write] |= NVME_CMD_EFFECTS_LBCC;
+ log->iocs[nvme_cmd_write_zeroes] |= NVME_CMD_EFFECTS_LBCC;
+ log->iocs[nvme_cmd_write_uncor] |= NVME_CMD_EFFECTS_LBCC;
+}
+
static int nvme_init_identify(struct nvme_ctrl *ctrl)
{
struct nvme_id_ctrl *id;
@@ -3113,10 +3108,21 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
return -EIO;
}

- if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
- ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
- if (ret < 0)
- goto out_free;
+ if (!ctrl->effects) {
+ if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
+ ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM,
+ &ctrl->effects);
+ if (ret < 0)
+ goto out_free;
+ } else {
+ ctrl->effects = kzalloc(sizeof(*ctrl->effects),
+ GFP_KERNEL);
+ if (!ctrl->effects) {
+ ret = -ENOMEM;
+ goto out_free;
+ }
+ }
+ nvme_init_known_nvm_effects(ctrl);
}

if (!(ctrl->ops->flags & NVME_F_FABRICS))
--