[RESEND PATCHv3 0/2] nvme effects handling updates

[RESEND PATCHv3 0/2] nvme effects handling updates

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Resending due to errors in the previous email.

Version 3 fixes the memory leak from not including the default log page
in the tracked xarray.

I've included the Send Receive filtering patch in this series now since
it depends on the first patch.

This series also depends on nvme-6.2 branch that includes the recent
commit "nvme: fix passthrough csi check".

Keith Busch (2):
  nvme: always initialize known command effects
  nvme: mask CSE effects for security receive

 drivers/nvme/host/core.c | 101 ++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 39 deletions(-)

-- 
2.30.2

[RESEND PATCHv3 1/2] nvme: always initialize known command effects

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Instead of appending command effects flags per IO, set the known effects
flags the driver needs to react to just once during initial setup.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 84 +++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 564ce60bad148..df929ba9bcc21 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1061,41 +1061,12 @@ 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;
-}
-
-static u32 nvme_known_nvm_effects(u8 opcode)
-{
-	switch (opcode) {
-	case nvme_cmd_write:
-	case nvme_cmd_write_zeroes:
-	case nvme_cmd_write_uncor:
-		 return NVME_CMD_EFFECTS_LBCC;
-	default:
-		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]);
-		if (ns->head->ids.csi == NVME_CSI_NVM)
-			effects |= nvme_known_nvm_effects(opcode);
+		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 unusual effects:%08x\n",
@@ -1108,9 +1079,7 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 		 */
 		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
 	} else {
-		if (ctrl->effects)
-			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-		effects |= nvme_known_admin_effects(opcode);
+		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
 	}
 
 	return effects;
@@ -3112,6 +3081,45 @@ 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] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_NCC |
+						NVME_CMD_EFFECTS_CSE_MASK);
+	log->acs[nvme_admin_sanitize_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_CSE_MASK);
+
+	log->iocs[nvme_cmd_write] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+	log->iocs[nvme_cmd_write_zeroes] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+	log->iocs[nvme_cmd_write_uncor] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+}
+
+static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
+{
+	int ret = 0;
+
+	if (ctrl->effects)
+		return 0;
+
+	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
+		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (!ctrl->effects) {
+		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
+		if (!ctrl->effects)
+			return -ENOMEM;
+		xa_store(&ctrl->cels, NVME_CSI_NVM, ctrl->effects, GFP_KERNEL);
+	}
+
+	nvme_init_known_nvm_effects(ctrl);
+	return 0;
+}
+
 static int nvme_init_identify(struct nvme_ctrl *ctrl)
 {
 	struct nvme_id_ctrl *id;
@@ -3125,12 +3133,6 @@ 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->ops->flags & NVME_F_FABRICS))
 		ctrl->cntlid = le16_to_cpu(id->cntlid);
 
@@ -3153,6 +3155,10 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		ret = nvme_init_subsystem(ctrl, id);
 		if (ret)
 			goto out_free;
+
+		ret = nvme_init_effects(ctrl, id);
+		if (ret)
+			goto out_free;
 	}
 	memcpy(ctrl->subsys->firmware_rev, id->fr,
 	       sizeof(ctrl->subsys->firmware_rev));
-- 
2.30.2

[RESEND PATCHv3 2/2] nvme: mask CSE effects for security receive

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The nvme driver will freeze the IO queues in response to an admin
command with CSE bits set. These bits notify the host that the command
that's about to be executed needs to be done exclusively, hence the
freeze.

The Security Receive command is often reported by multiple vendors with
CSE bits set. The reason for this is that the result depends on the
previous Security Send. This has nothing to do with IO queues, though,
so the driver is taking an overly cautious response to seeing this
passthrough command, while unable to fufill the intended admin queue
action.

Rather than freeze IO during this harmless command, mask off the
effects. This freezing is observed to cause IO latency spikes when host
software periodically validates the security state of the drives.

Reviewed-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index df929ba9bcc21..89ef7abb0910c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3091,6 +3091,23 @@ static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
 	log->acs[nvme_admin_sanitize_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
 						NVME_CMD_EFFECTS_CSE_MASK);
 
+	/*
+	 * The spec says the result of a security receive command depends on
+	 * the previous security send command. As such, many vendors log this
+	 * command as one to submitted only when no other commands to the same
+	 * namespace are outstanding. The intention is to tell the host to
+	 * prevent mixing security send and receive.
+	 *
+	 * This driver can only enforce such exclusive access against IO
+	 * queues, though. We are not readily able to enforce such a rule for
+	 * two commands to the admin queue, which is the only queue that
+	 * matters for this command.
+	 *
+	 * Rather than blindly freezing the IO queues for this effect that
+	 * doesn't even apply to IO, mask it off.
+	 */
+	log->acs[nvme_admin_security_recv] &= ~NVME_CMD_EFFECTS_CSE_MASK;
+
 	log->iocs[nvme_cmd_write] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
 	log->iocs[nvme_cmd_write_zeroes] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
 	log->iocs[nvme_cmd_write_uncor] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
-- 
2.30.2

Re: [RESEND PATCHv3 1/2] nvme: always initialize known command effects

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On Fri, Jan 27, 2023 at 08:56:19AM -0800, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>Instead of appending command effects flags per IO, set the known effects
>flags the driver needs to react to just once during initial setup.
>
>Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good.
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RESEND PATCHv3 0/2] nvme effects handling updates

[Chaitanya Kulkarni]

On 1/27/23 08:56, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Resending due to errors in the previous email.
> 
> Version 3 fixes the memory leak from not including the default log page
> in the tracked xarray.
> 
> I've included the Send Receive filtering patch in this series now since
> it depends on the first patch.
> 
> This series also depends on nvme-6.2 branch that includes the recent
> commit "nvme: fix passthrough csi check".
> 
> Keith Busch (2):
>    nvme: always initialize known command effects
>    nvme: mask CSE effects for security receive
> 
>   drivers/nvme/host/core.c | 101 ++++++++++++++++++++++++---------------
>   1 file changed, 62 insertions(+), 39 deletions(-)
> 


This looks good to me and thanks a lot for adding a big comment
in nvme_init_known_nvm_effects().

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [RESEND PATCHv3 0/2] nvme effects handling updates

[Christoph Hellwig]

Thanks,

applied to nvme-6.2.

Re: [RESEND PATCHv3 0/2] nvme effects handling updates

[Keith Busch]

On Wed, Feb 01, 2023 at 02:20:44PM +0100, Christoph Hellwig wrote:
> Thanks,
> 
> applied to nvme-6.2.

Totally fine by me to put this in 6.2, though I assumed this was 6.3 material.
The Security Receive behavior this series addresses is a little annoying, but
not urgent. 

Re: [RESEND PATCHv3 0/2] nvme effects handling updates

[Christoph Hellwig]

On Wed, Feb 01, 2023 at 08:09:00AM -0700, Keith Busch wrote:
> On Wed, Feb 01, 2023 at 02:20:44PM +0100, Christoph Hellwig wrote:
> > Thanks,
> > 
> > applied to nvme-6.2.
> 
> Totally fine by me to put this in 6.2, though I assumed this was 6.3 material.
> The Security Receive behavior this series addresses is a little annoying, but
> not urgent. 

I'll move it to 6.3.