Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
Thanks, applied to scsi-for-3.19. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On Fri, Dec 5, 2014 at 11:49 PM, Paolo Bonzini 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On 05/12/2014 14:58, Martin K. Petersen wrote:
>> "Ming" == Ming Lei 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
> Signed-off-by: Martin K. Petersen
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On Fri, Dec 5, 2014 at 9:58 PM, Martin K. Petersen
wrote:
>> "Ming" == Ming Lei 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
> Signed-off-by: Martin K. Petersen
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
> "Ming" == Ming Lei 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
Signed-off-by: Martin K. Petersen
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On Fri, Dec 5, 2014 at 9:38 PM, Martin K. Petersen wrote: >> "Ming" == Ming Lei writes: > > Ming> It isn't set in LBP VPG page. > > What about in READ CAPACITY(16)? It isn't set too. Thanks, Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
> "Ming" == Ming Lei writes: Ming> It isn't set in LBP VPG page. What about in READ CAPACITY(16)? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On Fri, Dec 5, 2014 at 9:22 PM, Martin K. Petersen wrote: >> "Ming" == Ming Lei 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, -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
> "Ming" == Ming Lei 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
On Fri, Dec 5, 2014 at 8:56 PM, Martin K. Petersen wrote: >> "Ming" == Ming Lei 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
> "Ming" == Ming Lei 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] SCSI: SD: set max_ws_blocks as max_unmap_blocks if it isn't provided
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
Cc: Paolo Bonzini
Cc: Christoph Hellwig
Signed-off-by: Ming Lei
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
