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

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

[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.

Signed-off-by: Dan McLeran <daniel.mcleran at intel.com>
---
 drivers/block/nvme-scsi.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/block/nvme-scsi.c b/drivers/block/nvme-scsi.c
index a4cd6d6..5bcbe04 100644
--- a/drivers/block/nvme-scsi.c
+++ b/drivers/block/nvme-scsi.c
@@ -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;
-- 
1.7.10.4

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

[Keith Busch]

On Wed, 18 Jun 2014, Dan McLeran wrote:
> 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".

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

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

[Dan McLeran]

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

On Wed, 18 Jun 2014, Keith Busch wrote:

> On Wed, 18 Jun 2014, Dan McLeran wrote:
>> 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".
>
>> Signed-off-by: Dan McLeran <daniel.mcleran at intel.com>
>

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

[Matthew Wilcox]

On Wed, Jun 18, 2014@09:13:27AM -0600, Keith Busch wrote:
> On Wed, 18 Jun 2014, Dan McLeran wrote:
> >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?

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

[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.

On Wed, 18 Jun 2014, Matthew Wilcox wrote:

> On Wed, Jun 18, 2014@09:13:27AM -0600, Keith Busch wrote:
>> On Wed, 18 Jun 2014, Dan McLeran wrote:
>>> 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?
>

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

[Matthew Wilcox]

On Wed, Jun 18, 2014@09:30:24AM -0600, Dan McLeran wrote:
> 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.

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

[Mayes, Barrett N]

> 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.

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

[Keith Busch]

On Wed, 18 Jun 2014, Mayes, Barrett N wrote:
>> 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.

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

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

[Matthew Wilcox]

On Wed, Jun 18, 2014@03:51:52PM +0000, Mayes, Barrett N wrote:
> > 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 ...

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

[Mayes, Barrett N]

>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

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

[Keith Busch]

On Wed, 18 Jun 2014, Matthew Wilcox wrote:
> On Wed, Jun 18, 2014@09:13:27AM -0600, Keith Busch wrote:
>> On Wed, 18 Jun 2014, Dan McLeran wrote:
>>> 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.