[PATCH] block: fix detection of unsupported WRITE SAME in blkdev_issue_write_zeroes

[PATCH] block: fix detection of unsupported WRITE SAME in blkdev_issue_write_zeroes

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

On error, blkdev_issue_write_zeroes used to recheck the block device's
WRITE SAME queue limits after submitting WRITE SAME bios.  As stated in
the comment, the purpose of this was to collapse all IO errors to
EOPNOTSUPP if the effect of issuing bios was that WRITE SAME got turned
off in the queue limits.  Therefore, it does not make sense to reuse the
zeroes limit that was read earlier in the function because we only care
about the queue limit *now*, not what it was at the start of the
function.

Found by running generic/351 from fstests.

Fixes: 64b582ca88ca1 ("block: Read max write zeroes once for __blkdev_issue_write_zeroes()")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 block/blk-lib.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 83eb7761c2bfb..4c9f20a689f7b 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -174,7 +174,7 @@ static int blkdev_issue_write_zeroes(struct block_device *bdev, sector_t sector,
 	 * on an I/O error, in which case we'll turn any error into
 	 * "not supported" here.
 	 */
-	if (ret && !limit)
+	if (ret && !bdev_write_zeroes_sectors(bdev))
 		return -EOPNOTSUPP;
 	return ret;
 }

Re: [PATCH] block: fix detection of unsupported WRITE SAME in blkdev_issue_write_zeroes

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 10:53:40AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> On error, blkdev_issue_write_zeroes used to recheck the block device's
> WRITE SAME queue limits after submitting WRITE SAME bios.  As stated in
> the comment, the purpose of this was to collapse all IO errors to
> EOPNOTSUPP if the effect of issuing bios was that WRITE SAME got turned
> off in the queue limits.  Therefore, it does not make sense to reuse the
> zeroes limit that was read earlier in the function because we only care
> about the queue limit *now*, not what it was at the start of the
> function.

Yes, that was a bit overeager..

Looks good:

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

Re: [PATCH] block: fix detection of unsupported WRITE SAME in blkdev_issue_write_zeroes

[John Garry]

On 27/08/2024 18:53, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> On error, blkdev_issue_write_zeroes used to recheck the block device's
> WRITE SAME queue limits after submitting WRITE SAME bios.  As stated in
> the comment, the purpose of this was to collapse all IO errors to
> EOPNOTSUPP if the effect of issuing bios was that WRITE SAME got turned
> off in the queue limits.  Therefore, it does not make sense to reuse the
> zeroes limit that was read earlier in the function because we only care
> about the queue limit *now*, not what it was at the start of the
> function.
> 
> Found by running generic/351 from fstests.

thanks for the fix

Reviewed-by: John Garry <john.g.garry@oracle.com>

> 
> Fixes: 64b582ca88ca1 ("block: Read max write zeroes once for __blkdev_issue_write_zeroes()")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>   block/blk-lib.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 83eb7761c2bfb..4c9f20a689f7b 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -174,7 +174,7 @@ static int blkdev_issue_write_zeroes(struct block_device *bdev, sector_t sector,
>   	 * on an I/O error, in which case we'll turn any error into
>   	 * "not supported" here.
>   	 */
> -	if (ret && !limit)

I don't think that we still require local variable @limit. Actually we 
can probably clean this up later, and Nitesh's suggestion on the 
original patch could have been considered more

> +	if (ret && !bdev_write_zeroes_sectors(bdev))
>   		return -EOPNOTSUPP;
>   	return ret;
>   }
> 

Re: [PATCH] block: fix detection of unsupported WRITE SAME in blkdev_issue_write_zeroes

[Jens Axboe]

On Tue, 27 Aug 2024 10:53:40 -0700, Darrick J. Wong wrote:
> On error, blkdev_issue_write_zeroes used to recheck the block device's
> WRITE SAME queue limits after submitting WRITE SAME bios.  As stated in
> the comment, the purpose of this was to collapse all IO errors to
> EOPNOTSUPP if the effect of issuing bios was that WRITE SAME got turned
> off in the queue limits.  Therefore, it does not make sense to reuse the
> zeroes limit that was read earlier in the function because we only care
> about the queue limit *now*, not what it was at the start of the
> function.
> 
> [...]

Applied, thanks!

[1/1] block: fix detection of unsupported WRITE SAME in blkdev_issue_write_zeroes
      (no commit info)

Best regards,
-- 
Jens Axboe