fix a few zoned append issues

fix a few zoned append issues

[Christoph Hellwig]

Hi Jens, hi Damien, hi btrfs maintainers,

this fixes a few issues found with zoned xfs testing that affect block
layer interfaces used by file systems, and the btrfs code making use
of them.

[PATCH 1/4] block: fix bio_split_rw_at to take zone_write_granularity into account

[Christoph Hellwig]

Otherwise it can create unaligned writes on zoned devices.

Fixes: a805a4fa4fa3 ("block: introduce zone_write_granularity limit")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index d813d799cee7..d6895859a2fb 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -358,7 +358,8 @@ int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
 	 * split size so that each bio is properly block size aligned, even if
 	 * we do not use the full hardware limits.
 	 */
-	bytes = ALIGN_DOWN(bytes, lim->logical_block_size);
+	bytes = ALIGN_DOWN(bytes, lim->zone_write_granularity ?
+			lim->zone_write_granularity : lim->logical_block_size);
 
 	/*
 	 * Bio splitting may cause subtle trouble such as hang when doing sync
-- 
2.45.2

Re: [PATCH 1/4] block: fix bio_split_rw_at to take zone_write_granularity into account

[Damien Le Moal]

On 11/1/24 14:21, Christoph Hellwig wrote:
> Otherwise it can create unaligned writes on zoned devices.
> 
> Fixes: a805a4fa4fa3 ("block: introduce zone_write_granularity limit")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-merge.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index d813d799cee7..d6895859a2fb 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -358,7 +358,8 @@ int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
>  	 * split size so that each bio is properly block size aligned, even if
>  	 * we do not use the full hardware limits.
>  	 */
> -	bytes = ALIGN_DOWN(bytes, lim->logical_block_size);
> +	bytes = ALIGN_DOWN(bytes, lim->zone_write_granularity ?
> +			lim->zone_write_granularity : lim->logical_block_size);

Nit: we could also do:

	bytes = ALIGN_DOWN(bytes,
		max(lim->logical_block_size, lim->zone_write_granularity));

Also, I wonder if we should leave read split as is based on the logical block
size only ? Probably does not matter much...

>  
>  	/*
>  	 * Bio splitting may cause subtle trouble such as hang when doing sync


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/4] block: fix bio_split_rw_at to take zone_write_granularity into account

[Christoph Hellwig]

On Fri, Nov 01, 2024 at 02:34:31PM +0900, Damien Le Moal wrote:
> > -	bytes = ALIGN_DOWN(bytes, lim->logical_block_size);
> > +	bytes = ALIGN_DOWN(bytes, lim->zone_write_granularity ?
> > +			lim->zone_write_granularity : lim->logical_block_size);
> 
> Nit: we could also do:
> 
> 	bytes = ALIGN_DOWN(bytes,
> 		max(lim->logical_block_size, lim->zone_write_granularity));

That's what I had first.  It is a little odd as zone_write_granularity
is defined to be >= logical_block_size, though.

> Also, I wonder if we should leave read split as is based on the logical block
> size only ? Probably does not matter much...

Good point.  Probably doesn't matter much, but randomly forcing it on
reads seems odd.

[PATCH 2/4] block: lift bio_is_zone_append to bio.h

[Christoph Hellwig]

Make bio_is_zone_append globally available, because file systems need
to use to check for a zone append bio in their end_io handlers to deal
with the block layer emulation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk.h         |  9 ---------
 include/linux/bio.h | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/block/blk.h b/block/blk.h
index 63d5df0dc29c..6060e1e180a3 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -457,11 +457,6 @@ static inline bool bio_zone_write_plugging(struct bio *bio)
 {
 	return bio_flagged(bio, BIO_ZONE_WRITE_PLUGGING);
 }
-static inline bool bio_is_zone_append(struct bio *bio)
-{
-	return bio_op(bio) == REQ_OP_ZONE_APPEND ||
-		bio_flagged(bio, BIO_EMULATES_ZONE_APPEND);
-}
 void blk_zone_write_plug_bio_merged(struct bio *bio);
 void blk_zone_write_plug_init_request(struct request *rq);
 static inline void blk_zone_update_request_bio(struct request *rq,
@@ -510,10 +505,6 @@ static inline bool bio_zone_write_plugging(struct bio *bio)
 {
 	return false;
 }
-static inline bool bio_is_zone_append(struct bio *bio)
-{
-	return false;
-}
 static inline void blk_zone_write_plug_bio_merged(struct bio *bio)
 {
 }
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 4a1bf43ca53d..60830a6a5939 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -675,6 +675,23 @@ static inline void bio_clear_polled(struct bio *bio)
 	bio->bi_opf &= ~REQ_POLLED;
 }
 
+/**
+ * bio_is_zone_append - is this a zone append bio?
+ * @bio:	bio to check
+ *
+ * Check if @bio is a zone append operation.  Core block layer code and end_io
+ * handlers must use this instead of an open coded REQ_OP_ZONE_APPEND check
+ * because the block layer can rewrite REQ_OP_ZONE_APPEND to REQ_OP_WRITE if
+ * it is not natively supported.
+ */
+static inline bool bio_is_zone_append(struct bio *bio)
+{
+	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
+		return false;
+	return bio_op(bio) == REQ_OP_ZONE_APPEND ||
+		bio_flagged(bio, BIO_EMULATES_ZONE_APPEND);
+}
+
 struct bio *blk_next_bio(struct bio *bio, struct block_device *bdev,
 		unsigned int nr_pages, blk_opf_t opf, gfp_t gfp);
 struct bio *bio_chain_and_submit(struct bio *prev, struct bio *new);
-- 
2.45.2

Re: [PATCH 2/4] block: lift bio_is_zone_append to bio.h

[Damien Le Moal]

On 11/1/24 14:21, Christoph Hellwig wrote:
> Make bio_is_zone_append globally available, because file systems need
> to use to check for a zone append bio in their end_io handlers to deal
> with the block layer emulation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good (one nit below).

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  block/blk.h         |  9 ---------
>  include/linux/bio.h | 17 +++++++++++++++++
>  2 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/block/blk.h b/block/blk.h
> index 63d5df0dc29c..6060e1e180a3 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -457,11 +457,6 @@ static inline bool bio_zone_write_plugging(struct bio *bio)
>  {
>  	return bio_flagged(bio, BIO_ZONE_WRITE_PLUGGING);
>  }
> -static inline bool bio_is_zone_append(struct bio *bio)
> -{
> -	return bio_op(bio) == REQ_OP_ZONE_APPEND ||
> -		bio_flagged(bio, BIO_EMULATES_ZONE_APPEND);
> -}
>  void blk_zone_write_plug_bio_merged(struct bio *bio);
>  void blk_zone_write_plug_init_request(struct request *rq);
>  static inline void blk_zone_update_request_bio(struct request *rq,
> @@ -510,10 +505,6 @@ static inline bool bio_zone_write_plugging(struct bio *bio)
>  {
>  	return false;
>  }
> -static inline bool bio_is_zone_append(struct bio *bio)
> -{
> -	return false;
> -}
>  static inline void blk_zone_write_plug_bio_merged(struct bio *bio)
>  {
>  }
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 4a1bf43ca53d..60830a6a5939 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -675,6 +675,23 @@ static inline void bio_clear_polled(struct bio *bio)
>  	bio->bi_opf &= ~REQ_POLLED;
>  }
>  
> +/**
> + * bio_is_zone_append - is this a zone append bio?
> + * @bio:	bio to check
> + *
> + * Check if @bio is a zone append operation.  Core block layer code and end_io
> + * handlers must use this instead of an open coded REQ_OP_ZONE_APPEND check
> + * because the block layer can rewrite REQ_OP_ZONE_APPEND to REQ_OP_WRITE if
> + * it is not natively supported.
> + */
> +static inline bool bio_is_zone_append(struct bio *bio)
> +{
> +	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> +		return false;

Nit: this "if" is probably not needed. But it does not hurt either. Since we
should never be seeing this function being called for the
!IS_ENABLED(CONFIG_BLK_DEV_ZONED) case, should we add a WARN_ON_ONCE() ?

> +	return bio_op(bio) == REQ_OP_ZONE_APPEND ||
> +		bio_flagged(bio, BIO_EMULATES_ZONE_APPEND);
> +}
> +
>  struct bio *blk_next_bio(struct bio *bio, struct block_device *bdev,
>  		unsigned int nr_pages, blk_opf_t opf, gfp_t gfp);
>  struct bio *bio_chain_and_submit(struct bio *prev, struct bio *new);


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] block: lift bio_is_zone_append to bio.h

[Christoph Hellwig]

On Fri, Nov 01, 2024 at 02:37:29PM +0900, Damien Le Moal wrote:
> > +static inline bool bio_is_zone_append(struct bio *bio)
> > +{
> > +	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> > +		return false;
> 
> Nit: this "if" is probably not needed. But it does not hurt either. Since we
> should never be seeing this function being called for the
> !IS_ENABLED(CONFIG_BLK_DEV_ZONED) case, should we add a WARN_ON_ONCE() ?

The point of the IS_ENALBED is to optimize away the code when it can't
be used.  The WARN_ON_ONCE would generate worse code than just leaving
the check in.

Re: [PATCH 2/4] block: lift bio_is_zone_append to bio.h

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 3/4] btrfs: use bio_is_zone_append in the completion handler

[Christoph Hellwig]

Otherwise it won't catch bios turned into regular writes by the
block level zone write plugging.

Fixes: 9b1ce7f0c6f8 ("block: Implement zone append emulation")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/bio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index fec5c6cde0a7..0f096e226908 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -366,7 +366,7 @@ static void btrfs_simple_end_io(struct bio *bio)
 		INIT_WORK(&bbio->end_io_work, btrfs_end_bio_work);
 		queue_work(btrfs_end_io_wq(fs_info, bio), &bbio->end_io_work);
 	} else {
-		if (bio_op(bio) == REQ_OP_ZONE_APPEND && !bio->bi_status)
+		if (bio_is_zone_append(bio) && !bio->bi_status)
 			btrfs_record_physical_zoned(bbio);
 		btrfs_bio_end_io(bbio, bbio->bio.bi_status);
 	}
@@ -409,7 +409,7 @@ static void btrfs_orig_write_end_io(struct bio *bio)
 	else
 		bio->bi_status = BLK_STS_OK;
 
-	if (bio_op(bio) == REQ_OP_ZONE_APPEND && !bio->bi_status)
+	if (bio_is_zone_append(bio) && !bio->bi_status)
 		stripe->physical = bio->bi_iter.bi_sector << SECTOR_SHIFT;
 
 	btrfs_bio_end_io(bbio, bbio->bio.bi_status);
@@ -423,7 +423,7 @@ static void btrfs_clone_write_end_io(struct bio *bio)
 	if (bio->bi_status) {
 		atomic_inc(&stripe->bioc->error);
 		btrfs_log_dev_io_error(bio, stripe->dev);
-	} else if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
+	} else if (bio_is_zone_append(bio)) {
 		stripe->physical = bio->bi_iter.bi_sector << SECTOR_SHIFT;
 	}
 
-- 
2.45.2

Re: [PATCH 3/4] btrfs: use bio_is_zone_append in the completion handler

[Damien Le Moal]

On 11/1/24 14:21, Christoph Hellwig wrote:
> Otherwise it won't catch bios turned into regular writes by the
> block level zone write plugging.
> 
> Fixes: 9b1ce7f0c6f8 ("block: Implement zone append emulation")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] btrfs: use bio_is_zone_append in the completion handler

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/4] btrfs: use bio_is_zone_append in the completion handler

[Naohiro Aota]

On Fri, Nov 01, 2024 at 06:21:46AM GMT, Christoph Hellwig wrote:
> Otherwise it won't catch bios turned into regular writes by the
> block level zone write plugging.
> 
> Fixes: 9b1ce7f0c6f8 ("block: Implement zone append emulation")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/bio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

[PATCH 4/4] btrfs: split bios to the fs sector size boundary

[Christoph Hellwig]

Btrfs like other file systems can't really deal with I/O not aligned to
it's internal block size (which strangely is called sector size in
btrfs), but the block layer split helper doesn't even know about that.

Round down the split boundary so that all I/Os are aligned.

Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/bio.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 0f096e226908..299b9a0ed68b 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -660,8 +660,15 @@ static u64 btrfs_append_map_length(struct btrfs_bio *bbio, u64 map_length)
 	map_length = min(map_length, bbio->fs_info->max_zone_append_size);
 	sector_offset = bio_split_rw_at(&bbio->bio, &bbio->fs_info->limits,
 					&nr_segs, map_length);
-	if (sector_offset)
-		return sector_offset << SECTOR_SHIFT;
+	if (sector_offset) {
+		/*
+		 * bio_split_rw_at could split at a size smaller than the
+		 * file system sector size and thus cause unaligned I/Os.
+		 * Fix that by always rounding down to the nearest boundary.
+		 */
+		return ALIGN_DOWN(sector_offset << SECTOR_SHIFT,
+				  bbio->fs_info->sectorsize);
+	}
 	return map_length;
 }
 
-- 
2.45.2

Re: [PATCH 4/4] btrfs: split bios to the fs sector size boundary

[Damien Le Moal]

On 11/1/24 14:21, Christoph Hellwig wrote:
> Btrfs like other file systems can't really deal with I/O not aligned to
> it's internal block size (which strangely is called sector size in
> btrfs), but the block layer split helper doesn't even know about that.
> 
> Round down the split boundary so that all I/Os are aligned.
> 
> Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] btrfs: split bios to the fs sector size boundary

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/4] btrfs: split bios to the fs sector size boundary

[Naohiro Aota]

On Fri, Nov 01, 2024 at 06:21:47AM GMT, Christoph Hellwig wrote:
> Btrfs like other file systems can't really deal with I/O not aligned to
> it's internal block size (which strangely is called sector size in
> btrfs), but the block layer split helper doesn't even know about that.
> 
> Round down the split boundary so that all I/Os are aligned.
> 
> Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/bio.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Looks good to me.

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>