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@xxxxxx>
>>> Cc: Keith Busch <kbusch@xxxxxxxxxx>
>>> Cc: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
>>> Cc: Josef Bacik <josef@xxxxxxxxxxxxxx>
>>> Cc: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
>>> Fixes: d5e4377d5051 ("btrfs: split zone append bios in btrfs_submit_bio")
>>> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
>>
>> 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
