[PATCH v2 0/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[PATCH v2 0/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Ilya Dryomov]

Hi Christoph, Martin,

blkdev_issue_zeroout() now checks for any error.  This required a minor
refactor, so I dropped the stable tag, Jens can add it back if needed.

v1 -> v2:
- changed code flow in blkdev_issue_zeroout() according to Christoph's
  suggestion
- this required adding additional checks to blkdev_issue_zeroout() and
  __blkdev_issue_zero_pages(), the latter is no longer void

Previous patch(es) and discussion at

  https://marc.info/?l=linux-block&m=150471953327942&w=2
  https://marc.info/?l=linux-block&m=150601399031909&w=2

Thanks,

                Ilya


Ilya Dryomov (2):
  block: factor out __blkdev_issue_zero_pages()
  block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

 block/blk-lib.c | 104 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 36 deletions(-)

-- 
2.4.3

[PATCH v2 1/2] block: factor out __blkdev_issue_zero_pages()

[Ilya Dryomov]

blkdev_issue_zeroout() will use this in !BLKDEV_ZERO_NOFALLBACK case.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 block/blk-lib.c | 63 +++++++++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 62240f8832ca..9d2ab8bba52a 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -274,6 +274,40 @@ static unsigned int __blkdev_sectors_to_bio_pages(sector_t nr_sects)
 	return min(pages, (sector_t)BIO_MAX_PAGES);
 }
 
+static int __blkdev_issue_zero_pages(struct block_device *bdev,
+		sector_t sector, sector_t nr_sects, gfp_t gfp_mask,
+		struct bio **biop)
+{
+	struct request_queue *q = bdev_get_queue(bdev);
+	struct bio *bio = *biop;
+	int bi_size = 0;
+	unsigned int sz;
+
+	if (!q)
+		return -ENXIO;
+
+	while (nr_sects != 0) {
+		bio = next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects),
+			       gfp_mask);
+		bio->bi_iter.bi_sector = sector;
+		bio_set_dev(bio, bdev);
+		bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
+
+		while (nr_sects != 0) {
+			sz = min((sector_t) PAGE_SIZE, nr_sects << 9);
+			bi_size = bio_add_page(bio, ZERO_PAGE(0), sz, 0);
+			nr_sects -= bi_size >> 9;
+			sector += bi_size >> 9;
+			if (bi_size < sz)
+				break;
+		}
+		cond_resched();
+	}
+
+	*biop = bio;
+	return 0;
+}
+
 /**
  * __blkdev_issue_zeroout - generate number of zero filed write bios
  * @bdev:	blockdev to issue
@@ -304,9 +338,6 @@ int __blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 		unsigned flags)
 {
 	int ret;
-	int bi_size = 0;
-	struct bio *bio = *biop;
-	unsigned int sz;
 	sector_t bs_mask;
 
 	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
@@ -316,30 +347,10 @@ int __blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	ret = __blkdev_issue_write_zeroes(bdev, sector, nr_sects, gfp_mask,
 			biop, flags);
 	if (ret != -EOPNOTSUPP || (flags & BLKDEV_ZERO_NOFALLBACK))
-		goto out;
-
-	ret = 0;
-	while (nr_sects != 0) {
-		bio = next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects),
-			       gfp_mask);
-		bio->bi_iter.bi_sector = sector;
-		bio_set_dev(bio, bdev);
-		bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
-
-		while (nr_sects != 0) {
-			sz = min((sector_t) PAGE_SIZE, nr_sects << 9);
-			bi_size = bio_add_page(bio, ZERO_PAGE(0), sz, 0);
-			nr_sects -= bi_size >> 9;
-			sector += bi_size >> 9;
-			if (bi_size < sz)
-				break;
-		}
-		cond_resched();
-	}
+		return ret;
 
-	*biop = bio;
-out:
-	return ret;
+	return __blkdev_issue_zero_pages(bdev, sector, nr_sects, gfp_mask,
+					 biop);
 }
 EXPORT_SYMBOL(__blkdev_issue_zeroout);
 
-- 
2.4.3

[PATCH v2 2/2] block: cope with WRITE ZEROES 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.  Because REQ_OP_WRITE_ZEROES is implemented in terms of WRITE
SAME, blkdev_issue_zeroout() may fail with -EREMOTEIO:

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

The following calls succeed because sd_done() sets ->no_write_same in
response to a sense that would become BLK_STS_TARGET/-EREMOTEIO, causing
__blkdev_issue_zeroout() to fall back to generating ZERO_PAGE bios.

This means blkdev_issue_zeroout() must cope with WRITE ZEROES failing
and fall back to manually zeroing, unless BLKDEV_ZERO_NOFALLBACK is
specified.  For BLKDEV_ZERO_NOFALLBACK case, return -EOPNOTSUPP if
sd_done() has just set ->no_write_same thus indicating lack of offload
support.

Fixes: c20cfc27a473 ("block: stop using blkdev_issue_write_same for zeroing")
Cc: Christoph Hellwig <hch@lst.de>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 block/blk-lib.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 9d2ab8bba52a..17494275673e 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -321,12 +321,6 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
  *  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.
  *
@@ -370,18 +364,45 @@ EXPORT_SYMBOL(__blkdev_issue_zeroout);
 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;
+	int ret = 0;
+	sector_t bs_mask;
+	struct bio *bio;
 	struct blk_plug plug;
+	bool try_write_zeroes = !!bdev_write_zeroes_sectors(bdev);
+
+	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
+	if ((sector | nr_sects) & bs_mask)
+		return -EINVAL;
 
+retry:
+	bio = NULL;
 	blk_start_plug(&plug);
-	ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
-			&bio, flags);
+	if (try_write_zeroes) {
+		ret = __blkdev_issue_write_zeroes(bdev, sector, nr_sects,
+						  gfp_mask, &bio, flags);
+	} else if (!(flags & BLKDEV_ZERO_NOFALLBACK)) {
+		ret = __blkdev_issue_zero_pages(bdev, sector, nr_sects,
+						gfp_mask, &bio);
+	} else if (!bdev_write_zeroes_sectors(bdev)) {
+		/*
+		 * Manual zeroout is not allowed and either:
+		 * - no zeroing offload support
+		 * - zeroing offload support was indicated, but the device
+		 *   reported ILLEGAL REQUEST (for some devices there is no
+		 *   non-destructive way to verify whether WRITE ZEROES is
+		 *   actually supported)
+		 */
+		ret = -EOPNOTSUPP;
+	}
 	if (ret == 0 && bio) {
 		ret = submit_bio_wait(bio);
 		bio_put(bio);
 	}
 	blk_finish_plug(&plug);
+	if (ret && try_write_zeroes) {
+		try_write_zeroes = false;
+		goto retry;
+	}
 
 	return ret;
 }
-- 
2.4.3

Re: [PATCH v2 1/2] block: factor out __blkdev_issue_zero_pages()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Christoph Hellwig]

On Wed, Oct 04, 2017 at 05:03:16PM +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.  Because REQ_OP_WRITE_ZEROES is implemented in terms of WRITE
> SAME, blkdev_issue_zeroout() may fail with -EREMOTEIO:
> 
>   $ fallocate -zn -l 1k /dev/sdg
>   fallocate: fallocate failed: Remote I/O error
>   $ fallocate -zn -l 1k /dev/sdg  # OK
>   $ fallocate -zn -l 1k /dev/sdg  # OK

Can we wire this up for blktests somehow?

> 
> The following calls succeed because sd_done() sets ->no_write_same in
> response to a sense that would become BLK_STS_TARGET/-EREMOTEIO, causing
> __blkdev_issue_zeroout() to fall back to generating ZERO_PAGE bios.
> 
> This means blkdev_issue_zeroout() must cope with WRITE ZEROES failing
> and fall back to manually zeroing, unless BLKDEV_ZERO_NOFALLBACK is
> specified.  For BLKDEV_ZERO_NOFALLBACK case, return -EOPNOTSUPP if
> sd_done() has just set ->no_write_same thus indicating lack of offload
> support.
> 
> Fixes: c20cfc27a473 ("block: stop using blkdev_issue_write_same for zeroing")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  block/blk-lib.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 9d2ab8bba52a..17494275673e 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -321,12 +321,6 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
>   *  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.
>   *
> @@ -370,18 +364,45 @@ EXPORT_SYMBOL(__blkdev_issue_zeroout);
>  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;
> +	int ret = 0;
> +	sector_t bs_mask;
> +	struct bio *bio;
>  	struct blk_plug plug;
> +	bool try_write_zeroes = !!bdev_write_zeroes_sectors(bdev);
> +
> +	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
> +	if ((sector | nr_sects) & bs_mask)
> +		return -EINVAL;
>  
> +retry:
> +	bio = NULL;
>  	blk_start_plug(&plug);
> -	ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
> -			&bio, flags);
> +	if (try_write_zeroes) {
> +		ret = __blkdev_issue_write_zeroes(bdev, sector, nr_sects,
> +						  gfp_mask, &bio, flags);
> +	} else if (!(flags & BLKDEV_ZERO_NOFALLBACK)) {
> +		ret = __blkdev_issue_zero_pages(bdev, sector, nr_sects,
> +						gfp_mask, &bio);
> +	} else if (!bdev_write_zeroes_sectors(bdev)) {
> +		/*
> +		 * Manual zeroout is not allowed and either:
> +		 * - no zeroing offload support
> +		 * - zeroing offload support was indicated, but the device
> +		 *   reported ILLEGAL REQUEST (for some devices there is no
> +		 *   non-destructive way to verify whether WRITE ZEROES is
> +		 *   actually supported)
> +		 */
> +		ret = -EOPNOTSUPP;

I don't understand the conditional above this error return - if
we can't zero using either method we should always return an error.

Except for that the patch looks fine.

Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Ilya Dryomov]

On Thu, Oct 5, 2017 at 7:13 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Oct 04, 2017 at 05:03:16PM +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.  Because REQ_OP_WRITE_ZEROES is implemented in terms of WRITE
>> SAME, blkdev_issue_zeroout() may fail with -EREMOTEIO:
>>
>>   $ fallocate -zn -l 1k /dev/sdg
>>   fallocate: fallocate failed: Remote I/O error
>>   $ fallocate -zn -l 1k /dev/sdg  # OK
>>   $ fallocate -zn -l 1k /dev/sdg  # OK
>
> Can we wire this up for blktests somehow?

This is covered by Darrick's generic/351, part of fstests blockdev
group.

>
>>
>> The following calls succeed because sd_done() sets ->no_write_same in
>> response to a sense that would become BLK_STS_TARGET/-EREMOTEIO, causing
>> __blkdev_issue_zeroout() to fall back to generating ZERO_PAGE bios.
>>
>> This means blkdev_issue_zeroout() must cope with WRITE ZEROES failing
>> and fall back to manually zeroing, unless BLKDEV_ZERO_NOFALLBACK is
>> specified.  For BLKDEV_ZERO_NOFALLBACK case, return -EOPNOTSUPP if
>> sd_done() has just set ->no_write_same thus indicating lack of offload
>> support.
>>
>> Fixes: c20cfc27a473 ("block: stop using blkdev_issue_write_same for zeroing")
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
>> Cc: Hannes Reinecke <hare@suse.com>
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> ---
>>  block/blk-lib.c | 41 +++++++++++++++++++++++++++++++----------
>>  1 file changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/blk-lib.c b/block/blk-lib.c
>> index 9d2ab8bba52a..17494275673e 100644
>> --- a/block/blk-lib.c
>> +++ b/block/blk-lib.c
>> @@ -321,12 +321,6 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
>>   *  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.
>>   *
>> @@ -370,18 +364,45 @@ EXPORT_SYMBOL(__blkdev_issue_zeroout);
>>  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;
>> +     int ret = 0;
>> +     sector_t bs_mask;
>> +     struct bio *bio;
>>       struct blk_plug plug;
>> +     bool try_write_zeroes = !!bdev_write_zeroes_sectors(bdev);
>> +
>> +     bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
>> +     if ((sector | nr_sects) & bs_mask)
>> +             return -EINVAL;
>>
>> +retry:
>> +     bio = NULL;
>>       blk_start_plug(&plug);
>> -     ret = __blkdev_issue_zeroout(bdev, sector, nr_sects, gfp_mask,
>> -                     &bio, flags);
>> +     if (try_write_zeroes) {
>> +             ret = __blkdev_issue_write_zeroes(bdev, sector, nr_sects,
>> +                                               gfp_mask, &bio, flags);
>> +     } else if (!(flags & BLKDEV_ZERO_NOFALLBACK)) {
>> +             ret = __blkdev_issue_zero_pages(bdev, sector, nr_sects,
>> +                                             gfp_mask, &bio);
>> +     } else if (!bdev_write_zeroes_sectors(bdev)) {
>> +             /*
>> +              * Manual zeroout is not allowed and either:
>> +              * - no zeroing offload support
>> +              * - zeroing offload support was indicated, but the device
>> +              *   reported ILLEGAL REQUEST (for some devices there is no
>> +              *   non-destructive way to verify whether WRITE ZEROES is
>> +              *   actually supported)
>> +              */
>> +             ret = -EOPNOTSUPP;
>
> I don't understand the conditional above this error return - if
> we can't zero using either method we should always return an error.

This is to avoid returning -EREMOTEIO in the following case: device
doesn't support WRITE SAME but scsi_disk::max_ws_blocks != 0, zeroout
is called with BLKDEV_ZERO_NOFALLBACK.  Enter blkdev_issue_zeroout(),
bdev_write_zeroes_sectors() != 0, so we issue WRITE ZEROES.  The
request fails with ILLEGAL REQUEST, sd_done() sets ->no_write_same and
updates queue_limits, ILLEGAL REQUEST is translated into -EREMOTEIO,
which is returned from submit_bio_wait().  Manual zeroing is not
allowed, so we must return an error, but it shouldn't be -EREMOTEIO if
queue_limits just got updated because of ILLEGAL REQUEST.  Without this
conditional, we'd get

  $ fallocate -pn -l 1k /dev/sdg
  fallocate: fallocate failed: Remote I/O error
  $ fallocate -pn -l 1k /dev/sdg  # -EOPNOTSUPP
  fallocate: keep size mode (-n option) unsupported
  $ fallocate -pn -l 1k /dev/sdg  # -EOPNOTSUPP
  fallocate: keep size mode (-n option) unsupported

I tried to explain this between the comment and the commit message.
Basically, just mopping up after sd_config_write_same().

Thanks,

                Ilya

Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Christoph Hellwig]

On Thu, Oct 05, 2017 at 09:32:33PM +0200, Ilya Dryomov wrote:
> This is to avoid returning -EREMOTEIO in the following case: device
> doesn't support WRITE SAME but scsi_disk::max_ws_blocks != 0, zeroout
> is called with BLKDEV_ZERO_NOFALLBACK.  Enter blkdev_issue_zeroout(),
> bdev_write_zeroes_sectors() != 0, so we issue WRITE ZEROES.  The
> request fails with ILLEGAL REQUEST, sd_done() sets ->no_write_same and
> updates queue_limits, ILLEGAL REQUEST is translated into -EREMOTEIO,
> which is returned from submit_bio_wait().  Manual zeroing is not
> allowed, so we must return an error, but it shouldn't be -EREMOTEIO if
> queue_limits just got updated because of ILLEGAL REQUEST.  Without this
> conditional, we'd get

Hmm.  I think we'd better off to just do the before the retry loop:

	if (ret && try_write_zeroes) {
		if (!(flags & BLKDEV_ZERO_NOFALLBACK))
			try_write_zeroes = false;
			goto retry;
		}
		ret = -EOPNOTSUPP;
	}

Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Ilya Dryomov]

On Fri, Oct 6, 2017 at 2:05 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Oct 05, 2017 at 09:32:33PM +0200, Ilya Dryomov wrote:
>> This is to avoid returning -EREMOTEIO in the following case: device
>> doesn't support WRITE SAME but scsi_disk::max_ws_blocks != 0, zeroout
>> is called with BLKDEV_ZERO_NOFALLBACK.  Enter blkdev_issue_zeroout(),
>> bdev_write_zeroes_sectors() != 0, so we issue WRITE ZEROES.  The
>> request fails with ILLEGAL REQUEST, sd_done() sets ->no_write_same and
>> updates queue_limits, ILLEGAL REQUEST is translated into -EREMOTEIO,
>> which is returned from submit_bio_wait().  Manual zeroing is not
>> allowed, so we must return an error, but it shouldn't be -EREMOTEIO if
>> queue_limits just got updated because of ILLEGAL REQUEST.  Without this
>> conditional, we'd get
>
> Hmm.  I think we'd better off to just do the before the retry loop:
>
>         if (ret && try_write_zeroes) {
>                 if (!(flags & BLKDEV_ZERO_NOFALLBACK))
>                         try_write_zeroes = false;
>                         goto retry;
>                 }
>                 ret = -EOPNOTSUPP;
>         }

This would unconditionally overwrite any WRITE ZEROS error.  If we get
e.g. -EIO, and manual zeroing is not allowed, I don't think we want to
return -EOPNOTSUPP?

Returning -EOPNOTSUPP to mean "can't zero using either method" doesn't
make sense to me, because manual zeroing is always supported, just not
always allowed.

Thanks,

                Ilya

Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Ilya Dryomov]

On Fri, Oct 6, 2017 at 2:31 PM, Ilya Dryomov <idryomov@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 2:05 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Thu, Oct 05, 2017 at 09:32:33PM +0200, Ilya Dryomov wrote:
>>> This is to avoid returning -EREMOTEIO in the following case: device
>>> doesn't support WRITE SAME but scsi_disk::max_ws_blocks != 0, zeroout
>>> is called with BLKDEV_ZERO_NOFALLBACK.  Enter blkdev_issue_zeroout(),
>>> bdev_write_zeroes_sectors() != 0, so we issue WRITE ZEROES.  The
>>> request fails with ILLEGAL REQUEST, sd_done() sets ->no_write_same and
>>> updates queue_limits, ILLEGAL REQUEST is translated into -EREMOTEIO,
>>> which is returned from submit_bio_wait().  Manual zeroing is not
>>> allowed, so we must return an error, but it shouldn't be -EREMOTEIO if
>>> queue_limits just got updated because of ILLEGAL REQUEST.  Without this
>>> conditional, we'd get
>>
>> Hmm.  I think we'd better off to just do the before the retry loop:
>>
>>         if (ret && try_write_zeroes) {
>>                 if (!(flags & BLKDEV_ZERO_NOFALLBACK))
>>                         try_write_zeroes = false;
>>                         goto retry;
>>                 }
>>                 ret = -EOPNOTSUPP;
>>         }
>
> This would unconditionally overwrite any WRITE ZEROS error.  If we get
> e.g. -EIO, and manual zeroing is not allowed, I don't think we want to
> return -EOPNOTSUPP?
>
> Returning -EOPNOTSUPP to mean "can't zero using either method" doesn't
> make sense to me, because manual zeroing is always supported, just not
> always allowed.

Ping...

Thanks,

                Ilya

Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Christoph Hellwig]

On Fri, Oct 06, 2017 at 02:31:20PM +0200, Ilya Dryomov wrote:
> This would unconditionally overwrite any WRITE ZEROS error.  If we get
> e.g. -EIO, and manual zeroing is not allowed, I don't think we want to
> return -EOPNOTSUPP?
> 
> Returning -EOPNOTSUPP to mean "can't zero using either method" doesn't
> make sense to me, because manual zeroing is always supported, just not
> always allowed.

Then thow the throw bdev_write_zeroes_sectors check back in:

	if (ret && try_write_zeroes) {
                if (!(flags & BLKDEV_ZERO_NOFALLBACK))
                        try_write_zeroes = false;
                        goto retry;
                }
		if (!bdev_write_zeroes_sectors(bdev))
	                ret = -EOPNOTSUPP;
        }

The important bit is that the structure in the current patch where the
bdev_write_zeroes_sectors check is on the same level as the method
selection is extremely confusing.

Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

[Ilya Dryomov]

On Mon, Oct 16, 2017 at 1:44 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Oct 06, 2017 at 02:31:20PM +0200, Ilya Dryomov wrote:
>> This would unconditionally overwrite any WRITE ZEROS error.  If we get
>> e.g. -EIO, and manual zeroing is not allowed, I don't think we want to
>> return -EOPNOTSUPP?
>>
>> Returning -EOPNOTSUPP to mean "can't zero using either method" doesn't
>> make sense to me, because manual zeroing is always supported, just not
>> always allowed.
>
> Then thow the throw bdev_write_zeroes_sectors check back in:
>
>         if (ret && try_write_zeroes) {
>                 if (!(flags & BLKDEV_ZERO_NOFALLBACK))
>                         try_write_zeroes = false;
>                         goto retry;
>                 }
>                 if (!bdev_write_zeroes_sectors(bdev))
>                         ret = -EOPNOTSUPP;
>         }
>
> The important bit is that the structure in the current patch where the
> bdev_write_zeroes_sectors check is on the same level as the method
> selection is extremely confusing.

I see.  An updated version should be in your inbox.

Thanks,

                Ilya