Re: [PATCH v4 04/13] btrfs: introduce a new helper to submit write bio for scrub

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/3/27 11:44, Christoph Hellwig wrote:
> On Sun, Mar 26, 2023 at 07:06:33PM +0800, Qu Wenruo wrote:
>> +	/* Caller should ensure the @bbio doesn't cross stripe boundary. */
>> +	ASSERT(map_length >= length);
>> +	if (btrfs_op(&bbio->bio) == BTRFS_MAP_WRITE && btrfs_is_zoned(fs_info)) {
>> +		bbio->bio.bi_opf &= ~REQ_OP_WRITE;
>> +		bbio->bio.bi_opf |= REQ_OP_ZONE_APPEND;
>> +	}
> 
> Not crossing the stripe boundary is not enough, for zone append it
> also must not cross the zone append limit, which (at least in theory)
> can be arbitrarily small.

Do you mean that we can have some real zone append limit which is even 
smaller than 64K?

If so, then the write helper can be more complex than I thought...

Thanks,
Qu

> 
>> +	if (bioc->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
>> +		int data_stripes = (bioc->map_type & BTRFS_BLOCK_GROUP_RAID5) ?
>> +				    bioc->num_stripes - 1 : bioc->num_stripes - 2;
> 
> This calculation feels like it should be in a well document simple
> helper.
> 
>> +		smap.physical = bioc->stripes[i].physical +
>> +				((logical - bioc->full_stripe_logical) &
>> +				 BTRFS_STRIPE_LEN_MASK);
> 
> This calculation feels like another candidate for a self contained
> a well documented helper.
> 
> 
>> +		goto submit;
>> +	}
>> +	ASSERT(mirror_num <= bioc->num_stripes);
>> +	smap.dev = bioc->stripes[mirror_num - 1].dev;
>> +	smap.physical = bioc->stripes[mirror_num - 1].physical;
>> +submit:
> 
> Why no else instead of the goto here?  That would read a lot easier.