[PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Akinobu Mita]

This patch set includes bug fixes which I hit when I was tried testing
the data integrity support in scsi_debug on x86_32.

And it also includes cleanups which helps increasing readability and
further bug fixing in data integrity support.

* Changes from v2
- Add new bug fix patch for UNMAP command support
- Change the way to fix for the patch "fix invalid address passed to
  kunmap_atomic()"
- Reduce more lines of code for the patch "reduce duplication between
  prot_verify_read and prot_verify_writ"

* Changes from v1
- Split the patch "fix data integrity support on highmem machine" into
  two separate patches.
- Add new cleanup patch "reduce duplication between prot_verify_read and
  prot_verify_write".

Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org

Akinobu Mita (6):
  scsi_debug: fix invalid address passed to kunmap_atomic()
  scsi_debug: fix incorrectly nested kmap_atomic()
  scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1
  scsi_debug: invalidate protection info for unmapped region
  scsi_debug: simplify offset calculation for dif_storep
  scsi_debug: reduce duplication between prot_verify_read and
    prot_verify_write

 drivers/scsi/scsi_debug.c | 176 +++++++++++++++++++---------------------------
 1 file changed, 72 insertions(+), 104 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/6] scsi_debug: fix invalid address passed to kunmap_atomic()

[Akinobu Mita]

In the function prot_verify_write(), the kmap address 'daddr' is
incremented in the loop for each data page.  Finally 'daddr' reaches
the next page boundary in the end of the loop, and the invalid address
is passed to kunmap_atomic().

Fix the issue by not incrementing 'daddr' in the loop and offsetting it
by the loop counter on demand.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---

* Change from v2
- It was not very clear that incrementing 'daddr' in the loop and restoring
  the original value by subtracting the sum of increments.  Instead of
  doing that, fix the issue by not incrementing 'daddr' in the loop and
  offsetting it by the loop counter on demand.

 drivers/scsi/scsi_debug.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 0a537a0..d51bddd 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1899,7 +1899,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 		daddr = kmap_atomic(sg_page(dsgl)) + dsgl->offset;
 
 		/* For each sector-sized chunk in data page */
-		for (j = 0 ; j < dsgl->length ; j += scsi_debug_sector_size) {
+		for (j = 0; j < dsgl->length; j += scsi_debug_sector_size) {
 
 			/* If we're at the end of the current
 			 * protection page advance to the next one
@@ -1917,11 +1917,11 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 			switch (scsi_debug_guard) {
 			case 1:
-				csum = ip_compute_csum(daddr,
+				csum = ip_compute_csum(daddr + j,
 						       scsi_debug_sector_size);
 				break;
 			case 0:
-				csum = cpu_to_be16(crc_t10dif(daddr,
+				csum = cpu_to_be16(crc_t10dif(daddr + j,
 						      scsi_debug_sector_size));
 				break;
 			default:
@@ -1938,7 +1938,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				       be16_to_cpu(sdt->guard_tag),
 				       be16_to_cpu(csum));
 				ret = 0x01;
-				dump_sector(daddr, scsi_debug_sector_size);
+				dump_sector(daddr + j, scsi_debug_sector_size);
 				goto out;
 			}
 
@@ -1949,7 +1949,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				       "%s: REF check failed on sector %lu\n",
 				       __func__, (unsigned long)sector);
 				ret = 0x03;
-				dump_sector(daddr, scsi_debug_sector_size);
+				dump_sector(daddr + j, scsi_debug_sector_size);
 				goto out;
 			}
 
@@ -1959,7 +1959,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				       "%s: REF check failed on sector %lu\n",
 				       __func__, (unsigned long)sector);
 				ret = 0x03;
-				dump_sector(daddr, scsi_debug_sector_size);
+				dump_sector(daddr + j, scsi_debug_sector_size);
 				goto out;
 			}
 
@@ -1977,7 +1977,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 			start_sec++;
 			ei_lba++;
-			daddr += scsi_debug_sector_size;
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
 
-- 
1.8.1.4

[PATCH v3 2/6] scsi_debug: fix incorrectly nested kmap_atomic()

[Akinobu Mita]

In the function prot_verify_write(), kmap_atomic()/kunmap_atomic() for
data page and kmap_atomic()/kunmap_atomic() for protection information
page are not nested each other.

It worked perfectly before commit 3e4d3af501cccdc8a8cca41bdbe57d54ad7e7e73
("mm: stack based kmap_atomic()").  Because the kmap_atomic slot KM_IRQ0
was used for data page and the slot KM_IRQ1 was used for protection page.

But KM_types are gone and kmap_atomic() is using stack based implementation.
So two different kmap_atomic() usages must be strictly nested now.

This change ensures kmap_atomic() usage is strictly nested.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
---

* No changes from v2

 drivers/scsi/scsi_debug.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index d51bddd..bcf73e4 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1891,12 +1891,12 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
-	paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
 	ppage_offset = 0;
 
 	/* For each data page */
 	scsi_for_each_sg(SCpnt, dsgl, scsi_sg_count(SCpnt), i) {
 		daddr = kmap_atomic(sg_page(dsgl)) + dsgl->offset;
+		paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
 
 		/* For each sector-sized chunk in data page */
 		for (j = 0; j < dsgl->length; j += scsi_debug_sector_size) {
@@ -1980,19 +1980,18 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
 
+		kunmap_atomic(paddr);
 		kunmap_atomic(daddr);
 	}
 
-	kunmap_atomic(paddr);
-
 	dix_writes++;
 
 	return 0;
 
 out:
 	dif_errors++;
-	kunmap_atomic(daddr);
 	kunmap_atomic(paddr);
+	kunmap_atomic(daddr);
 	return ret;
 }
 
-- 
1.8.1.4

[PATCH v3 3/6] scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1

[Akinobu Mita]

The protection info dif_storep is allocated only when parameter dif is
not zero.  But it will be accessed when reading or writing to the storage
installed with parameter dix is not zero.

So kernel crashes if scsi_debug module is loaded with parameters dix=1 and
dif=0.

This fixes it by making dif_storep available if parameter dix is not zero
instead of checking if parameter dif is not zero.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
Acked-by: "Martin K. Petersen" <martin.petersen@oracle.com>
---

* No changes from v1

 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index bcf73e4..e83e661 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3372,7 +3372,7 @@ static int __init scsi_debug_init(void)
 	if (scsi_debug_num_parts > 0)
 		sdebug_build_parts(fake_storep, sz);
 
-	if (scsi_debug_dif) {
+	if (scsi_debug_dix) {
 		int dif_size;
 
 		dif_size = sdebug_store_sectors * sizeof(struct sd_dif_tuple);
-- 
1.8.1.4

[PATCH v3 4/6] scsi_debug: invalidate protection info for unmapped region

[Akinobu Mita]

When UNMAP command is issued with the data integrity support enabled,
the protection info for the unmapped region is remain unchanged.
So READ command for the region later on causes data integrity failure.

This fixes it by invalidating protection info for the unmapped region
by filling with 0xff pattern.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---

* New patch from v3

 drivers/scsi/scsi_debug.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index e83e661..83efec2 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2064,6 +2064,11 @@ static void unmap_region(sector_t lba, unsigned int len)
 				       scsi_debug_sector_size *
 				       scsi_debug_unmap_granularity);
 			}
+			if (dif_storep) {
+				memset(dif_storep + lba, 0xff,
+				       sizeof(*dif_storep) *
+				       scsi_debug_unmap_granularity);
+			}
 		}
 		lba = map_index_to_lba(index + 1);
 	}
-- 
1.8.1.4

[PATCH v3 5/6] scsi_debug: simplify offset calculation for dif_storep

[Akinobu Mita]

dif_storep is declared as pointer to unsigned char type.  But it is
actually used to store vmalloced array of struct sd_dif_tuple.

This changes the type of dif_storep to the pointer to struct sd_dif_tuple.
It simplifies offset calculation for dif_storep and enables to remove
hardcoded size of struct sd_dif_tuple.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
---

* No changes from v1

 drivers/scsi/scsi_debug.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 83efec2..fc8b3aa 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -258,7 +258,7 @@ struct sdebug_queued_cmd {
 static struct sdebug_queued_cmd queued_arr[SCSI_DEBUG_CANQUEUE];
 
 static unsigned char * fake_storep;	/* ramdisk storage */
-static unsigned char *dif_storep;	/* protection info */
+static struct sd_dif_tuple *dif_storep;	/* protection info */
 static void *map_storep;		/* provisioning map */
 
 static unsigned long map_size;
@@ -277,11 +277,6 @@ static char sdebug_proc_name[] = "scsi_debug";
 
 static struct bus_type pseudo_lld_bus;
 
-static inline sector_t dif_offset(sector_t sector)
-{
-	return sector << 3;
-}
-
 static struct device_driver sdebug_driverfs_driver = {
 	.name 		= sdebug_proc_name,
 	.bus		= &pseudo_lld_bus,
@@ -1727,7 +1722,7 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 	start_sec = do_div(tmp_sec, sdebug_store_sectors);
 
-	sdt = (struct sd_dif_tuple *)(dif_storep + dif_offset(start_sec));
+	sdt = dif_storep + start_sec;
 
 	for (i = 0 ; i < sectors ; i++) {
 		u16 csum;
@@ -1782,16 +1777,17 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 		ei_lba++;
 	}
 
-	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
+	/* Bytes of protection data to copy into sgl */
+	resid = sectors * sizeof(*dif_storep);
 	sector = start_sec;
 
 	scsi_for_each_prot_sg(SCpnt, psgl, scsi_prot_sg_count(SCpnt), i) {
 		int len = min(psgl->length, resid);
 
 		paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
-		memcpy(paddr, dif_storep + dif_offset(sector), len);
+		memcpy(paddr, dif_storep + sector, len);
 
-		sector += len >> 3;
+		sector += len / sizeof(*dif_storep);
 		if (sector >= sdebug_store_sectors) {
 			/* Force wrap */
 			tmp_sec = sector;
@@ -1968,7 +1964,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			 * correctness we need to verify each sector
 			 * before writing it to "stable" storage
 			 */
-			memcpy(dif_storep + dif_offset(sector), sdt, 8);
+			memcpy(dif_storep + sector, sdt, sizeof(*sdt));
 
 			sector++;
 
-- 
1.8.1.4

[PATCH v3 6/6] scsi_debug: reduce duplication between prot_verify_read and prot_verify_write

[Akinobu Mita]

In order to reduce code duplication between prot_verify_read() and
prot_verify_write(), this moves common code into the new functions.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---

* Change from v2
- Change dif_verify() interface and it reduces more lines of code

 drivers/scsi/scsi_debug.c | 139 ++++++++++++++++++----------------------------
 1 file changed, 54 insertions(+), 85 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fc8b3aa..9790d56 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1710,6 +1710,52 @@ static int do_device_access(struct scsi_cmnd *scmd,
 	return ret;
 }
 
+static u16 dif_compute_csum(const void *buf, int len)
+{
+	u16 csum;
+
+	switch (scsi_debug_guard) {
+	case 1:
+		csum = ip_compute_csum(buf, len);
+		break;
+	case 0:
+		csum = cpu_to_be16(crc_t10dif(buf, len));
+		break;
+	default:
+		BUG();
+	}
+	return csum;
+}
+
+static int dif_verify(struct sd_dif_tuple *sdt, const void *data,
+		      sector_t sector, u32 ei_lba)
+{
+	u16 csum = dif_compute_csum(data, scsi_debug_sector_size);
+
+	if (sdt->guard_tag != csum) {
+		pr_err("%s: GUARD check failed on sector %lu rcvd 0x%04x, data 0x%04x\n",
+			__func__,
+			(unsigned long)sector,
+			be16_to_cpu(sdt->guard_tag),
+			be16_to_cpu(csum));
+		return 0x01;
+	}
+	if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
+	    be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
+		pr_err("%s: REF check failed on sector %lu\n",
+			__func__, (unsigned long)sector);
+		return 0x03;
+	}
+	if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+	    be32_to_cpu(sdt->ref_tag) != ei_lba) {
+		pr_err("%s: REF check failed on sector %lu\n",
+			__func__, (unsigned long)sector);
+			dif_errors++;
+		return 0x03;
+	}
+	return 0;
+}
+
 static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			    unsigned int sectors, u32 ei_lba)
 {
@@ -1725,53 +1771,19 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 	sdt = dif_storep + start_sec;
 
 	for (i = 0 ; i < sectors ; i++) {
-		u16 csum;
+		int ret;
 
 		if (sdt[i].app_tag == 0xffff)
 			continue;
 
 		sector = start_sec + i;
 
-		switch (scsi_debug_guard) {
-		case 1:
-			csum = ip_compute_csum(fake_storep +
-					       sector * scsi_debug_sector_size,
-					       scsi_debug_sector_size);
-			break;
-		case 0:
-			csum = crc_t10dif(fake_storep +
-					  sector * scsi_debug_sector_size,
-					  scsi_debug_sector_size);
-			csum = cpu_to_be16(csum);
-			break;
-		default:
-			BUG();
-		}
-
-		if (sdt[i].guard_tag != csum) {
-			printk(KERN_ERR "%s: GUARD check failed on sector %lu" \
-			       " rcvd 0x%04x, data 0x%04x\n", __func__,
-			       (unsigned long)sector,
-			       be16_to_cpu(sdt[i].guard_tag),
-			       be16_to_cpu(csum));
+		ret = dif_verify(&sdt[i],
+				 fake_storep + sector * scsi_debug_sector_size,
+				 sector, ei_lba);
+		if (ret) {
 			dif_errors++;
-			return 0x01;
-		}
-
-		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
-		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
-			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
-			       __func__, (unsigned long)sector);
-			dif_errors++;
-			return 0x03;
-		}
-
-		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
-		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {
-			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
-			       __func__, (unsigned long)sector);
-			dif_errors++;
-			return 0x03;
+			return ret;
 		}
 
 		ei_lba++;
@@ -1880,7 +1892,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 	sector_t tmp_sec = start_sec;
 	sector_t sector;
 	int ppage_offset;
-	unsigned short csum;
 
 	sector = do_div(tmp_sec, sdebug_store_sectors);
 
@@ -1911,50 +1922,8 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 			sdt = paddr + ppage_offset;
 
-			switch (scsi_debug_guard) {
-			case 1:
-				csum = ip_compute_csum(daddr + j,
-						       scsi_debug_sector_size);
-				break;
-			case 0:
-				csum = cpu_to_be16(crc_t10dif(daddr + j,
-						      scsi_debug_sector_size));
-				break;
-			default:
-				BUG();
-				ret = 0;
-				goto out;
-			}
-
-			if (sdt->guard_tag != csum) {
-				printk(KERN_ERR
-				       "%s: GUARD check failed on sector %lu " \
-				       "rcvd 0x%04x, calculated 0x%04x\n",
-				       __func__, (unsigned long)sector,
-				       be16_to_cpu(sdt->guard_tag),
-				       be16_to_cpu(csum));
-				ret = 0x01;
-				dump_sector(daddr + j, scsi_debug_sector_size);
-				goto out;
-			}
-
-			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
-			    be32_to_cpu(sdt->ref_tag)
-			    != (start_sec & 0xffffffff)) {
-				printk(KERN_ERR
-				       "%s: REF check failed on sector %lu\n",
-				       __func__, (unsigned long)sector);
-				ret = 0x03;
-				dump_sector(daddr + j, scsi_debug_sector_size);
-				goto out;
-			}
-
-			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
-			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
-				printk(KERN_ERR
-				       "%s: REF check failed on sector %lu\n",
-				       __func__, (unsigned long)sector);
-				ret = 0x03;
+			ret = dif_verify(sdt, daddr + j, start_sec, ei_lba);
+			if (ret) {
 				dump_sector(daddr + j, scsi_debug_sector_size);
 				goto out;
 			}
-- 
1.8.1.4

Re: [PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Douglas Gilbert]

Perhaps I'm missing something but I can never get
DIF stuff to do what I want in scsi_debug. It was
like this before your patches as well.

Assume this set up:
# lsscsi -gp
[1:0:0:0]    disk    ATA      ST3320620AS      3.AA  /dev/sda   /dev/sg0   - 
       -
[6:0:9:0]    disk    SEAGATE  ST33000650SS     0002  /dev/sdb   /dev/sg2 
DIF/Type1  -
[7:0:0:0]    disk    Linux    scsi_debug       0004  /dev/sdc   /dev/sg3 
DIF/Type1  -

So 6:0:9:0 is a real disk formatted with protection type 1
and 7:0:0:0 is simulating the same thing (with scsi_debug).
Now I try to read the first block with RDPROTECT=1 so in
the data-out buffer I expect the LB plus the protection
info. That as 512+8 bytes. So now I use ddpt to read the
first block plus PI from the Seagate disk:

# ddpt if=/dev/sg2 bs=512 verbose=3 protect=1 count=1
   ....
Output file not specified so no copy, just reading input
     READ cdb: 28 20 00 00 00 00 00 00 01 00
1+0 records in
0+0 records out
time to read data: 0.000574 secs at 892.0 KB/sec

Good, asked for 520 bytes and got them. But when I try
that with scsi_debug disk:

# ddpt if=/dev/sg3 bs=512 verbose=3 protect=1 count=1
...
Output file not specified so no copy, just reading input
     READ cdb: 28 20 00 00 00 00 00 00 01 00
     READ: pass-through requested 520 bytes but got 512 bytes
1+0 records in
0+0 records out
 >> Non-zero sum of residual counts=8
time to read data: 0.000833 secs at 614.6 KB/sec

No PI, why not? This was tested on lk 3.9.4 with your patches
applied.

Doug Gilbert


On 13-05-26 04:01 AM, Akinobu Mita wrote:
> This patch set includes bug fixes which I hit when I was tried testing
> the data integrity support in scsi_debug on x86_32.
>
> And it also includes cleanups which helps increasing readability and
> further bug fixing in data integrity support.
>
> * Changes from v2
> - Add new bug fix patch for UNMAP command support
> - Change the way to fix for the patch "fix invalid address passed to
>    kunmap_atomic()"
> - Reduce more lines of code for the patch "reduce duplication between
>    prot_verify_read and prot_verify_writ"
>
> * Changes from v1
> - Split the patch "fix data integrity support on highmem machine" into
>    two separate patches.
> - Add new cleanup patch "reduce duplication between prot_verify_read and
>    prot_verify_write".
>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
>
> Akinobu Mita (6):
>    scsi_debug: fix invalid address passed to kunmap_atomic()
>    scsi_debug: fix incorrectly nested kmap_atomic()
>    scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1
>    scsi_debug: invalidate protection info for unmapped region
>    scsi_debug: simplify offset calculation for dif_storep
>    scsi_debug: reduce duplication between prot_verify_read and
>      prot_verify_write
>
>   drivers/scsi/scsi_debug.c | 176 +++++++++++++++++++---------------------------
>   1 file changed, 72 insertions(+), 104 deletions(-)
>

Re: [PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Martin K. Petersen]

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

Doug> No PI, why not? This was tested on lk 3.9.4 with your patches
Doug> applied.

I've been communicating with Akinobu offline about this.

The reason is that I really only implemented DIX support in
scsi_debug. The dif module parameter is only there to select the
reported protection type.

The main problem with scsi_debug is that it's both initiator and target
in one. And there isn't any notion of a data transfer between the
initiator and the target. We just fill out the scatterlists with stuff
from the in-memory buffer.

To support DIX/DIF correctly we'd have to have the notion of a handoff
between the front and back of scsi_debug. And we'd have to support all
combinations of DIX and DIF with the relevant slicing and dicing of data
and PI buffers.

I have some patches pending as part of my next DIF/DIX update that makes
some of these things more palatable at the block/SCSI level. Akinobu
voiced interest in finishing the scsi_debug work on top of my code.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Akinobu Mita]

2013/5/29 Martin K. Petersen <martin.petersen@oracle.com>:
> I have some patches pending as part of my next DIF/DIX update that makes
> some of these things more palatable at the block/SCSI level. Akinobu
> voiced interest in finishing the scsi_debug work on top of my code.

Yes.  I'm interested in that work.  Before I start working on it, I would
like to fix the problems which I found recently with virtual_gb option in
scsi_debug.  Because the change is not small and may touch the DIX/DIF
support code, too.

So Martin and Douglas, can I have your ACKs on this patch series for now?

Re: [PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Douglas Gilbert]

On 13-06-01 10:51 PM, Akinobu Mita wrote:
> 2013/5/29 Martin K. Petersen <martin.petersen@oracle.com>:
>> I have some patches pending as part of my next DIF/DIX update that makes
>> some of these things more palatable at the block/SCSI level. Akinobu
>> voiced interest in finishing the scsi_debug work on top of my code.
>
> Yes.  I'm interested in that work.  Before I start working on it, I would
> like to fix the problems which I found recently with virtual_gb option in
> scsi_debug.  Because the change is not small and may touch the DIX/DIF
> support code, too.
>
> So Martin and Douglas, can I have your ACKs on this patch series for now?

Done.

Just doing some debugging using scsi_debug and noticed
that 'blockdev --rereadpt /dev/sdb' causes a SCSI
REPORT SUPPORTED OPERATION CODES command to be issued.
Perhaps we could add that one (and ... SUPPORTED TMFs) to
scsi_debug's code.

I was also testing setting the SWP bit in the Control
mode page and that works, even though that field is
marked as 'changeable=n'. Since scsi_debug allows all
fields in that page to be changed, perhaps they could
all be marked as changeable.

Doug Gilbert

Re: [PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Martin K. Petersen]

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

Akinobu> So Martin and Douglas, can I have your ACKs on this patch
Akinobu> series for now?

Only concern is the one I mentioned before: You're mapping and unmapping
the ppage for every 8 bytes of protection information. That seems a bit
excessive.

However, I personally don't care too much about 32-bit platforms, so...

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Akinobu Mita]

2013/6/7 Martin K. Petersen <martin.petersen@oracle.com>:
>>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
>
> Akinobu> So Martin and Douglas, can I have your ACKs on this patch
> Akinobu> series for now?
>
> Only concern is the one I mentioned before: You're mapping and unmapping
> the ppage for every 8 bytes of protection information. That seems a bit
> excessive.

I understand your concern.  That could be cured by changing the outer loop from
'scsi_for_each_sg' to 'scsi_for_each_prot_sg'.  I'll try to do this and see
how it looks.

> However, I personally don't care too much about 32-bit platforms, so...
>
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

Thanks.

Re: [PATCH v3 0/6] scsi_debug: bug fixes and cleanups for data integrity support

[Douglas Gilbert]

On 13-05-26 04:01 AM, Akinobu Mita wrote:
> This patch set includes bug fixes which I hit when I was tried testing
> the data integrity support in scsi_debug on x86_32.
>
> And it also includes cleanups which helps increasing readability and
> further bug fixing in data integrity support.
>
> * Changes from v2
> - Add new bug fix patch for UNMAP command support
> - Change the way to fix for the patch "fix invalid address passed to
>    kunmap_atomic()"
> - Reduce more lines of code for the patch "reduce duplication between
>    prot_verify_read and prot_verify_writ"
>
> * Changes from v1
> - Split the patch "fix data integrity support on highmem machine" into
>    two separate patches.
> - Add new cleanup patch "reduce duplication between prot_verify_read and
>    prot_verify_write".
>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
>
> Akinobu Mita (6):
>    scsi_debug: fix invalid address passed to kunmap_atomic()
>    scsi_debug: fix incorrectly nested kmap_atomic()
>    scsi_debug: fix NULL pointer dereference with parameters dif=0 dix=1
>    scsi_debug: invalidate protection info for unmapped region
>    scsi_debug: simplify offset calculation for dif_storep
>    scsi_debug: reduce duplication between prot_verify_read and
>      prot_verify_write
>
>   drivers/scsi/scsi_debug.c | 176 +++++++++++++++++++---------------------------
>   1 file changed, 72 insertions(+), 104 deletions(-)

Acked-by: Douglas Gilbert <dgilbert@interlog.com>