[PATCH] scsi_debug: add LBPRZ support

[PATCH] scsi_debug: add LBPRZ support

[Eric Sandeen]

Add LBPRZ support to scsi_debug; i.e. return zero for
unmapped blocks.

Rather than checking for unmapped blocks at
read time, this just zeroes them on the backing store
at unmap time so it behaves the same way.

This also adds a module parameter to disable it, since
some SSDs have this behavior.

unmap_zeroes, "unmapped blocks return 0 on read (def=1)"

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Note: This was sent long ago as "TPRZ" support, but lost, I guess.

Note2: dgilbert preferred "zeros" to "zeroes" at the time,
but since we have "discard_zeroes_data" in sysfs it seems like
we should be consistent with the kernel precedent, rather than
the spec spelling.

But I really don't are if this gets searched & replaced before 
commit.  :)

Thanks,
-Eric

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6888b2c..32e6c19 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -114,6 +114,7 @@ static const char * scsi_debug_version_date = "20100324";
 #define DEF_UNMAP_GRANULARITY 1
 #define DEF_UNMAP_MAX_BLOCKS 0xFFFFFFFF
 #define DEF_UNMAP_MAX_DESC 256
+#define DEF_UNMAP_ZEROES 1
 #define DEF_VIRTUAL_GB   0
 #define DEF_VPD_USE_HOSTNO 1
 #define DEF_WRITESAME_LENGTH 0xFFFF
@@ -189,6 +190,7 @@ static unsigned int scsi_debug_unmap_alignment = DEF_UNMAP_ALIGNMENT;
 static unsigned int scsi_debug_unmap_granularity = DEF_UNMAP_GRANULARITY;
 static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
 static unsigned int scsi_debug_unmap_max_desc = DEF_UNMAP_MAX_DESC;
+static unsigned int scsi_debug_unmap_zeroes = DEF_UNMAP_ZEROES;
 static unsigned int scsi_debug_write_same_length = DEF_WRITESAME_LENGTH;
 
 static int scsi_debug_cmnd_count = 0;
@@ -1070,8 +1072,11 @@ static int resp_readcap16(struct scsi_cmnd * scp,
 	arr[13] = scsi_debug_physblk_exp & 0xf;
 	arr[14] = (scsi_debug_lowest_aligned >> 8) & 0x3f;
 
-	if (scsi_debug_lbp())
+	if (scsi_debug_lbp()) {
 		arr[14] |= 0x80; /* LBPME */
+		if (scsi_debug_unmap_zeroes)
+			arr[14] |= 0x40; /* LBPRZ */
+	}
 
 	arr[15] = scsi_debug_lowest_aligned & 0xff;
 
@@ -2045,10 +2050,13 @@ static void unmap_region(sector_t lba, unsigned int len)
 		block = lba + alignment;
 		rem = do_div(block, granularity);
 
-		if (rem == 0 && lba + granularity <= end &&
-		    block < map_size)
+		if (rem == 0 && lba + granularity <= end && block < map_size) {
 			clear_bit(block, map_storep);
-
+			if (scsi_debug_unmap_zeroes)
+				memset(fake_storep +
+				       block * scsi_debug_sector_size, 0,
+				       scsi_debug_sector_size);
+		}
 		lba += granularity - rem;
 	}
 }
@@ -2747,6 +2755,7 @@ module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
 module_param_named(unmap_granularity, scsi_debug_unmap_granularity, int, S_IRUGO);
 module_param_named(unmap_max_blocks, scsi_debug_unmap_max_blocks, int, S_IRUGO);
 module_param_named(unmap_max_desc, scsi_debug_unmap_max_desc, int, S_IRUGO);
+module_param_named(unmap_zeroes, scsi_debug_unmap_zeroes, int, S_IRUGO);
 module_param_named(virtual_gb, scsi_debug_virtual_gb, int, S_IRUGO | S_IWUSR);
 module_param_named(vpd_use_hostno, scsi_debug_vpd_use_hostno, int,
 		   S_IRUGO | S_IWUSR);
@@ -2788,6 +2797,7 @@ MODULE_PARM_DESC(unmap_alignment, "lowest aligned thin provisioning lba (def=0)"
 MODULE_PARM_DESC(unmap_granularity, "thin provisioning granularity in blocks (def=1)");
 MODULE_PARM_DESC(unmap_max_blocks, "max # of blocks can be unmapped in one cmd (def=0xffffffff)");
 MODULE_PARM_DESC(unmap_max_desc, "max # of ranges that can be unmapped in one cmd (def=256)");
+MODULE_PARM_DESC(unmap_zeroes, "unmapped blocks return 0 on read (def=1)");
 MODULE_PARM_DESC(virtual_gb, "virtual gigabyte size (def=0 -> use dev_size_mb)");
 MODULE_PARM_DESC(vpd_use_hostno, "0 -> dev ids ignore hostno (def=1 -> unique dev ids)");
 MODULE_PARM_DESC(write_same_length, "Maximum blocks per WRITE SAME cmd (def=0xffff)");

Re: [PATCH] scsi_debug: add LBPRZ support

[Douglas Gilbert]

On 12-03-07 03:09 PM, Eric Sandeen wrote:
> Add LBPRZ support to scsi_debug; i.e. return zero for
> unmapped blocks.
>
> Rather than checking for unmapped blocks at
> read time, this just zeroes them on the backing store
> at unmap time so it behaves the same way.
>
> This also adds a module parameter to disable it, since
> some SSDs have this behavior.
>
> unmap_zeroes, "unmapped blocks return 0 on read (def=1)"
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>
> Note: This was sent long ago as "TPRZ" support, but lost, I guess.
>
> Note2: dgilbert preferred "zeros" to "zeroes" at the time,
> but since we have "discard_zeroes_data" in sysfs it seems like
> we should be consistent with the kernel precedent, rather than
> the spec spelling.

Eric,
I checked the latest drafts of SPC-4 and SBC-3 and they
contain both "zeros" and "zeroes". Take your pick!

More seriously the LBPRZ flag now appears in the Logical
Block Provisioning VPD page and the READ CAPACITY (16)
response. Your patch sets the latter, could you add the
LBPRZ flag setting in the inquiry_evpd_b2() function as
well. Perhaps:
   if (scsi_debug_unmap_zeroes)
     arr[1] |= 1 << 2;

And if your are editing that function in the comment introducing
that function:
    s/Thin/Logical block/
to reflect the renaming done by t10.org .

Ah, and inquiry_evpd_b2() should return 4 (not 8).

Otherwise it looks good.

Doug Gilbert

Re: [PATCH] scsi_debug: add LBPRZ support

[Eric Sandeen]

On 3/7/12 7:00 PM, Douglas Gilbert wrote:
> On 12-03-07 03:09 PM, Eric Sandeen wrote:
>> Add LBPRZ support to scsi_debug; i.e. return zero for
>> unmapped blocks.
>>
>> Rather than checking for unmapped blocks at
>> read time, this just zeroes them on the backing store
>> at unmap time so it behaves the same way.
>>
>> This also adds a module parameter to disable it, since
>> some SSDs have this behavior.
>>
>> unmap_zeroes, "unmapped blocks return 0 on read (def=1)"
>>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> ---
>>
>> Note: This was sent long ago as "TPRZ" support, but lost, I guess.
>>
>> Note2: dgilbert preferred "zeros" to "zeroes" at the time,
>> but since we have "discard_zeroes_data" in sysfs it seems like
>> we should be consistent with the kernel precedent, rather than
>> the spec spelling.
> 
> Eric,
> I checked the latest drafts of SPC-4 and SBC-3 and they
> contain both "zeros" and "zeroes". Take your pick!

While we're talking about names, looking at other scsi_debug_* flags
should it be lbprz / scsi_debug_lbprz / DEF_LBPRZ instead of
unmap_zeroes / scsi_debug_unmap_zeroes / DEF_UNMAP_ZEROES?
 
> More seriously the LBPRZ flag now appears in the Logical
> Block Provisioning VPD page and the READ CAPACITY (16)
> response. Your patch sets the latter, could you add the
> LBPRZ flag setting in the inquiry_evpd_b2() function as
> well. Perhaps:
>   if (scsi_debug_unmap_zeroes)
>     arr[1] |= 1 << 2;

ok; I'm no scsi guy, just a pattern-following monkey but I'll
change that if you say it's right. :)

> And if your are editing that function in the comment introducing
> that function:
>    s/Thin/Logical block/
> to reflect the renaming done by t10.org .

ok.

> Ah, and inquiry_evpd_b2() should return 4 (not 8).

Is that at all related to this change or some other random bug?
Should that return be unconditional?  Should that be a separate
patch?  By someone who knows what it means? :)

> Otherwise it looks good.


Thanks,

-Eric


 
> Doug Gilbert
> 

Re: [PATCH] scsi_debug: add LBPRZ support

[Martin K. Petersen]

>>>>> "Eric" == Eric Sandeen <sandeen@redhat.com> writes:

Eric> While we're talking about names, looking at other scsi_debug_*
Eric> flags should it be lbprz / scsi_debug_lbprz / DEF_LBPRZ instead of
Eric> unmap_zeroes / scsi_debug_unmap_zeroes / DEF_UNMAP_ZEROES?

Yeah, that's more consistent with my recent updates which use the lbpfoo
flag names.


>> Ah, and inquiry_evpd_b2() should return 4 (not 8).

Eric> Is that at all related to this change or some other random bug?
Eric> Should that return be unconditional?  Should that be a separate
Eric> patch?  By someone who knows what it means? :)

The returned page length should not include the length of the header. I
mess that up all the time. The spec says: "If the DP bit is set to
zero, then the page length shall be set to 0004h." The memset also needs
to be fixed accordingly.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH V2] scsi_debug: add LBPRZ support

[Eric Sandeen]

Add LBPRZ support to scsi_debug; i.e. read zeros for
unmapped blocks.

Rather than checking for unmapped blocks at
read time, this just zeroes them on the backing store
at unmap time so it behaves the same way.

This also adds a module parameter to disable it.

lbprz, "unmapped blocks return 0 on read (def=1)"

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: set flag in inquiry_evpd_b2() as well, and rename *_zeroes
type symbols to *_lbprz

note, I didn't change the return value of inquiry_evpd_b2();
that seems unrelated to this change, and should be a separate
patch, no?

Thanks,
-Eric

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6888b2c..fc12a39 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -101,6 +101,7 @@ static const char * scsi_debug_version_date = "20100324";
 #define DEF_LBPU 0
 #define DEF_LBPWS 0
 #define DEF_LBPWS10 0
+#define DEF_LBPRZ 1
 #define DEF_LOWEST_ALIGNED 0
 #define DEF_NO_LUN_0   0
 #define DEF_NUM_PARTS   0
@@ -185,6 +186,7 @@ static int scsi_debug_vpd_use_hostno = DEF_VPD_USE_HOSTNO;
 static unsigned int scsi_debug_lbpu = DEF_LBPU;
 static unsigned int scsi_debug_lbpws = DEF_LBPWS;
 static unsigned int scsi_debug_lbpws10 = DEF_LBPWS10;
+static unsigned int scsi_debug_lbprz = DEF_LBPRZ;
 static unsigned int scsi_debug_unmap_alignment = DEF_UNMAP_ALIGNMENT;
 static unsigned int scsi_debug_unmap_granularity = DEF_UNMAP_GRANULARITY;
 static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
@@ -774,7 +776,7 @@ static int inquiry_evpd_b1(unsigned char *arr)
 	return 0x3c;
 }
 
-/* Thin provisioning VPD page (SBC-3) */
+/* Logical block provisioning VPD page (SBC-3) */
 static int inquiry_evpd_b2(unsigned char *arr)
 {
 	memset(arr, 0, 0x8);
@@ -789,6 +791,9 @@ static int inquiry_evpd_b2(unsigned char *arr)
 	if (scsi_debug_lbpws10)
 		arr[1] |= 1 << 5;
 
+	if (scsi_debug_lbprz)
+		arr[1] |= 1 << 2; 
+
 	return 0x8;
 }
 
@@ -1070,8 +1075,11 @@ static int resp_readcap16(struct scsi_cmnd * scp,
 	arr[13] = scsi_debug_physblk_exp & 0xf;
 	arr[14] = (scsi_debug_lowest_aligned >> 8) & 0x3f;
 
-	if (scsi_debug_lbp())
+	if (scsi_debug_lbp()) {
 		arr[14] |= 0x80; /* LBPME */
+		if (scsi_debug_lbprz)
+			arr[14] |= 0x40; /* LBPRZ */
+	}
 
 	arr[15] = scsi_debug_lowest_aligned & 0xff;
 
@@ -2045,10 +2053,13 @@ static void unmap_region(sector_t lba, unsigned int len)
 		block = lba + alignment;
 		rem = do_div(block, granularity);
 
-		if (rem == 0 && lba + granularity <= end &&
-		    block < map_size)
+		if (rem == 0 && lba + granularity <= end && block < map_size) {
 			clear_bit(block, map_storep);
-
+			if (scsi_debug_lbprz)
+				memset(fake_storep +
+				       block * scsi_debug_sector_size, 0,
+				       scsi_debug_sector_size);
+		}
 		lba += granularity - rem;
 	}
 }
@@ -2730,6 +2741,7 @@ module_param_named(guard, scsi_debug_guard, int, S_IRUGO);
 module_param_named(lbpu, scsi_debug_lbpu, int, S_IRUGO);
 module_param_named(lbpws, scsi_debug_lbpws, int, S_IRUGO);
 module_param_named(lbpws10, scsi_debug_lbpws10, int, S_IRUGO);
+module_param_named(lbprz, scsi_debug_lbprz, int, S_IRUGO);
 module_param_named(lowest_aligned, scsi_debug_lowest_aligned, int, S_IRUGO);
 module_param_named(max_luns, scsi_debug_max_luns, int, S_IRUGO | S_IWUSR);
 module_param_named(max_queue, scsi_debug_max_queue, int, S_IRUGO | S_IWUSR);
@@ -2771,6 +2783,7 @@ MODULE_PARM_DESC(guard, "protection checksum: 0=crc, 1=ip (def=0)");
 MODULE_PARM_DESC(lbpu, "enable LBP, support UNMAP command (def=0)");
 MODULE_PARM_DESC(lbpws, "enable LBP, support WRITE SAME(16) with UNMAP bit (def=0)");
 MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (def=0)");
+MODULE_PARM_DESC(lbprz, "unmapped blocks return 0 on read (def=1)");
 MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
 MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
 MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to 255(def))");

Re: [PATCH V2] scsi_debug: add LBPRZ support

[Martin K. Petersen]

>>>>> "Eric" == Eric Sandeen <sandeen@redhat.com> writes:

Looks good.

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

Re: [PATCH V2] scsi_debug: add LBPRZ support

[Martin K. Petersen]

>>>>> "Eric" == Eric Sandeen <sandeen@redhat.com> writes:

Eric> note, I didn't change the return value of inquiry_evpd_b2(); that
Eric> seems unrelated to this change, and should be a separate patch,
Eric> no?


scsi_debug: Fix incorrect page length in logical block provisioning VPD

The page length for the 0xb2 VPD page is defined to be 4 bytes when no
provisioning descriptors are provided (DP=0).

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

---

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6888b2c..a393a61 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -777,7 +777,7 @@ static int inquiry_evpd_b1(unsigned char *arr)
 /* Thin provisioning VPD page (SBC-3) */
 static int inquiry_evpd_b2(unsigned char *arr)
 {
-	memset(arr, 0, 0x8);
+	memset(arr, 0, 0x4);
 	arr[0] = 0;			/* threshold exponent */
 
 	if (scsi_debug_lbpu)
@@ -789,7 +789,7 @@ static int inquiry_evpd_b2(unsigned char *arr)
 	if (scsi_debug_lbpws10)
 		arr[1] |= 1 << 5;
 
-	return 0x8;
+	return 0x4;
 }
 
 #define SDEBUG_LONG_INQ_SZ 96

Re: [PATCH V2] scsi_debug: add LBPRZ support

[Douglas Gilbert]

On 12-03-08 04:48 PM, Martin K. Petersen wrote:
>>>>>> "Eric" == Eric Sandeen<sandeen@redhat.com>  writes:
>
> Eric>  note, I didn't change the return value of inquiry_evpd_b2(); that
> Eric>  seems unrelated to this change, and should be a separate patch,
> Eric>  no?
>
>
> scsi_debug: Fix incorrect page length in logical block provisioning VPD
>
> The page length for the 0xb2 VPD page is defined to be 4 bytes when no
> provisioning descriptors are provided (DP=0).
>
> Signed-off-by: Martin K. Petersen<martin.petersen@oracle.com>
Acked-by: Douglas Gilbert <dgilbert@interlog.com>

Re: [PATCH V2] scsi_debug: add LBPRZ support

[Douglas Gilbert]

On 12-03-08 07:03 AM, Eric Sandeen wrote:
> Add LBPRZ support to scsi_debug; i.e. read zeros for
> unmapped blocks.
>
> Rather than checking for unmapped blocks at
> read time, this just zeroes them on the backing store
> at unmap time so it behaves the same way.
>
> This also adds a module parameter to disable it.
>
> lbprz, "unmapped blocks return 0 on read (def=1)"
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>
> V2: set flag in inquiry_evpd_b2() as well, and rename *_zeroes
> type symbols to *_lbprz
>
> note, I didn't change the return value of inquiry_evpd_b2();
> that seems unrelated to this change, and should be a separate
> patch, no?
>
> Thanks,
> -Eric

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