[PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Ming Lei]

The commit 7985090aa0201(sd: Disable discard_zeroes_data for UNMAP)
introduces regresion for QEMU SCSI.

QEMU SCSI device claims to support UNMAP, WRITE SAME and WRITE SAME
16 in LBP VPD page, but only provides "Maximum unmap LBA count"
in block limits VPD page, and "Maximum write same length" isn't set.

The default max_discard_sectors(SD_MAX_WS16_BLOCKS) can't work at
all since it is much bigger than the actual Maximum unmap LBA count.

This patch trys to fix the regression by setting 'max_ws_blocks'
as 'max_unmap_blocks' when block limits VPD page doesn't provide
"Maximum write same length" under the situation. This approach is
reasonable because device server supports the use of the WRITE
SAME or WRITE SAME 10 command to unmap LBAs when LBPWS or LBPWS10
is set in LBP VPD page.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/scsi/sd.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fedab3c..2a69fee 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2624,6 +2624,14 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 		} else {	/* LBP VPD page tells us what to use */
 
+			/*
+			 * Borrow max_unmap_blocks if max_ws_blocks isn't
+			 * provided from block limits VPD page
+			 */
+			if ((sdkp->lbpws10 || sdkp->lbpws) &&
+			    !sdkp->max_ws_blocks)
+				sdkp->max_ws_blocks = sdkp->max_unmap_blocks;
+
 			if (sdkp->lbpws)
 				sd_config_discard(sdkp, SD_LBP_WS16);
 			else if (sdkp->lbpws10)
-- 
1.7.9.5

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Martin K. Petersen]

>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:

Ming,

Ming> QEMU SCSI device claims to support UNMAP, WRITE SAME and WRITE
Ming> SAME 16 in LBP VPD page, but only provides "Maximum unmap LBA
Ming> count" in block limits VPD page, and "Maximum write same length"
Ming> isn't set.

That really sounds like a problem that should be fixed in QEMU SCSI.

Ming> The default max_discard_sectors(SD_MAX_WS16_BLOCKS) can't work at
Ming> all since it is much bigger than the actual Maximum unmap LBA
Ming> count.

There is absolutely no correlation between max write same blocks and max
unmap blocks. They are two entirely different commands. If QEMU SCSI
advertises support for WRITE SAME(16) and does not report a cap in the
block limits VPD then we must expect that it supports the maximum number
of blocks that can be expressed by the command.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Ming Lei]

On Fri, Dec 5, 2014 at 8:56 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:
>
> Ming,
>
> Ming> QEMU SCSI device claims to support UNMAP, WRITE SAME and WRITE
> Ming> SAME 16 in LBP VPD page, but only provides "Maximum unmap LBA
> Ming> count" in block limits VPD page, and "Maximum write same length"
> Ming> isn't set.
>
> That really sounds like a problem that should be fixed in QEMU SCSI.

It can be fixed, but there are lots of old QEMU in production.

>
> Ming> The default max_discard_sectors(SD_MAX_WS16_BLOCKS) can't work at
> Ming> all since it is much bigger than the actual Maximum unmap LBA
> Ming> count.
>
> There is absolutely no correlation between max write same blocks and max
> unmap blocks. They are two entirely different commands. If QEMU SCSI

Do you have any better idea for the problem?

> advertises support for WRITE SAME(16) and does not report a cap in the
> block limits VPD then we must expect that it supports the maximum number
> of blocks that can be expressed by the command.

Unfortunately it doesn't support that, see below log:

[   50.112885] sd 0:0:1:0: [sda] FAILED Result: hostbyte=DID_OK
driverbyte=DRIVER_SENSE
[   50.113859] sd 0:0:1:0: [sda] Sense Key : Illegal Request [current]
[   50.113859] sd 0:0:1:0: [sda] Add. Sense: Invalid field in cdb
[   50.113859] sd 0:0:1:0: [sda] CDB:
[   50.113859] Write same(16): 93 08 00 00 00 00 00 00 80 00 00 40 00 00 00 00
[   50.113859] blk_update_request: critical target error, dev sda, sector
32768


Thanks,
Ming Lei

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Martin K. Petersen]

>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:

>> There is absolutely no correlation between max write same blocks and
>> max unmap blocks. They are two entirely different commands. If QEMU
>> SCSI

Ming> Do you have any better idea for the problem?

Does QEMU SCSI set LBPRZ?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Ming Lei]

On Fri, Dec 5, 2014 at 9:22 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:
>
>>> There is absolutely no correlation between max write same blocks and
>>> max unmap blocks. They are two entirely different commands. If QEMU
>>> SCSI
>
> Ming> Do you have any better idea for the problem?
>
> Does QEMU SCSI set LBPRZ?

It isn't set in LBP VPG page.

Thanks,

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Martin K. Petersen]

>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:

Ming> It isn't set in LBP VPG page.

What about in READ CAPACITY(16)?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Ming Lei]

On Fri, Dec 5, 2014 at 9:38 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:
>
> Ming> It isn't set in LBP VPG page.
>
> What about in READ CAPACITY(16)?

It isn't set too.

Thanks,
Ming Lei

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Martin K. Petersen]

>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:

>> What about in READ CAPACITY(16)?

Ming> It isn't set too.

Please try the following patch:


[SCSI] sd: Tweak discard heuristics to work around QEMU SCSI issue

7985090aa020 changed the discard heuristics to give preference to the
WRITE SAME commands that (unlike UNMAP) guarantee deterministic results.

Ming Lei discovered that QEMU SCSI's WRITE SAME implementation
internally relied on limits that were only communicated for the UNMAP
case. And therefore discard commands backed by WRITE SAME would fail.

Tweak the heuristics so we still pick UNMAP in the LBPRZ=0 case and only
prefer the WRITE SAME variants if the device has the LBPRZ flag set.

Reported-by: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 95bfb7bfbb9d..76c5d1388417 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2623,8 +2623,9 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 				sd_config_discard(sdkp, SD_LBP_WS16);
 
 		} else {	/* LBP VPD page tells us what to use */
-
-			if (sdkp->lbpws)
+			if (sdkp->lbpu && sdkp->max_unmap_blocks && !sdkp->lbprz)
+				sd_config_discard(sdkp, SD_LBP_UNMAP);
+			else if (sdkp->lbpws)
 				sd_config_discard(sdkp, SD_LBP_WS16);
 			else if (sdkp->lbpws10)
 				sd_config_discard(sdkp, SD_LBP_WS10);

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Ming Lei]

On Fri, Dec 5, 2014 at 9:58 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:
>
>>> What about in READ CAPACITY(16)?
>
> Ming> It isn't set too.
>
> Please try the following patch:
>
>
> [SCSI] sd: Tweak discard heuristics to work around QEMU SCSI issue
>
> 7985090aa020 changed the discard heuristics to give preference to the
> WRITE SAME commands that (unlike UNMAP) guarantee deterministic results.
>
> Ming Lei discovered that QEMU SCSI's WRITE SAME implementation
> internally relied on limits that were only communicated for the UNMAP

I just found it should be one QEMU bug for emulating write same, but
limiting write same sectors number as max_unmap_blocks can
workaround the issue.

> case. And therefore discard commands backed by WRITE SAME would fail.
>
> Tweak the heuristics so we still pick UNMAP in the LBPRZ=0 case and only
> prefer the WRITE SAME variants if the device has the LBPRZ flag set.
>
> Reported-by: Ming Lei <ming.lei@canonical.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 95bfb7bfbb9d..76c5d1388417 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2623,8 +2623,9 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
>                                 sd_config_discard(sdkp, SD_LBP_WS16);
>
>                 } else {        /* LBP VPD page tells us what to use */
> -
> -                       if (sdkp->lbpws)
> +                       if (sdkp->lbpu && sdkp->max_unmap_blocks && !sdkp->lbprz)
> +                               sd_config_discard(sdkp, SD_LBP_UNMAP);
> +                       else if (sdkp->lbpws)
>                                 sd_config_discard(sdkp, SD_LBP_WS16);
>                         else if (sdkp->lbpws10)
>                                 sd_config_discard(sdkp, SD_LBP_WS10);

Looks it does work.

Thanks,
Ming Lei

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Paolo Bonzini]

On 05/12/2014 14:58, Martin K. Petersen wrote:
>>>>>> "Ming" == Ming Lei <ming.lei@canonical.com> writes:
> 
>>> What about in READ CAPACITY(16)?
> 
> Ming> It isn't set too.
> 
> Please try the following patch:
> 
> 
> [SCSI] sd: Tweak discard heuristics to work around QEMU SCSI issue
> 
> 7985090aa020 changed the discard heuristics to give preference to the
> WRITE SAME commands that (unlike UNMAP) guarantee deterministic results.
> 
> Ming Lei discovered that QEMU SCSI's WRITE SAME implementation
> internally relied on limits that were only communicated for the UNMAP
> case. And therefore discard commands backed by WRITE SAME would fail.
> 
> Tweak the heuristics so we still pick UNMAP in the LBPRZ=0 case and only
> prefer the WRITE SAME variants if the device has the LBPRZ flag set.
> 
> Reported-by: Ming Lei <ming.lei@canonical.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 95bfb7bfbb9d..76c5d1388417 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2623,8 +2623,9 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
>  				sd_config_discard(sdkp, SD_LBP_WS16);
>  
>  		} else {	/* LBP VPD page tells us what to use */
> -
> -			if (sdkp->lbpws)
> +			if (sdkp->lbpu && sdkp->max_unmap_blocks && !sdkp->lbprz)
> +				sd_config_discard(sdkp, SD_LBP_UNMAP);
> +			else if (sdkp->lbpws)
>  				sd_config_discard(sdkp, SD_LBP_WS16);
>  			else if (sdkp->lbpws10)
>  				sd_config_discard(sdkp, SD_LBP_WS10);
> 

This is the right fix.  Ming, how do you reproduce the QEMU bug?

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Paolo Bonzini]

On 05/12/2014 14:05, Ming Lei wrote:
> 
> [   50.112885] sd 0:0:1:0: [sda] FAILED Result: hostbyte=DID_OK
> driverbyte=DRIVER_SENSE
> [   50.113859] sd 0:0:1:0: [sda] Sense Key : Illegal Request [current]
> [   50.113859] sd 0:0:1:0: [sda] Add. Sense: Invalid field in cdb
> [   50.113859] sd 0:0:1:0: [sda] CDB:
> [   50.113859] Write same(16): 93 08 00 00 00 00 00 00 80 00 00 40 00 00 00 00
> [   50.113859] blk_update_request: critical target error, dev sda, sector
> 32768

So this command is zeroing 2^22 sectors (2GB) starting from sector 128.
 How did you run QEMU and what command produced this request?

Paolo

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Ming Lei]

On Fri, Dec 5, 2014 at 11:49 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 05/12/2014 14:05, Ming Lei wrote:
>>
>> [   50.112885] sd 0:0:1:0: [sda] FAILED Result: hostbyte=DID_OK
>> driverbyte=DRIVER_SENSE
>> [   50.113859] sd 0:0:1:0: [sda] Sense Key : Illegal Request [current]
>> [   50.113859] sd 0:0:1:0: [sda] Add. Sense: Invalid field in cdb
>> [   50.113859] sd 0:0:1:0: [sda] CDB:
>> [   50.113859] Write same(16): 93 08 00 00 00 00 00 00 80 00 00 40 00 00 00 00
>> [   50.113859] blk_update_request: critical target error, dev sda, sector
>> 32768
>
> So this command is zeroing 2^22 sectors (2GB) starting from sector 128.
>  How did you run QEMU and what command produced this request?

It was found in xfstests, and turns out it is a QEMU INT_MAX bug.

Thanks

Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided

[Christoph Hellwig]

Thanks,

applied to scsi-for-3.19.