[PATCH 0/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation

[PATCH 0/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi folks,

This series for-3.8 adds support for proper WRITE_SAME w/ UNMAP=0 emulation
for IBLOCK device backends to follow MKP's WRITE_SAME patches that have
been merged for v3.7-rc1.

Currently it uses a bio_add_page() call for each sector in order to allow
scatterlist w/ page offsets to work, as blkdev_issue_write_same() currently
assumes underlying hw support + zero page offset.

So far it has been tested on target-pending/for-next code with iscsi-target
and tcm_loop fabric ports.

Please review,

--nab

Nicholas Bellinger (3):
  target/sbc: Make WRITE_SAME check differentiate between UNMAP=[1,0]
  target: Add max_write_same_len device attribute
  target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

 drivers/target/target_core_configfs.c |    4 ++
 drivers/target/target_core_device.c   |   11 +++++
 drivers/target/target_core_iblock.c   |   78 +++++++++++++++++++++++++++++---
 drivers/target/target_core_internal.h |    1 +
 drivers/target/target_core_sbc.c      |   19 +++-----
 drivers/target/target_core_spc.c      |    8 +++-
 include/target/target_core_base.h     |    4 ++
 7 files changed, 104 insertions(+), 21 deletions(-)

-- 
1.7.2.5

[PATCH 1/3] target/sbc: Make WRITE_SAME check differentiate between UNMAP=[1,0]

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch updates sbc_write_same_supported() to set SCF_WRITE_SAME_DISCARD
to signal when WRITE_SAME w/ UNMAP=1 is requested.

Also, allow WRITE_SAME w/ UNMAP=0 to be passed to backend driver logic.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c  |   19 +++++++------------
 include/target/target_core_base.h |    1 +
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index b802421..ee9fa52 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -235,7 +235,7 @@ static inline unsigned long long transport_lba_64_ext(unsigned char *cdb)
 	return ((unsigned long long)__v2) | (unsigned long long)__v1 << 32;
 }
 
-static int sbc_write_same_supported(struct se_device *dev,
+static int sbc_write_same_supported(struct se_cmd *cmd,
 		unsigned char *flags)
 {
 	if ((flags[0] & 0x04) || (flags[0] & 0x02)) {
@@ -244,16 +244,11 @@ static int sbc_write_same_supported(struct se_device *dev,
 			" Emulation\n");
 		return -ENOSYS;
 	}
-
 	/*
-	 * Currently for the emulated case we only accept
-	 * tpws with the UNMAP=1 bit set.
+	 * UNMAP=1 bit translantes to blkdev_issue_discard() execution
 	 */
-	if (!(flags[0] & 0x08)) {
-		pr_err("WRITE_SAME w/o UNMAP bit not"
-			" supported for Block Discard Emulation\n");
-		return -ENOSYS;
-	}
+	if (flags[0] & 0x08)
+		cmd->se_cmd_flags |= SCF_WRITE_SAME_DISCARD;
 
 	return 0;
 }
@@ -431,7 +426,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			size = sbc_get_size(cmd, 1);
 			cmd->t_task_lba = get_unaligned_be64(&cdb[12]);
 
-			if (sbc_write_same_supported(dev, &cdb[10]) < 0)
+			if (sbc_write_same_supported(cmd, &cdb[10]) < 0)
 				return TCM_UNSUPPORTED_SCSI_OPCODE;
 			cmd->execute_cmd = ops->execute_write_same;
 			break;
@@ -507,7 +502,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		size = sbc_get_size(cmd, 1);
 		cmd->t_task_lba = get_unaligned_be64(&cdb[2]);
 
-		if (sbc_write_same_supported(dev, &cdb[1]) < 0)
+		if (sbc_write_same_supported(cmd, &cdb[1]) < 0)
 			return TCM_UNSUPPORTED_SCSI_OPCODE;
 		cmd->execute_cmd = ops->execute_write_same;
 		break;
@@ -528,7 +523,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		 * Follow sbcr26 with WRITE_SAME (10) and check for the existence
 		 * of byte 1 bit 3 UNMAP instead of original reserved field
 		 */
-		if (sbc_write_same_supported(dev, &cdb[1]) < 0)
+		if (sbc_write_same_supported(cmd, &cdb[1]) < 0)
 			return TCM_UNSUPPORTED_SCSI_OPCODE;
 		cmd->execute_cmd = ops->execute_write_same;
 		break;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 5350f6e..2787b85 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -154,6 +154,7 @@ enum se_cmd_flags_table {
 	SCF_ALUA_NON_OPTIMIZED		= 0x00008000,
 	SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000,
 	SCF_ACK_KREF			= 0x00040000,
+	SCF_WRITE_SAME_DISCARD		= 0x00080000,
 };
 
 /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
-- 
1.7.2.5

Re: [PATCH 1/3] target/sbc: Make WRITE_SAME check differentiate between UNMAP=[1,0]

[Christoph Hellwig]

> +	if (flags[0] & 0x08)
> +		cmd->se_cmd_flags |= SCF_WRITE_SAME_DISCARD;

I don't like this flag at all.  We can still simply check the CDB
during ->execute_cmd and avoid this redundant flag.

Except for that bit the changes look fine, but should not be a patch
on their own.  Without an actual implementation this relaxation is
actively harmful.

[PATCH 2/3] target: Add max_write_same_len device attribute

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a new max_write_same_len device attribute for use with
WRITE_SAME w/ UNMAP=0 backend emulation.

Also, update block limits VPD emulation code in spc_emulate_evpd_b0() to
set the default MAXIMUM WRITE SAME LENGTH value of zero.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_configfs.c |    4 ++++
 drivers/target/target_core_device.c   |   11 +++++++++++
 drivers/target/target_core_internal.h |    1 +
 drivers/target/target_core_spc.c      |    8 +++++++-
 include/target/target_core_base.h     |    3 +++
 5 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 7b473b6..2b14164 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -676,6 +676,9 @@ SE_DEV_ATTR(unmap_granularity, S_IRUGO | S_IWUSR);
 DEF_DEV_ATTRIB(unmap_granularity_alignment);
 SE_DEV_ATTR(unmap_granularity_alignment, S_IRUGO | S_IWUSR);
 
+DEF_DEV_ATTRIB(max_write_same_len);
+SE_DEV_ATTR(max_write_same_len, S_IRUGO | S_IWUSR);
+
 CONFIGFS_EATTR_OPS(target_core_dev_attrib, se_dev_attrib, da_group);
 
 static struct configfs_attribute *target_core_dev_attrib_attrs[] = {
@@ -701,6 +704,7 @@ static struct configfs_attribute *target_core_dev_attrib_attrs[] = {
 	&target_core_dev_attrib_max_unmap_block_desc_count.attr,
 	&target_core_dev_attrib_unmap_granularity.attr,
 	&target_core_dev_attrib_unmap_granularity_alignment.attr,
+	&target_core_dev_attrib_max_write_same_len.attr,
 	NULL,
 };
 
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 599374e..54439bc 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -706,6 +706,16 @@ int se_dev_set_unmap_granularity_alignment(
 	return 0;
 }
 
+int se_dev_set_max_write_same_len(
+	struct se_device *dev,
+	u32 max_write_same_len)
+{
+	dev->dev_attrib.max_write_same_len = max_write_same_len;
+	pr_debug("dev[%p]: Set max_write_same_len: %u\n",
+			dev, dev->dev_attrib.max_write_same_len);
+	return 0;
+}
+
 int se_dev_set_emulate_dpo(struct se_device *dev, int flag)
 {
 	if (flag != 0 && flag != 1) {
@@ -1393,6 +1403,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
 	dev->dev_attrib.unmap_granularity = DA_UNMAP_GRANULARITY_DEFAULT;
 	dev->dev_attrib.unmap_granularity_alignment =
 				DA_UNMAP_GRANULARITY_ALIGNMENT_DEFAULT;
+	dev->dev_attrib.max_write_same_len = DA_MAX_WRITE_SAME_LEN;
 	dev->dev_attrib.fabric_max_sectors = DA_FABRIC_MAX_SECTORS;
 	dev->dev_attrib.optimal_sectors = DA_FABRIC_MAX_SECTORS;
 
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index bc9c522..93e9c1f 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -24,6 +24,7 @@ int	se_dev_set_max_unmap_lba_count(struct se_device *, u32);
 int	se_dev_set_max_unmap_block_desc_count(struct se_device *, u32);
 int	se_dev_set_unmap_granularity(struct se_device *, u32);
 int	se_dev_set_unmap_granularity_alignment(struct se_device *, u32);
+int	se_dev_set_max_write_same_len(struct se_device *, u32);
 int	se_dev_set_emulate_dpo(struct se_device *, int);
 int	se_dev_set_emulate_fua_write(struct se_device *, int);
 int	se_dev_set_emulate_fua_read(struct se_device *, int);
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 4b3c183..cf1b8bb 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -465,7 +465,7 @@ spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
 	 * Exit now if we don't support TP.
 	 */
 	if (!have_tp)
-		return 0;
+		goto max_write_same;
 
 	/*
 	 * Set MAXIMUM UNMAP LBA COUNT
@@ -491,6 +491,12 @@ spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
 	if (dev->dev_attrib.unmap_granularity_alignment != 0)
 		buf[32] |= 0x80; /* Set the UGAVALID bit */
 
+	/*
+	 * MAXIMUM WRITE SAME LENGTH
+	 */
+max_write_same:
+	put_unaligned_be64(dev->dev_attrib.max_write_same_len, &buf[36]);
+
 	return 0;
 }
 
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 2787b85..1b45879 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -71,6 +71,8 @@
 #define DA_UNMAP_GRANULARITY_DEFAULT		0
 /* Default unmap_granularity_alignment */
 #define DA_UNMAP_GRANULARITY_ALIGNMENT_DEFAULT	0
+/* Default max_write_same_len, disabled by default */
+#define DA_MAX_WRITE_SAME_LEN			0
 /* Default max transfer length */
 #define DA_FABRIC_MAX_SECTORS			8192
 /* Emulation for Direct Page Out */
@@ -610,6 +612,7 @@ struct se_dev_attrib {
 	u32		max_unmap_block_desc_count;
 	u32		unmap_granularity;
 	u32		unmap_granularity_alignment;
+	u32		max_write_same_len;
 	struct se_device *da_dev;
 	struct config_group da_group;
 };
-- 
1.7.2.5

Re: [PATCH 2/3] target: Add max_write_same_len device attribute

[Christoph Hellwig]

On Thu, Nov 08, 2012 at 08:07:17PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch adds a new max_write_same_len device attribute for use with
> WRITE_SAME w/ UNMAP=0 backend emulation.
> 
> Also, update block limits VPD emulation code in spc_emulate_evpd_b0() to
> set the default MAXIMUM WRITE SAME LENGTH value of zero.

why do we need an exposed attribute for this?

Re: [PATCH 2/3] target: Add max_write_same_len device attribute

[Nicholas A. Bellinger]

On Thu, 2012-11-15 at 05:53 -0500, Christoph Hellwig wrote:
> On Thu, Nov 08, 2012 at 08:07:17PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch adds a new max_write_same_len device attribute for use with
> > WRITE_SAME w/ UNMAP=0 backend emulation.
> > 
> > Also, update block limits VPD emulation code in spc_emulate_evpd_b0() to
> > set the default MAXIMUM WRITE SAME LENGTH value of zero.
> 
> why do we need an exposed attribute for this?
> 

This is useful for userspace to reduce the allowed maximum from the
default 0xFFFF set by IBLOCK.  Allowing huge WRITE_SAMEs can very much
effect performance (esp. with spinning media), so being able to reduce
the max we accept via a userspace tunable is helpful.

--nab

Re: [PATCH 2/3] target: Add max_write_same_len device attribute

[Paolo Bonzini]

Il 15/11/2012 20:23, Nicholas A. Bellinger ha scritto:
>>> > > 
>>> > > This patch adds a new max_write_same_len device attribute for use with
>>> > > WRITE_SAME w/ UNMAP=0 backend emulation.
>>> > > 
>>> > > Also, update block limits VPD emulation code in spc_emulate_evpd_b0() to
>>> > > set the default MAXIMUM WRITE SAME LENGTH value of zero.
>> > 
>> > why do we need an exposed attribute for this?
>> > 
> This is useful for userspace to reduce the allowed maximum from the
> default 0xFFFF set by IBLOCK.  Allowing huge WRITE_SAMEs can very much
> effect performance (esp. with spinning media), so being able to reduce
> the max we accept via a userspace tunable is helpful.

Unfortunately this doesn't really help.  Linux will submit the smaller
WRITE SAMEs in parallel, and this could easily bring the target to its
knees.

(This was reported to me with a Linux virtual machine sending WRITE SAME
commands to a Nexenta iSCSI target running OpenSolaris.  QEMU can be
easily replaced with LIO, with the same effect).

Paolo

[PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds support for emulation of WRITE_SAME w/ UNMAP=0 within
iblock_execute_write_same() backend code.

The emulation uses a bio_add_page() call for each sector, and by default
enforces a limit of max_write_same_len=0xFFFF (65536) sectors following
what scsi_debug reports per default for MAXIMUM WRITE SAME LENGTH.

It also sets max_write_same_len to the operational default at setup ->
iblock_configure_device() time.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_iblock.c |   78 +++++++++++++++++++++++++++++++----
 1 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 53f4501..33a5248 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -151,6 +151,10 @@ static int iblock_configure_device(struct se_device *dev)
 		pr_debug("IBLOCK: BLOCK Discard support available,"
 				" disabled by default\n");
 	}
+	/*
+	 * Enable WRITE_SAME emulation for IBLOCK, use scsi_debug.c default
+	 */
+	dev->dev_attrib.max_write_same_len = 0xFFFF;
 
 	if (blk_queue_nonrot(q))
 		dev->dev_attrib.is_nonrot = 1;
@@ -375,22 +379,80 @@ err:
 	return ret;
 }
 
+static struct bio *iblock_get_bio(struct se_cmd *, sector_t, u32);
+static void iblock_submit_bios(struct bio_list *, int);
+static void iblock_complete_cmd(struct se_cmd *);
+
 static sense_reason_t
 iblock_execute_write_same(struct se_cmd *cmd)
 {
 	struct iblock_dev *ib_dev = IBLOCK_DEV(cmd->se_dev);
-	int ret;
+	struct iblock_req *ibr;
+	struct scatterlist *sg;
+	struct bio *bio;
+	struct bio_list list;
+	sector_t block_lba = cmd->t_task_lba;
+	unsigned int sectors = spc_get_write_same_sectors(cmd);
+	int rc;
+
+	if (cmd->se_cmd_flags & SCF_WRITE_SAME_DISCARD) {
+		rc = blkdev_issue_discard(ib_dev->ibd_bd, cmd->t_task_lba,
+				spc_get_write_same_sectors(cmd), GFP_KERNEL, 0);
+		if (rc < 0) {
+			pr_warn("blkdev_issue_discard() failed: %d\n", rc);
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		}
+		target_complete_cmd(cmd, GOOD);
+		return 0;
+	}
+	if (sectors > cmd->se_dev->dev_attrib.max_write_same_len) {
+		pr_warn("WRITE_SAME sectors: %u exceeds max_write_same_len: %u\n",
+			sectors, cmd->se_dev->dev_attrib.max_write_same_len);
+		return TCM_INVALID_CDB_FIELD;
+	}
+	sg = &cmd->t_data_sg[0];
 
-	ret = blkdev_issue_discard(ib_dev->ibd_bd, cmd->t_task_lba,
-				   spc_get_write_same_sectors(cmd), GFP_KERNEL,
-				   0);
-	if (ret < 0) {
-		pr_debug("blkdev_issue_discard() failed for WRITE_SAME\n");
-		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	ibr = kzalloc(sizeof(struct iblock_req), GFP_KERNEL);
+	if (!ibr)
+		goto fail;
+	cmd->priv = ibr;
+
+	bio = iblock_get_bio(cmd, block_lba, 1);
+	if (!bio)
+		goto fail_free_ibr;
+
+	bio_list_init(&list);
+	bio_list_add(&list, bio);
+
+	atomic_set(&ibr->pending, 1);
+
+	while (sectors) {
+		while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
+				!= sg->length) {
+
+			bio = iblock_get_bio(cmd, block_lba, 1);
+			if (!bio)
+				goto fail_put_bios;
+
+			atomic_inc(&ibr->pending);
+			bio_list_add(&list, bio);
+		}
+
+		/* Always in 512 byte units for Linux/Block */
+		block_lba += sg->length >> IBLOCK_LBA_SHIFT;
+		sectors -= 1;
 	}
 
-	target_complete_cmd(cmd, GOOD);
+	iblock_submit_bios(&list, WRITE);
 	return 0;
+
+fail_put_bios:
+	while ((bio = bio_list_pop(&list)))
+		bio_put(bio);
+fail_free_ibr:
+	kfree(ibr);
+fail:
+	return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 }
 
 enum {
-- 
1.7.2.5

Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Christoph Hellwig]

> +	/*
> +	 * Enable WRITE_SAME emulation for IBLOCK, use scsi_debug.c default
> +	 */

Why would we care what scsi_debug.c uses?

> +	dev->dev_attrib.max_write_same_len = 0xFFFF;
>  
>  	if (blk_queue_nonrot(q))
>  		dev->dev_attrib.is_nonrot = 1;
> @@ -375,22 +379,80 @@ err:
>  	return ret;
>  }

> +static struct bio *iblock_get_bio(struct se_cmd *, sector_t, u32);
> +static void iblock_submit_bios(struct bio_list *, int);
> +static void iblock_complete_cmd(struct se_cmd *);

I'd suggest moving the write_same callback below these to avoid
forward declarations.

> +	if (cmd->se_cmd_flags & SCF_WRITE_SAME_DISCARD) {

I'd probably add separate write_same and write_same_unmap members to
the sbc_ops structure.  That'll keep decoding which one is used in the
SBC code, and it'll keep the implementations nicely separated.

> +	if (sectors > cmd->se_dev->dev_attrib.max_write_same_len) {

This sort of check should stay in the SBC code.

> +	sg = &cmd->t_data_sg[0];

Btw, it seems like we don't bother to ensure the S/G list length
is just one sector for WRITE SAME with either the unmap bit set or not.

Also please add testcases for WRITE SAME including corner cases like
incorrect transfer length to the scsi testsuite to ensure this code
has proper QA coverage.

Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Douglas Gilbert]

On 12-11-15 06:04 AM, Christoph Hellwig wrote:
>> +	/*
>> +	 * Enable WRITE_SAME emulation for IBLOCK, use scsi_debug.c default
>> +	 */
>
> Why would we care what scsi_debug.c uses?

Would you prefer no hint of where the magic number came
from? At least somebody who cares when they see that comment
might contact Martin Petersen and ask why he chose that
value.

t10.org documents are not big on sensible default values;
my favourite is where "0" means vendor specific.

Doug Gilbert

>> +	dev->dev_attrib.max_write_same_len = 0xFFFF;
>>
>>   	if (blk_queue_nonrot(q))
>>   		dev->dev_attrib.is_nonrot = 1;
>> @@ -375,22 +379,80 @@ err:
>>   	return ret;
>>   }
>
>> +static struct bio *iblock_get_bio(struct se_cmd *, sector_t, u32);
>> +static void iblock_submit_bios(struct bio_list *, int);
>> +static void iblock_complete_cmd(struct se_cmd *);
>
> I'd suggest moving the write_same callback below these to avoid
> forward declarations.
>
>> +	if (cmd->se_cmd_flags & SCF_WRITE_SAME_DISCARD) {
>
> I'd probably add separate write_same and write_same_unmap members to
> the sbc_ops structure.  That'll keep decoding which one is used in the
> SBC code, and it'll keep the implementations nicely separated.
>
>> +	if (sectors > cmd->se_dev->dev_attrib.max_write_same_len) {
>
> This sort of check should stay in the SBC code.
>
>> +	sg = &cmd->t_data_sg[0];
>
> Btw, it seems like we don't bother to ensure the S/G list length
> is just one sector for WRITE SAME with either the unmap bit set or not.
>
> Also please add testcases for WRITE SAME including corner cases like
> incorrect transfer length to the scsi testsuite to ensure this code
> has proper QA coverage.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Martin K. Petersen]

>>>>> "Doug" == Douglas Gilbert <dgilbert@interlog.com> writes:

Doug> On 12-11-15 06:04 AM, Christoph Hellwig wrote:
>>> + /*
>>> + * Enable WRITE_SAME emulation for IBLOCK, use scsi_debug.c default
>>> + */
>> 
>> Why would we care what scsi_debug.c uses?

Doug> Would you prefer no hint of where the magic number came from? At
Doug> least somebody who cares when they see that comment might contact
Doug> Martin Petersen and ask why he chose that value.

And the answer is that WRITE SAME(10) takes a two-byte block count.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Nicholas A. Bellinger]

On Thu, 2012-11-15 at 06:04 -0500, Christoph Hellwig wrote:
> > +	/*
> > +	 * Enable WRITE_SAME emulation for IBLOCK, use scsi_debug.c default
> > +	 */
> 
> Why would we care what scsi_debug.c uses?
> 

Fixing up this comment based upon MKP's earlier response.

> > +	dev->dev_attrib.max_write_same_len = 0xFFFF;
> >  
> >  	if (blk_queue_nonrot(q))
> >  		dev->dev_attrib.is_nonrot = 1;
> > @@ -375,22 +379,80 @@ err:
> >  	return ret;
> >  }
> 
> > +static struct bio *iblock_get_bio(struct se_cmd *, sector_t, u32);
> > +static void iblock_submit_bios(struct bio_list *, int);
> > +static void iblock_complete_cmd(struct se_cmd *);
> 
> I'd suggest moving the write_same callback below these to avoid
> forward declarations.
> 

Will take care of this in a separate patch..

> > +	if (cmd->se_cmd_flags & SCF_WRITE_SAME_DISCARD) {
> 
> I'd probably add separate write_same and write_same_unmap members to
> the sbc_ops structure.  That'll keep decoding which one is used in the
> SBC code, and it'll keep the implementations nicely separated.
> 

Done.

> > +	if (sectors > cmd->se_dev->dev_attrib.max_write_same_len) {
> 
> This sort of check should stay in the SBC code.
> 

Fixing this up now.

> > +	sg = &cmd->t_data_sg[0];
> 
> Btw, it seems like we don't bother to ensure the S/G list length
> is just one sector for WRITE SAME with either the unmap bit set or not.
> 

Well at least for the latter that is because UNMAP=0 does not have a
payload.  ;)

Adding a check to ensure that we have one SGL, and that SGL length
matches sector_size for UNMAP=0 logic.

> Also please add testcases for WRITE SAME including corner cases like
> incorrect transfer length to the scsi testsuite to ensure this code
> has proper QA coverage.

<nod>

--nab

Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Christoph Hellwig]

On Thu, Nov 15, 2012 at 11:29:46AM -0800, Nicholas A. Bellinger wrote:
> Well at least for the latter that is because UNMAP=0 does not have a
> payload.  ;)

UNMAP=0 does have a payload, we just ignore it.  In fact I was told
that targets should check for a completely zeroed sector sized payload
for it if being pedantic.  I can't really find the justification for
that in the standard - the closest thing to it is the stance about
ignoring the unmap bit if the device is fully provisioned.

RE: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Elliott, Robert (Server Storage)]

WRITE SAME always has a payload, regardless of the UNMAP bit value.

For WRITE SAME with UNMAP=0, it's extremely important; that's how what to write is specified.

For WRITE SAME with UNMAP=1, the device server is required to check that the payload matches the data that is returned for unmapped LBAs.  lf LBPRZ=1 (read zeros for unmapped LBAs), that means checking that the payload has all zeros.  In sbc3r33, this rule is tucked away in model section 4.7.3.4.3, not the command section 5.41.

I would like to change that rule (it's a nuisance and a performance burden), but that's the current rule going into SBC-3 letter ballot.  

Changing WRITE SAME with UNMAP=1 to ignore the payload would provide essentially the same functionality as changing the UNMAP command to be mandatory, not just a hint; both approaches have been discussed.



> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Thursday, 15 November, 2012 1:33 PM
> To: Nicholas A. Bellinger
> Cc: Christoph Hellwig; target-devel; linux-scsi; linux-kernel; Christoph Hellwig;
> Martin K. Petersen
> Subject: Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0
> emulation support
> 
> On Thu, Nov 15, 2012 at 11:29:46AM -0800, Nicholas A. Bellinger wrote:
> > Well at least for the latter that is because UNMAP=0 does not have a
> > payload.  ;)
> 
> UNMAP=0 does have a payload, we just ignore it.  In fact I was told
> that targets should check for a completely zeroed sector sized payload
> for it if being pedantic.  I can't really find the justification for
> that in the standard - the closest thing to it is the stance about
> ignoring the unmap bit if the device is fully provisioned.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Nicholas A. Bellinger]

Hi Robert,

On Thu, 2012-11-15 at 20:01 +0000, Elliott, Robert (Server Storage)
wrote:
> WRITE SAME always has a payload, regardless of the UNMAP bit value.
> 
> For WRITE SAME with UNMAP=0, it's extremely important; that's how what
> to write is specified.
> 
> For WRITE SAME with UNMAP=1, the device server is required to check
> that the payload matches the data that is returned for unmapped LBAs.
> lf LBPRZ=1 (read zeros for unmapped LBAs), that means checking that
> the payload has all zeros.  In sbc3r33, this rule is tucked away in
> model section 4.7.3.4.3, not the command section 5.41.
> 
> I would like to change that rule (it's a nuisance and a performance
> burden), but that's the current rule going into SBC-3 letter ballot.  
> 
> Changing WRITE SAME with UNMAP=1 to ignore the payload would provide
> essentially the same functionality as changing the UNMAP command to be
> mandatory, not just a hint; both approaches have been discussed.
> 

Thanks for the heads up here..  I'm making the slight change to the -v2
patch series that just went out to always check the max_write_same_len
value for both WRITE_SAME w/ UNMAP=[1,0] cases.

Thank you,

--nab

Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Paolo Bonzini]

Il 15/11/2012 21:01, Elliott, Robert (Server Storage) ha scritto:
> WRITE SAME always has a payload, regardless of the UNMAP bit value.
> 
> For WRITE SAME with UNMAP=0, it's extremely important; that's how
> what to write is specified.
> 
> For WRITE SAME with UNMAP=1, the device server is required to check
> that the payload matches the data that is returned for unmapped LBAs.
> lf LBPRZ=1 (read zeros for unmapped LBAs), that means checking that
> the payload has all zeros.  In sbc3r33, this rule is tucked away in
> model section 4.7.3.4.3, not the command section 5.41.

Does that mean that LBPRZ=0, LPBWS=1 is practically an invalid
combination?  Because there's no real way for the device server to
perform the check successfully.

Paolo

> I would like to change that rule (it's a nuisance and a performance
> burden), but that's the current rule going into SBC-3 letter ballot.
> 
> Changing WRITE SAME with UNMAP=1 to ignore the payload would provide
> essentially the same functionality as changing the UNMAP command to
> be mandatory, not just a hint; both approaches have been discussed.

RE: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0 emulation support

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, 19 November, 2012 5:38 AM
> To: Elliott, Robert (Server Storage)
> Cc: Christoph Hellwig; Nicholas A. Bellinger; Christoph Hellwig; target-devel;
> linux-scsi; linux-kernel; Martin K. Petersen
> Subject: Re: [PATCH 3/3] target/iblock: Add WRITE_SAME w/ UNMAP=0
> emulation support
> 
> Il 15/11/2012 21:01, Elliott, Robert (Server Storage) ha scritto:
> > WRITE SAME always has a payload, regardless of the UNMAP bit value.
> >
> > For WRITE SAME with UNMAP=0, it's extremely important; that's how
> > what to write is specified.
> >
> > For WRITE SAME with UNMAP=1, the device server is required to check
> > that the payload matches the data that is returned for unmapped LBAs.
> > lf LBPRZ=1 (read zeros for unmapped LBAs), that means checking that
> > the payload has all zeros.  In sbc3r33, this rule is tucked away in
> > model section 4.7.3.4.3, not the command section 5.41.
> 
> Does that mean that LBPRZ=0, LPBWS=1 is practically an invalid
> combination?  Because there's no real way for the device server to
> perform the check successfully.
>

Yes; that would only be practical if the device server knows the pattern
(e.g., all ones) that it will return for reads of unmapped LBAs. It's not
very useful if the pattern is simply the last content and could differ
for every LBA.

For consumer SSDs, the data returned for unmapped (i.e. trimmed)
LBAs didn't really matter.

For SSDs to be used in RAID arrays, however:
a) non-deterministic data is unworkable (RAID parity would never be right);
b) deterministic data can be accommodated, but the RAID stack needs to read the
LBA after unmapping to see what pattern resulted and recalculate parity, hurting
performance; and
c) deterministic zeros are ideal (0 XOR n = n).



> Paolo
> 
> > I would like to change that rule (it's a nuisance and a performance
> > burden), but that's the current rule going into SBC-3 letter ballot.
> >
> > Changing WRITE SAME with UNMAP=1 to ignore the payload would
> provide
> > essentially the same functionality as changing the UNMAP command to
> > be mandatory, not just a hint; both approaches have been discussed.