[PATCH v4 0/4] target: Fix several problems related to T10-PI support

[PATCH v4 0/4] target: Fix several problems related to T10-PI support

[Akinobu Mita]

This patchset aims to fix several problems related to T10-PI support.

These patches can be applied on top of Sagi's "[v1] Simlify dif_verify
routines and fixup fileio protection information code" patchset.

* Changes from v3:
- Drop WRITE SAME support from this patch series, as it requires more
  works than I expected in the first place.
- Improve the condition in transport_generic_new_cmd() a bit
- Reduce duplicated code between crc_t10dif_update and crc_t10dif,
  suggested by Tim and Herbert
- Fix inconsistent address passed to kunmap_atomic, reported by Sagi
- Stop operation in sbc_dif_generate() and sbc_dif_verify() when
  reaching the end of data SG elements

* Changes from v2:
- Introduces crc_t10dif_update() to calculate CRC by multiple calls
- Handle odd SG mapping correctly instead of giving up

* Changes from v1:
- Reduce code duplication a bit in target_read_prot_action()
- Fix sbc_dif_verify() for WRITE_SAME command
- Fix inverted rw argument for fd_do_rw()
- Perform DIF verify before write for WRITE_SAME

Akinobu Mita (4):
  target: Fix inconsistent address passed to kunmap_atomic() in
    sbc_dif_copy_prot()
  target: ensure se_cmd->t_prot_sg is allocated when required
  lib: introduce crc_t10dif_update()
  target: handle odd SG mapping for data transfer memory

 drivers/target/target_core_sbc.c       | 127 ++++++++++++++++++++++-----------
 drivers/target/target_core_transport.c |  27 +++----
 include/linux/crc-t10dif.h             |   1 +
 include/target/target_core_base.h      |   1 +
 lib/crc-t10dif.c                       |  13 +++-
 5 files changed, 113 insertions(+), 56 deletions(-)

Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
-- 
1.9.1

[PATCH v4 1/4] target: Fix inconsistent address passed to kunmap_atomic() in sbc_dif_copy_prot()

[Akinobu Mita]

In sbc_dif_copy_prot(), the addresses passed to kunmap_atomic() are
inconsistent with the addresses which are mapped by kmap_atomic().
That could be problematic if an SG element has its length larger than
PAGE_SIZE as kunmap_atomic() will attempt to unmap different page.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* New patch from v3

 drivers/target/target_core_sbc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index edba39f..b765cdd 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1299,13 +1299,14 @@ void sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
 			copied += len;
 			psg_len -= len;
 
+			kunmap_atomic(addr - sg->offset - offset);
+
 			if (offset >= sg->length) {
 				sg = sg_next(sg);
 				offset = 0;
 			}
-			kunmap_atomic(addr);
 		}
-		kunmap_atomic(paddr);
+		kunmap_atomic(paddr - psg->offset);
 	}
 }
 EXPORT_SYMBOL(sbc_dif_copy_prot);
-- 
1.9.1

[PATCH v4 2/4] target: ensure se_cmd->t_prot_sg is allocated when required

[Akinobu Mita]

Even if the device backend is initialized with protection info is
enabled, some requests don't have the protection info attached for
WRITE SAME command issued by block device helpers, WRITE command with
WRPROTECT=0 by SG_IO ioctl, etc.

So when TCM loopback fabric module is used, se_cmd->t_prot_sg is NULL
for these requests and performing WRITE_INSERT of PI using software
emulation by sbc_dif_generate() causes kernel crash.

To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for
se_cmd_flags, which is used to determine that se_cmd->t_prot_sg needs
to be allocated or use pre-allocated protection information by scsi
mid-layer.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* Changes from v3:
- Improve the condition in transport_generic_new_cmd() a bit

 drivers/target/target_core_transport.c | 27 +++++++++++++++------------
 include/target/target_core_base.h      |  1 +
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7a9e7e2..1947e67 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	if (sgl_prot_count) {
 		se_cmd->t_prot_sg = sgl_prot;
 		se_cmd->t_prot_nents = sgl_prot_count;
+		se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC;
 	}
 
 	/*
@@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd)
 
 static inline void transport_free_pages(struct se_cmd *cmd)
 {
+	if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
+		transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents);
+		cmd->t_prot_sg = NULL;
+		cmd->t_prot_nents = 0;
+	}
+
 	if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) {
 		/*
 		 * Release special case READ buffer payload required for
@@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd)
 	transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents);
 	cmd->t_bidi_data_sg = NULL;
 	cmd->t_bidi_data_nents = 0;
-
-	transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents);
-	cmd->t_prot_sg = NULL;
-	cmd->t_prot_nents = 0;
 }
 
 /**
@@ -2346,6 +2349,14 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 	int ret = 0;
 	bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB);
 
+	if (cmd->prot_op != TARGET_PROT_NORMAL &&
+	    !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
+		ret = target_alloc_sgl(&cmd->t_prot_sg, &cmd->t_prot_nents,
+				       cmd->prot_length, true);
+		if (ret < 0)
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
+
 	/*
 	 * Determine is the TCM fabric module has already allocated physical
 	 * memory, and is directly calling transport_generic_map_mem_to_cmd()
@@ -2371,14 +2382,6 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
 
-		if (cmd->prot_op != TARGET_PROT_NORMAL) {
-			ret = target_alloc_sgl(&cmd->t_prot_sg,
-					       &cmd->t_prot_nents,
-					       cmd->prot_length, true);
-			if (ret < 0)
-				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-		}
-
 		ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents,
 				       cmd->data_length, zero_flag);
 		if (ret < 0)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 480e9f8..13efcdd 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -167,6 +167,7 @@ enum se_cmd_flags_table {
 	SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000,
 	SCF_COMPARE_AND_WRITE		= 0x00080000,
 	SCF_COMPARE_AND_WRITE_POST	= 0x00100000,
+	SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
 };
 
 /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
-- 
1.9.1

[PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Akinobu Mita]

This introduces crc_t10dif_update() which enables to calculate CRC
for a block which straddles multiple SG elements by calling multiple
times.  This also converts crc_t10dif() to use crc_t10dif_update() as
they are almost same.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
---
* Changes from v3:
- Reduce duplicated code between crc_t10dif_update and crc_t10dif,
  suggested by Tim and Herbert

 include/linux/crc-t10dif.h |  1 +
 lib/crc-t10dif.c           | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h
index cf53d07..d81961e 100644
--- a/include/linux/crc-t10dif.h
+++ b/include/linux/crc-t10dif.h
@@ -9,5 +9,6 @@
 extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer,
 				size_t len);
 extern __u16 crc_t10dif(unsigned char const *, size_t);
+extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t);
 
 #endif
diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
index dfe6ec1..d775737 100644
--- a/lib/crc-t10dif.c
+++ b/lib/crc-t10dif.c
@@ -19,7 +19,7 @@
 static struct crypto_shash *crct10dif_tfm;
 static struct static_key crct10dif_fallback __read_mostly;
 
-__u16 crc_t10dif(const unsigned char *buffer, size_t len)
+__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
 {
 	struct {
 		struct shash_desc shash;
@@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len)
 	int err;
 
 	if (static_key_false(&crct10dif_fallback))
-		return crc_t10dif_generic(0, buffer, len);
+		return crc_t10dif_generic(crc, buffer, len);
 
 	desc.shash.tfm = crct10dif_tfm;
 	desc.shash.flags = 0;
-	*(__u16 *)desc.ctx = 0;
 
+	err = crypto_shash_import(&desc.shash, &crc);
+	BUG_ON(err);
 	err = crypto_shash_update(&desc.shash, buffer, len);
 	BUG_ON(err);
 
 	return *(__u16 *)desc.ctx;
 }
+EXPORT_SYMBOL(crc_t10dif_update);
+
+__u16 crc_t10dif(const unsigned char *buffer, size_t len)
+{
+	return crc_t10dif_update(0, buffer, len);
+}
 EXPORT_SYMBOL(crc_t10dif);
 
 static int __init crc_t10dif_mod_init(void)
-- 
1.9.1

[PATCH v4 4/4] target: handle odd SG mapping for data transfer memory

[Akinobu Mita]

sbc_dif_generate() and sbc_dif_verify() currently assume that each
SG element for data transfer memory doesn't straddle the block size
boundary.

However, when using SG_IO ioctl, we can choose the data transfer
memory which doesn't satisfy that alignment requirement.

In order to handle such cases correctly, this change inverts the outer
loop to iterate data transfer memory and the inner loop to iterate
protection information and enables to calculate CRC for a block which
straddles multiple SG elements.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: target-devel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* Changes from v3:
- Fix inconsistent address passed to kunmap_atomic, reported by Sagi
- Stop operation in sbc_dif_generate() and sbc_dif_verify() when
  reaching the end of data SG elements

 drivers/target/target_core_sbc.c | 122 ++++++++++++++++++++++++++-------------
 1 file changed, 83 insertions(+), 39 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index b765cdd..4a2df6d 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1182,27 +1182,50 @@ sbc_dif_generate(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct se_dif_v1_tuple *sdt;
-	struct scatterlist *dsg, *psg = cmd->t_prot_sg;
+	struct scatterlist *dsg = cmd->t_data_sg, *psg;
 	sector_t sector = cmd->t_task_lba;
 	void *daddr, *paddr;
 	int i, j, offset = 0;
+	unsigned int block_size = dev->dev_attrib.block_size;
 
-	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
-		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
 		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
 
-		for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
+		for (j = 0; j < psg->length;
+				j += sizeof(struct se_dif_v1_tuple)) {
+			__u16 crc;
+			unsigned int avail;
 
-			if (offset >= psg->length) {
-				kunmap_atomic(paddr);
-				psg = sg_next(psg);
-				paddr = kmap_atomic(sg_page(psg)) + psg->offset;
-				offset = 0;
+			if (offset >= dsg->length) {
+				offset -= dsg->length;
+				kunmap_atomic(daddr - dsg->offset);
+				dsg = sg_next(dsg);
+				if (!dsg) {
+					kunmap_atomic(paddr - psg->offset);
+					return;
+				}
+				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
 			}
 
-			sdt = paddr + offset;
-			sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j,
-						dev->dev_attrib.block_size));
+			sdt = paddr + j;
+			avail = min(block_size, dsg->length - offset);
+			crc = crc_t10dif(daddr + offset, avail);
+			if (avail < block_size) {
+				kunmap_atomic(daddr - dsg->offset);
+				dsg = sg_next(dsg);
+				if (!dsg) {
+					kunmap_atomic(paddr - psg->offset);
+					return;
+				}
+				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+				offset = block_size - avail;
+				crc = crc_t10dif_update(crc, daddr, offset);
+			} else {
+				offset += block_size;
+			}
+
+			sdt->guard_tag = cpu_to_be16(crc);
 			if (cmd->prot_type == TARGET_DIF_TYPE1_PROT)
 				sdt->ref_tag = cpu_to_be32(sector & 0xffffffff);
 			sdt->app_tag = 0;
@@ -1215,26 +1238,23 @@ sbc_dif_generate(struct se_cmd *cmd)
 				 be32_to_cpu(sdt->ref_tag));
 
 			sector++;
-			offset += sizeof(struct se_dif_v1_tuple);
 		}
 
-		kunmap_atomic(paddr);
-		kunmap_atomic(daddr);
+		kunmap_atomic(daddr - dsg->offset);
+		kunmap_atomic(paddr - psg->offset);
 	}
 }
 
 static sense_reason_t
 sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
-		  const void *p, sector_t sector, unsigned int ei_lba)
+		  __u16 crc, sector_t sector, unsigned int ei_lba)
 {
-	struct se_device *dev = cmd->se_dev;
-	int block_size = dev->dev_attrib.block_size;
 	__be16 csum;
 
 	if (!(cmd->prot_checks & TARGET_DIF_CHECK_GUARD))
 		goto check_ref;
 
-	csum = cpu_to_be16(crc_t10dif(p, block_size));
+	csum = cpu_to_be16(crc);
 
 	if (sdt->guard_tag != csum) {
 		pr_err("DIFv1 checksum failed on sector %llu guard tag 0x%04x"
@@ -1317,26 +1337,36 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 {
 	struct se_device *dev = cmd->se_dev;
 	struct se_dif_v1_tuple *sdt;
-	struct scatterlist *dsg;
+	struct scatterlist *dsg = cmd->t_data_sg;
 	sector_t sector = start;
 	void *daddr, *paddr;
-	int i, j;
+	int i;
 	sense_reason_t rc;
+	int dsg_off = 0;
+	unsigned int block_size = dev->dev_attrib.block_size;
 
-	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
-		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+	for (; psg && sector < start + sectors; psg = sg_next(psg)) {
 		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
 
-		for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
+		for (i = psg_off; i < psg->length &&
+				sector < start + sectors;
+				i += sizeof(struct se_dif_v1_tuple)) {
+			__u16 crc;
+			unsigned int avail;
 
-			if (psg_off >= psg->length) {
-				kunmap_atomic(paddr - psg->offset);
-				psg = sg_next(psg);
-				paddr = kmap_atomic(sg_page(psg)) + psg->offset;
-				psg_off = 0;
+			if (dsg_off >= dsg->length) {
+				dsg_off -= dsg->length;
+				kunmap_atomic(daddr - dsg->offset);
+				dsg = sg_next(dsg);
+				if (!dsg) {
+					kunmap_atomic(paddr - psg->offset);
+					return 0;
+				}
+				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
 			}
 
-			sdt = paddr + psg_off;
+			sdt = paddr + i;
 
 			pr_debug("DIF READ sector: %llu guard_tag: 0x%04x"
 				 " app_tag: 0x%04x ref_tag: %u\n",
@@ -1344,27 +1374,41 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
 
 			if (sdt->app_tag == cpu_to_be16(0xffff)) {
-				sector++;
-				psg_off += sizeof(struct se_dif_v1_tuple);
-				continue;
+				dsg_off += block_size;
+				goto next;
+			}
+
+			avail = min(block_size, dsg->length - dsg_off);
+			crc = crc_t10dif(daddr + dsg_off, avail);
+			if (avail < block_size) {
+				kunmap_atomic(daddr - dsg->offset);
+				dsg = sg_next(dsg);
+				if (!dsg) {
+					kunmap_atomic(paddr - psg->offset);
+					return 0;
+				}
+				daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+				dsg_off = block_size - avail;
+				crc = crc_t10dif_update(crc, daddr, dsg_off);
+			} else {
+				dsg_off += block_size;
 			}
 
-			rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector,
-					       ei_lba);
+			rc = sbc_dif_v1_verify(cmd, sdt, crc, sector, ei_lba);
 			if (rc) {
-				kunmap_atomic(paddr - psg->offset);
 				kunmap_atomic(daddr - dsg->offset);
+				kunmap_atomic(paddr - psg->offset);
 				cmd->bad_sector = sector;
 				return rc;
 			}
-
+next:
 			sector++;
 			ei_lba++;
-			psg_off += sizeof(struct se_dif_v1_tuple);
 		}
 
-		kunmap_atomic(paddr - psg->offset);
+		psg_off = 0;
 		kunmap_atomic(daddr - dsg->offset);
+		kunmap_atomic(paddr - psg->offset);
 	}
 
 	return 0;
-- 
1.9.1

Re: [PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Tim Chen]

On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote:
> This introduces crc_t10dif_update() which enables to calculate CRC
> for a block which straddles multiple SG elements by calling multiple
> times.  This also converts crc_t10dif() to use crc_t10dif_update() as
> they are almost same.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: target-devel@vger.kernel.org
> ---
> * Changes from v3:
> - Reduce duplicated code between crc_t10dif_update and crc_t10dif,
>   suggested by Tim and Herbert
> 
>  include/linux/crc-t10dif.h |  1 +
>  lib/crc-t10dif.c           | 13 ++++++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h
> index cf53d07..d81961e 100644
> --- a/include/linux/crc-t10dif.h
> +++ b/include/linux/crc-t10dif.h
> @@ -9,5 +9,6 @@
>  extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer,
>  				size_t len);
>  extern __u16 crc_t10dif(unsigned char const *, size_t);
> +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t);
>  
>  #endif
> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
> index dfe6ec1..d775737 100644
> --- a/lib/crc-t10dif.c
> +++ b/lib/crc-t10dif.c
> @@ -19,7 +19,7 @@
>  static struct crypto_shash *crct10dif_tfm;
>  static struct static_key crct10dif_fallback __read_mostly;
>  
> -__u16 crc_t10dif(const unsigned char *buffer, size_t len)
> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
>  {
>  	struct {
>  		struct shash_desc shash;
> @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len)
>  	int err;
>  
>  	if (static_key_false(&crct10dif_fallback))
> -		return crc_t10dif_generic(0, buffer, len);
> +		return crc_t10dif_generic(crc, buffer, len);
>  
>  	desc.shash.tfm = crct10dif_tfm;
>  	desc.shash.flags = 0;
> -	*(__u16 *)desc.ctx = 0;
>  
> +	err = crypto_shash_import(&desc.shash, &crc);
> +	BUG_ON(err);
>  	err = crypto_shash_update(&desc.shash, buffer, len);
>  	BUG_ON(err);
>  
>  	return *(__u16 *)desc.ctx;
>  }
> +EXPORT_SYMBOL(crc_t10dif_update);
> +
> +__u16 crc_t10dif(const unsigned char *buffer, size_t len)
> +{
> +	return crc_t10dif_update(0, buffer, len);

Nitpicking a bit:

Will putting an extra function call to crc_t10dif_update will add extra
overhead to crc_t10dif, which is what most driver uses? As
we are calling crc_t10dif a lot (millions of times) as we go
through a block device, this extra function call
is undesirable.  Using a __crc_t10dif_update
inline function that both crc_t10dif_update and crc_t10dif
invokes can will avoid this overhead.

Tim

> +}
>  EXPORT_SYMBOL(crc_t10dif);
>  
>  static int __init crc_t10dif_mod_init(void)

Re: [PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Akinobu Mita]

2015-05-02 1:15 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>:
> On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote:
>> This introduces crc_t10dif_update() which enables to calculate CRC
>> for a block which straddles multiple SG elements by calling multiple
>> times.  This also converts crc_t10dif() to use crc_t10dif_update() as
>> they are almost same.
>>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>> Cc: Tim Chen <tim.c.chen@linux.intel.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: linux-crypto@vger.kernel.org
>> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
>> Cc: Sagi Grimberg <sagig@mellanox.com>
>> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
>> Cc: target-devel@vger.kernel.org
>> ---
>> * Changes from v3:
>> - Reduce duplicated code between crc_t10dif_update and crc_t10dif,
>>   suggested by Tim and Herbert
>>
>>  include/linux/crc-t10dif.h |  1 +
>>  lib/crc-t10dif.c           | 13 ++++++++++---
>>  2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h
>> index cf53d07..d81961e 100644
>> --- a/include/linux/crc-t10dif.h
>> +++ b/include/linux/crc-t10dif.h
>> @@ -9,5 +9,6 @@
>>  extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer,
>>                               size_t len);
>>  extern __u16 crc_t10dif(unsigned char const *, size_t);
>> +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t);
>>
>>  #endif
>> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
>> index dfe6ec1..d775737 100644
>> --- a/lib/crc-t10dif.c
>> +++ b/lib/crc-t10dif.c
>> @@ -19,7 +19,7 @@
>>  static struct crypto_shash *crct10dif_tfm;
>>  static struct static_key crct10dif_fallback __read_mostly;
>>
>> -__u16 crc_t10dif(const unsigned char *buffer, size_t len)
>> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
>>  {
>>       struct {
>>               struct shash_desc shash;
>> @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len)
>>       int err;
>>
>>       if (static_key_false(&crct10dif_fallback))
>> -             return crc_t10dif_generic(0, buffer, len);
>> +             return crc_t10dif_generic(crc, buffer, len);
>>
>>       desc.shash.tfm = crct10dif_tfm;
>>       desc.shash.flags = 0;
>> -     *(__u16 *)desc.ctx = 0;
>>
>> +     err = crypto_shash_import(&desc.shash, &crc);
>> +     BUG_ON(err);
>>       err = crypto_shash_update(&desc.shash, buffer, len);
>>       BUG_ON(err);
>>
>>       return *(__u16 *)desc.ctx;
>>  }
>> +EXPORT_SYMBOL(crc_t10dif_update);
>> +
>> +__u16 crc_t10dif(const unsigned char *buffer, size_t len)
>> +{
>> +     return crc_t10dif_update(0, buffer, len);
>
> Nitpicking a bit:
>
> Will putting an extra function call to crc_t10dif_update will add extra
> overhead to crc_t10dif, which is what most driver uses? As
> we are calling crc_t10dif a lot (millions of times) as we go
> through a block device, this extra function call
> is undesirable.  Using a __crc_t10dif_update
> inline function that both crc_t10dif_update and crc_t10dif
> invokes can will avoid this overhead.

I'll convert crc_t10dif to inline function.  But do we also need to
make crc_t10dif_update inline function? (i.e. is there any difference
between __crc_t10dif_update and crc_t10dif_update?)

Re: [PATCH v4 0/4] target: Fix several problems related to T10-PI support

[Nicholas A. Bellinger]

On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote:
> This patchset aims to fix several problems related to T10-PI support.
> 
> These patches can be applied on top of Sagi's "[v1] Simlify dif_verify
> routines and fixup fileio protection information code" patchset.
> 
> * Changes from v3:
> - Drop WRITE SAME support from this patch series, as it requires more
>   works than I expected in the first place.
> - Improve the condition in transport_generic_new_cmd() a bit
> - Reduce duplicated code between crc_t10dif_update and crc_t10dif,
>   suggested by Tim and Herbert
> - Fix inconsistent address passed to kunmap_atomic, reported by Sagi
> - Stop operation in sbc_dif_generate() and sbc_dif_verify() when
>   reaching the end of data SG elements
> 
> * Changes from v2:
> - Introduces crc_t10dif_update() to calculate CRC by multiple calls
> - Handle odd SG mapping correctly instead of giving up
> 
> * Changes from v1:
> - Reduce code duplication a bit in target_read_prot_action()
> - Fix sbc_dif_verify() for WRITE_SAME command
> - Fix inverted rw argument for fd_do_rw()
> - Perform DIF verify before write for WRITE_SAME
> 
> Akinobu Mita (4):
>   target: Fix inconsistent address passed to kunmap_atomic() in
>     sbc_dif_copy_prot()
>   target: ensure se_cmd->t_prot_sg is allocated when required
>   lib: introduce crc_t10dif_update()
>   target: handle odd SG mapping for data transfer memory
> 
>  drivers/target/target_core_sbc.c       | 127 ++++++++++++++++++++++-----------
>  drivers/target/target_core_transport.c |  27 +++----
>  include/linux/crc-t10dif.h             |   1 +
>  include/target/target_core_base.h      |   1 +
>  lib/crc-t10dif.c                       |  13 +++-
>  5 files changed, 113 insertions(+), 56 deletions(-)

Thanks Akinobu and Sagi.

Applied to target-pending/for-next as v4.2 code.

Feel free to send incremental patch to address the nit in patch #3, and
I'll apply + squash appropriately.

--nab

Re: [PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Tim Chen]

On Sat, 2015-05-02 at 10:38 +0900, Akinobu Mita wrote:
> 2015-05-02 1:15 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>:
> > On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote:
> >> This introduces crc_t10dif_update() which enables to calculate CRC
> >> for a block which straddles multiple SG elements by calling multiple
> >> times.  This also converts crc_t10dif() to use crc_t10dif_update() as
> >> they are almost same.
> >>
> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> >> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> >> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> >> Cc: "David S. Miller" <davem@davemloft.net>
> >> Cc: linux-crypto@vger.kernel.org
> >> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> >> Cc: Sagi Grimberg <sagig@mellanox.com>
> >> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> >> Cc: Christoph Hellwig <hch@lst.de>
> >> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> >> Cc: target-devel@vger.kernel.org
> >> ---
> >> * Changes from v3:
> >> - Reduce duplicated code between crc_t10dif_update and crc_t10dif,
> >>   suggested by Tim and Herbert
> >>
> >>  include/linux/crc-t10dif.h |  1 +
> >>  lib/crc-t10dif.c           | 13 ++++++++++---
> >>  2 files changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h
> >> index cf53d07..d81961e 100644
> >> --- a/include/linux/crc-t10dif.h
> >> +++ b/include/linux/crc-t10dif.h
> >> @@ -9,5 +9,6 @@
> >>  extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer,
> >>                               size_t len);
> >>  extern __u16 crc_t10dif(unsigned char const *, size_t);
> >> +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t);
> >>
> >>  #endif
> >> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
> >> index dfe6ec1..d775737 100644
> >> --- a/lib/crc-t10dif.c
> >> +++ b/lib/crc-t10dif.c
> >> @@ -19,7 +19,7 @@
> >>  static struct crypto_shash *crct10dif_tfm;
> >>  static struct static_key crct10dif_fallback __read_mostly;
> >>
> >> -__u16 crc_t10dif(const unsigned char *buffer, size_t len)
> >> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
> >>  {
> >>       struct {
> >>               struct shash_desc shash;
> >> @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len)
> >>       int err;
> >>
> >>       if (static_key_false(&crct10dif_fallback))
> >> -             return crc_t10dif_generic(0, buffer, len);
> >> +             return crc_t10dif_generic(crc, buffer, len);
> >>
> >>       desc.shash.tfm = crct10dif_tfm;
> >>       desc.shash.flags = 0;
> >> -     *(__u16 *)desc.ctx = 0;
> >>
> >> +     err = crypto_shash_import(&desc.shash, &crc);
> >> +     BUG_ON(err);
> >>       err = crypto_shash_update(&desc.shash, buffer, len);
> >>       BUG_ON(err);
> >>
> >>       return *(__u16 *)desc.ctx;
> >>  }
> >> +EXPORT_SYMBOL(crc_t10dif_update);
> >> +
> >> +__u16 crc_t10dif(const unsigned char *buffer, size_t len)
> >> +{
> >> +     return crc_t10dif_update(0, buffer, len);
> >
> > Nitpicking a bit:
> >
> > Will putting an extra function call to crc_t10dif_update will add extra
> > overhead to crc_t10dif, which is what most driver uses? As
> > we are calling crc_t10dif a lot (millions of times) as we go
> > through a block device, this extra function call
> > is undesirable.  Using a __crc_t10dif_update
> > inline function that both crc_t10dif_update and crc_t10dif
> > invokes can will avoid this overhead.
> 
> I'll convert crc_t10dif to inline function.  But do we also need to
> make crc_t10dif_update inline function? (i.e. is there any difference
> between __crc_t10dif_update and crc_t10dif_update?)

I don't mean convert crc_t10dif to inline, but make a local inline function
__crc_t10dif_update that both crc_t10dif_update and crc_t10dif can use
so crc_t10dif don't have to do one more function call.  Yes, crc_t10dif_update
and __crc_t10dif_update are equivalent but since the latter is inlined, so there's no
performance impact and we get rid of the overhead of the extra function
call in crc_t10dif.

Like

+
+__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
+{
+       return __crc_t10dif_update(crc, buffer, len, true);
+}
+EXPORT_SYMBOL(crc_t10dif_update);
+
+__u16 crc_t10dif(const unsigned char *buffer, size_t len)
+{
+       return __crc_t10dif_update(0, buffer, len, false);
+}
 EXPORT_SYMBOL(crc_t10dif);

Thanks.

Tim

Re: [PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Akinobu Mita]

2015-05-05 1:09 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>:
> On Sat, 2015-05-02 at 10:38 +0900, Akinobu Mita wrote:
>> 2015-05-02 1:15 GMT+09:00 Tim Chen <tim.c.chen@linux.intel.com>:
>> > On Fri, 2015-05-01 at 15:23 +0900, Akinobu Mita wrote:
>> >> This introduces crc_t10dif_update() which enables to calculate CRC
>> >> for a block which straddles multiple SG elements by calling multiple
>> >> times.  This also converts crc_t10dif() to use crc_t10dif_update() as
>> >> they are almost same.
>> >>
>> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> >> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
>> >> Cc: Tim Chen <tim.c.chen@linux.intel.com>
>> >> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> >> Cc: "David S. Miller" <davem@davemloft.net>
>> >> Cc: linux-crypto@vger.kernel.org
>> >> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
>> >> Cc: Sagi Grimberg <sagig@mellanox.com>
>> >> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
>> >> Cc: Christoph Hellwig <hch@lst.de>
>> >> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
>> >> Cc: target-devel@vger.kernel.org
>> >> ---
>> >> * Changes from v3:
>> >> - Reduce duplicated code between crc_t10dif_update and crc_t10dif,
>> >>   suggested by Tim and Herbert
>> >>
>> >>  include/linux/crc-t10dif.h |  1 +
>> >>  lib/crc-t10dif.c           | 13 ++++++++++---
>> >>  2 files changed, 11 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/include/linux/crc-t10dif.h b/include/linux/crc-t10dif.h
>> >> index cf53d07..d81961e 100644
>> >> --- a/include/linux/crc-t10dif.h
>> >> +++ b/include/linux/crc-t10dif.h
>> >> @@ -9,5 +9,6 @@
>> >>  extern __u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer,
>> >>                               size_t len);
>> >>  extern __u16 crc_t10dif(unsigned char const *, size_t);
>> >> +extern __u16 crc_t10dif_update(__u16 crc, unsigned char const *, size_t);
>> >>
>> >>  #endif
>> >> diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c
>> >> index dfe6ec1..d775737 100644
>> >> --- a/lib/crc-t10dif.c
>> >> +++ b/lib/crc-t10dif.c
>> >> @@ -19,7 +19,7 @@
>> >>  static struct crypto_shash *crct10dif_tfm;
>> >>  static struct static_key crct10dif_fallback __read_mostly;
>> >>
>> >> -__u16 crc_t10dif(const unsigned char *buffer, size_t len)
>> >> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
>> >>  {
>> >>       struct {
>> >>               struct shash_desc shash;
>> >> @@ -28,17 +28,24 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len)
>> >>       int err;
>> >>
>> >>       if (static_key_false(&crct10dif_fallback))
>> >> -             return crc_t10dif_generic(0, buffer, len);
>> >> +             return crc_t10dif_generic(crc, buffer, len);
>> >>
>> >>       desc.shash.tfm = crct10dif_tfm;
>> >>       desc.shash.flags = 0;
>> >> -     *(__u16 *)desc.ctx = 0;
>> >>
>> >> +     err = crypto_shash_import(&desc.shash, &crc);
>> >> +     BUG_ON(err);
>> >>       err = crypto_shash_update(&desc.shash, buffer, len);
>> >>       BUG_ON(err);
>> >>
>> >>       return *(__u16 *)desc.ctx;
>> >>  }
>> >> +EXPORT_SYMBOL(crc_t10dif_update);
>> >> +
>> >> +__u16 crc_t10dif(const unsigned char *buffer, size_t len)
>> >> +{
>> >> +     return crc_t10dif_update(0, buffer, len);
>> >
>> > Nitpicking a bit:
>> >
>> > Will putting an extra function call to crc_t10dif_update will add extra
>> > overhead to crc_t10dif, which is what most driver uses? As
>> > we are calling crc_t10dif a lot (millions of times) as we go
>> > through a block device, this extra function call
>> > is undesirable.  Using a __crc_t10dif_update
>> > inline function that both crc_t10dif_update and crc_t10dif
>> > invokes can will avoid this overhead.
>>
>> I'll convert crc_t10dif to inline function.  But do we also need to
>> make crc_t10dif_update inline function? (i.e. is there any difference
>> between __crc_t10dif_update and crc_t10dif_update?)
>
> I don't mean convert crc_t10dif to inline, but make a local inline function
> __crc_t10dif_update that both crc_t10dif_update and crc_t10dif can use
> so crc_t10dif don't have to do one more function call.  Yes, crc_t10dif_update
> and __crc_t10dif_update are equivalent but since the latter is inlined, so there's no
> performance impact and we get rid of the overhead of the extra function
> call in crc_t10dif.
>
> Like
>
> +
> +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
> +{
> +       return __crc_t10dif_update(crc, buffer, len, true);
> +}
> +EXPORT_SYMBOL(crc_t10dif_update);
> +
> +__u16 crc_t10dif(const unsigned char *buffer, size_t len)
> +{
> +       return __crc_t10dif_update(0, buffer, len, false);
> +}
>  EXPORT_SYMBOL(crc_t10dif);

You mean that we should avoid crypto_shash_import() function call
in crc_t10dif(), right?

The reason for crypto_shash_import() was I wanted to make
crc_t10dif_update() more generic.  But as far as I can see existing
crct10dif modules (crct10dif-generic and crct10dif-pclmul), we don't
need it (i.e. '*(__u16 *)desc.ctx = crc' instead of crc_t10dif_import)

Do you think it's better to remove crc_t10dif_import call from
crc_t10dif() and crc_t10dif_update() altogether?

Re: [PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Herbert Xu]

On Tue, May 05, 2015 at 01:22:03PM +0900, Akinobu Mita wrote:
>
> The reason for crypto_shash_import() was I wanted to make
> crc_t10dif_update() more generic.  But as far as I can see existing
> crct10dif modules (crct10dif-generic and crct10dif-pclmul), we don't
> need it (i.e. '*(__u16 *)desc.ctx = crc' instead of crc_t10dif_import)

Yes we do the same thing with crc32c so I don't see why you couldn't
do that here.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Tim Chen]

On Tue, 2015-05-05 at 13:22 +0900, Akinobu Mita wrote:

> >
> > +
> > +__u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len)
> > +{
> > +       return __crc_t10dif_update(crc, buffer, len, true);
> > +}
> > +EXPORT_SYMBOL(crc_t10dif_update);
> > +
> > +__u16 crc_t10dif(const unsigned char *buffer, size_t len)
> > +{
> > +       return __crc_t10dif_update(0, buffer, len, false);
> > +}
> >  EXPORT_SYMBOL(crc_t10dif);
> 
> You mean that we should avoid crypto_shash_import() function call
> in crc_t10dif(), right?
> 

I think it is okay for crypto_shash_import().
 
Just that we can eliminate an extra call from crc_t10dif
to crc_t10_dif as in your proposed patch, 

> +EXPORT_SYMBOL(crc_t10dif_update);
> +
> +__u16 crc_t10dif(const unsigned char *buffer, size_t len)
> + {
> +     return crc_t10dif_update(0, buffer, len);
> 

but use a local inlined __crc_t10dif_update.

Tim

Re: [PATCH v4 3/4] lib: introduce crc_t10dif_update()

[Tim Chen]

On Tue, 2015-05-05 at 13:22 +0900, Akinobu Mita wrote:

> The reason for crypto_shash_import() was I wanted to make
> crc_t10dif_update() more generic.  But as far as I can see existing
> crct10dif modules (crct10dif-generic and crct10dif-pclmul), we don't
> need it (i.e. '*(__u16 *)desc.ctx = crc' instead of crc_t10dif_import)
> 
> Do you think it's better to remove crc_t10dif_import call from
> crc_t10dif() and crc_t10dif_update() altogether?

Yes, it will be better if we can eliminate crc_t10dif_import call if
we can do a direct assignment.

Tim