Re: scsi-debug regression with 4.5-rc?

Re: scsi-debug regression with 4.5-rc?

[Ewan Milne]

So I have a report from our test people that the optimal_io_size sysfs
value is now different by a factor of 512 from what it used to be...

>Here is what is executed:
>
>modprobe scsi_debug dev_size_mb=32 sector_size=4096 opt_blks=64
>num_tgts=1
>
>And here is what our test is capturing:
>
>/sys/dev/block/8:0/queue/optimal_io_size = 512 differs from expected
>value 
>1048576!
>
>
>(So with 64 it's 512,  with opt_blks=256 it's 2048,  but it used to be
>1048576)

This looks like it might be due to commit:

commit ca369d51b3e1649be4a72addd6d6a168cfb3f537
Author: Martin K. Petersen <martin.petersen@oracle.com>
Date:   Fri Nov 13 16:46:48 2015 -0500

    block/sd: Fix device-imposed transfer length limits

It would appear that this commit has changed the meaning of
the queue limits io_opt field to be 512-byte normalized
sectors instead of bytes.  Parts of the diff:

-       blk_queue_io_opt(sdkp->disk->queue,
-                        get_unaligned_be32(&buffer[12]) * sector_sz);

...

+       sdkp->opt_xfer_blocks = get_unaligned_be32(&buffer[12]);

...

+       if (sdkp->opt_xfer_blocks && sdkp->opt_xfer_blocks <= dev_max &&
+           sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS)
+               rw_max = q->limits.io_opt =
+                       logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
+       else
+               rw_max = BLK_DEF_MAX_SECTORS;


---

Was this intentional?  I would have thought we would not want to change
the usermode meaning of this field.

-Ewan

Re: scsi-debug regression with 4.5-rc?

[Martin K. Petersen]

>>>>> "Ewan" == Ewan Milne <emilne@redhat.com> writes:

Ewan> So I have a report from our test people that the optimal_io_size
Ewan> sysfs value is now different by a factor of 512 from what it used
Ewan> to be...

Yes, just prepared a patch this morning. I messed up sectors vs. bytes.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: scsi-debug regression with 4.5-rc?

[Mike Snitzer]

On Wed, Jan 20 2016 at 10:57am -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Ewan" == Ewan Milne <emilne@redhat.com> writes:
> 
> Ewan> So I have a report from our test people that the optimal_io_size
> Ewan> sysfs value is now different by a factor of 512 from what it used
> Ewan> to be...
> 
> Yes, just prepared a patch this morning. I messed up sectors vs. bytes.

Would your fix (commit d0eb20a863ba7dc) address this BZ too?
https://bugzilla.redhat.com/show_bug.cgi?id=1314687

Seems not, I think that BZ may be due to the LBPRZ clause in commit
397737223 ("sd: Make discard granularity match logical block size when LBPRZ=1") ?

should be: q->limits.discard_granularity = 1 * logical_block_size;

Mike

Re: scsi-debug regression with 4.5-rc?

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike> Seems not, I think that BZ may be due to the LBPRZ clause in
Mike> commit 397737223 ("sd: Make discard granularity match logical
Mike> block size when LBPRZ=1") ?

Mike> should be: q->limits.discard_granularity = 1 * logical_block_size;

*blush*

Will fix.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] sd: Fix discard granularity when LBPRZ=1

[Martin K. Petersen]

Commit 397737223c59 ("sd: Make discard granularity match logical block
size when LBPRZ=1") accidentally set the granularity to one byte instead
of one logical block on devices that provide determistic zeroes after
UNMAP.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Reported-by: Mike Snitzer <snitzer@redhat.com>
Fixes: 397737223c59e89dca7305feb6528caef8fbef84
Cc: <stable@vger.kernel.org> # 4.4+
---
 drivers/scsi/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d749da765df1..5a5457ac9cdb 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -648,7 +648,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 	 */
 	if (sdkp->lbprz) {
 		q->limits.discard_alignment = 0;
-		q->limits.discard_granularity = 1;
+		q->limits.discard_granularity = logical_block_size;
 	} else {
 		q->limits.discard_alignment = sdkp->unmap_alignment *
 			logical_block_size;
-- 
2.7.0

Re: [PATCH] sd: Fix discard granularity when LBPRZ=1

[Ewan Milne]

On Sat, 2016-03-05 at 18:10 -0500, Martin K. Petersen wrote:
> Commit 397737223c59 ("sd: Make discard granularity match logical block
> size when LBPRZ=1") accidentally set the granularity to one byte instead
> of one logical block on devices that provide determistic zeroes after
> UNMAP.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Mike Snitzer <snitzer@redhat.com>
> Fixes: 397737223c59e89dca7305feb6528caef8fbef84
> Cc: <stable@vger.kernel.org> # 4.4+
> ---
>  drivers/scsi/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d749da765df1..5a5457ac9cdb 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -648,7 +648,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
>  	 */
>  	if (sdkp->lbprz) {
>  		q->limits.discard_alignment = 0;
> -		q->limits.discard_granularity = 1;
> +		q->limits.discard_granularity = logical_block_size;
>  	} else {
>  		q->limits.discard_alignment = sdkp->unmap_alignment *
>  			logical_block_size;

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH] sd: Fix discard granularity when LBPRZ=1

[Martin K. Petersen]

>>>>> "Ewan" == Ewan Milne <emilne@redhat.com> writes:
>> Commit 397737223c59 ("sd: Make discard granularity match logical
>> block size when LBPRZ=1") accidentally set the granularity to one
>> byte instead of one logical block on devices that provide determistic
>> zeroes after UNMAP.

Ewan> Reviewed-by: Ewan D. Milne <emilne@redhat.com>

I'll need another reviewer for this.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: Fix discard granularity when LBPRZ=1

[Bart Van Assche]

On 03/05/2016 03:10 PM, Martin K. Petersen wrote:
> Commit 397737223c59 ("sd: Make discard granularity match logical block
> size when LBPRZ=1") accidentally set the granularity to one byte instead
> of one logical block on devices that provide determistic zeroes after
> UNMAP.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Mike Snitzer <snitzer@redhat.com>
> Fixes: 397737223c59e89dca7305feb6528caef8fbef84
> Cc: <stable@vger.kernel.org> # 4.4+
> ---
>   drivers/scsi/sd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d749da765df1..5a5457ac9cdb 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -648,7 +648,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
>   	 */
>   	if (sdkp->lbprz) {
>   		q->limits.discard_alignment = 0;
> -		q->limits.discard_granularity = 1;
> +		q->limits.discard_granularity = logical_block_size;
>   	} else {
>   		q->limits.discard_alignment = sdkp->unmap_alignment *
>   			logical_block_size;

Please fix the spelling of "deterministic" in the patch description. 
With or without that change:

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH] sd: Fix discard granularity when LBPRZ=1

[Martin K. Petersen]

>>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:

Bart> Please fix the spelling of "deterministic" in the patch
Bart> description. With or without that change:

Bart> Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Fixed, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering