fix a few zoned append issues v2 (now with Ccs)

fix a few zoned append issues v2 (now with Ccs)

[Christoph Hellwig]

Hi Jens, hi Damien, hi btrfs maintainers,

(sorry for the third resend, I finally debugged why git-send-email dropped
the Cc to the lists)

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.

Changes since v1:
 - don't apply the zone_write_boundary to reads
 - add another patch to fix write_zeroes

[PATCH 1/5] block: take chunk_sectors into account in bio_split_write_zeroes

[Christoph Hellwig]

For zoned devices, write zeroes must be split at the zone boundary
which is represented as chunk_sectors.  For other uses like the
internally RAIDed NVMe devices it is probably at least useful.

Enhance get_max_io_size to know about write zeroes and use it in
bio_split_write_zeroes.  Also add a comment about the seemingly
nonsensical zero max_write_zeroes limit.

Fixes: 885fa13f6559 ("block: implement splitting of REQ_OP_WRITE_ZEROES bios")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index d813d799cee7..f440919b6c6f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -166,17 +166,6 @@ struct bio *bio_split_discard(struct bio *bio, const struct queue_limits *lim,
 	return bio_submit_split(bio, split_sectors);
 }
 
-struct bio *bio_split_write_zeroes(struct bio *bio,
-		const struct queue_limits *lim, unsigned *nsegs)
-{
-	*nsegs = 0;
-	if (!lim->max_write_zeroes_sectors)
-		return bio;
-	if (bio_sectors(bio) <= lim->max_write_zeroes_sectors)
-		return bio;
-	return bio_submit_split(bio, lim->max_write_zeroes_sectors);
-}
-
 static inline unsigned int blk_boundary_sectors(const struct queue_limits *lim,
 						bool is_atomic)
 {
@@ -211,7 +200,9 @@ static inline unsigned get_max_io_size(struct bio *bio,
 	 * We ignore lim->max_sectors for atomic writes because it may less
 	 * than the actual bio size, which we cannot tolerate.
 	 */
-	if (is_atomic)
+	if (bio_op(bio) == REQ_OP_WRITE_ZEROES)
+		max_sectors = lim->max_write_zeroes_sectors;
+	else if (is_atomic)
 		max_sectors = lim->atomic_write_max_sectors;
 	else
 		max_sectors = lim->max_sectors;
@@ -398,6 +389,26 @@ struct bio *bio_split_zone_append(struct bio *bio,
 	return bio_submit_split(bio, split_sectors);
 }
 
+struct bio *bio_split_write_zeroes(struct bio *bio,
+		const struct queue_limits *lim, unsigned *nsegs)
+{
+	unsigned int max_sectors = get_max_io_size(bio, lim);
+
+	*nsegs = 0;
+
+	/*
+	 * An unset limit should normally not happen, as bio submission is keyed
+	 * off having a non-zero limit.  But SCSI can clear the limit in the
+	 * I/O completion handler, and we can race and see this.  Splitting to a
+	 * zero limit obviously doesn't make sense, so band-aid it here.
+	 */
+	if (!max_sectors)
+		return bio;
+	if (bio_sectors(bio) <= max_sectors)
+		return bio;
+	return bio_submit_split(bio, max_sectors);
+}
+
 /**
  * bio_split_to_limits - split a bio to fit the queue limits
  * @bio:     bio to be split
-- 
2.45.2

Re: [PATCH 1/5] block: take chunk_sectors into account in bio_split_write_zeroes

[Damien Le Moal]

On 11/4/24 15:26, Christoph Hellwig wrote:
> For zoned devices, write zeroes must be split at the zone boundary
> which is represented as chunk_sectors.  For other uses like the
> internally RAIDed NVMe devices it is probably at least useful.
> 
> Enhance get_max_io_size to know about write zeroes and use it in
> bio_split_write_zeroes.  Also add a comment about the seemingly
> nonsensical zero max_write_zeroes limit.
> 
> Fixes: 885fa13f6559 ("block: implement splitting of REQ_OP_WRITE_ZEROES bios")
> 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: (subset) [PATCH 1/5] block: take chunk_sectors into account in bio_split_write_zeroes

[Jens Axboe]

On Mon, 04 Nov 2024 07:26:29 +0100, Christoph Hellwig wrote:
> For zoned devices, write zeroes must be split at the zone boundary
> which is represented as chunk_sectors.  For other uses like the
> internally RAIDed NVMe devices it is probably at least useful.
> 
> Enhance get_max_io_size to know about write zeroes and use it in
> bio_split_write_zeroes.  Also add a comment about the seemingly
> nonsensical zero max_write_zeroes limit.
> 
> [...]

Applied, thanks!

[1/5] block: take chunk_sectors into account in bio_split_write_zeroes
      commit: 60dc5ea6bcfd078b71419640d49afa649acf9450
[2/5] block: fix bio_split_rw_at to take zone_write_granularity into account
      commit: 7ecd2cd4fae3e8410c0a6620f3a83dcdbb254f02
[3/5] block: lift bio_is_zone_append to bio.h
      commit: 0ef2b9e698dbf9ba78f67952a747f35eb7060470

Best regards,
-- 
Jens Axboe

[PATCH 2/5] 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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index f440919b6c6f..4e6c0a52009c 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -287,6 +287,14 @@ static bool bvec_split_segs(const struct queue_limits *lim,
 	return len > 0 || bv->bv_len > max_len;
 }
 
+static unsigned int bio_split_alignment(struct bio *bio,
+		const struct queue_limits *lim)
+{
+	if (op_is_write(bio_op(bio)) && lim->zone_write_granularity)
+		return lim->zone_write_granularity;
+	return lim->logical_block_size;
+}
+
 /**
  * bio_split_rw_at - check if and where to split a read/write bio
  * @bio:  [in] bio to be split
@@ -349,7 +357,7 @@ 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, bio_split_alignment(bio, lim));
 
 	/*
 	 * Bio splitting may cause subtle trouble such as hang when doing sync
-- 
2.45.2

Re: [PATCH 2/5] block: fix bio_split_rw_at to take zone_write_granularity into account

[Damien Le Moal]

On 11/4/24 15:26, 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>

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/5] block: fix bio_split_rw_at to take zone_write_granularity into account

[Johannes Thumshirn]

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

[PATCH 3/5] 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>
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;
+	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 3/5] block: lift bio_is_zone_append to bio.h

[Johannes Thumshirn]

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

[PATCH 4/5] 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>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 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 4/5] btrfs: use bio_is_zone_append in the completion handler

[Johannes Thumshirn]

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

[PATCH 5/5] 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>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 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 5/5] btrfs: split bios to the fs sector size boundary

[Johannes Thumshirn]

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

Re: fix a few zoned append issues v2 (now with Ccs)

[Christoph Hellwig]

On Mon, Nov 04, 2024 at 07:26:28AM +0100, Christoph Hellwig wrote:
> Hi Jens, hi Damien, hi btrfs maintainers,

How should we proceed with this?  Should Jens just pick up the block
bits and the btrfs maintainers pick up the btrfs once they are
ready and the block bits made it upstream?

Re: fix a few zoned append issues v2 (now with Ccs)

[David Sterba]

On Fri, Nov 08, 2024 at 03:48:57PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 04, 2024 at 07:26:28AM +0100, Christoph Hellwig wrote:
> > Hi Jens, hi Damien, hi btrfs maintainers,
> 
> How should we proceed with this?  Should Jens just pick up the block
> bits and the btrfs maintainers pick up the btrfs once they are
> ready and the block bits made it upstream?

Sounds ok for me from the btrfs side.

Re: fix a few zoned append issues v2 (now with Ccs)

[David Sterba]

On Fri, Nov 08, 2024 at 03:48:57PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 04, 2024 at 07:26:28AM +0100, Christoph Hellwig wrote:
> > Hi Jens, hi Damien, hi btrfs maintainers,
> 
> How should we proceed with this?  Should Jens just pick up the block
> bits and the btrfs maintainers pick up the btrfs once they are
> ready and the block bits made it upstream?

The block layer patches are now in master, I'm adding the remaining
patches to btrfs for-next.

Re: fix a few zoned append issues v2 (now with Ccs)

[Christoph Hellwig]

On Tue, Dec 03, 2024 at 06:01:20PM +0100, David Sterba wrote:
> On Fri, Nov 08, 2024 at 03:48:57PM +0100, Christoph Hellwig wrote:
> > On Mon, Nov 04, 2024 at 07:26:28AM +0100, Christoph Hellwig wrote:
> > > Hi Jens, hi Damien, hi btrfs maintainers,
> > 
> > How should we proceed with this?  Should Jens just pick up the block
> > bits and the btrfs maintainers pick up the btrfs once they are
> > ready and the block bits made it upstream?
> 
> The block layer patches are now in master, I'm adding the remaining
> patches to btrfs for-next.

Thanks.  I wanted to resend them, but my baseline testing still showed
crashes in generic/475 so I was looking into an expunge config to
check if there are no regressions first (there really shouldn't, but
I wanted to double check).

Re: fix a few zoned append issues v2 (now with Ccs)

[Qu Wenruo]

在 2024/12/4 10:47, Christoph Hellwig 写道:
> On Tue, Dec 03, 2024 at 06:01:20PM +0100, David Sterba wrote:
>> On Fri, Nov 08, 2024 at 03:48:57PM +0100, Christoph Hellwig wrote:
>>> On Mon, Nov 04, 2024 at 07:26:28AM +0100, Christoph Hellwig wrote:
>>>> Hi Jens, hi Damien, hi btrfs maintainers,
>>>
>>> How should we proceed with this?  Should Jens just pick up the block
>>> bits and the btrfs maintainers pick up the btrfs once they are
>>> ready and the block bits made it upstream?
>>
>> The block layer patches are now in master, I'm adding the remaining
>> patches to btrfs for-next.
>
> Thanks.  I wanted to resend them, but my baseline testing still showed
> crashes in generic/475 so I was looking into an expunge config to
> check if there are no regressions first (there really shouldn't, but
> I wanted to double check).
>

Talking about generic/475 crash, it looks like there is something wrong
inside btrfs' space reservation code, which causes run_delalloc_range()
to fail with ENOSPC (which should not happen).

If my memory works correctly, at least 3 release ago we didn't hit such
ENOSPC during run_delalloc_range().


I'm working on fixing the involved error handling, there are some
patches to reduce involved crashes:

For ordered extent double freeing:
https://lore.kernel.org/linux-btrfs/cover.1732695237.git.wqu@suse.com/

For ENOSPC error handling in run_delalloc_range():
https://lore.kernel.org/linux-btrfs/0b4675971b718709497ca35c0d69e06db0c69d58.1732867087.git.wqu@suse.com/
https://lore.kernel.org/linux-btrfs/3e5d5665ef36ee43e310be321073210785b89adc.1733273653.git.wqu@suse.com/

Which can help you to continue, but at least on my aarch64 VMs with 64K
page size and 4K block size, I can still hit random failures related to
unfinished iput or run-away locked pages.


Unfortunately I'm not educated enough to pin down the
run_delalloc_range() ENOSPC errors, but only use this chance to fix the
error handling...

Thanks,
Qu