Re: [PATCH] block: cope with WRITE SAME failing in blkdev_issue_zeroout()

[Martin K. Petersen]

Christoph,

> I'm really not sure we should check for -EREMOTEIO specifically, but
> Martin who is more familiar with the SCSI code might be able to
> correct me, I'd feel safer about checking for any error which is
> what the old code did.
>
> Except for that the patch looks fine to me.

We explicitly return -EREMOTEIO when the device reports ILLEGAL
REQUEST. But I agree that we should fall back to manually zeroing for
any error.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] block: cope with WRITE SAME failing in blkdev_issue_zeroout()

[Ilya Dryomov]

On Tue, Sep 19, 2017 at 10:32 PM, Christoph Hellwig  wrote:
> On Wed, Sep 06, 2017 at 07:38:10PM +0200, Ilya Dryomov wrote:
>> sd_config_write_same() ignores ->max_ws_blocks == 0 and resets it to
>> permit trying WRITE SAME on older SCSI devices, unless ->no_write_same
>> is set.  This means blkdev_issue_zeroout() must cope with WRITE SAME
>> failing with BLK_STS_TARGET/-EREMOTEIO and explicitly write zeroes,
>> unless BLKDEV_ZERO_NOFALLBACK is specified.
>>
>> Commit 71027e97d796 ("block: stop using discards for zeroing") added
>> the following to __blkdev_issue_zeroout() comment:
>>
>>   "Note that this function may fail with -EOPNOTSUPP if the driver signals
>>   zeroing offload support, but the device fails to process the command (for
>>   some devices there is no non-destructive way to verify whether this
>>   operation is actually supported).  In this case the caller should call
>>   retry the call to blkdev_issue_zeroout() and the fallback path will be 
>> used."
>>
>> But __blkdev_issue_zeroout() doesn't fail in this case: if WRITE SAME
>> support is indicated, a REQ_OP_WRITE_ZEROES bio is built and returned
>> to blkdev_issue_zeroout().  -EREMOTEIO is then propagated up:
>>
>>   $ fallocate -zn -l 1k /dev/sdg
>>   fallocate: fallocate failed: Remote I/O error
>>   $ fallocate -zn -l 1k /dev/sdg  # OK
>>
>> (The second fallocate(1) succeeds because sd_done() sets ->no_write_same
>> in response to a sense that would become BLK_STS_TARGET.)
>>
>> Retry __blkdev_issue_zeroout() if the I/O fails with -EREMOTEIO.  This
>> is roughly what we did until 4.12, sans BLKDEV_ZERO_NOFALLBACK knob.
>
> I'm really not sure we should check for -EREMOTEIO specifically, but
> Martin who is more familiar with the SCSI code might be able to
> correct me, I'd feel safer about checking for any error which is
> what the old code did.

Yeah, I pondered that.  The old code did check for any error, but it
wouldn't attempt another WRITE SAME after a failure.  To do that here,
we'd need to refactor __blkdev_issue_zeroout(); as it is, if we retry
__blkdev_issue_zeroout() on a random error, it would happily go the
REQ_OP_WRITE_ZEROES way again.

Martin, what do you think?

Thanks,

Ilya

Re: [PATCH] block: cope with WRITE SAME failing in blkdev_issue_zeroout()

[Christoph Hellwig]

On Wed, Sep 06, 2017 at 07:38:10PM +0200, Ilya Dryomov wrote:
> sd_config_write_same() ignores ->max_ws_blocks == 0 and resets it to
> permit trying WRITE SAME on older SCSI devices, unless ->no_write_same
> is set.  This means blkdev_issue_zeroout() must cope with WRITE SAME
> failing with BLK_STS_TARGET/-EREMOTEIO and explicitly write zeroes,
> unless BLKDEV_ZERO_NOFALLBACK is specified.
> 
> Commit 71027e97d796 ("block: stop using discards for zeroing") added
> the following to __blkdev_issue_zeroout() comment:
> 
>   "Note that this function may fail with -EOPNOTSUPP if the driver signals
>   zeroing offload support, but the device fails to process the command (for
>   some devices there is no non-destructive way to verify whether this
>   operation is actually supported).  In this case the caller should call
>   retry the call to blkdev_issue_zeroout() and the fallback path will be 
> used."
> 
> But __blkdev_issue_zeroout() doesn't fail in this case: if WRITE SAME
> support is indicated, a REQ_OP_WRITE_ZEROES bio is built and returned
> to blkdev_issue_zeroout().  -EREMOTEIO is then propagated up:
> 
>   $ fallocate -zn -l 1k /dev/sdg
>   fallocate: fallocate failed: Remote I/O error
>   $ fallocate -zn -l 1k /dev/sdg  # OK
> 
> (The second fallocate(1) succeeds because sd_done() sets ->no_write_same
> in response to a sense that would become BLK_STS_TARGET.)
> 
> Retry __blkdev_issue_zeroout() if the I/O fails with -EREMOTEIO.  This
> is roughly what we did until 4.12, sans BLKDEV_ZERO_NOFALLBACK knob.

I'm really not sure we should check for -EREMOTEIO specifically, but
Martin who is more familiar with the SCSI code might be able to
correct me, I'd feel safer about checking for any error which is
what the old code did.

Except for that the patch looks fine to me.

Re: [PATCH] block: cope with WRITE SAME failing in blkdev_issue_zeroout()

[Ilya Dryomov]

On Wed, Sep 13, 2017 at 11:50 AM, Ilya Dryomov  wrote:
> On Wed, Sep 6, 2017 at 7:38 PM, Ilya Dryomov  wrote:
>> sd_config_write_same() ignores ->max_ws_blocks == 0 and resets it to
>> permit trying WRITE SAME on older SCSI devices, unless ->no_write_same
>> is set.  This means blkdev_issue_zeroout() must cope with WRITE SAME
>> failing with BLK_STS_TARGET/-EREMOTEIO and explicitly write zeroes,
>> unless BLKDEV_ZERO_NOFALLBACK is specified.
>>
>> Commit 71027e97d796 ("block: stop using discards for zeroing") added
>> the following to __blkdev_issue_zeroout() comment:
>>
>>   "Note that this function may fail with -EOPNOTSUPP if the driver signals
>>   zeroing offload support, but the device fails to process the command (for
>>   some devices there is no non-destructive way to verify whether this
>>   operation is actually supported).  In this case the caller should call
>>   retry the call to blkdev_issue_zeroout() and the fallback path will be 
>> used."
>>
>> But __blkdev_issue_zeroout() doesn't fail in this case: if WRITE SAME
>> support is indicated, a REQ_OP_WRITE_ZEROES bio is built and returned
>> to blkdev_issue_zeroout().  -EREMOTEIO is then propagated up:
>>
>>   $ fallocate -zn -l 1k /dev/sdg
>>   fallocate: fallocate failed: Remote I/O error
>>   $ fallocate -zn -l 1k /dev/sdg  # OK
>>
>> (The second fallocate(1) succeeds because sd_done() sets ->no_write_same
>> in response to a sense that would become BLK_STS_TARGET.)
>>
>> Retry __blkdev_issue_zeroout() if the I/O fails with -EREMOTEIO.  This
>> is roughly what we did until 4.12, sans BLKDEV_ZERO_NOFALLBACK knob.
>>
>> Fixes: c20cfc27a473 ("block: stop using blkdev_issue_write_same for zeroing")
>> Cc: Christoph Hellwig 
>> Cc: "Martin K. Petersen" 
>> Cc: Hannes Reinecke 
>> Cc: sta...@vger.kernel.org # 4.12+
>> Signed-off-by: Ilya Dryomov 
>> ---
>>  block/blk-lib.c | 49 +
>>  1 file changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/blk-lib.c b/block/blk-lib.c
>> index 3fe0aec90597..876b5478c1a2 100644
>> --- a/block/blk-lib.c
>> +++ b/block/blk-lib.c
>> @@ -287,12 +287,6 @@ static unsigned int 
>> __blkdev_sectors_to_bio_pages(sector_t nr_sects)
>>   *  Zero-fill a block range, either using hardware offload or by explicitly
>>   *  writing zeroes to the device.
>>   *
>> - *  Note that this function may fail with -EOPNOTSUPP if the driver signals
>> - *  zeroing offload support, but the device fails to process the command 
>> (for
>> - *  some devices there is no non-destructive way to verify whether this
>> - *  operation is actually supported).  In this case the caller should call
>> - *  retry the call to blkdev_issue_zeroout() and the fallback path will be 
>> used.
>> - *
>>   *  If a device is using logical block provisioning, the underlying space 
>> will
>>   *  not be released if %flags contains BLKDEV_ZERO_NOUNMAP.
>>   *
>> @@ -343,6 +337,34 @@ int __blkdev_issue_zeroout(struct block_device *bdev, 
>> sector_t sector,
>>  }
>>  EXPORT_SYMBOL(__blkdev_issue_zeroout);
>>
>> +/*
>> + * This function may fail with -EREMOTEIO if the driver signals zeroing
>> + * offload support, but the device fails to process the command (for
>> + * some devices there is no non-destructive way to verify whether this
>> + * operation is actually supported).  In this case the caller should
>> + * retry and the fallback path in __blkdev_issue_zeroout() will be used
>> + * unless %flags contains BLKDEV_ZERO_NOFALLBACK.
>> + */
>> +static int __blkdev_issue_zeroout_wait(struct block_device *bdev,
>> +  sector_t sector, sector_t nr_sects,
>> +  gfp_t gfp_mask, unsigned flags)
>> +{
>> +   int ret;
>> +   struct bio *bio = NULL;
>> +   struct blk_plug plug;
>> +
>> +   blk_start_plug();
>> +   ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
>> +   , flags);
>> +   if (ret == 0 && bio) {
>> +   ret = submit_bio_wait(bio);
>> +   bio_put(bio);
>> +   }
>> +   blk_finish_plug();
>> +
>> +   return ret;
>> +}
>> +
>>  /**
>>   * blkdev_issue_zeroout - zero-fill a block range
>>   * @bdev:  blockdev to write
>> @@ -360,17 +382,12 @@ int blkdev_issue_zeroout(struct block_device *bdev, 
>> sector_t sector,
>> sector_t nr_sects, gfp_t gfp_mask, unsigned flags)
>>  {
>> int ret;
>> -   struct bio *bio = NULL;
>> -   struct blk_plug plug;
>>
>> -   blk_start_plug();
>> -   ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
>> -   , flags);
>> -   if (ret == 0 && bio) {
>> -   ret = submit_bio_wait(bio);
>> -   bio_put(bio);
>> -   }
>> -   blk_finish_plug();
>> +   ret = __blkdev_issue_zeroout_wait(bdev, 

Re: [PATCH] block: cope with WRITE SAME failing in blkdev_issue_zeroout()

[Ilya Dryomov]

On Wed, Sep 6, 2017 at 7:38 PM, Ilya Dryomov  wrote:
> sd_config_write_same() ignores ->max_ws_blocks == 0 and resets it to
> permit trying WRITE SAME on older SCSI devices, unless ->no_write_same
> is set.  This means blkdev_issue_zeroout() must cope with WRITE SAME
> failing with BLK_STS_TARGET/-EREMOTEIO and explicitly write zeroes,
> unless BLKDEV_ZERO_NOFALLBACK is specified.
>
> Commit 71027e97d796 ("block: stop using discards for zeroing") added
> the following to __blkdev_issue_zeroout() comment:
>
>   "Note that this function may fail with -EOPNOTSUPP if the driver signals
>   zeroing offload support, but the device fails to process the command (for
>   some devices there is no non-destructive way to verify whether this
>   operation is actually supported).  In this case the caller should call
>   retry the call to blkdev_issue_zeroout() and the fallback path will be 
> used."
>
> But __blkdev_issue_zeroout() doesn't fail in this case: if WRITE SAME
> support is indicated, a REQ_OP_WRITE_ZEROES bio is built and returned
> to blkdev_issue_zeroout().  -EREMOTEIO is then propagated up:
>
>   $ fallocate -zn -l 1k /dev/sdg
>   fallocate: fallocate failed: Remote I/O error
>   $ fallocate -zn -l 1k /dev/sdg  # OK
>
> (The second fallocate(1) succeeds because sd_done() sets ->no_write_same
> in response to a sense that would become BLK_STS_TARGET.)
>
> Retry __blkdev_issue_zeroout() if the I/O fails with -EREMOTEIO.  This
> is roughly what we did until 4.12, sans BLKDEV_ZERO_NOFALLBACK knob.
>
> Fixes: c20cfc27a473 ("block: stop using blkdev_issue_write_same for zeroing")
> Cc: Christoph Hellwig 
> Cc: "Martin K. Petersen" 
> Cc: Hannes Reinecke 
> Cc: sta...@vger.kernel.org # 4.12+
> Signed-off-by: Ilya Dryomov 
> ---
>  block/blk-lib.c | 49 +
>  1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 3fe0aec90597..876b5478c1a2 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -287,12 +287,6 @@ static unsigned int 
> __blkdev_sectors_to_bio_pages(sector_t nr_sects)
>   *  Zero-fill a block range, either using hardware offload or by explicitly
>   *  writing zeroes to the device.
>   *
> - *  Note that this function may fail with -EOPNOTSUPP if the driver signals
> - *  zeroing offload support, but the device fails to process the command (for
> - *  some devices there is no non-destructive way to verify whether this
> - *  operation is actually supported).  In this case the caller should call
> - *  retry the call to blkdev_issue_zeroout() and the fallback path will be 
> used.
> - *
>   *  If a device is using logical block provisioning, the underlying space 
> will
>   *  not be released if %flags contains BLKDEV_ZERO_NOUNMAP.
>   *
> @@ -343,6 +337,34 @@ int __blkdev_issue_zeroout(struct block_device *bdev, 
> sector_t sector,
>  }
>  EXPORT_SYMBOL(__blkdev_issue_zeroout);
>
> +/*
> + * This function may fail with -EREMOTEIO if the driver signals zeroing
> + * offload support, but the device fails to process the command (for
> + * some devices there is no non-destructive way to verify whether this
> + * operation is actually supported).  In this case the caller should
> + * retry and the fallback path in __blkdev_issue_zeroout() will be used
> + * unless %flags contains BLKDEV_ZERO_NOFALLBACK.
> + */
> +static int __blkdev_issue_zeroout_wait(struct block_device *bdev,
> +  sector_t sector, sector_t nr_sects,
> +  gfp_t gfp_mask, unsigned flags)
> +{
> +   int ret;
> +   struct bio *bio = NULL;
> +   struct blk_plug plug;
> +
> +   blk_start_plug();
> +   ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
> +   , flags);
> +   if (ret == 0 && bio) {
> +   ret = submit_bio_wait(bio);
> +   bio_put(bio);
> +   }
> +   blk_finish_plug();
> +
> +   return ret;
> +}
> +
>  /**
>   * blkdev_issue_zeroout - zero-fill a block range
>   * @bdev:  blockdev to write
> @@ -360,17 +382,12 @@ int blkdev_issue_zeroout(struct block_device *bdev, 
> sector_t sector,
> sector_t nr_sects, gfp_t gfp_mask, unsigned flags)
>  {
> int ret;
> -   struct bio *bio = NULL;
> -   struct blk_plug plug;
>
> -   blk_start_plug();
> -   ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
> -   , flags);
> -   if (ret == 0 && bio) {
> -   ret = submit_bio_wait(bio);
> -   bio_put(bio);
> -   }
> -   blk_finish_plug();
> +   ret = __blkdev_issue_zeroout_wait(bdev, sector, nr_sects,
> + gfp_mask, flags);
> +   if (ret == -EREMOTEIO)
> +   ret = __blkdev_issue_zeroout_wait(bdev, sector, nr_sects,
> + 

[PATCH] block: cope with WRITE SAME failing in blkdev_issue_zeroout()

[Ilya Dryomov]

sd_config_write_same() ignores ->max_ws_blocks == 0 and resets it to
permit trying WRITE SAME on older SCSI devices, unless ->no_write_same
is set.  This means blkdev_issue_zeroout() must cope with WRITE SAME
failing with BLK_STS_TARGET/-EREMOTEIO and explicitly write zeroes,
unless BLKDEV_ZERO_NOFALLBACK is specified.

Commit 71027e97d796 ("block: stop using discards for zeroing") added
the following to __blkdev_issue_zeroout() comment:

  "Note that this function may fail with -EOPNOTSUPP if the driver signals
  zeroing offload support, but the device fails to process the command (for
  some devices there is no non-destructive way to verify whether this
  operation is actually supported).  In this case the caller should call
  retry the call to blkdev_issue_zeroout() and the fallback path will be used."

But __blkdev_issue_zeroout() doesn't fail in this case: if WRITE SAME
support is indicated, a REQ_OP_WRITE_ZEROES bio is built and returned
to blkdev_issue_zeroout().  -EREMOTEIO is then propagated up:

  $ fallocate -zn -l 1k /dev/sdg
  fallocate: fallocate failed: Remote I/O error
  $ fallocate -zn -l 1k /dev/sdg  # OK

(The second fallocate(1) succeeds because sd_done() sets ->no_write_same
in response to a sense that would become BLK_STS_TARGET.)

Retry __blkdev_issue_zeroout() if the I/O fails with -EREMOTEIO.  This
is roughly what we did until 4.12, sans BLKDEV_ZERO_NOFALLBACK knob.

Fixes: c20cfc27a473 ("block: stop using blkdev_issue_write_same for zeroing")
Cc: Christoph Hellwig 
Cc: "Martin K. Petersen" 
Cc: Hannes Reinecke 
Cc: sta...@vger.kernel.org # 4.12+
Signed-off-by: Ilya Dryomov 
---
 block/blk-lib.c | 49 +
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 3fe0aec90597..876b5478c1a2 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -287,12 +287,6 @@ static unsigned int __blkdev_sectors_to_bio_pages(sector_t 
nr_sects)
  *  Zero-fill a block range, either using hardware offload or by explicitly
  *  writing zeroes to the device.
  *
- *  Note that this function may fail with -EOPNOTSUPP if the driver signals
- *  zeroing offload support, but the device fails to process the command (for
- *  some devices there is no non-destructive way to verify whether this
- *  operation is actually supported).  In this case the caller should call
- *  retry the call to blkdev_issue_zeroout() and the fallback path will be 
used.
- *
  *  If a device is using logical block provisioning, the underlying space will
  *  not be released if %flags contains BLKDEV_ZERO_NOUNMAP.
  *
@@ -343,6 +337,34 @@ int __blkdev_issue_zeroout(struct block_device *bdev, 
sector_t sector,
 }
 EXPORT_SYMBOL(__blkdev_issue_zeroout);
 
+/*
+ * This function may fail with -EREMOTEIO if the driver signals zeroing
+ * offload support, but the device fails to process the command (for
+ * some devices there is no non-destructive way to verify whether this
+ * operation is actually supported).  In this case the caller should
+ * retry and the fallback path in __blkdev_issue_zeroout() will be used
+ * unless %flags contains BLKDEV_ZERO_NOFALLBACK.
+ */
+static int __blkdev_issue_zeroout_wait(struct block_device *bdev,
+  sector_t sector, sector_t nr_sects,
+  gfp_t gfp_mask, unsigned flags)
+{
+   int ret;
+   struct bio *bio = NULL;
+   struct blk_plug plug;
+
+   blk_start_plug();
+   ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
+   , flags);
+   if (ret == 0 && bio) {
+   ret = submit_bio_wait(bio);
+   bio_put(bio);
+   }
+   blk_finish_plug();
+
+   return ret;
+}
+
 /**
  * blkdev_issue_zeroout - zero-fill a block range
  * @bdev:  blockdev to write
@@ -360,17 +382,12 @@ int blkdev_issue_zeroout(struct block_device *bdev, 
sector_t sector,
sector_t nr_sects, gfp_t gfp_mask, unsigned flags)
 {
int ret;
-   struct bio *bio = NULL;
-   struct blk_plug plug;
 
-   blk_start_plug();
-   ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
-   , flags);
-   if (ret == 0 && bio) {
-   ret = submit_bio_wait(bio);
-   bio_put(bio);
-   }
-   blk_finish_plug();
+   ret = __blkdev_issue_zeroout_wait(bdev, sector, nr_sects,
+ gfp_mask, flags);
+   if (ret == -EREMOTEIO)
+   ret = __blkdev_issue_zeroout_wait(bdev, sector, nr_sects,
+ gfp_mask, flags);
 
return ret;
 }
-- 
2.4.3