[PATCH 0/9] target: Add support for COMPARE_AND_WRITE (VAAI) emulation

[PATCH 0/9] target: Add support for COMPARE_AND_WRITE (VAAI) emulation

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

Hi folks,

This series adds support to target-core for generic COMPARE_AND_WRITE
emulation as defined by SBC-3 using virtual (IBLOCK, FILEIO, RAMDISK)
backends.

COMPARE_AND_WRITE is a VMWare ESX VAAI primitive that is currently used
by VMFS to perform array side locking of filesystem extents.  The logic
is the functional equivilent of an atomic test and set, which allows a
cluster filesystem to scale across multiple clients by locking individual
regions, without having to obtain a traditional SCSI reservation for
exclusive access to the entire logical unit.

This implemenation is currently limited to a single number of logical
block (NoLB).

It's also currently lacking the necessary sychronization between I/O
submission of COMPARE_AND_WRITE verify instance and write instance
user data, which is still being worked on in order to avoid additional
overhead in the main I/O fast path.

Please review as v3.12 material.

Thanks!

--nab

Nicholas Bellinger (9):
  scsi: Add CDB definition for COMPARE_AND_WRITE
  target: Add return for se_cmd->transport_complete_callback
  target: Add memory allocation for bidirectional commands
  target: Add TCM_MISCOMPARE_VERIFY sense handling
  target: Skip ->queue_data_in() callbacks for COMPARE_AND_WRITE
  target: Allow sbc_ops->execute_rw() to accept SGLs + data_direction
  target: Add transport_reset_sgl_orig() for COMPARE_AND_WRITE
  target: Add support for COMPARE_AND_WRITE emulation
  tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |    9 +-
 drivers/target/target_core_file.c      |    6 +-
 drivers/target/target_core_iblock.c    |    6 +-
 drivers/target/target_core_rd.c        |    6 +-
 drivers/target/target_core_sbc.c       |  205 +++++++++++++++++++++++++++++---
 drivers/target/target_core_transport.c |  109 ++++++++++++++++-
 include/scsi/scsi.h                    |    1 +
 include/target/target_core_backend.h   |    3 +-
 include/target/target_core_base.h      |    9 +-
 9 files changed, 317 insertions(+), 37 deletions(-)

-- 
1.7.10.4

[PATCH 1/9] scsi: Add CDB definition for COMPARE_AND_WRITE

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 include/scsi/scsi.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 4b87d99..6268062 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -144,6 +144,7 @@ enum scsi_timeouts {
 #define ACCESS_CONTROL_IN     0x86
 #define ACCESS_CONTROL_OUT    0x87
 #define READ_16               0x88
+#define COMPARE_AND_WRITE     0x89
 #define WRITE_16              0x8a
 #define READ_ATTRIBUTE        0x8c
 #define WRITE_ATTRIBUTE	      0x8d
-- 
1.7.10.4

Re: [PATCH 1/9] scsi: Add CDB definition for COMPARE_AND_WRITE

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/9] target: Add return for se_cmd->transport_complete_callback

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

This patch adds a sense_reason_t return to ->transport_complete_callback(),
and updates target_complete_ok_work() to invoke the call if necessary to
transport_send_check_condition_and_sense() during the failure case.

Also update xdreadwrite_callback() to use this return value.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_sbc.c       |   13 ++++++++-----
 drivers/target/target_core_transport.c |   20 +++++++++++++++++---
 include/target/target_core_base.h      |    2 +-
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 8a46277..be5234a 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -280,13 +280,13 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char *flags, struct sbc_ops *o
 	return 0;
 }
 
-static void xdreadwrite_callback(struct se_cmd *cmd)
+static sense_reason_t xdreadwrite_callback(struct se_cmd *cmd)
 {
 	unsigned char *buf, *addr;
 	struct scatterlist *sg;
 	unsigned int offset;
-	int i;
-	int count;
+	sense_reason_t ret = TCM_NO_SENSE;
+	int i, count;
 	/*
 	 * From sbc3r22.pdf section 5.48 XDWRITEREAD (10) command
 	 *
@@ -301,7 +301,7 @@ static void xdreadwrite_callback(struct se_cmd *cmd)
 	buf = kmalloc(cmd->data_length, GFP_KERNEL);
 	if (!buf) {
 		pr_err("Unable to allocate xor_callback buf\n");
-		return;
+		return TCM_OUT_OF_RESOURCES;
 	}
 	/*
 	 * Copy the scatterlist WRITE buffer located at cmd->t_data_sg
@@ -320,8 +320,10 @@ static void xdreadwrite_callback(struct se_cmd *cmd)
 	offset = 0;
 	for_each_sg(cmd->t_bidi_data_sg, sg, cmd->t_bidi_data_nents, count) {
 		addr = kmap_atomic(sg_page(sg));
-		if (!addr)
+		if (!addr) {
+			ret = TCM_OUT_OF_RESOURCES;
 			goto out;
+		}
 
 		for (i = 0; i < sg->length; i++)
 			*(addr + sg->offset + i) ^= *(buf + offset + i);
@@ -332,6 +334,7 @@ static void xdreadwrite_callback(struct se_cmd *cmd)
 
 out:
 	kfree(buf);
+	return ret;
 }
 
 sense_reason_t
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 98ec711..53d1d75 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1904,10 +1904,24 @@ static void target_complete_ok_work(struct work_struct *work)
 	}
 	/*
 	 * Check for a callback, used by amongst other things
-	 * XDWRITE_READ_10 emulation.
+	 * XDWRITE_READ_10 and COMPARE_AND_WRITE emulation.
 	 */
-	if (cmd->transport_complete_callback)
-		cmd->transport_complete_callback(cmd);
+	if (cmd->transport_complete_callback) {
+		sense_reason_t rc;
+
+		rc = cmd->transport_complete_callback(cmd);
+		if (!rc)
+			return;
+
+		ret = transport_send_check_condition_and_sense(cmd,
+					rc, 0);
+		if (ret == -EAGAIN || ret == -ENOMEM)
+			goto queue_full;
+
+		transport_lun_remove_cmd(cmd);
+		transport_cmd_check_stop_to_fabric(cmd);
+		return;
+	}
 
 	switch (cmd->data_direction) {
 	case DMA_FROM_DEVICE:
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 360e4a3..6e946f3 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -447,7 +447,7 @@ struct se_cmd {
 	struct kref		cmd_kref;
 	struct target_core_fabric_ops *se_tfo;
 	sense_reason_t		(*execute_cmd)(struct se_cmd *);
-	void (*transport_complete_callback)(struct se_cmd *);
+	sense_reason_t (*transport_complete_callback)(struct se_cmd *);
 
 	unsigned char		*t_task_cdb;
 	unsigned char		__t_task_cdb[TCM_MAX_COMMAND_SIZE];
-- 
1.7.10.4

[PATCH 3/9] target: Add memory allocation for bidirectional commands

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

This adds transport_generic_get_mem_bidi() to perform scatterlist
allocation for bidirectional commands.

Also, update transport_generic_new_cmd() to call this new function
when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC has not been set.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_transport.c |   53 ++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 53d1d75..5746d85 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2092,6 +2092,53 @@ void transport_kunmap_data_sg(struct se_cmd *cmd)
 EXPORT_SYMBOL(transport_kunmap_data_sg);
 
 static int
+transport_generic_get_mem_bidi(struct se_cmd *cmd)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct page *page;
+	gfp_t zero_flag;
+	u32 length;
+	unsigned int nents;
+	int i = 0;
+
+	if (cmd->t_task_cdb[0] == COMPARE_AND_WRITE)
+		length = cmd->t_task_nolb * dev->dev_attrib.block_size;
+	else
+		length = cmd->data_length;
+
+	nents = DIV_ROUND_UP(length, PAGE_SIZE);
+	cmd->t_bidi_data_sg = kmalloc(sizeof(struct scatterlist) * nents, GFP_KERNEL);
+	if (!cmd->t_bidi_data_sg)
+		return -ENOMEM;
+
+	cmd->t_bidi_data_nents = nents;
+	sg_init_table(cmd->t_bidi_data_sg, nents);
+
+	zero_flag = cmd->se_cmd_flags & SCF_SCSI_DATA_CDB ? 0 : __GFP_ZERO;
+
+	while (length) {
+		u32 page_len = min_t(u32, length, PAGE_SIZE);
+		page = alloc_page(GFP_KERNEL | zero_flag);
+		if (!page)
+			goto out;
+
+		sg_set_page(&cmd->t_bidi_data_sg[i], page, page_len, 0);
+		length -= page_len;
+		i++;
+	}
+	return 0;
+
+out:
+	while (i > 0) {
+		i--;
+		__free_page(sg_page(&cmd->t_bidi_data_sg[i]));
+	}
+	kfree(cmd->t_bidi_data_sg);
+	cmd->t_bidi_data_sg = NULL;
+	return -ENOMEM;
+}
+
+static int
 transport_generic_get_mem(struct se_cmd *cmd)
 {
 	u32 length = cmd->data_length;
@@ -2149,6 +2196,12 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 	 */
 	if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) &&
 	    cmd->data_length) {
+		if (cmd->se_cmd_flags & SCF_BIDI) {
+			ret = transport_generic_get_mem_bidi(cmd);
+			if (ret < 0)
+				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		}
+
 		ret = transport_generic_get_mem(cmd);
 		if (ret < 0)
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-- 
1.7.10.4

[PATCH 4/9] target: Add TCM_MISCOMPARE_VERIFY sense handling

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

This patch adds TCM_MISCOMPARE_VERIFY (ASC=0x1d, ASCQ=0x00) sense
handling to transport_send_check_condition_and_sense(), which is
required for a COMPARE_AND_WRITE comparision failure.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_transport.c |    9 +++++++++
 include/target/target_core_base.h      |    1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 5746d85..f7dc479 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2844,6 +2844,15 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
 		buffer[SPC_ASC_KEY_OFFSET] = asc;
 		buffer[SPC_ASCQ_KEY_OFFSET] = ascq;
 		break;
+	case TCM_MISCOMPARE_VERIFY:
+		/* CURRENT ERROR */
+		buffer[0] = 0x70;
+		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
+		buffer[SPC_SENSE_KEY_OFFSET] = MISCOMPARE;
+		/* MISCOMPARE DURING VERIFY OPERATION */
+		buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
+		buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
+		break;
 	case TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE:
 	default:
 		/* CURRENT ERROR */
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 6e946f3..fac25c5 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -197,6 +197,7 @@ enum tcm_sense_reason_table {
 	TCM_ADDRESS_OUT_OF_RANGE		= R(0x11),
 	TCM_OUT_OF_RESOURCES			= R(0x12),
 	TCM_PARAMETER_LIST_LENGTH_ERROR		= R(0x13),
+	TCM_MISCOMPARE_VERIFY			= R(0x14),
 #undef R
 };
 
-- 
1.7.10.4

[PATCH 5/9] target: Skip ->queue_data_in() callbacks for COMPARE_AND_WRITE

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

COMPARE_AND_WRITE uses cmd->t_bidi_data_sg for it's READ payload
and cmd->data_direction == DMA_TO_DEVICE, but the payload is
only used for internal comparision, and never actually sent over
the wire.

So, check for this special case in to avoid TFO->queue_data_in()
within transport_complete_qf() + target_complete_ok_work() code.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_transport.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f7dc479..60d1336 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1832,7 +1832,8 @@ static void transport_complete_qf(struct se_cmd *cmd)
 		ret = cmd->se_tfo->queue_data_in(cmd);
 		break;
 	case DMA_TO_DEVICE:
-		if (cmd->t_bidi_data_sg) {
+		if (cmd->t_bidi_data_sg &&
+		    cmd->t_task_cdb[0] != COMPARE_AND_WRITE) {
 			ret = cmd->se_tfo->queue_data_in(cmd);
 			if (ret < 0)
 				break;
@@ -1947,7 +1948,8 @@ static void target_complete_ok_work(struct work_struct *work)
 		/*
 		 * Check if we need to send READ payload for BIDI-COMMAND
 		 */
-		if (cmd->t_bidi_data_sg) {
+		if (cmd->t_bidi_data_sg &&
+		    cmd->t_task_cdb[0] != COMPARE_AND_WRITE) {
 			spin_lock(&cmd->se_lun->lun_sep_lock);
 			if (cmd->se_lun->lun_sep) {
 				cmd->se_lun->lun_sep->sep_stats.tx_data_octets +=
-- 
1.7.10.4

Re: [PATCH 5/9] target: Skip ->queue_data_in() callbacks for COMPARE_AND_WRITE

[Christoph Hellwig]

> @@ -1832,7 +1832,8 @@ static void transport_complete_qf(struct se_cmd *cmd)
>  		ret = cmd->se_tfo->queue_data_in(cmd);
>  		break;
>  	case DMA_TO_DEVICE:
> -		if (cmd->t_bidi_data_sg) {
> +		if (cmd->t_bidi_data_sg &&
> +		    cmd->t_task_cdb[0] != COMPARE_AND_WRITE) {

This is not the place to hardcode specific cdb opcodes.  Should be
a flag with a defined meaning on the command.

Re: [PATCH 5/9] target: Skip ->queue_data_in() callbacks for COMPARE_AND_WRITE

[Nicholas A. Bellinger]

On Tue, 2013-08-20 at 23:32 -0700, Christoph Hellwig wrote:
> > @@ -1832,7 +1832,8 @@ static void transport_complete_qf(struct se_cmd *cmd)
> >  		ret = cmd->se_tfo->queue_data_in(cmd);
> >  		break;
> >  	case DMA_TO_DEVICE:
> > -		if (cmd->t_bidi_data_sg) {
> > +		if (cmd->t_bidi_data_sg &&
> > +		    cmd->t_task_cdb[0] != COMPARE_AND_WRITE) {
> 
> This is not the place to hardcode specific cdb opcodes.  Should be
> a flag with a defined meaning on the command.
> 

Since this code path is only ever reached after a successful comparison
+ submission of the subsequent write payload, this is fine to change to
use SCF_COMPARE_AND_WRITE_POST flag..

Folding in the following patch.

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 60d1336..70a6adb 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1833,7 +1833,7 @@ static void transport_complete_qf(struct se_cmd *cmd)
                break;
        case DMA_TO_DEVICE:
                if (cmd->t_bidi_data_sg &&
-                   cmd->t_task_cdb[0] != COMPARE_AND_WRITE) {
+                   !(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE_POST)) {
                        ret = cmd->se_tfo->queue_data_in(cmd);
                        if (ret < 0)
                                break;
@@ -1949,7 +1949,7 @@ static void target_complete_ok_work(struct work_struct *work)
                 * Check if we need to send READ payload for BIDI-COMMAND
                 */
                if (cmd->t_bidi_data_sg &&
-                   cmd->t_task_cdb[0] != COMPARE_AND_WRITE) {
+                   !(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE_POST)) {
                        spin_lock(&cmd->se_lun->lun_sep_lock);
                        if (cmd->se_lun->lun_sep) {
                                cmd->se_lun->lun_sep->sep_stats.tx_data_octets +=

[PATCH 6/9] target: Allow sbc_ops->execute_rw() to accept SGLs + data_direction

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

COMPARE_AND_WRITE expects to be able to send down a DMA_FROM_DEVICE
to obtain the necessary READ payload for comparision against the
first half of the WRITE payload containing the verify user data.

Currently virtual backends expect to internally reference SGLs,
SGL nents, and data_direction, so change IBLOCK, FILEIO and RD
sbc_ops->execute_rw() to accept this values as function parameters.

Also add the sbc_execute_rw() wrapper to handle the special case
for the initial COMPARE_AND_WRITE DMA_FROM_DEVICE -> READ I/O
submission.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_file.c    |    6 ++--
 drivers/target/target_core_iblock.c  |    6 ++--
 drivers/target/target_core_rd.c      |    6 ++--
 drivers/target/target_core_sbc.c     |   53 +++++++++++++++++++++++++++-------
 include/target/target_core_backend.h |    3 +-
 include/target/target_core_base.h    |    3 ++
 6 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index bc3245d..c5448a5 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -547,11 +547,9 @@ fd_execute_unmap(struct se_cmd *cmd)
 }
 
 static sense_reason_t
-fd_execute_rw(struct se_cmd *cmd)
+fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
+	      enum dma_data_direction data_direction)
 {
-	struct scatterlist *sgl = cmd->t_data_sg;
-	u32 sgl_nents = cmd->t_data_nents;
-	enum dma_data_direction data_direction = cmd->data_direction;
 	struct se_device *dev = cmd->se_dev;
 	int ret = 0;
 
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 0a460f3..81464eb 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -587,11 +587,9 @@ static ssize_t iblock_show_configfs_dev_params(struct se_device *dev, char *b)
 }
 
 static sense_reason_t
-iblock_execute_rw(struct se_cmd *cmd)
+iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
+		  enum dma_data_direction data_direction)
 {
-	struct scatterlist *sgl = cmd->t_data_sg;
-	u32 sgl_nents = cmd->t_data_nents;
-	enum dma_data_direction data_direction = cmd->data_direction;
 	struct se_device *dev = cmd->se_dev;
 	struct iblock_req *ibr;
 	struct bio *bio;
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 51127d1..958d17ad 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -280,11 +280,9 @@ static struct rd_dev_sg_table *rd_get_sg_table(struct rd_dev *rd_dev, u32 page)
 }
 
 static sense_reason_t
-rd_execute_rw(struct se_cmd *cmd)
+rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
+	      enum dma_data_direction data_direction)
 {
-	struct scatterlist *sgl = cmd->t_data_sg;
-	u32 sgl_nents = cmd->t_data_nents;
-	enum dma_data_direction data_direction = cmd->data_direction;
 	struct se_device *se_dev = cmd->se_dev;
 	struct rd_dev *dev = RD_DEV(se_dev);
 	struct rd_dev_sg_table *table;
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index be5234a..e98581a 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -337,6 +337,29 @@ out:
 	return ret;
 }
 
+static sense_reason_t
+sbc_execute_rw(struct se_cmd *cmd)
+{
+	struct scatterlist *sgl;
+	u32 sgl_nents;
+	enum dma_data_direction data_direction;
+	/*
+	 * Submit READ first for COMPARE_AND_WRITE..
+	 */
+	if (cmd->t_task_cdb[0] == COMPARE_AND_WRITE &&
+	    !(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE_POST)) {
+		sgl = cmd->t_bidi_data_sg;
+		sgl_nents = cmd->t_bidi_data_nents;
+		data_direction = DMA_FROM_DEVICE;
+	} else {
+		sgl = cmd->t_data_sg;
+		sgl_nents = cmd->t_data_nents;
+		data_direction = cmd->data_direction;
+	}
+
+	return cmd->execute_rw(cmd, sgl, sgl_nents, data_direction);
+}
+
 sense_reason_t
 sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 {
@@ -351,31 +374,36 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		sectors = transport_get_sectors_6(cdb);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case READ_10:
 		sectors = transport_get_sectors_10(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case READ_12:
 		sectors = transport_get_sectors_12(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case READ_16:
 		sectors = transport_get_sectors_16(cdb);
 		cmd->t_task_lba = transport_lba_64(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_6:
 		sectors = transport_get_sectors_6(cdb);
 		cmd->t_task_lba = transport_lba_21(cdb);
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_10:
 	case WRITE_VERIFY:
@@ -384,7 +412,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		if (cdb[1] & 0x8)
 			cmd->se_cmd_flags |= SCF_FUA;
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_12:
 		sectors = transport_get_sectors_12(cdb);
@@ -392,7 +421,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		if (cdb[1] & 0x8)
 			cmd->se_cmd_flags |= SCF_FUA;
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_16:
 		sectors = transport_get_sectors_16(cdb);
@@ -400,7 +430,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		if (cdb[1] & 0x8)
 			cmd->se_cmd_flags |= SCF_FUA;
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case XDWRITEREAD_10:
 		if (cmd->data_direction != DMA_TO_DEVICE ||
@@ -414,7 +445,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		/*
 		 * Setup BIDI XOR callback to be run after I/O completion.
 		 */
-		cmd->execute_cmd = ops->execute_rw;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
 		cmd->transport_complete_callback = &xdreadwrite_callback;
 		if (cdb[1] & 0x8)
 			cmd->se_cmd_flags |= SCF_FUA;
@@ -437,7 +469,8 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			 * Setup BIDI XOR callback to be run during after I/O
 			 * completion.
 			 */
-			cmd->execute_cmd = ops->execute_rw;
+			cmd->execute_rw = ops->execute_rw;
+			cmd->execute_cmd = sbc_execute_rw;
 			cmd->transport_complete_callback = &xdreadwrite_callback;
 			if (cdb[1] & 0x8)
 				cmd->se_cmd_flags |= SCF_FUA;
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index ffa2696..77f25e0 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -39,7 +39,8 @@ struct se_subsystem_api {
 };
 
 struct sbc_ops {
-	sense_reason_t (*execute_rw)(struct se_cmd *cmd);
+	sense_reason_t (*execute_rw)(struct se_cmd *cmd, struct scatterlist *,
+				     u32, enum dma_data_direction);
 	sense_reason_t (*execute_sync_cache)(struct se_cmd *cmd);
 	sense_reason_t (*execute_write_same)(struct se_cmd *cmd);
 	sense_reason_t (*execute_write_same_unmap)(struct se_cmd *cmd);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index fac25c5..e1e0843 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -159,6 +159,7 @@ enum se_cmd_flags_table {
 	SCF_ALUA_NON_OPTIMIZED		= 0x00008000,
 	SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000,
 	SCF_ACK_KREF			= 0x00040000,
+	SCF_COMPARE_AND_WRITE_POST	= 0x00080000,
 };
 
 /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
@@ -448,6 +449,8 @@ struct se_cmd {
 	struct kref		cmd_kref;
 	struct target_core_fabric_ops *se_tfo;
 	sense_reason_t		(*execute_cmd)(struct se_cmd *);
+	sense_reason_t		(*execute_rw)(struct se_cmd *, struct scatterlist *,
+					      u32, enum dma_data_direction);
 	sense_reason_t (*transport_complete_callback)(struct se_cmd *);
 
 	unsigned char		*t_task_cdb;
-- 
1.7.10.4

Re: [PATCH 6/9] target: Allow sbc_ops->execute_rw() to accept SGLs + data_direction

[Christoph Hellwig]

On Tue, Aug 20, 2013 at 08:07:57PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@daterainc.com>
> 
> COMPARE_AND_WRITE expects to be able to send down a DMA_FROM_DEVICE
> to obtain the necessary READ payload for comparision against the
> first half of the WRITE payload containing the verify user data.
> 
> Currently virtual backends expect to internally reference SGLs,
> SGL nents, and data_direction, so change IBLOCK, FILEIO and RD
> sbc_ops->execute_rw() to accept this values as function parameters.
> 
> Also add the sbc_execute_rw() wrapper to handle the special case
> for the initial COMPARE_AND_WRITE DMA_FROM_DEVICE -> READ I/O
> submission.

I don't like the way this is structured with the new method.  It seems
like we should just pass the sgl and associated data to execute_cmd
and have the read vs write logic driven by command code, using generic
flags instead of specificly checking for compare and write.

Re: [PATCH 6/9] target: Allow sbc_ops->execute_rw() to accept SGLs + data_direction

[Nicholas A. Bellinger]

On Tue, 2013-08-20 at 23:35 -0700, Christoph Hellwig wrote:
> On Tue, Aug 20, 2013 at 08:07:57PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@daterainc.com>
> > 
> > COMPARE_AND_WRITE expects to be able to send down a DMA_FROM_DEVICE
> > to obtain the necessary READ payload for comparision against the
> > first half of the WRITE payload containing the verify user data.
> > 
> > Currently virtual backends expect to internally reference SGLs,
> > SGL nents, and data_direction, so change IBLOCK, FILEIO and RD
> > sbc_ops->execute_rw() to accept this values as function parameters.
> > 
> > Also add the sbc_execute_rw() wrapper to handle the special case
> > for the initial COMPARE_AND_WRITE DMA_FROM_DEVICE -> READ I/O
> > submission.
> 
> I don't like the way this is structured with the new method.  It seems
> like we should just pass the sgl and associated data to execute_cmd
> and have the read vs write logic driven by command code, using generic
> flags instead of specificly checking for compare and write.

I considered that approach as well, but in the end all of the non
sbc_ops->execute_rw() users of se_cmd->execute_cmd() will never make use
of a passed *sgl, sgl_nents, or data_direction that is different than
se_cmd assignments.

So in the end, the approach of changing all se_cmd->execute_cmd() users
to accommodate COMPARE_AND_WRITE did not end up making sense outside of
this particular special case..

--nab

[PATCH 7/9] target: Add transport_reset_sgl_orig() for COMPARE_AND_WRITE

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

After COMPARE_AND_WRITE completes it's comparision, the WRITE
payload SGLs head expect to be updated to point from the verify
instance of user data, to the write instance of user data.

So for this special case, add transport_reset_sgl_orig() usage
within transport_free_pages() and add se_cmd->t_data_[sg,nents]_orig
members to save the original assignments.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_transport.c |   21 ++++++++++++++++++++-
 include/target/target_core_base.h      |    2 ++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 60d1336..bb98684 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1994,10 +1994,29 @@ static inline void transport_free_sgl(struct scatterlist *sgl, int nents)
 	kfree(sgl);
 }
 
+static inline void transport_reset_sgl_orig(struct se_cmd *cmd)
+{
+	/*
+	 * Check for saved t_data_sg that may be used for COMPARE_AND_WRITE
+	 * emulation, and free + reset pointers if necessary..
+	 */
+	if (!cmd->t_data_sg_orig)
+		return;
+
+	kfree(cmd->t_data_sg);
+	cmd->t_data_sg = cmd->t_data_sg_orig;
+	cmd->t_data_sg_orig = NULL;
+	cmd->t_data_nents = cmd->t_data_nents_orig;
+	cmd->t_data_nents_orig = 0;
+}
+
 static inline void transport_free_pages(struct se_cmd *cmd)
 {
-	if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)
+	if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) {
+		transport_reset_sgl_orig(cmd);
 		return;
+	}
+	transport_reset_sgl_orig(cmd);
 
 	transport_free_sgl(cmd->t_data_sg, cmd->t_data_nents);
 	cmd->t_data_sg = NULL;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index e1e0843..2f9a438 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -476,7 +476,9 @@ struct se_cmd {
 	struct work_struct	work;
 
 	struct scatterlist	*t_data_sg;
+	struct scatterlist	*t_data_sg_orig;
 	unsigned int		t_data_nents;
+	unsigned int		t_data_nents_orig;
 	void			*t_data_vmap;
 	struct scatterlist	*t_bidi_data_sg;
 	unsigned int		t_bidi_data_nents;
-- 
1.7.10.4

[PATCH 8/9] target: Add support for COMPARE_AND_WRITE emulation

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

This patch adds support for COMPARE_AND_WRITE emulation on a per block
basis.  This logic is used as an atomic test and set primative currently
used by VMWare ESX VAAI for performing array side locking of individual
VMFS extent ownership.

This includes the COMPARE_AND_WRITE CDB parsing within sbc_parse_cdb(),
and does the majority of the work within the compare_and_write_callback()
to perform the verify instance user data comparision, and subsequent
write instance user data I/O submission upon a successfull comparision.

The implementation currently assumes a single logical block (NoLB=1).

FIXME: Determine sychronization necessary between I/O submission path
and compare_and_write_callback() I/O submission path.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_sbc.c  |  139 +++++++++++++++++++++++++++++++++++++
 include/target/target_core_base.h |    1 +
 2 files changed, 140 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index e98581a..1370efc 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -25,6 +25,7 @@
 #include <linux/ratelimit.h>
 #include <asm/unaligned.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_tcq.h>
 
 #include <target/target_core_base.h>
 #include <target/target_core_backend.h>
@@ -337,6 +338,122 @@ out:
 	return ret;
 }
 
+static sense_reason_t compare_and_write_done(struct se_cmd *cmd)
+{
+	return TCM_NO_SENSE;
+}
+
+static sense_reason_t compare_and_write_callback(struct se_cmd *cmd)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct scatterlist *write_sg = NULL, *sg;
+	unsigned char *buf, *addr;
+	struct sg_mapping_iter m;
+	unsigned int offset = 0, len;
+	unsigned int nlbas = cmd->t_task_nolb;
+	unsigned int block_size = dev->dev_attrib.block_size;
+	unsigned int compare_len = (nlbas * block_size);
+	sense_reason_t ret = TCM_NO_SENSE;
+	int rc, i;
+
+	buf = kzalloc(cmd->data_length, GFP_KERNEL);
+	if (!buf) {
+		pr_err("Unable to allocate compare_and_write buf\n");
+		return TCM_OUT_OF_RESOURCES;
+	}
+
+	write_sg = kzalloc(sizeof(struct scatterlist) * cmd->t_data_nents,
+			   GFP_KERNEL);
+	if (!write_sg) {
+		pr_err("Unable to allocate compare_and_write sg\n");
+		ret = TCM_OUT_OF_RESOURCES;
+		goto out;
+	}
+	/*
+	 * Setup verify and write data payloads from total NumberLBAs.
+	 */
+	rc = sg_copy_to_buffer(cmd->t_data_sg, cmd->t_data_nents, buf,
+			       cmd->data_length);
+	if (!rc) {
+		pr_err("sg_copy_to_buffer() failed for compare_and_write\n");
+		ret = TCM_OUT_OF_RESOURCES;
+		goto out;
+	}
+	/*
+	 * Compare against SCSI READ payload against verify payload
+	 */
+	for_each_sg(cmd->t_bidi_data_sg, sg, cmd->t_bidi_data_nents, i) {
+		addr = (unsigned char *)kmap_atomic(sg_page(sg));
+		if (!addr) {
+			ret = TCM_OUT_OF_RESOURCES;
+			goto out;
+		}
+
+		len = min(sg->length, compare_len);
+
+		if (memcmp(addr, buf + offset, len)) {
+			pr_warn("Detected MISCOMPARE for addr: %p buf: %p\n",
+				addr, buf + offset);
+			kunmap_atomic(addr);
+			goto miscompare;
+		}
+		kunmap_atomic(addr);
+
+		offset += len;
+		compare_len -= len;
+		if (!compare_len)
+			break;
+	}
+
+	i = 0;
+	len = cmd->t_task_nolb * block_size;
+	sg_miter_start(&m, cmd->t_data_sg, cmd->t_data_nents, SG_MITER_TO_SG);
+	/*
+	 * Currently assumes NoLB=1 and SGLs are PAGE_SIZE..
+	 */
+	while (len) {
+		sg_miter_next(&m);
+
+		if (block_size < PAGE_SIZE) {
+			sg_set_page(&write_sg[i], m.page, block_size,
+				    block_size);
+		} else {
+			sg_miter_next(&m);
+			sg_set_page(&write_sg[i], m.page, block_size,
+				    0);
+		}
+	}
+	sg_miter_stop(&m);
+	/*
+	 * Save the original SGL + nents values before updating to new
+	 * assignments, to be released in transport_free_pages() ->
+	 * transport_reset_sgl_orig()
+	 */
+	cmd->t_data_sg_orig = cmd->t_data_sg;
+	cmd->t_data_sg = write_sg;
+	cmd->t_data_nents_orig = cmd->t_data_nents;
+	cmd->t_data_nents = 1;
+
+	cmd->sam_task_attr = MSG_HEAD_TAG;
+	cmd->se_cmd_flags |= SCF_COMPARE_AND_WRITE_POST;
+	cmd->transport_complete_callback = compare_and_write_done;
+
+#warning FIXME: Replace with __target_execute_cmd()..?
+	target_execute_cmd(cmd);
+
+	kfree(buf);
+	return ret;
+
+miscompare:
+	pr_warn("Target/%s: Send MISCOMPARE check condition and sense\n",
+		dev->transport->name);
+	ret = TCM_MISCOMPARE_VERIFY;
+out:
+	kfree(write_sg);
+	kfree(buf);
+	return ret;
+}
+
 static sense_reason_t
 sbc_execute_rw(struct se_cmd *cmd)
 {
@@ -497,6 +614,28 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		}
 		break;
 	}
+	case COMPARE_AND_WRITE:
+		sectors = cdb[13];
+		/*
+		 * Currently enforce COMPARE_AND_WRITE for a single sector
+		 */
+		if (sectors > 1) {
+			pr_err("COMPARE_AND_WRITE contains NoLB: %u greater"
+			       " than 1\n", sectors);
+			return TCM_INVALID_CDB_FIELD;
+		}
+		/*
+		 * Double size because we have two buffers, note that
+		 * zero is not an error..
+		 */
+		size = 2 * sbc_get_size(cmd, sectors);
+		cmd->t_task_lba = get_unaligned_be64(&cdb[2]);
+		cmd->t_task_nolb = sectors;
+		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB | SCF_BIDI;
+		cmd->execute_rw = ops->execute_rw;
+		cmd->execute_cmd = sbc_execute_rw;
+		cmd->transport_complete_callback = compare_and_write_callback;
+		break;
 	case READ_CAPACITY:
 		size = READ_CAP_LEN;
 		cmd->execute_cmd = sbc_emulate_readcapacity;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 2f9a438..c7bf1e3 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -456,6 +456,7 @@ struct se_cmd {
 	unsigned char		*t_task_cdb;
 	unsigned char		__t_task_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned long long	t_task_lba;
+	unsigned int		t_task_nolb;
 	unsigned int		transport_state;
 #define CMD_T_ABORTED		(1 << 0)
 #define CMD_T_ACTIVE		(1 << 1)
-- 
1.7.10.4

Re: [PATCH 8/9] target: Add support for COMPARE_AND_WRITE emulation

[Christoph Hellwig]

I don't like the layering here.  The re-execution of the same command
for both reading and writing the data from/to the backend device already
looks sketchy here due to doubling work of task attribute handling, the
various state bits, etc.  And it will only get more complicated when
the required locking is added.  In addition we have all that confusion
about overloading the data direction.

I think the way this should be handled is:


 - cmd->execute_cmd gets set to a new sbc_emulate_compare_and_write
 - sbc_emulate_compare_and_write does all the setup for the locking,
   sets up the read buffer, then calls ops->execute_rw to do the
   read.  The complete callback does the comparism, then calls
   ops->execute_rw to do the write, and the second time we hit
   the complete callback we teard down the read buffer, stop the
   locking, etc.

This also avoids bloating the command with another function pointer
or having to change all execute_cmd prototypes.

Re: [PATCH 8/9] target: Add support for COMPARE_AND_WRITE emulation

[Nicholas A. Bellinger]

On Wed, 2013-08-21 at 09:14 -0700, Christoph Hellwig wrote:
> I don't like the layering here.  The re-execution of the same command
> for both reading and writing the data from/to the backend device already
> looks sketchy here due to doubling work of task attribute handling, the
> various state bits, etc.  And it will only get more complicated when
> the required locking is added.  In addition we have all that confusion
> about overloading the data direction.
> 
> I think the way this should be handled is:
> 
> 
>  - cmd->execute_cmd gets set to a new sbc_emulate_compare_and_write
>  - sbc_emulate_compare_and_write does all the setup for the locking,
>    sets up the read buffer, then calls ops->execute_rw to do the
>    read.  The complete callback does the comparism, then calls
>    ops->execute_rw to do the write, and the second time we hit
>    the complete callback we teard down the read buffer, stop the
>    locking, etc.
> 

I do like this approach better, however calling ops->execute_rw() the
second time around does require at least the TRANSPORT_PROCESSING and
other transport_state bits from target_execute_cmd() to be set for
things to work correctly.  Bypassing the aborted + CMD_*STOP checks
should be OK for the write submission, and will kick (if necessary)
during the final completion.

Setting up the read buffer from sbc_emulate_compare_and_write() would
require two extra COMPARE_AND_WRITE specific se_cmd elements, so I'm
tempted to continue to use the bidi fields for this (because they
already exist) with transport_generic_get_mem_bidi(), and use a
SCF_COMPARE_AND_WRITE flag to avoid any CDB specific checks in
transport_complete_ok(), and reverse dma direction mapping case.

> This also avoids bloating the command with another function pointer
> or having to change all execute_cmd prototypes.

Indeed.

--nab

[PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

Add a special case for COMPARE_AND_WRITE for the reverse data direction
mapping used for pci_map_sg() + friends.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Chris Mason <chris.mason@fusionio.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 6a93a91..4e20d51 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -508,8 +508,13 @@ static u32 tcm_qla2xxx_sess_get_index(struct se_session *se_sess)
  */
 static enum dma_data_direction tcm_qla2xxx_mapping_dir(struct se_cmd *se_cmd)
 {
-	if (se_cmd->se_cmd_flags & SCF_BIDI)
-		return DMA_BIDIRECTIONAL;
+	if (se_cmd->se_cmd_flags & SCF_BIDI) {
+		/*
+		 * Special fall-through case for COMPARE_AND_WRITE
+		 */
+		if (se_cmd->t_task_cdb[0] != COMPARE_AND_WRITE)
+			return DMA_BIDIRECTIONAL;
+	}
 
 	switch (se_cmd->data_direction) {
 	case DMA_TO_DEVICE:
-- 
1.7.10.4

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Christoph Hellwig]

On Tue, Aug 20, 2013 at 08:08:00PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@daterainc.com>
> 
> Add a special case for COMPARE_AND_WRITE for the reverse data direction
> mapping used for pci_map_sg() + friends.

A low level driver is an even worse place to hardcode a specific cdb
opcode.  As written before this should be done by a flag on the command.

Also it might make sense to lift this helper to get a dma direction from
a command into common code.

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Nicholas A. Bellinger]

On Tue, 2013-08-20 at 23:37 -0700, Christoph Hellwig wrote:
> On Tue, Aug 20, 2013 at 08:08:00PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@daterainc.com>
> > 
> > Add a special case for COMPARE_AND_WRITE for the reverse data direction
> > mapping used for pci_map_sg() + friends.
> 
> A low level driver is an even worse place to hardcode a specific cdb
> opcode.  As written before this should be done by a flag on the command.
> 

Which means adding another COMPARE_AND_WRITE specific flag to
se_cmd_flags_Table, as the SCF_COMPARE_AND_WRITE_POST is ony set after
the comparsion of the verify instance data is complete..

Is it really worth having two se_cmd_flags for COMPARE_AND_WRITE..?

> Also it might make sense to lift this helper to get a dma direction from
> a command into common code.
> 

Mmm, perhaps.  I don't recall of the top of my head why tcm_qla2xxx
actually needed to reverse it's dma direction (I'm sure that Roland
knows, CC'ed), but IIRC it was a tcm_qla2xxx specific thing..?

--nab

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Christoph Hellwig]

On Wed, Aug 21, 2013 at 12:31:07AM -0700, Nicholas A. Bellinger wrote:
> Is it really worth having two se_cmd_flags for COMPARE_AND_WRITE..?

Not leaking the abstraction into the driver is always worth the effort.

But looking at the other patches I haven't reviewed yet I think the
issue is more severe anyway, see my next reply.

> > Also it might make sense to lift this helper to get a dma direction from
> > a command into common code.
> > 
> 
> Mmm, perhaps.  I don't recall of the top of my head why tcm_qla2xxx
> actually needed to reverse it's dma direction (I'm sure that Roland
> knows, CC'ed), but IIRC it was a tcm_qla2xxx specific thing..?

It's the same issue for any hardware driver that directly maps a
se_cmd - the direction the target expects is reversed to what the
driver expects, in addition any BIDI or other special meanings will
need handling.

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Roland Dreier]

On Tue, Aug 20, 2013 at 1:08 PM, Nicholas A. Bellinger
<nab@daterainc.com> wrote:
> Add a special case for COMPARE_AND_WRITE for the reverse data direction
> mapping used for pci_map_sg() + friends.

I don't understand this.  In fact the whole patch series looks quite
confused.  COMPARE AND WRITE is a normal Data-Out command, with no
requirement for special bidirectional handling or anything like that.
The only slightly unusual thing is that a CAW command with a NUMBER OF
LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data --
one set of data for the compare operation and a second set to write if
the compare succeeds.  But just to be clear, the transfer of those 2*N
blocks happens as a single transfer during the Data-Out phase.

 - R.

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Christoph Hellwig]

On Wed, Aug 21, 2013 at 07:38:21AM -0700, Roland Dreier wrote:
> I don't understand this.  In fact the whole patch series looks quite
> confused.  COMPARE AND WRITE is a normal Data-Out command, with no
> requirement for special bidirectional handling or anything like that.
> The only slightly unusual thing is that a CAW command with a NUMBER OF
> LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data --
> one set of data for the compare operation and a second set to write if
> the compare succeeds.  But just to be clear, the transfer of those 2*N
> blocks happens as a single transfer during the Data-Out phase.

I think the confusion is that the implementation of COMPARE AND WRITE
obviously requires a read and a write phase, and the implementation
tries to mix this up with an actual bidirectional scsi command.

If the core stopped keying off t_bidi_data_sg and used better flag
this could be easily solved.

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Nicholas A. Bellinger]

On Wed, 2013-08-21 at 08:53 -0700, Christoph Hellwig wrote:
> On Wed, Aug 21, 2013 at 07:38:21AM -0700, Roland Dreier wrote:
> > I don't understand this.  In fact the whole patch series looks quite
> > confused.  COMPARE AND WRITE is a normal Data-Out command, with no
> > requirement for special bidirectional handling or anything like that.
> > The only slightly unusual thing is that a CAW command with a NUMBER OF
> > LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data --
> > one set of data for the compare operation and a second set to write if
> > the compare succeeds.  But just to be clear, the transfer of those 2*N
> > blocks happens as a single transfer during the Data-Out phase.
> 
> I think the confusion is that the implementation of COMPARE AND WRITE
> obviously requires a read and a write phase, and the implementation
> tries to mix this up with an actual bidirectional scsi command.
> 
> If the core stopped keying off t_bidi_data_sg and used better flag
> this could be easily solved.

Good point here as well..  Changing these cases to check for SCF_BIDI
instead, and adding a extra SCF_COMPARE_AND_WRITE check for the case in
transport_generic_new_cmd() to call transport_generic_get_mem_bidi().

--nab

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Roland Dreier]

On Wed, Aug 21, 2013 at 7:38 AM, Roland Dreier <roland@purestorage.com> wrote:
> I don't understand this.  In fact the whole patch series looks quite
> confused.  COMPARE AND WRITE is a normal Data-Out command, with no
> requirement for special bidirectional handling or anything like that.
> The only slightly unusual thing is that a CAW command with a NUMBER OF
> LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data --
> one set of data for the compare operation and a second set to write if
> the compare succeeds.  But just to be clear, the transfer of those 2*N
> blocks happens as a single transfer during the Data-Out phase.

OK, I understand the patch set a bit better.  You're using the bidi
infrastructure to have a place to stick the data that you internally
read to implement the compare, but then you end up having places like
this where you have to say, "oh it's not really a bidi command, it's
just a compare and write."

Shouldn't there be a way to confine the COMPARE AND WRITE handling to
the actual implementation of that command?  Or maybe make the bidi
handling more generic so that this becomes clearer?

 - R.

Re: [PATCH 0/9] target: Add support for COMPARE_AND_WRITE (VAAI) emulation

[Christoph Hellwig]

On Tue, Aug 20, 2013 at 08:07:51PM +0000, Nicholas A. Bellinger wrote:
> 
> It's also currently lacking the necessary sychronization between I/O
> submission of COMPARE_AND_WRITE verify instance and write instance
> user data, which is still being worked on in order to avoid additional
> overhead in the main I/O fast path.

I don't think merging such a non-conforming implementation makes any sense.

Also for a complex command like this with all it's race potential I would
really like to see some test cases to go along with it.

Re: [PATCH 0/9] target: Add support for COMPARE_AND_WRITE (VAAI) emulation

[Nicholas A. Bellinger]

On Tue, 2013-08-20 at 23:29 +0200, Christoph Hellwig wrote:
> On Tue, Aug 20, 2013 at 08:07:51PM +0000, Nicholas A. Bellinger wrote:
> > 
> > It's also currently lacking the necessary sychronization between I/O
> > submission of COMPARE_AND_WRITE verify instance and write instance
> > user data, which is still being worked on in order to avoid additional
> > overhead in the main I/O fast path.
> 
> I don't think merging such a non-conforming implementation makes any sense.
> 

Yes, I don't intend to merge anything that's not fully functional.

The idea was to get review going on these pieces first.  I'll be posting
an PATCH-v2 to complete the implementation over the next days.

> Also for a complex command like this with all it's race potential I would
> really like to see some test cases to go along with it.
> 

Yes, Eric @ PureStorage has a sg_compare_write that I'm using to test
this.  It's probably about time that this be included in upstream
sg3-utils too..

--nab

Re: [PATCH 0/9] target: Add support for COMPARE_AND_WRITE (VAAI) emulation

[Douglas Gilbert]

On 13-08-20 05:53 PM, Nicholas A. Bellinger wrote:
> On Tue, 2013-08-20 at 23:29 +0200, Christoph Hellwig wrote:
>> On Tue, Aug 20, 2013 at 08:07:51PM +0000, Nicholas A. Bellinger wrote:
>>>
>>> It's also currently lacking the necessary sychronization between I/O
>>> submission of COMPARE_AND_WRITE verify instance and write instance
>>> user data, which is still being worked on in order to avoid additional
>>> overhead in the main I/O fast path.
>>
>> I don't think merging such a non-conforming implementation makes any sense.
>>
>
> Yes, I don't intend to merge anything that's not fully functional.
>
> The idea was to get review going on these pieces first.  I'll be posting
> an PATCH-v2 to complete the implementation over the next days.
>
>> Also for a complex command like this with all it's race potential I would
>> really like to see some test cases to go along with it.
>>
>
> Yes, Eric @ PureStorage has a sg_compare_write that I'm using to test
> this.  It's probably about time that this be included in upstream
> sg3-utils too..

Changelog for sg3_utils-1.35 [20130117] [svn: r476]
   - sg_compare_and_write: new utility
...

So it has been released for 6 months. Also version 1.36
has been released since then so you might check more
often. Does Eric's version have any improvements over the
version already in sg3_utils? [Apart from a shorter name ...]

Doug Gilbert

Re: [PATCH 0/9] target: Add support for COMPARE_AND_WRITE (VAAI) emulation

[Nicholas A. Bellinger]

On Tue, 2013-08-20 at 18:01 -0400, Douglas Gilbert wrote:
> On 13-08-20 05:53 PM, Nicholas A. Bellinger wrote:
> > On Tue, 2013-08-20 at 23:29 +0200, Christoph Hellwig wrote:

<SNIP>

> >> Also for a complex command like this with all it's race potential I would
> >> really like to see some test cases to go along with it.
> >>
> >
> > Yes, Eric @ PureStorage has a sg_compare_write that I'm using to test
> > this.  It's probably about time that this be included in upstream
> > sg3-utils too..
> 
> Changelog for sg3_utils-1.35 [20130117] [svn: r476]
>    - sg_compare_and_write: new utility
> ...
> 
> So it has been released for 6 months. Also version 1.36
> has been released since then so you might check more
> often. Does Eric's version have any improvements over the
> version already in sg3_utils? [Apart from a shorter name ...]
> 

Mmm, that I'm not sure about..  Eric's version (CC'ed) is inline below.

--nab

Index: src/sg_compare_and_write.c
===================================================================
--- src/sg_compare_and_write.c	(revision 0)
+++ src/sg_compare_and_write.c	(revision 10195)
@@ -0,0 +1,560 @@
+/*
+ * Copyright (c) 2009-2010 Douglas Gilbert.
+ * Copyright (c) 2011 Pure Storage, Inc.
+ * All rights reserved.
+ * Use of this source code is governed by a BSD-style
+ * license that can be found in the BSD_LICENSE file.
+ */
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <limits.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <getopt.h>
+#define __STDC_FORMAT_MACROS 1
+#include <inttypes.h>
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+#include "sg_lib.h"
+#include "sg_pt.h"
+#include "sg_cmds_basic.h"
+#include "sg_cmds_extra.h"
+
+static char * version_str = "0.01 20110803";
+
+
+#define ME "sg_compare_and_write: "
+
+#define COMPARE_AND_WRITE_OP 0x89
+#define COMPARE_AND_WRITE_LEN 16
+#define RCAP10_RESP_LEN 8
+#define RCAP16_RESP_LEN 32
+#define SENSE_BUFF_LEN 32       /* Arbitrary, could be larger */
+#define DEF_TIMEOUT_SECS 60
+#define DEF_WS_NUMBLOCKS 1
+#define MAX_XFER_LEN (64 * 1024)
+#define EBUFF_SZ 256
+
+static struct option long_options[] = {
+    {"fua", no_argument, 0, 'f'},
+    {"grpnum", required_argument, 0, 'g'},
+    {"help", no_argument, 0, 'h'},
+    {"inc", required_argument, 0, 'C'},
+    {"inw", required_argument, 0, 'W'},
+    {"lba", required_argument, 0, 'l'},
+    {"num", required_argument, 0, 'n'},
+    {"timeout", required_argument, 0, 'r'},
+    {"verbose", no_argument, 0, 'v'},
+    {"version", no_argument, 0, 'V'},
+    {"wrprotect", required_argument, 0, 'w'},
+    {"xferlen", required_argument, 0, 'x'},
+    {0, 0, 0, 0},
+};
+
+struct opts_t {
+    int fua;
+    int grpnum;
+    char ifilenamec[256];
+    char ifilenamew[256];
+    uint64_t lba;
+    int numblocks;
+    int timeout;
+    int verbose;
+    int wrprotect;
+    int xfer_len;
+    int xfer_len_override;
+};
+
+
+
+static void
+usage()
+{
+  fprintf(stderr, "Usage: "
+          "sg_compare_and_write [--fua] [--grpnum=GN] [--help]\n"
+          "                     [--inc=IF] [--inw=IF] [--lba=LBA] [--lbdata] [--num=NUM] [--pbdata]\n"
+          "                     [--timeout=TO] [--unmap] [--verbose] [--version]\n"
+          "                     [--wrprotect=WRP] [xferlen=LEN] DEVICE\n"
+          "  where:\n"
+          "    --fua                Set FUA bit\n"
+          "    --grpnum=GN|-g GN    GN is group number field (def: 0)\n"
+          "    --help|-h            print out usage message\n"
+          "    --compare=FILE|-C FILE  FILE is file to fetch compare data from (use LEN\n"
+          "                            bytes or whole file).\n"
+          "    --write=FILE|-W FILE    FILE is file to fetch write data from (use LEN\n"
+          "                            bytes or whole file).\n"
+          "    --lba=LBA|-l LBA     LBA is the logical block address to start (def: 0)\n"
+          "    --num=NUM|-n NUM     NUM is number of logical blocks to write (def: 1)\n"
+          "    --timeout=TO|-t TO   command timeout (unit: seconds) (def: 60)\n"
+          "    --verbose|-v         increase verbosity\n"
+          "    --version|-V         print version string then exit\n"
+          "    --wrprotect=WPR|-w WPR    WPR is the WRPROTECT field value (def: 0)\n"
+          "    --xferlen=LEN|-x LEN    LEN is number of bytes from input to send to\n"
+          "                            DEVICE (def: IF file length)\n\n"
+          "Performs a SCSI COMPARE AND WRITE command\n"
+          );
+}
+
+static int
+do_compare_and_write(int sg_fd, const struct opts_t * optsp, const void * dataoutp)
+{
+    int k, ret, res, sense_cat, cdb_len;
+    uint64_t llba;
+    uint32_t unum;
+    unsigned char wsCmdBlk[COMPARE_AND_WRITE_LEN];
+    unsigned char sense_b[SENSE_BUFF_LEN];
+    struct sg_pt_base * ptvp;
+
+    cdb_len = 16;
+
+    memset(wsCmdBlk, 0, sizeof(wsCmdBlk));
+
+    wsCmdBlk[0] = COMPARE_AND_WRITE_OP;
+    wsCmdBlk[1] = ((optsp->wrprotect & 0x7) << 5);
+    if (optsp->fua)
+        wsCmdBlk[1] |= 0x08;
+    llba = optsp->lba;
+    
+    // logical block address
+    for (k = 7; k >= 0; --k) {
+        wsCmdBlk[2 + k] = (llba & 0xff);
+        llba >>= 8;
+    }
+    
+    // number of logical blocks
+    unum = optsp->numblocks;
+    wsCmdBlk[13] = (unum & 0xff);
+    
+    // Group number
+    wsCmdBlk[14] = (optsp->grpnum & 0x1f);
+
+    if (optsp->verbose > 1) {
+        fprintf(stderr, "    Compare and write cmd: ");
+        for (k = 0; k < cdb_len; ++k)
+            fprintf(stderr, "%02x ", wsCmdBlk[k]);
+        fprintf(stderr, "\n    Data-out buffer length=%d\n",
+                optsp->xfer_len);
+    }
+    if ((optsp->verbose > 3) && (optsp->xfer_len > 0)) {
+        fprintf(stderr, "    Data-out buffer contents:\n");
+        dStrHex((const char *)dataoutp, optsp->xfer_len, 1);
+    }
+    ptvp = construct_scsi_pt_obj();
+    if (NULL == ptvp) {
+        fprintf(sg_warnings_strm, "Compare_and_write: out of memory\n");
+        return -1;
+    }
+    set_scsi_pt_cdb(ptvp, wsCmdBlk, cdb_len);
+    set_scsi_pt_sense(ptvp, sense_b, sizeof(sense_b));
+    set_scsi_pt_data_out(ptvp, (unsigned char *)dataoutp, optsp->xfer_len);
+    res = do_scsi_pt(ptvp, sg_fd, optsp->timeout, optsp->verbose);
+    if (0) {
+        int ii;
+        fprintf(stderr, "Raw sense data:\n");
+        for (ii = 0; ii < SENSE_BUFF_LEN; ii++) {
+            fprintf(stderr, "%02X ", sense_b[ii]);
+            if ((ii % 16) == 15)
+                fprintf(stderr, "\n");
+        }
+    }
+    ret = sg_cmds_process_resp(ptvp, "Compare and write", res, 0, sense_b,
+                               1 /*noisy */, optsp->verbose, &sense_cat);
+    if (-1 == ret)
+        ;
+    else if (-2 == ret) {
+        switch (sense_cat) {
+        case SG_LIB_CAT_NOT_READY:
+        case SG_LIB_CAT_UNIT_ATTENTION:
+        case SG_LIB_CAT_INVALID_OP:
+        case SG_LIB_CAT_ILLEGAL_REQ:
+        case SG_LIB_CAT_ABORTED_COMMAND:
+            ret = sense_cat;
+            break;
+        case SG_LIB_CAT_RECOVERED:
+        case SG_LIB_CAT_NO_SENSE:
+            ret = 0;
+            break;
+        case SG_LIB_CAT_MEDIUM_HARD:
+            ret = sense_cat;
+            break;
+        default:
+            {
+                unsigned char sense_key;
+                int valid, slen;
+                uint64_t info_fld = 0;
+
+                sense_key = (sense_b[1] & 0xf);
+                if (sense_key != SPC_SK_MISCOMPARE)
+                    break;
+
+                slen = get_scsi_pt_sense_len(ptvp);
+                valid = sg_get_sense_info_fld(sense_b, slen, &info_fld);
+                if (valid)
+                    fprintf(stderr, "Miscompare at byte offset "
+                            "lba=%"PRIu64" [0x%"PRIx64"]\n", info_fld, info_fld);
+            }
+            ret = -1;
+            break;
+        }
+    } else
+        ret = 0;
+
+    destruct_scsi_pt_obj(ptvp);
+    return ret;
+}
+
+/*
+ * Returns 0 on success, SG_LIB_FILE_ERROR on error.
+ */
+int read_file_data(char* filename, unsigned char* buf, int nbytes)
+{
+    int got_stdin, fd, res;
+    char ebuff[EBUFF_SZ];
+    
+    got_stdin = (0 == strcmp(filename, "-")) ? 1 : 0;
+
+    if (got_stdin) {
+        fd = STDIN_FILENO;
+        if (sg_set_binary_mode(STDIN_FILENO) < 0)
+            perror("sg_set_binary_mode");
+    } else {
+        if ((fd = open(filename, O_RDONLY)) < 0) {
+            snprintf(ebuff, EBUFF_SZ,
+                     ME "could not open %s for reading", filename);
+            perror(ebuff);
+            return SG_LIB_FILE_ERROR;
+        } else if (sg_set_binary_mode(fd) < 0)
+            perror("sg_set_binary_mode");
+    }
+    res = read(fd, buf, nbytes);
+    if (res < 0) {
+        snprintf(ebuff, EBUFF_SZ, ME "couldn't read from %s",
+                 filename);
+        perror(ebuff);
+        if (! got_stdin)
+            close(fd);
+        return SG_LIB_FILE_ERROR;
+    }
+    if (res < nbytes) {
+        fprintf(stderr, "tried to read %d bytes from %s, got %d "
+                "bytes\n", nbytes, filename, res);
+        fprintf(stderr, "  so pad with 0x0 bytes and continue\n");
+    }
+    if (! got_stdin)
+        close(fd);
+
+    if (0) { /* enable to dump buffers to stderr */
+        int ii=0;
+        while(1) {
+            if (ii && (!(ii % 32))) fprintf(stderr, "\n");
+            if (ii >= nbytes) break;
+            fprintf(stderr, "%02x", buf[ii]);
+            ii++;
+        }
+    }
+
+    return 0;
+}
+
+
+int
+main(int argc, char * argv[])
+{
+    int sg_fd, res, c, prot_en, vb;
+    int num_given = 0;
+    int lba_given = 0;
+    int if_given = 0;
+    int got_stdin = 0;
+    int64_t ll;
+    uint32_t block_size;
+    const char * device_name = NULL;
+    unsigned char resp_buff[RCAP16_RESP_LEN];
+    unsigned char * wBuff = NULL;
+    int ret = -1;
+    struct opts_t opts;
+    struct stat a_stat;
+
+    memset(&opts, 0, sizeof(opts));
+    opts.numblocks = DEF_WS_NUMBLOCKS;
+    opts.timeout = DEF_TIMEOUT_SECS;
+    vb = 0;
+    while (1) {
+        int option_index = 0;
+
+        c = getopt_long(argc, argv, "ag:hi:l:Ln:PRSt:TUvVw:x:", long_options,
+                        &option_index);
+        if (c == -1)
+            break;
+
+        switch (c) {
+        case 'f':
+            ++opts.fua;
+            break;
+        case 'g':
+            opts.grpnum = sg_get_num(optarg);
+            if ((opts.grpnum < 0) || (opts.grpnum > 31))  {
+                fprintf(stderr, "bad argument to '--grpnum'\n");
+                return SG_LIB_SYNTAX_ERROR;
+            }
+            break;
+        case 'h':
+        case '?':
+            usage();
+            return 0;
+        case 'C':
+            strncpy(opts.ifilenamec, optarg, sizeof(opts.ifilenamec));
+            if_given = 1;
+            break;
+        case 'W':
+            strncpy(opts.ifilenamew, optarg, sizeof(opts.ifilenamew));
+            if_given = 1;
+            break;
+        case 'l':
+            ll = sg_get_llnum(optarg);
+            if (-1 == ll) {
+                fprintf(stderr, "bad argument to '--lba'\n");
+                return SG_LIB_SYNTAX_ERROR;
+            }
+            opts.lba = (uint64_t)ll;
+            lba_given = 1;
+            break;
+        case 'n':
+            opts.numblocks = sg_get_num(optarg);
+            if (opts.numblocks < 0)  {
+                fprintf(stderr, "bad argument to '--num'\n");
+                return SG_LIB_SYNTAX_ERROR;
+            }
+            num_given = 1;
+            break;
+        case 't':
+            opts.timeout = sg_get_num(optarg);
+            if (opts.timeout < 0)  {
+                fprintf(stderr, "bad argument to '--timeout'\n");
+                return SG_LIB_SYNTAX_ERROR;
+            }
+            break;
+        case 'v':
+            ++opts.verbose;
+            break;
+        case 'V':
+            fprintf(stderr, ME "version: %s\n", version_str);
+            return 0;
+        case 'w':
+            opts.wrprotect = sg_get_num(optarg);
+            if ((opts.wrprotect < 0) || (opts.wrprotect > 7))  {
+                fprintf(stderr, "bad argument to '--wrprotect'\n");
+                return SG_LIB_SYNTAX_ERROR;
+            }
+            break;
+        case 'x':
+            opts.xfer_len_override=1;
+            opts.xfer_len = sg_get_num(optarg);
+            if (opts.xfer_len < 0) {
+                fprintf(stderr, "bad argument to '--xferlen'\n");
+                return SG_LIB_SYNTAX_ERROR;
+            }
+            break;
+        default:
+            fprintf(stderr, "unrecognised option code 0x%x ??\n", c);
+            usage();
+            return SG_LIB_SYNTAX_ERROR;
+        }
+    }
+    if (optind < argc) {
+        if (NULL == device_name) {
+            device_name = argv[optind];
+            ++optind;
+        }
+        if (optind < argc) {
+            for (; optind < argc; ++optind)
+                fprintf(stderr, "Unexpected extra argument: %s\n",
+                        argv[optind]);
+            usage();
+            return SG_LIB_SYNTAX_ERROR;
+        }
+    }
+    if (NULL == device_name) {
+        fprintf(stderr, "missing device name!\n");
+        usage();
+        return SG_LIB_SYNTAX_ERROR;
+    }
+    vb = opts.verbose;
+
+    if ((! if_given) && (! lba_given) && (! num_given)) {
+        fprintf(stderr, "As a precaution require one of '--in=', '--lba=' "
+                "or '--num=' to be given\n");
+        return SG_LIB_SYNTAX_ERROR;
+    }
+
+    memset(&a_stat, 0, sizeof(a_stat));
+    if (opts.ifilenamec[0]) {
+        got_stdin = (0 == strcmp(opts.ifilenamec, "-")) ? 1 : 0;
+        if (! got_stdin) {
+            if (stat(opts.ifilenamec, &a_stat) < 0) {
+                if (vb)
+                    fprintf(stderr, "unable to stat(%s): %s\n",
+                            opts.ifilenamec, safe_strerror(errno));
+                return SG_LIB_FILE_ERROR;
+            }
+            if (!opts.xfer_len_override)
+                opts.xfer_len = (int)a_stat.st_size;
+        }
+    }
+    if (opts.ifilenamew[0]) {
+        got_stdin = (0 == strcmp(opts.ifilenamew, "-")) ? 1 : 0;
+        if (! got_stdin) {
+            if (stat(opts.ifilenamew, &a_stat) < 0) {
+                if (vb)
+                    fprintf(stderr, "unable to stat(%s): %s\n",
+                            opts.ifilenamew, safe_strerror(errno));
+                return SG_LIB_FILE_ERROR;
+            }
+            if (!opts.xfer_len_override) {
+                if (a_stat.st_size != opts.xfer_len) {
+                    fprintf(stderr, "compare and write buffers are different sizes\n");
+                    return SG_LIB_FILE_ERROR;
+                }
+                opts.xfer_len += (int)a_stat.st_size;
+            }
+        }
+    }
+    
+    sg_fd = sg_cmds_open_device(device_name, 0 /* rw */, vb);
+    if (sg_fd < 0) {
+        fprintf(stderr, ME "open error: %s: %s\n", device_name,
+                safe_strerror(-sg_fd));
+        return SG_LIB_FILE_ERROR;
+    }
+
+    prot_en = 0;
+    if (0 == opts.xfer_len) {
+        res = sg_ll_readcap_16(sg_fd, 0 /* pmi */, 0 /* llba */, resp_buff,
+                               RCAP16_RESP_LEN, 0, (vb ? (vb - 1): 0));
+        if (0 == res) {
+            block_size = ((resp_buff[8] << 24) |
+                          (resp_buff[9] << 16) |
+                          (resp_buff[10] << 8) |
+                          resp_buff[11]);
+            prot_en = !!(resp_buff[12] & 0x1);
+            opts.xfer_len = 2 * opts.numblocks * (block_size + (prot_en ? 8 : 0));
+        } else if ((SG_LIB_CAT_INVALID_OP == res) ||
+                   (SG_LIB_CAT_ILLEGAL_REQ == res)) {
+            if (vb)
+                fprintf(stderr, "Read capacity(16) not supported, try Read "
+                        "capacity(10)\n");
+            res = sg_ll_readcap_10(sg_fd, 0 /* pmi */, 0 /* lba */, resp_buff,
+                                   RCAP10_RESP_LEN, 0, (vb ? (vb - 1): 0));
+            if (0 == res) {
+                block_size = ((resp_buff[4] << 24) |
+                              (resp_buff[5] << 16) |
+                              (resp_buff[6] << 8) |
+                              resp_buff[7]);
+                opts.xfer_len = 2 * opts.numblocks * block_size;
+            }
+        } else if (vb)
+            fprintf(stderr, "Read capacity(16) failed. Unable to calculate "
+                    "block size\n");
+        if (res)
+            fprintf(stderr, "Read capacity(10) failed. Unable to calculate "
+                    "block size\n");
+    }
+    if (opts.xfer_len < 1) {
+        fprintf(stderr, "unable to deduce block size, please give "
+                "'--xferlen=' argument\n");
+        ret = SG_LIB_SYNTAX_ERROR;
+        goto err_out;
+    }
+    if (opts.xfer_len > MAX_XFER_LEN) {
+        fprintf(stderr, "'--xferlen=%d is out of range ( want <= %d)\n",
+                opts.xfer_len, MAX_XFER_LEN);
+        ret = SG_LIB_SYNTAX_ERROR;
+        goto err_out;
+    }
+    // note double-size buffer because we send compare data and write data
+    wBuff = (unsigned char*)calloc(2 * opts.xfer_len, 1);
+    if (NULL == wBuff) {
+        fprintf(stderr, "unable to allocate %d bytes of memory with "
+                "calloc()\n", opts.xfer_len);
+        ret = SG_LIB_SYNTAX_ERROR;
+        goto err_out;
+    }
+    if (opts.ifilenamec[0]) {
+        ret = read_file_data(opts.ifilenamec, wBuff, opts.xfer_len / 2);
+        if (ret)
+            goto err_out;
+    } else {
+        if (vb)
+            fprintf(stderr, "Default compare buffer set to %d zeros\n",
+                    opts.xfer_len / 2);
+        /* disabled for now until I sort out buffer offsets. */
+        /*if (prot_en) { // default for protection is 0xff, rest get 0x0
+            memset(wBuff + opts.xfer_len - 8, 0xff, 8);
+            if (vb)
+                fprintf(stderr, " ... apart from last 8 bytes which are set "
+                        "to 0xff\n");
+        }*/
+    }
+    if (opts.ifilenamew[0]) {
+        ret = read_file_data(opts.ifilenamew, wBuff + opts.xfer_len / 2, opts.xfer_len / 2);
+        if (ret)
+            goto err_out;
+    } else {
+        if (vb)
+            fprintf(stderr, "Default write buffer set to %d zeros\n",
+                    opts.xfer_len / 2);
+        /* disabled for now until I sort out buffer offsets. */
+        /*if (prot_en) { // default for protection is 0xff, rest get 0x0
+            memset(wBuff + opts.xfer_len - 8, 0xff, 8);
+            if (vb)
+                fprintf(stderr, " ... apart from last 8 bytes which are set "
+                        "to 0xff\n");
+        }*/
+    }
+
+    ret = do_compare_and_write(sg_fd, &opts, wBuff);
+    if (ret) {
+        switch (ret) {
+        case SG_LIB_CAT_NOT_READY:
+            fprintf(stderr, "Compare_and_write failed, device not ready\n");
+            break;
+        case SG_LIB_CAT_UNIT_ATTENTION:
+            fprintf(stderr, "Compare_and_write, unit attention\n");
+            break;
+        case SG_LIB_CAT_ABORTED_COMMAND:
+            fprintf(stderr, "Compare_and_write, aborted command\n");
+            break;
+        case SG_LIB_CAT_INVALID_OP:
+            fprintf(stderr, "Compare_and_write command not supported\n");
+            break;
+        case SG_LIB_CAT_ILLEGAL_REQ:
+            fprintf(stderr, "bad field in Compare_and_write cdb, option "
+                    "probably not supported\n");
+            break;
+        case SG_LIB_CAT_MEDIUM_HARD:
+            fprintf(stderr, "Compare_and_write command reported medium or "
+                    "hardware error\n");
+            break;
+        default:
+            fprintf(stderr, "Compare_and_write command failed\n");
+            break;
+        }
+    }
+
+err_out:
+    if (wBuff)
+        free(wBuff);
+    res = sg_cmds_close_device(sg_fd);
+    if (res < 0) {
+        fprintf(stderr, "close error: %s\n", safe_strerror(-res));
+        if (0 == ret)
+            return SG_LIB_FILE_ERROR;
+    }
+    return (ret >= 0) ? ret : SG_LIB_CAT_OTHER;
+}
Index: src/Makefile.in
===================================================================
--- src/Makefile.in	(revision 9997)
+++ src/Makefile.in	(working copy)
@@ -252,7 +252,8 @@
 @OS_FREEBSD_FALSE@@OS_LINUX_TRUE@	sg_write_buffer$(EXEEXT) \
 @OS_FREEBSD_FALSE@@OS_LINUX_TRUE@	sg_write_long$(EXEEXT) \
 @OS_FREEBSD_FALSE@@OS_LINUX_TRUE@	sg_write_same$(EXEEXT) \
-@OS_FREEBSD_FALSE@@OS_LINUX_TRUE@	sg_wr_mode$(EXEEXT)
+@OS_FREEBSD_FALSE@@OS_LINUX_TRUE@	sg_wr_mode$(EXEEXT) \
+@OS_FREEBSD_FALSE@@OS_LINUX_TRUE@	sg_compare_and_write$(EXEEXT)
 @OS_FREEBSD_TRUE@bin_PROGRAMS = sg_decode_sense$(EXEEXT) \
 @OS_FREEBSD_TRUE@	sg_format$(EXEEXT) sg_get_config$(EXEEXT) \
 @OS_FREEBSD_TRUE@	sg_get_lba_status$(EXEEXT) sg_ident$(EXEEXT) \
@@ -288,6 +289,9 @@
 CONFIG_CLEAN_VPATH_FILES =
 am__installdirs = "$(DESTDIR)$(bindir)"
 PROGRAMS = $(bin_PROGRAMS)
+am_sg_compare_and_write_OBJECTS = sg_compare_and_write.$(OBJEXT)
+sg_compare_and_write_OBJECTS = $(am_sg_compare_and_write_OBJECTS)
+sg_compare_and_write_DEPENDENCIES = ../lib/libsgutils2.la
 am_sg_dd_OBJECTS = sg_dd.$(OBJEXT)
 sg_dd_OBJECTS = $(am_sg_dd_OBJECTS)
 sg_dd_DEPENDENCIES = ../lib/libsgutils2.la
@@ -460,14 +464,15 @@
 LINK = $(LIBTOOL) --tag=CC $(AM_LIBTOOLFLAGS) $(LIBTOOLFLAGS) \
 	--mode=link $(CCLD) $(AM_CFLAGS) $(CFLAGS) $(AM_LDFLAGS) \
 	$(LDFLAGS) -o $@
-SOURCES = $(sg_dd_SOURCES) $(sg_decode_sense_SOURCES) \
-	$(sg_emc_trespass_SOURCES) $(sg_format_SOURCES) \
-	$(sg_get_config_SOURCES) $(sg_get_lba_status_SOURCES) \
-	$(sg_ident_SOURCES) $(sg_inq_SOURCES) $(sg_logs_SOURCES) \
-	$(sg_luns_SOURCES) $(sg_map_SOURCES) $(sg_map26_SOURCES) \
-	$(sg_modes_SOURCES) $(sg_opcodes_SOURCES) \
-	$(sg_persist_SOURCES) $(sg_prevent_SOURCES) $(sg_raw_SOURCES) \
-	$(sg_rbuf_SOURCES) $(sg_rdac_SOURCES) $(sg_read_SOURCES) \
+SOURCES = $(sg_compare_and_write_SOURCES) $(sg_dd_SOURCES) \
+	$(sg_decode_sense_SOURCES) $(sg_emc_trespass_SOURCES) \
+	$(sg_format_SOURCES) $(sg_get_config_SOURCES) \
+	$(sg_get_lba_status_SOURCES) $(sg_ident_SOURCES) \
+	$(sg_inq_SOURCES) $(sg_logs_SOURCES) $(sg_luns_SOURCES) \
+	$(sg_map_SOURCES) $(sg_map26_SOURCES) $(sg_modes_SOURCES) \
+	$(sg_opcodes_SOURCES) $(sg_persist_SOURCES) \
+	$(sg_prevent_SOURCES) $(sg_raw_SOURCES) $(sg_rbuf_SOURCES) \
+	$(sg_rdac_SOURCES) $(sg_read_SOURCES) \
 	$(sg_read_block_limits_SOURCES) $(sg_read_buffer_SOURCES) \
 	$(sg_read_long_SOURCES) $(sg_readcap_SOURCES) \
 	$(sg_reassign_SOURCES) $(sg_referrals_SOURCES) \
@@ -482,14 +487,15 @@
 	$(sg_write_buffer_SOURCES) $(sg_write_long_SOURCES) \
 	$(sg_write_same_SOURCES) $(sginfo_SOURCES) $(sgm_dd_SOURCES) \
 	$(sgp_dd_SOURCES)
-DIST_SOURCES = $(sg_dd_SOURCES) $(sg_decode_sense_SOURCES) \
-	$(sg_emc_trespass_SOURCES) $(sg_format_SOURCES) \
-	$(sg_get_config_SOURCES) $(sg_get_lba_status_SOURCES) \
-	$(sg_ident_SOURCES) $(sg_inq_SOURCES) $(sg_logs_SOURCES) \
-	$(sg_luns_SOURCES) $(sg_map_SOURCES) $(sg_map26_SOURCES) \
-	$(sg_modes_SOURCES) $(sg_opcodes_SOURCES) \
-	$(sg_persist_SOURCES) $(sg_prevent_SOURCES) $(sg_raw_SOURCES) \
-	$(sg_rbuf_SOURCES) $(sg_rdac_SOURCES) $(sg_read_SOURCES) \
+DIST_SOURCES = $(sg_compare_and_write_SOURCES) $(sg_dd_SOURCES) \
+	$(sg_decode_sense_SOURCES) $(sg_emc_trespass_SOURCES) \
+	$(sg_format_SOURCES) $(sg_get_config_SOURCES) \
+	$(sg_get_lba_status_SOURCES) $(sg_ident_SOURCES) \
+	$(sg_inq_SOURCES) $(sg_logs_SOURCES) $(sg_luns_SOURCES) \
+	$(sg_map_SOURCES) $(sg_map26_SOURCES) $(sg_modes_SOURCES) \
+	$(sg_opcodes_SOURCES) $(sg_persist_SOURCES) \
+	$(sg_prevent_SOURCES) $(sg_raw_SOURCES) $(sg_rbuf_SOURCES) \
+	$(sg_rdac_SOURCES) $(sg_read_SOURCES) \
 	$(sg_read_block_limits_SOURCES) $(sg_read_buffer_SOURCES) \
 	$(sg_read_long_SOURCES) $(sg_readcap_SOURCES) \
 	$(sg_reassign_SOURCES) $(sg_referrals_SOURCES) \
@@ -727,6 +733,8 @@
 sg_write_long_LDADD = ../lib/libsgutils2.la @os_libs@
 sg_write_same_SOURCES = sg_write_same.c
 sg_write_same_LDADD = ../lib/libsgutils2.la @os_libs@
+sg_compare_and_write_SOURCES = sg_compare_and_write.c
+sg_compare_and_write_LDADD = ../lib/libsgutils2.la @os_libs@
 sg_wr_mode_SOURCES = sg_wr_mode.c
 sg_wr_mode_LDADD = ../lib/libsgutils2.la @os_libs@
 all: all-am
@@ -806,6 +814,9 @@
 	list=`for p in $$list; do echo "$$p"; done | sed 's/$(EXEEXT)$$//'`; \
 	echo " rm -f" $$list; \
 	rm -f $$list
+sg_compare_and_write$(EXEEXT): $(sg_compare_and_write_OBJECTS) $(sg_compare_and_write_DEPENDENCIES) 
+	@rm -f sg_compare_and_write$(EXEEXT)
+	$(LINK) $(sg_compare_and_write_OBJECTS) $(sg_compare_and_write_LDADD) $(LIBS)
 sg_dd$(EXEEXT): $(sg_dd_OBJECTS) $(sg_dd_DEPENDENCIES) 
 	@rm -f sg_dd$(EXEEXT)
 	$(LINK) $(sg_dd_OBJECTS) $(sg_dd_LDADD) $(LIBS)
@@ -972,6 +983,7 @@
 distclean-compile:
 	-rm -f *.tab.c
 
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/sg_compare_and_write.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/sg_dd.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/sg_decode_sense.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/sg_emc_trespass.Po@am__quote@
Index: src/sg_luns.c
===================================================================
--- src/sg_luns.c	(revision 9997)
+++ src/sg_luns.c	(working copy)
@@ -28,7 +28,7 @@
 
 static char * version_str = "1.15 20100312";
 
-#define MAX_RLUNS_BUFF_LEN (1024 * 64)
+#define MAX_RLUNS_BUFF_LEN ( 1LL << 32 )
 #define DEF_RLUNS_BUFF_LEN (1024 * 8)
 
 static unsigned char reportLunsBuff[MAX_RLUNS_BUFF_LEN];
Index: src/Makefile.am
===================================================================
--- src/Makefile.am	(revision 9997)
+++ src/Makefile.am	(working copy)
@@ -17,7 +17,7 @@
 	sg_sat_identify sg_sat_phy_event sg_sat_set_features sg_scan \
 	sg_senddiag sg_ses sg_start sg_stpg sg_sync sg_test_rwbuf sg_turs \
 	sg_unmap sg_verify sg_vpd sg_write_buffer sg_write_long \
-	sg_write_same sg_wr_mode
+	sg_write_same sg_wr_mode sg_compare_and_write
 
 distclean-local:
 	rm -f sg_scan.c
@@ -274,6 +274,9 @@
 sg_write_same_SOURCES = sg_write_same.c
 sg_write_same_LDADD = ../lib/libsgutils2.la @os_libs@
 
+sg_compare_and_write_SOURCES = sg_compare_and_write.c
+sg_compare_and_write_LDADD = ../lib/libsgutils2.la @os_libs@
+
 sg_wr_mode_SOURCES = sg_wr_mode.c
 sg_wr_mode_LDADD = ../lib/libsgutils2.la @os_libs@
 
Index: lib/sg_cmds_basic.c
===================================================================
--- lib/sg_cmds_basic.c	(revision 9997)
+++ lib/sg_cmds_basic.c	(working copy)
@@ -173,6 +173,12 @@
             fprintf(sg_warnings_strm, "%s: scsi status: %s\n", leadin, b);
         }
         return -1;
+    case SCSI_PT_RESULT_TRANSPORT_ERR:
+        if (verbose || noisy) {
+            get_scsi_pt_transport_err_str(ptvp, sizeof(b), b);
+            fprintf(sg_warnings_strm, "%s: transport: %s\n", leadin, b);
+        }
+        /* Note fall-through to pick up sense data */
     case SCSI_PT_RESULT_SENSE:
         scat = sg_err_category_sense(sbp, slen);
         switch (scat) {
@@ -205,12 +211,6 @@
         if (o_sense_cat)
             *o_sense_cat = scat;
         return -2;
-    case SCSI_PT_RESULT_TRANSPORT_ERR:
-        if (verbose || noisy) {
-            get_scsi_pt_transport_err_str(ptvp, sizeof(b), b);
-            fprintf(sg_warnings_strm, "%s: transport: %s\n", leadin, b);
-        }
-        return -1;
     case SCSI_PT_RESULT_OS_ERR:
         if (verbose || noisy) {
             get_scsi_pt_os_err_str(ptvp, sizeof(b), b);
Index: lib/sg_pt_linux.c
===================================================================
--- lib/sg_pt_linux.c	(revision 9997)
+++ lib/sg_pt_linux.c	(working copy)
@@ -33,7 +33,8 @@
     "DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT",
     "DID_BAD_TARGET", "DID_ABORT", "DID_PARITY", "DID_ERROR",
     "DID_RESET", "DID_BAD_INTR", "DID_PASSTHROUGH", "DID_SOFT_ERROR",
-    "DID_IMM_RETRY", "DID_REQUEUE"
+    "DID_IMM_RETRY", "DID_REQUEUE", "DID_TRANSPORT_DISRUPTED",
+    "DID_TRANSPORT_FAILFAST", "DID_TARGET_FAILURE", "DID_NEXUS_FAILURE"
 };
 
 #define LINUX_HOST_BYTES_SZ \