[PATCH] block: Support splitting REQ_OP_ZONE_APPEND bios

[PATCH] block: Support splitting REQ_OP_ZONE_APPEND bios

[Bart Van Assche]

Make it easier for filesystems to submit zone append bios that exceed
the block device limits by adding support for REQ_OP_ZONE_APPEND in
bio_split(). See also commit 0512a75b98f8 ("block: Introduce
REQ_OP_ZONE_APPEND").

This patch is a bug fix for commit d5e4377d5051 because that commit
introduces a call to bio_split() for zone append bios without adding
support for splitting REQ_OP_ZONE_APPEND bios in bio_split().

Untested.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/bio.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index fc98c1c723ca..8a4805565638 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1631,10 +1631,6 @@ struct bio *bio_split(struct bio *bio, int sectors,
 	BUG_ON(sectors <= 0);
 	BUG_ON(sectors >= bio_sectors(bio));
 
-	/* Zone append commands cannot be split */
-	if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND))
-		return NULL;
-
 	split = bio_alloc_clone(bio->bi_bdev, bio, gfp, bs);
 	if (!split)
 		return NULL;
@@ -1644,7 +1640,8 @@ struct bio *bio_split(struct bio *bio, int sectors,
 	if (bio_integrity(split))
 		bio_integrity_trim(split);
 
-	bio_advance(bio, split->bi_iter.bi_size);
+	if (bio_op(bio) != REQ_OP_ZONE_APPEND)
+		bio_advance(bio, split->bi_iter.bi_size);
 
 	if (bio_flagged(bio, BIO_TRACE_COMPLETION))
 		bio_set_flag(split, BIO_TRACE_COMPLETION);

Re: [PATCH] block: Support splitting REQ_OP_ZONE_APPEND bios

[Damien Le Moal]

On 3/18/23 04:50, Bart Van Assche wrote:
> Make it easier for filesystems to submit zone append bios that exceed
> the block device limits by adding support for REQ_OP_ZONE_APPEND in
> bio_split(). See also commit 0512a75b98f8 ("block: Introduce
> REQ_OP_ZONE_APPEND").
> 
> This patch is a bug fix for commit d5e4377d5051 because that commit
> introduces a call to bio_split() for zone append bios without adding
> support for splitting REQ_OP_ZONE_APPEND bios in bio_split().
> 
> Untested.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Nack. This will break zonefs.
zonefs uses zone append commands for sync writes. If zonefs does not have
guarantees that a single write is processed with a single command, the user data
can get corrupted because of the possible reordering of zone append commands.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: Support splitting REQ_OP_ZONE_APPEND bios

[Bart Van Assche]

On 3/17/23 15:39, Damien Le Moal wrote:
> On 3/18/23 04:50, Bart Van Assche wrote:
>> Make it easier for filesystems to submit zone append bios that exceed
>> the block device limits by adding support for REQ_OP_ZONE_APPEND in
>> bio_split(). See also commit 0512a75b98f8 ("block: Introduce
>> REQ_OP_ZONE_APPEND").
>>
>> This patch is a bug fix for commit d5e4377d5051 because that commit
>> introduces a call to bio_split() for zone append bios without adding
>> support for splitting REQ_OP_ZONE_APPEND bios in bio_split().
>>
>> Untested.
>>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Keith Busch <kbusch@kernel.org>
>> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Cc: Josef Bacik <josef@toxicpanda.com>
>> Cc: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 
> Nack. This will break zonefs.
> zonefs uses zone append commands for sync writes. If zonefs does not have
> guarantees that a single write is processed with a single command, the user data
> can get corrupted because of the possible reordering of zone append commands.

Hi Damien,

It is not clear to me how this would break zonefs? Is my understanding 
correct that the size of bios built by zonefs is such that bio_split() 
won't split these?

Thanks,

Bart.

Re: [PATCH] block: Support splitting REQ_OP_ZONE_APPEND bios

[Damien Le Moal]

On 3/18/23 09:09, Bart Van Assche wrote:
> On 3/17/23 15:39, Damien Le Moal wrote:
>> On 3/18/23 04:50, Bart Van Assche wrote:
>>> Make it easier for filesystems to submit zone append bios that exceed
>>> the block device limits by adding support for REQ_OP_ZONE_APPEND in
>>> bio_split(). See also commit 0512a75b98f8 ("block: Introduce
>>> REQ_OP_ZONE_APPEND").
>>>
>>> This patch is a bug fix for commit d5e4377d5051 because that commit
>>> introduces a call to bio_split() for zone append bios without adding
>>> support for splitting REQ_OP_ZONE_APPEND bios in bio_split().
>>>
>>> Untested.
>>>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Keith Busch <kbusch@kernel.org>
>>> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> Cc: Josef Bacik <josef@toxicpanda.com>
>>> Cc: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>> Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>
>> Nack. This will break zonefs.
>> zonefs uses zone append commands for sync writes. If zonefs does not have
>> guarantees that a single write is processed with a single command, the user data
>> can get corrupted because of the possible reordering of zone append commands.
> 
> Hi Damien,
> 
> It is not clear to me how this would break zonefs? Is my understanding 
> correct that the size of bios built by zonefs is such that bio_split() 
> won't split these?

It does, but your patch removes the guarantee that the split actually never
happens, and thus that we cannot see silent data corruptions due to append
commands being fragmented and the fragments being reordered. I do not like that.

I am not sure what is going on with the patch d5e4377d5051 you mention, I have
not followed that. For btrfs, since file data mapping for zone append commands
is updated on command completion, reordering does not matter so splitting is
fine I think. zonefs has static direct zone sector to file offset mapping which
cannot support reordering of zone append fragments.

Given that explicit split in commit d5e4377d5051, I can see that a fix is
needed, but I do not like the solution you have.

> 
> Thanks,
> 
> Bart.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: Support splitting REQ_OP_ZONE_APPEND bios

[Christoph Hellwig]

On Fri, Mar 17, 2023 at 12:50:35PM -0700, Bart Van Assche wrote:
> Make it easier for filesystems to submit zone append bios that exceed
> the block device limits by adding support for REQ_OP_ZONE_APPEND in
> bio_split(). See also commit 0512a75b98f8 ("block: Introduce
> REQ_OP_ZONE_APPEND").

You can't do that.  ZONE_APPEND reports the written sector in bi_sector,
so it can't be split.

> This patch is a bug fix for commit d5e4377d5051 because that commit
> introduces a call to bio_split() for zone append bios without adding
> support for splitting REQ_OP_ZONE_APPEND bios in bio_split().

That commit never splits ZONE_APPEND bios, and it pre-splits bios
about to become ZONE_APPEND bios specifically using bio_split_rw
instead of bio_split.

Re: [PATCH] block: Support splitting REQ_OP_ZONE_APPEND bios

[Bart Van Assche]

On 3/17/23 19:21, Damien Le Moal wrote:
> On 3/18/23 09:09, Bart Van Assche wrote:
>> It is not clear to me how this would break zonefs? Is my understanding
>> correct that the size of bios built by zonefs is such that bio_split()
>> won't split these?
> 
> It does, but your patch removes the guarantee that the split actually never
> happens, and thus that we cannot see silent data corruptions due to append
> commands being fragmented and the fragments being reordered. I do not like that.

That's fair. Let's drop this patch.

Thanks,

Bart.