[RFC] Simlify dif_verify routines and fixup fileio protection information code.

[RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

Hey All,

This set follows the patchset from Akinobu Mita that addresses
DIF bounce buffer sgl construction. Instead of trying to fix these
bugs, this removes it altogether and work with cmd->t_prot_sg
directly.

The first patch is a simplification of the DIF verify varius
routines leaving a single generic sbc_dif_verify that handles
the protection information sgl we are working on.

The second patch uses this simplification to remove the local
prot_fd bounce buffer altogether.

This passed minimal IO testing.

Sagi Grimberg (2):
  target: Merge sbc_verify_dif_read|write
  target/file: Remove fd_prot bounce buffer

 drivers/target/target_core_file.c      |  140 +++++++-------------------------
 drivers/target/target_core_file.h      |    6 --
 drivers/target/target_core_rd.c        |   20 +++--
 drivers/target/target_core_sbc.c       |   94 ++--------------------
 drivers/target/target_core_transport.c |   17 ++--
 include/target/target_core_backend.h   |    8 +-
 6 files changed, 62 insertions(+), 223 deletions(-)

[PATCH RFC 1/2] target: Merge sbc_verify_dif_read|write

[Sagi Grimberg]

Instead of providing DIF verify routines for read/write
that are almost identical and conditionally copy protection
information, just let the caller do the right thing.

Have a single sbc_dif_verify that handles an sgl (that
does NOT copy any data) and a protection information copy
routine used by rd_mcp and fileio backend.

In the WRITE case, call sbc_dif_verify with cmd->t_prot_sg
and then do the copy from it to local sgl (assuming the verify
succeeded of course). In the READ case, call sbc_dif_verify
with the local sgl and if it succeeds, copy it to t_prot_sg (or
not if we are stripping it).

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/target/target_core_file.c      |   14 ++++--
 drivers/target/target_core_rd.c        |   20 +++++---
 drivers/target/target_core_sbc.c       |   94 ++------------------------------
 drivers/target/target_core_transport.c |   17 +++---
 include/target/target_core_backend.h   |    8 +--
 5 files changed, 41 insertions(+), 112 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index fa54835..3476397 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -595,12 +595,15 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 		if (ret > 0 && cmd->prot_type && dev->dev_attrib.pi_prot_type) {
 			u32 sectors = cmd->data_length / dev->dev_attrib.block_size;
 
-			rc = sbc_dif_verify_read(cmd, cmd->t_task_lba, sectors,
-						 0, fd_prot.prot_sg, 0);
+			rc = sbc_dif_verify(cmd, cmd->t_task_lba, sectors,
+					    0, fd_prot.prot_sg, 0);
 			if (rc) {
 				kfree(fd_prot.prot_sg);
 				vfree(fd_prot.prot_buf);
 				return rc;
+			} else {
+				sbc_dif_copy_prot(cmd, sectors, true,
+						  fd_prot.prot_sg, 0);
 			}
 			kfree(fd_prot.prot_sg);
 			vfree(fd_prot.prot_buf);
@@ -615,12 +618,15 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 			if (ret < 0)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-			rc = sbc_dif_verify_write(cmd, cmd->t_task_lba, sectors,
-						  0, fd_prot.prot_sg, 0);
+			rc = sbc_dif_verify(cmd, cmd->t_task_lba, sectors,
+					    0, cmd->t_prot_sg, 0);
 			if (rc) {
 				kfree(fd_prot.prot_sg);
 				vfree(fd_prot.prot_buf);
 				return rc;
+			} else {
+				sbc_dif_copy_prot(cmd, sectors, false,
+						  fd_prot.prot_sg, 0);
 			}
 		}
 
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index a263bf5..12fbe90 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -403,10 +403,7 @@ static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page
 	return NULL;
 }
 
-typedef sense_reason_t (*dif_verify)(struct se_cmd *, sector_t, unsigned int,
-				     unsigned int, struct scatterlist *, int);
-
-static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify)
+static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, bool is_read)
 {
 	struct se_device *se_dev = cmd->se_dev;
 	struct rd_dev *dev = RD_DEV(se_dev);
@@ -466,7 +463,16 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify)
 
 #endif /* !CONFIG_ARCH_HAS_SG_CHAIN */
 
-	rc = dif_verify(cmd, cmd->t_task_lba, sectors, 0, prot_sg, prot_offset);
+	if (is_read)
+		rc = sbc_dif_verify(cmd, cmd->t_task_lba, sectors, 0,
+				    prot_sg, prot_offset);
+	else
+		rc = sbc_dif_verify(cmd, cmd->t_task_lba, sectors, 0,
+				    cmd->t_prot_sg, 0);
+
+	if (!rc)
+		sbc_dif_copy_prot(cmd, sectors, is_read, prot_sg, prot_offset);
+
 	if (need_to_release)
 		kfree(prot_sg);
 
@@ -512,7 +518,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 
 	if (cmd->prot_type && se_dev->dev_attrib.pi_prot_type &&
 	    data_direction == DMA_TO_DEVICE) {
-		rc = rd_do_prot_rw(cmd, sbc_dif_verify_write);
+		rc = rd_do_prot_rw(cmd, true);
 		if (rc)
 			return rc;
 	}
@@ -580,7 +586,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 
 	if (cmd->prot_type && se_dev->dev_attrib.pi_prot_type &&
 	    data_direction == DMA_FROM_DEVICE) {
-		rc = rd_do_prot_rw(cmd, sbc_dif_verify_read);
+		rc = rd_do_prot_rw(cmd, false);
 		if (rc)
 			return rc;
 	}
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 0064ffe..c0e4175 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1254,9 +1254,8 @@ sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
 	return 0;
 }
 
-static void
-sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
-		  struct scatterlist *sg, int sg_off)
+void sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
+		       struct scatterlist *sg, int sg_off)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct scatterlist *psg;
@@ -1297,68 +1296,11 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
 		kunmap_atomic(paddr);
 	}
 }
+EXPORT_SYMBOL(sbc_dif_copy_prot);
 
 sense_reason_t
-sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
-		     unsigned int ei_lba, struct scatterlist *sg, int sg_off)
-{
-	struct se_device *dev = cmd->se_dev;
-	struct se_dif_v1_tuple *sdt;
-	struct scatterlist *dsg, *psg = cmd->t_prot_sg;
-	sector_t sector = start;
-	void *daddr, *paddr;
-	int i, j, offset = 0;
-	sense_reason_t rc;
-
-	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
-		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
-		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
-
-		for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
-
-			if (offset >= psg->length) {
-				kunmap_atomic(paddr);
-				psg = sg_next(psg);
-				paddr = kmap_atomic(sg_page(psg)) + psg->offset;
-				offset = 0;
-			}
-
-			sdt = paddr + offset;
-
-			pr_debug("DIF WRITE sector: %llu guard_tag: 0x%04x"
-				 " app_tag: 0x%04x ref_tag: %u\n",
-				 (unsigned long long)sector, sdt->guard_tag,
-				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
-
-			rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector,
-					       ei_lba);
-			if (rc) {
-				kunmap_atomic(paddr);
-				kunmap_atomic(daddr);
-				cmd->bad_sector = sector;
-				return rc;
-			}
-
-			sector++;
-			ei_lba++;
-			offset += sizeof(struct se_dif_v1_tuple);
-		}
-
-		kunmap_atomic(paddr);
-		kunmap_atomic(daddr);
-	}
-	if (!sg)
-		return 0;
-
-	sbc_dif_copy_prot(cmd, sectors, false, sg, sg_off);
-
-	return 0;
-}
-EXPORT_SYMBOL(sbc_dif_verify_write);
-
-static sense_reason_t
-__sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
-		      unsigned int ei_lba, struct scatterlist *sg, int sg_off)
+sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
+	       unsigned int ei_lba, struct scatterlist *sg, int sg_off)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct se_dif_v1_tuple *sdt;
@@ -1414,28 +1356,4 @@ __sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 
 	return 0;
 }
-
-sense_reason_t
-sbc_dif_read_strip(struct se_cmd *cmd)
-{
-	struct se_device *dev = cmd->se_dev;
-	u32 sectors = cmd->prot_length / dev->prot_length;
-
-	return __sbc_dif_verify_read(cmd, cmd->t_task_lba, sectors, 0,
-				     cmd->t_prot_sg, 0);
-}
-
-sense_reason_t
-sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
-		    unsigned int ei_lba, struct scatterlist *sg, int sg_off)
-{
-	sense_reason_t rc;
-
-	rc = __sbc_dif_verify_read(cmd, start, sectors, ei_lba, sg, sg_off);
-	if (rc)
-		return rc;
-
-	sbc_dif_copy_prot(cmd, sectors, true, sg, sg_off);
-	return 0;
-}
-EXPORT_SYMBOL(sbc_dif_verify_read);
+EXPORT_SYMBOL(sbc_dif_verify);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4edb183..5e97757 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1758,8 +1758,8 @@ static int target_write_prot_action(struct se_cmd *cmd)
 			break;
 
 		sectors = cmd->data_length >> ilog2(cmd->se_dev->dev_attrib.block_size);
-		cmd->pi_err = sbc_dif_verify_write(cmd, cmd->t_task_lba,
-						   sectors, 0, NULL, 0);
+		cmd->pi_err = sbc_dif_verify(cmd, cmd->t_task_lba,
+					     sectors, 0, cmd->t_prot_sg, 0);
 		if (unlikely(cmd->pi_err)) {
 			spin_lock_irq(&cmd->t_state_lock);
 			cmd->transport_state &= ~CMD_T_BUSY|CMD_T_SENT;
@@ -1984,16 +1984,17 @@ static void transport_handle_queue_full(
 
 static bool target_read_prot_action(struct se_cmd *cmd)
 {
-	sense_reason_t rc;
-
 	switch (cmd->prot_op) {
 	case TARGET_PROT_DIN_STRIP:
 		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
-			rc = sbc_dif_read_strip(cmd);
-			if (rc) {
-				cmd->pi_err = rc;
+			u32 sectors = cmd->data_length >>
+				  ilog2(cmd->se_dev->dev_attrib.block_size);
+
+			cmd->pi_err = sbc_dif_verify(cmd, cmd->t_task_lba,
+						     sectors, 0, cmd->t_prot_sg,
+						     0);
+			if (cmd->pi_err)
 				return true;
-			}
 		}
 		break;
 	case TARGET_PROT_DIN_INSERT:
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index db81c65..45df9e6 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -86,12 +86,10 @@ sense_reason_t sbc_execute_unmap(struct se_cmd *cmd,
 				      sector_t lba, sector_t nolb),
 	void *priv);
 void	sbc_dif_generate(struct se_cmd *);
-sense_reason_t	sbc_dif_verify_write(struct se_cmd *, sector_t, unsigned int,
+sense_reason_t	sbc_dif_verify(struct se_cmd *, sector_t, unsigned int,
 				     unsigned int, struct scatterlist *, int);
-sense_reason_t	sbc_dif_verify_read(struct se_cmd *, sector_t, unsigned int,
-				    unsigned int, struct scatterlist *, int);
-sense_reason_t	sbc_dif_read_strip(struct se_cmd *);
-
+void sbc_dif_copy_prot(struct se_cmd *, unsigned int, bool,
+		       struct scatterlist *, int);
 void	transport_set_vpd_proto_id(struct t10_vpd *, unsigned char *);
 int	transport_set_vpd_assoc(struct t10_vpd *, unsigned char *);
 int	transport_set_vpd_ident_type(struct t10_vpd *, unsigned char *);
-- 
1.7.1

[PATCH RFC 2/2] target/file: Remove fd_prot bounce buffer

[Sagi Grimberg]

The reason this bounce buffer exists is to allow code
reuse between rd_mcp and fileio in DIF mode. But the fact is,
that this bounce is really not needed at all, we can simply call
sbc_dif_verify on cmd->t_prot_sg and use it for file IO.

This also removes fd_do_prot_rw as fd_do_rw was generalised
to receive file pointer, block size (8 bytes for DIF data) and
total data length.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/target/target_core_file.c |  140 +++++++-----------------------------
 drivers/target/target_core_file.h |    6 --
 2 files changed, 28 insertions(+), 118 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 3476397..d43638f 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -258,83 +258,15 @@ static void fd_free_device(struct se_device *dev)
 	kfree(fd_dev);
 }
 
-static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot,
-			 int is_write)
+static int fd_do_rw(struct se_cmd *cmd, struct file *fd,
+		    u32 block_size, struct scatterlist *sgl,
+		    u32 sgl_nents, u32 data_length, int is_write)
 {
-	struct se_device *se_dev = cmd->se_dev;
-	struct fd_dev *dev = FD_DEV(se_dev);
-	struct file *prot_fd = dev->fd_prot_file;
-	struct scatterlist *sg;
-	loff_t pos = (cmd->t_task_lba * se_dev->prot_length);
-	unsigned char *buf;
-	u32 prot_size, len, size;
-	int rc, ret = 1, i;
-
-	prot_size = (cmd->data_length / se_dev->dev_attrib.block_size) *
-		     se_dev->prot_length;
-
-	if (!is_write) {
-		fd_prot->prot_buf = vzalloc(prot_size);
-		if (!fd_prot->prot_buf) {
-			pr_err("Unable to allocate fd_prot->prot_buf\n");
-			return -ENOMEM;
-		}
-		buf = fd_prot->prot_buf;
-
-		fd_prot->prot_sg_nents = cmd->t_prot_nents;
-		fd_prot->prot_sg = kzalloc(sizeof(struct scatterlist) *
-					   fd_prot->prot_sg_nents, GFP_KERNEL);
-		if (!fd_prot->prot_sg) {
-			pr_err("Unable to allocate fd_prot->prot_sg\n");
-			vfree(fd_prot->prot_buf);
-			return -ENOMEM;
-		}
-		size = prot_size;
-
-		for_each_sg(fd_prot->prot_sg, sg, fd_prot->prot_sg_nents, i) {
-
-			len = min_t(u32, PAGE_SIZE, size);
-			sg_set_buf(sg, buf, len);
-			size -= len;
-			buf += len;
-		}
-	}
-
-	if (is_write) {
-		rc = kernel_write(prot_fd, fd_prot->prot_buf, prot_size, pos);
-		if (rc < 0 || prot_size != rc) {
-			pr_err("kernel_write() for fd_do_prot_rw failed:"
-			       " %d\n", rc);
-			ret = -EINVAL;
-		}
-	} else {
-		rc = kernel_read(prot_fd, pos, fd_prot->prot_buf, prot_size);
-		if (rc < 0) {
-			pr_err("kernel_read() for fd_do_prot_rw failed:"
-			       " %d\n", rc);
-			ret = -EINVAL;
-		}
-	}
-
-	if (is_write || ret < 0) {
-		kfree(fd_prot->prot_sg);
-		vfree(fd_prot->prot_buf);
-	}
-
-	return ret;
-}
-
-static int fd_do_rw(struct se_cmd *cmd, struct scatterlist *sgl,
-		u32 sgl_nents, int is_write)
-{
-	struct se_device *se_dev = cmd->se_dev;
-	struct fd_dev *dev = FD_DEV(se_dev);
-	struct file *fd = dev->fd_file;
 	struct scatterlist *sg;
 	struct iov_iter iter;
 	struct bio_vec *bvec;
 	ssize_t len = 0;
-	loff_t pos = (cmd->t_task_lba * se_dev->dev_attrib.block_size);
+	loff_t pos = (cmd->t_task_lba * block_size);
 	int ret = 0, i;
 
 	bvec = kcalloc(sgl_nents, sizeof(struct bio_vec), GFP_KERNEL);
@@ -360,7 +292,7 @@ static int fd_do_rw(struct se_cmd *cmd, struct scatterlist *sgl,
 	kfree(bvec);
 
 	if (is_write) {
-		if (ret < 0 || ret != cmd->data_length) {
+		if (ret < 0 || ret != data_length) {
 			pr_err("%s() write returned %d\n", __func__, ret);
 			return (ret < 0 ? ret : -EINVAL);
 		}
@@ -371,10 +303,10 @@ static int fd_do_rw(struct se_cmd *cmd, struct scatterlist *sgl,
 		 * block_device.
 		 */
 		if (S_ISBLK(file_inode(fd)->i_mode)) {
-			if (ret < 0 || ret != cmd->data_length) {
+			if (ret < 0 || ret != data_length) {
 				pr_err("%s() returned %d, expecting %u for "
 						"S_ISBLK\n", __func__, ret,
-						cmd->data_length);
+						data_length);
 				return (ret < 0 ? ret : -EINVAL);
 			}
 		} else {
@@ -564,7 +496,9 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	      enum dma_data_direction data_direction)
 {
 	struct se_device *dev = cmd->se_dev;
-	struct fd_prot fd_prot;
+	struct fd_dev *fd_dev = FD_DEV(dev);
+	struct file *file = fd_dev->fd_file;
+	struct file *pfile = fd_dev->fd_prot_file;
 	sense_reason_t rc;
 	int ret = 0;
 	/*
@@ -582,55 +516,39 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	 * physical memory addresses to struct iovec virtual memory.
 	 */
 	if (data_direction == DMA_FROM_DEVICE) {
-		memset(&fd_prot, 0, sizeof(struct fd_prot));
-
 		if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
-			ret = fd_do_prot_rw(cmd, &fd_prot, false);
+			ret = fd_do_rw(cmd, pfile, dev->prot_length,
+				       cmd->t_prot_sg, cmd->t_prot_nents,
+				       cmd->prot_length, 0);
 			if (ret < 0)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
 
-		ret = fd_do_rw(cmd, sgl, sgl_nents, 0);
+		ret = fd_do_rw(cmd, file, dev->dev_attrib.block_size,
+			       sgl, sgl_nents, cmd->data_length, 0);
 
 		if (ret > 0 && cmd->prot_type && dev->dev_attrib.pi_prot_type) {
-			u32 sectors = cmd->data_length / dev->dev_attrib.block_size;
+			u32 sectors = cmd->data_length >>
+					ilog2(dev->dev_attrib.block_size);
 
 			rc = sbc_dif_verify(cmd, cmd->t_task_lba, sectors,
-					    0, fd_prot.prot_sg, 0);
-			if (rc) {
-				kfree(fd_prot.prot_sg);
-				vfree(fd_prot.prot_buf);
+					    0, cmd->t_prot_sg, 0);
+			if (rc)
 				return rc;
-			} else {
-				sbc_dif_copy_prot(cmd, sectors, true,
-						  fd_prot.prot_sg, 0);
-			}
-			kfree(fd_prot.prot_sg);
-			vfree(fd_prot.prot_buf);
 		}
 	} else {
-		memset(&fd_prot, 0, sizeof(struct fd_prot));
-
 		if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
-			u32 sectors = cmd->data_length / dev->dev_attrib.block_size;
-
-			ret = fd_do_prot_rw(cmd, &fd_prot, false);
-			if (ret < 0)
-				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+			u32 sectors = cmd->data_length >>
+					ilog2(dev->dev_attrib.block_size);
 
 			rc = sbc_dif_verify(cmd, cmd->t_task_lba, sectors,
 					    0, cmd->t_prot_sg, 0);
-			if (rc) {
-				kfree(fd_prot.prot_sg);
-				vfree(fd_prot.prot_buf);
+			if (rc)
 				return rc;
-			} else {
-				sbc_dif_copy_prot(cmd, sectors, false,
-						  fd_prot.prot_sg, 0);
-			}
 		}
 
-		ret = fd_do_rw(cmd, sgl, sgl_nents, 1);
+		ret = fd_do_rw(cmd, file, dev->dev_attrib.block_size,
+			       sgl, sgl_nents, cmd->data_length, 1);
 		/*
 		 * Perform implicit vfs_fsync_range() for fd_do_writev() ops
 		 * for SCSI WRITEs with Forced Unit Access (FUA) set.
@@ -639,7 +557,6 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 		if (ret > 0 &&
 		    dev->dev_attrib.emulate_fua_write > 0 &&
 		    (cmd->se_cmd_flags & SCF_FUA)) {
-			struct fd_dev *fd_dev = FD_DEV(dev);
 			loff_t start = cmd->t_task_lba *
 				dev->dev_attrib.block_size;
 			loff_t end;
@@ -653,17 +570,16 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 		}
 
 		if (ret > 0 && cmd->prot_type && dev->dev_attrib.pi_prot_type) {
-			ret = fd_do_prot_rw(cmd, &fd_prot, true);
+			ret = fd_do_rw(cmd, pfile, dev->prot_length,
+				       cmd->t_prot_sg, cmd->t_prot_nents,
+				       cmd->prot_length, 1);
 			if (ret < 0)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
 	}
 
-	if (ret < 0) {
-		kfree(fd_prot.prot_sg);
-		vfree(fd_prot.prot_buf);
+	if (ret < 0)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-	}
 
 	if (ret)
 		target_complete_cmd(cmd, SAM_STAT_GOOD);
diff --git a/drivers/target/target_core_file.h b/drivers/target/target_core_file.h
index 182cbb2..068966f 100644
--- a/drivers/target/target_core_file.h
+++ b/drivers/target/target_core_file.h
@@ -21,12 +21,6 @@
 #define FDBD_HAS_BUFFERED_IO_WCE 0x04
 #define FDBD_FORMAT_UNIT_SIZE	2048
 
-struct fd_prot {
-	unsigned char	*prot_buf;
-	struct scatterlist *prot_sg;
-	u32 prot_sg_nents;
-};
-
 struct fd_dev {
 	struct se_device dev;
 
-- 
1.7.1

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@mellanox.com> writes:

Sagi> This set follows the patchset from Akinobu Mita that addresses DIF
Sagi> bounce buffer sgl construction. Instead of trying to fix these
Sagi> bugs, this removes it altogether and work with cmd->t_prot_sg
Sagi> directly.

Sagi> The first patch is a simplification of the DIF verify varius
Sagi> routines leaving a single generic sbc_dif_verify that handles the
Sagi> protection information sgl we are working on.

Sagi> The second patch uses this simplification to remove the local
Sagi> prot_fd bounce buffer altogether.

Looks like a nice cleanup to me.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Akinobu Mita]

2015-04-14 2:19 GMT+09:00 Sagi Grimberg <sagig@mellanox.com>:
> Hey All,
>
> This set follows the patchset from Akinobu Mita that addresses
> DIF bounce buffer sgl construction. Instead of trying to fix these
> bugs, this removes it altogether and work with cmd->t_prot_sg
> directly.
>
> The first patch is a simplification of the DIF verify varius
> routines leaving a single generic sbc_dif_verify that handles
> the protection information sgl we are working on.
>
> The second patch uses this simplification to remove the local
> prot_fd bounce buffer altogether.
>
> This passed minimal IO testing.

Looks good...

I'll test with these patches and check if the problems I met
disappear.

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

On 4/14/2015 3:17 PM, Akinobu Mita wrote:
> 2015-04-14 2:19 GMT+09:00 Sagi Grimberg <sagig@mellanox.com>:
>> Hey All,
>>
>> This set follows the patchset from Akinobu Mita that addresses
>> DIF bounce buffer sgl construction. Instead of trying to fix these
>> bugs, this removes it altogether and work with cmd->t_prot_sg
>> directly.
>>
>> The first patch is a simplification of the DIF verify varius
>> routines leaving a single generic sbc_dif_verify that handles
>> the protection information sgl we are working on.
>>
>> The second patch uses this simplification to remove the local
>> prot_fd bounce buffer altogether.
>>
>> This passed minimal IO testing.
>
> Looks good...
>
> I'll test with these patches and check if the problems I met
> disappear.

Thanks Akinobu,

Waiting to hear your verdict before sending a formal patchset.

Sagi.

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Akinobu Mita]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

2015-04-15 2:20 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
> On 4/14/2015 3:17 PM, Akinobu Mita wrote:
>>
>> 2015-04-14 2:19 GMT+09:00 Sagi Grimberg <sagig@mellanox.com>:
>>>
>>> Hey All,
>>>
>>> This set follows the patchset from Akinobu Mita that addresses
>>> DIF bounce buffer sgl construction. Instead of trying to fix these
>>> bugs, this removes it altogether and work with cmd->t_prot_sg
>>> directly.
>>>
>>> The first patch is a simplification of the DIF verify varius
>>> routines leaving a single generic sbc_dif_verify that handles
>>> the protection information sgl we are working on.
>>>
>>> The second patch uses this simplification to remove the local
>>> prot_fd bounce buffer altogether.
>>>
>>> This passed minimal IO testing.
>>
>>
>> Looks good...
>>
>> I'll test with these patches and check if the problems I met
>> disappear.
>
>
> Thanks Akinobu,
>
> Waiting to hear your verdict before sending a formal patchset.

I hit a original bug in sbc_dif_verify() which is not introduced by
your patch set, though.  Please consider to include attached patch.
I'm still seeing another problem and trying to find out a root cause,
but it seems like it's caused by other change in -next.

[-- Attachment #2: fix-sbc_dif_verify.patch --]
[-- Type: text/x-patch, Size: 571 bytes --]

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 3d88c00..65a0b5f 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1311,7 +1311,7 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 
 	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
 		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
-		paddr = kmap_atomic(sg_page(psg)) + sg->offset;
+		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
 
 		for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
 

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

On 4/15/2015 2:52 AM, Akinobu Mita wrote:

>>>
>>> Looks good...
>>>
>>> I'll test with these patches and check if the problems I met
>>> disappear.
>>
>>
>> Thanks Akinobu,
>>
>> Waiting to hear your verdict before sending a formal patchset.
>
> I hit a original bug in sbc_dif_verify() which is not introduced by
> your patch set, though.

What is this original bug?

>  Please consider to include attached patch.

I'll include it. thanks.

> I'm still seeing another problem and trying to find out a root cause,
> but it seems like it's caused by other change in -next.
>

care to elaborate?

Sagi.

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Akinobu Mita]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]

2015-04-15 19:07 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
> On 4/15/2015 2:52 AM, Akinobu Mita wrote:
>
>>>>
>>>> Looks good...
>>>>
>>>> I'll test with these patches and check if the problems I met
>>>> disappear.
>>>
>>>
>>>
>>> Thanks Akinobu,
>>>
>>> Waiting to hear your verdict before sending a formal patchset.
>>
>>
>> I hit a original bug in sbc_dif_verify() which is not introduced by
>> your patch set, though.
>
>
> What is this original bug?

I meant to say about the problem I fixed by fix-sbc_dif_verify.patch.

>>  Please consider to include attached patch.
>
>
> I'll include it. thanks.
>
>> I'm still seeing another problem and trying to find out a root cause,
>> but it seems like it's caused by other change in -next.
>>
>
> care to elaborate?

When mounting ext4 filesystem at the first time after mkfs, a lot of
WRITE_SAME commands are issued for lazy initialization or something.

By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
support"), When WRITE_SAME command with WRPROTECT=0 is executed,
sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block layer
didn't allocate it for WRITE_SAME.

I could work around with the attached patch, as the WRITE_SAME command
will fail after all when protection info is enabled with FILEIO, we only need to
avoid null pointer dereference.  But I need to ask Nic about the right
way to fix.

For this patch set, please feel free to add:

Tested-by: Akinobu Mita <akinobu.mita@gmail.com>

[-- Attachment #2: fix-sbc_dif_generate.patch --]
[-- Type: text/x-patch, Size: 485 bytes --]

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 3d88c00..d8d6267 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1183,6 +1183,9 @@ sbc_dif_generate(struct se_cmd *cmd)
 	void *daddr, *paddr;
 	int i, j, offset = 0;
 
+	if (!psg)
+		return;
+
 	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
 		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
 		paddr = kmap_atomic(sg_page(psg)) + psg->offset;

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

On 4/15/2015 5:16 PM, Akinobu Mita wrote:
> 2015-04-15 19:07 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
>> On 4/15/2015 2:52 AM, Akinobu Mita wrote:
>>
>>>>>
>>>>> Looks good...
>>>>>
>>>>> I'll test with these patches and check if the problems I met
>>>>> disappear.
>>>>
>>>>
>>>>
>>>> Thanks Akinobu,
>>>>
>>>> Waiting to hear your verdict before sending a formal patchset.
>>>
>>>
>>> I hit a original bug in sbc_dif_verify() which is not introduced by
>>> your patch set, though.
>>
>>
>> What is this original bug?
>
> I meant to say about the problem I fixed by fix-sbc_dif_verify.patch.
>
>>>   Please consider to include attached patch.
>>
>>
>> I'll include it. thanks.
>>
>>> I'm still seeing another problem and trying to find out a root cause,
>>> but it seems like it's caused by other change in -next.
>>>
>>
>> care to elaborate?
>
> When mounting ext4 filesystem at the first time after mkfs, a lot of
> WRITE_SAME commands are issued for lazy initialization or something.
>
> By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
> support"), When WRITE_SAME command with WRPROTECT=0 is executed,
> sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block layer
> didn't allocate it for WRITE_SAME.
>
> I could work around with the attached patch, as the WRITE_SAME command
> will fail after all when protection info is enabled with FILEIO, we only need to
> avoid null pointer dereference.  But I need to ask Nic about the right
> way to fix.
>

I don't think this is sufficient. With this we actually write
unprotected data for WRITE_SAME (i.e. write data blocks but not storing
the corresponding PI information). When this data will be read back you
will see PI errors (you currently don't see those because your backend
drive contains escape values I assume).

I'd say the correct fix is to calc PI for the block and "write_same"
it...

Sagi.

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:

Sagi> I don't think this is sufficient. With this we actually write
Sagi> unprotected data for WRITE_SAME (i.e. write data blocks but not
Sagi> storing the corresponding PI information). When this data will be
Sagi> read back you will see PI errors (you currently don't see those
Sagi> because your backend drive contains escape values I assume).

Sagi> I'd say the correct fix is to calc PI for the block

Indeed!

Sagi> and "write_same" it...

Well, the ref tag needs to be incremented for each block (for Type 1).

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

On 4/15/2015 5:33 PM, Sagi Grimberg wrote:
> On 4/15/2015 5:16 PM, Akinobu Mita wrote:
>> 2015-04-15 19:07 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
>>> On 4/15/2015 2:52 AM, Akinobu Mita wrote:
>>>
>>>>>>
>>>>>> Looks good...
>>>>>>
>>>>>> I'll test with these patches and check if the problems I met
>>>>>> disappear.
>>>>>
>>>>>
>>>>>
>>>>> Thanks Akinobu,
>>>>>
>>>>> Waiting to hear your verdict before sending a formal patchset.
>>>>
>>>>
>>>> I hit a original bug in sbc_dif_verify() which is not introduced by
>>>> your patch set, though.
>>>
>>>
>>> What is this original bug?
>>
>> I meant to say about the problem I fixed by fix-sbc_dif_verify.patch.
>>
>>>>   Please consider to include attached patch.
>>>
>>>
>>> I'll include it. thanks.
>>>
>>>> I'm still seeing another problem and trying to find out a root cause,
>>>> but it seems like it's caused by other change in -next.
>>>>
>>>
>>> care to elaborate?
>>
>> When mounting ext4 filesystem at the first time after mkfs, a lot of
>> WRITE_SAME commands are issued for lazy initialization or something.
>>
>> By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
>> support"), When WRITE_SAME command with WRPROTECT=0 is executed,
>> sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block layer
>> didn't allocate it for WRITE_SAME.
>>

Actually this is a bug. Why didn't the initiator allocate integrity
meta-data for WRITE_SAME? Looking at the code it looks like it should.

Martin?

>> I could work around with the attached patch, as the WRITE_SAME command
>> will fail after all when protection info is enabled with FILEIO, we
>> only need to
>> avoid null pointer dereference.  But I need to ask Nic about the right
>> way to fix.
>>
>
> I don't think this is sufficient. With this we actually write
> unprotected data for WRITE_SAME (i.e. write data blocks but not storing
> the corresponding PI information). When this data will be read back you
> will see PI errors (you currently don't see those because your backend
> drive contains escape values I assume).
>
> I'd say the correct fix is to calc PI for the block and "write_same"
> it...
>
> Sagi.

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:

>>> By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
>>> support"), When WRITE_SAME command with WRPROTECT=0 is executed,
>>> sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block
>>> layer didn't allocate it for WRITE_SAME.

Sagi> Actually this is a bug. Why didn't the initiator allocate
Sagi> integrity meta-data for WRITE_SAME? Looking at the code it looks
Sagi> like it should.

We don't issue WRITE SAME with PI so there is no prot SGL.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

On 4/15/2015 7:10 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
>
>>>> By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
>>>> support"), When WRITE_SAME command with WRPROTECT=0 is executed,
>>>> sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block
>>>> layer didn't allocate it for WRITE_SAME.
>
> Sagi> Actually this is a bug. Why didn't the initiator allocate
> Sagi> integrity meta-data for WRITE_SAME? Looking at the code it looks
> Sagi> like it should.
>
> We don't issue WRITE SAME with PI so there is no prot SGL.
>

Is there a specific reason why we don't?

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Akinobu Mita]

2015-04-16 17:52 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
> On 4/15/2015 7:10 PM, Martin K. Petersen wrote:
>>>>>>>
>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
>>
>>
>>>>> By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
>>>>> support"), When WRITE_SAME command with WRPROTECT=0 is executed,
>>>>> sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block
>>>>> layer didn't allocate it for WRITE_SAME.
>>
>>
>> Sagi> Actually this is a bug. Why didn't the initiator allocate
>> Sagi> integrity meta-data for WRITE_SAME? Looking at the code it looks
>> Sagi> like it should.
>>
>> We don't issue WRITE SAME with PI so there is no prot SGL.
>>
>
> Is there a specific reason why we don't?

It is not only for the WRITE SAME requests from block device but
also for READ/WRITE with PROTECT=0 requests by SG_IO.

So isn't is appropreate to allocate prot SGL in
target_write_prot_action() (and mark se_cmd->se_cmd_flags to release
it at deallocation time)?

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Martin K. Petersen]

>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:

>>> We don't issue WRITE SAME with PI so there is no prot SGL.

>> Is there a specific reason why we don't?

There really isn't much of a benefit when all you're doing is
replicating zeroes. So it hasn't been very high on my list.

Akinobu> It is not only for the WRITE SAME requests from block device
Akinobu> but also for READ/WRITE with PROTECT=0 requests by SG_IO.

Akinobu> So isn't is appropreate to allocate prot SGL in
Akinobu> target_write_prot_action() (and mark se_cmd->se_cmd_flags to
Akinobu> release it at deallocation time)?

Correct. Just because a target is formatted with PI does not mean that
every I/O it receives has PI attached. That's entirely driven by
RDPROTECT/WRPROTECT/VRPROTECT at the initiator's discretion.

It is an absolute requirement that the device, if formatted with PI,
will generate and write the correct protection information when
WRPROTECT is 0.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

On 4/16/2015 4:46 PM, Akinobu Mita wrote:
> 2015-04-16 17:52 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
>> On 4/15/2015 7:10 PM, Martin K. Petersen wrote:
>>>>>>>>
>>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
>>>
>>>
>>>>>> By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
>>>>>> support"), When WRITE_SAME command with WRPROTECT=0 is executed,
>>>>>> sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block
>>>>>> layer didn't allocate it for WRITE_SAME.
>>>
>>>
>>> Sagi> Actually this is a bug. Why didn't the initiator allocate
>>> Sagi> integrity meta-data for WRITE_SAME? Looking at the code it looks
>>> Sagi> like it should.
>>>
>>> We don't issue WRITE SAME with PI so there is no prot SGL.
>>>
>>
>> Is there a specific reason why we don't?
>
> It is not only for the WRITE SAME requests from block device but
> also for READ/WRITE with PROTECT=0 requests by SG_IO.
>

This is specific to loopback which is using target_submit_cmd_map_sgls()
Other fabrics would allocate sgls per IO and the core would allocate
protection SGLs as well.

> So isn't is appropreate to allocate prot SGL in
> target_write_prot_action() (and mark se_cmd->se_cmd_flags to release
> it at deallocation time)?
>

I'd say that given this is specific to loopback, than tcm_loop needs
to be fixed... But specifically for WRITE_SAME, I'd be careful with
allocating a single 8 byte protection buffer because as Martin said,
unlike the data block, the protection field may change from sector to
sector (ref_tag in Type 1).

So allocating a single 8 byte buf will take it's toll in the backend
(iblock backend would need to allocate all the protection information
and add it to the bio anyway, file/rd will need to do multiple writes).

It might be better that for the special WRITE_SAME case, allocate 8 *
sectors sgl and set it up (incrementing ref_tag for type 1). This way,
the backend code can stay the same (other than opening write_same with
PI in iblock).

Sagi.

Re: [RFC] Simlify dif_verify routines and fixup fileio protection information code.

[Sagi Grimberg]

On 4/16/2015 6:58 PM, Sagi Grimberg wrote:
> On 4/16/2015 4:46 PM, Akinobu Mita wrote:
>> 2015-04-16 17:52 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>:
>>> On 4/15/2015 7:10 PM, Martin K. Petersen wrote:
>>>>>>>>>
>>>>>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:
>>>>
>>>>
>>>>>>> By the commit 436f4a0a ("loopback: Add fabric_prot_type attribute
>>>>>>> support"), When WRITE_SAME command with WRPROTECT=0 is executed,
>>>>>>> sbc_dif_generate() is called but cmd->t_prot_sg is NULL as block
>>>>>>> layer didn't allocate it for WRITE_SAME.
>>>>
>>>>
>>>> Sagi> Actually this is a bug. Why didn't the initiator allocate
>>>> Sagi> integrity meta-data for WRITE_SAME? Looking at the code it looks
>>>> Sagi> like it should.
>>>>
>>>> We don't issue WRITE SAME with PI so there is no prot SGL.
>>>>
>>>
>>> Is there a specific reason why we don't?
>>
>> It is not only for the WRITE SAME requests from block device but
>> also for READ/WRITE with PROTECT=0 requests by SG_IO.
>>
>
> This is specific to loopback which is using target_submit_cmd_map_sgls()
> Other fabrics would allocate sgls per IO and the core would allocate
> protection SGLs as well.
>

By "This" I meant the NULL deref you are witnessing in for wrprotect=0.