NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

PATCH New Dan McLeran 4 participants 11 messages Jun 18, 2014

Subsystems (2)

Block layer NVMe

lore

Share & tools

lore

Dan McLeran

author

2014-06-18 09:03 UTC #0 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

SCSI UNMAP should fail if the host sends down the command with Anchor set. The current code does not check this condition. This patch checks Anchor and fails Scsi->NVMe translation if Anchor is set.

Signed-off-by Dan McLeran <daniel.mcleran at intel.com>

---
 drivers/block/nvme-scsi.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/block/nvme-scsi.c +12 -0

index a4cd6d6..5bcbe04 100644
@@ -96,6 +96,8 @@ static int sg_version_num = 30534;	/* 2 digits for each component */
 #define FORMAT_UNIT_PROT_INT_OFFSET			3
 #define FORMAT_UNIT_PROT_FIELD_USAGE_OFFSET		0
 #define FORMAT_UNIT_PROT_FIELD_USAGE_MASK		0x07
+#define UNMAP_CDB_PARAM_ANCHOR_OFFSET			1
+#define UNMAP_CDB_PARAM_ANCHOR_MASK				0x1
 #define UNMAP_CDB_PARAM_LIST_LENGTH_OFFSET		7
 
 /* Misc. defines */

@@ -2855,6 +2857,7 @@ static int nvme_trans_unmap(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	struct nvme_dsm_range *range;
 	struct nvme_command c;
 	int i, nvme_sc, res = -ENOMEM;
+	u8 anchor;
 	u16 ndesc, list_len;
 	dma_addr_t dma_addr;

@@ -2870,6 +2873,15 @@ static int nvme_trans_unmap(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	if (res != SNTI_TRANSLATION_SUCCESS)
 		goto out;
 
+	anchor = GET_U8_FROM_CDB(cmd, UNMAP_CDB_PARAM_ANCHOR_OFFSET);
+	anchor &= UNMAP_CDB_PARAM_ANCHOR_MASK;
+	if (anchor != 0) {
+		res = nvme_trans_completion(hdr, SAM_STAT_CHECK_CONDITION,
+				ILLEGAL_REQUEST, SCSI_ASC_INVALID_CDB,
+				SCSI_ASCQ_CAUSE_NOT_REPORTABLE);
+		goto out;
+	}
+
 	ndesc = be16_to_cpu(plist->unmap_blk_desc_data_len) >> 4;
 	if (!ndesc || ndesc > 256) {
 		res = -EINVAL;

Signature · 1 line

1.7.10.4

Keith Busch

2014-06-18 15:13 UTC #1 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

Quoted 3 lines · Dan McLeran

SCSI UNMAP should fail if the host sends down the command with Anchor set. The current code does not check this condition. This patch checks Anchor and fails Scsi->NVMe translation if Anchor is set.

The reference on nvmexpress.org says the command with anchor set is premissible "for resource-provisioned devices".

Quoted 1 line

Signed-off-by Dan McLeran <daniel.mcleran at intel.com>

Dan McLeran

author

2014-06-18 15:15 UTC #2 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

Wrong spec version. Gotta go clean house and get rid of my old ones.

Quoted 12 lines · Keith Busch

Quoted 5 lines · Dan McLeran
SCSI UNMAP should fail if the host sends down the command with Anchor set. The
current code does not check this condition. This patch checks Anchor and fails
Scsi->NVMe translation if Anchor is set.
The reference on nvmexpress.org says the command with anchor set is premissible "for resource-provisioned devices".
Quoted 1 line
Signed-off-by Dan McLeran <daniel.mcleran at intel.com>

Matthew Wilcox

2014-06-18 15:27 UTC #3 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

In reply to Keith Busch

Quoted 7 lines · Keith Busch

Quoted 3 lines · Dan McLeran
SCSI UNMAP should fail if the host sends down the command with Anchor set. The current code does not check this condition. This patch checks Anchor and fails Scsi->NVMe translation if Anchor is set.
The reference on nvmexpress.org says the command with anchor set is premissible "for resource-provisioned devices".

So ... we should check whether the namespace has NSFEAT bit 0 set before failing the command?

Dan McLeran

author

2014-06-18 15:30 UTC #4 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

Good point. So, is Anchor == 1 still illegal for non-resource-provisioned devices? The spec seems to imply that must be the case.

Quoted 12 lines · Matthew Wilcox

Quoted 7 lines · Keith Busch
Quoted 3 lines · Dan McLeran
SCSI UNMAP should fail if the host sends down the command with Anchor set. The current code does not check this condition. This patch checks Anchor and fails Scsi->NVMe translation if Anchor is set.
The reference on nvmexpress.org says the command with anchor set is premissible "for resource-provisioned devices".
So ... we should check whether the namespace has NSFEAT bit 0 set before failing the command?

Matthew Wilcox

2014-06-18 15:45 UTC #5 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

Quoted 2 lines · Dan McLeran

Good point. So, is Anchor == 1 still illegal for non-resource-provisioned devices? The spec seems to imply that must be the case.

The current revision of the spec seems contradictory. On the one hand, it says that ANC_SUP shall be set to 0. SBC says you're not allowed to set the ANCHOR bit if ANC_SUP is 0. And then the NVMe-SCSI spec says that you can set ANCHOR if the underlying namespace is "resource provisioned" which isn't something that actually exists in NVMe.

Since there's no way to actually pass the ANCHOR bit through, and we always set ANC_SUP to 0, I think we should go with your original patch, failing the command if ANCHOR is set.

Mayes, Barrett N

2014-06-18 15:51 UTC #6 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

Quoted 1 line

On the one hand, it says that ANC_SUP shall be set to 0.

LSI proposed the following and asked for feedback by June 9. I don't recall seeing any comments to this on the public reflector.

ANC_SUC
Shall be set to 0, to indicate that setting the ANCHOR bit in UNMAP is not supported if the namespace is not resource or thin provisioned.
Shall be set to 1, to indicate that setting the ANCHOR bit in UNMAP is supported if the namespace is resource provisioned. May be set to 1 if the namespace is thin provisioned.

Keith Busch

2014-06-18 16:04 UTC #7 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

Quoted 5 lines · Barrett N

Quoted 1 line
On the one hand, it says that ANC_SUP shall be set to 0.
LSI proposed the following and asked for feedback by June 9. I don't recall seeing any comments to this on the public reflector.

Quoted 7 lines

ANC_SUC
Shall be set to 0, to indicate that setting the ANCHOR bit in UNMAP is not supported if the namespace is not resource or thin provisioned.
Shall be set to 1, to indicate that setting the ANCHOR bit in UNMAP is supported if the namespace is resource provisioned. May be set to 1 if the namespace is thin provisioned.

I looked at the proposal and the translation for the PROVISIONING TYPE would indicate any device that supports UNMAP also supports ANCHOR.

Matthew Wilcox

2014-06-18 16:25 UTC #8 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

In reply to Mayes, Barrett N

Quoted 7 lines · Barrett N

Quoted 1 line
On the one hand, it says that ANC_SUP shall be set to 0.
LSI proposed the following and asked for feedback by June 9. I don't recall seeing any comments to this on the public reflector.
ANC_SUC
Shall be set to 0, to indicate that setting the ANCHOR bit in UNMAP is not supported if the namespace is not resource or thin provisioned.
Shall be set to 1, to indicate that setting the ANCHOR bit in UNMAP is supported if the namespace is resource provisioned. May be set to 1 if the namespace is thin provisioned.

Thanks! Just found that unread email ;-)

I'm not sure about Guy's reasoning that NVMe supports three different provisioning models like SCSI does. Should probably take this to the NVMe mailing list now ...

Mayes, Barrett N

2014-06-18 17:23 UTC #9 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

Quoted 3 lines

I'm not sure about Guy's reasoning that NVMe supports three different provisioning models like SCSI does. Should probably take this to the NVMe mailing list now ...

ANC_SUP=0 if ONCS bit 2 is 0 _and_ NSFEAT bit 0 is 0 ANC_SUP=1 if ONCS bit 2 is 1 _or_ NSFEAT bit 0 is 1

Keith Busch

2014-06-18 15:35 UTC #10 lore raw

PATCH 1/1 NVMe: Fail SCSI->NVMe translation for UNMAP when anchor is set

In reply to Matthew Wilcox

Quoted 11 lines · Matthew Wilcox

Quoted 7 lines · Keith Busch
Quoted 3 lines · Dan McLeran
SCSI UNMAP should fail if the host sends down the command with Anchor set. The current code does not check this condition. This patch checks Anchor and fails Scsi->NVMe translation if Anchor is set.
The reference on nvmexpress.org says the command with anchor set is premissible "for resource-provisioned devices".
So ... we should check whether the namespace has NSFEAT bit 0 set before failing the command?

This gets a little confusing (for me, at least). The translation for VPD B2 says the device is "resource-provisioned" if it supports DSM Deallocate, but the command is already going to be failed by the device if it doesn't support DSM.