Re: [PATCH 1/4] block: rework bio splitting

[Damien Le Moal <dlemoal@kernel.org>]

On 8/27/24 07:26, Damien Le Moal wrote:
> On 8/27/24 02:37, Christoph Hellwig wrote:
>> The current setup with bio_may_exceed_limit and __bio_split_to_limits
>> is a bit of a mess.
>>
>> Change it so that __bio_split_to_limits does all the work and is just
>> a variant of bio_split_to_limits that returns nr_segs.  This is done
>> by inlining it and instead have the various bio_split_* helpers directly
>> submit the potentially split bios.
>>
>> To support btrfs, the rw version has a lower level helper split out
>> that just returns the offset to split.  This turns out to nicely clean
>> up the btrfs flow as well.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  block/blk-merge.c   | 146 +++++++++++++++++---------------------------
>>  block/blk-mq.c      |  11 ++--
>>  block/blk.h         |  63 +++++++++++++------
>>  fs/btrfs/bio.c      |  30 +++++----
>>  include/linux/bio.h |   4 +-
>>  5 files changed, 125 insertions(+), 129 deletions(-)
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index de5281bcadc538..c7222c4685e060 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -105,9 +105,33 @@ static unsigned int bio_allowed_max_sectors(const struct queue_limits *lim)
>>  	return round_down(UINT_MAX, lim->logical_block_size) >> SECTOR_SHIFT;
>>  }
>>  
>> -static struct bio *bio_split_discard(struct bio *bio,
>> -				     const struct queue_limits *lim,
>> -				     unsigned *nsegs, struct bio_set *bs)
>> +static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
> 
> Why not "unsigned int" for split_sectors ? That would avoid the need for the
> first "if" of the function. Note that bio_split() also takes an int for the
> sector count and also checks for < 0 count with a BUG_ON(). We can clean that up
> too. BIOs sector count is unsigned int...
> 
>> +{
>> +	if (unlikely(split_sectors < 0)) {
>> +		bio->bi_status = errno_to_blk_status(split_sectors);
>> +		bio_endio(bio);
>> +		return NULL;
>> +	}
>> +
>> +	if (split_sectors) {
> 
> May be the simple case should come first ? E.g.:
> 
> 	if (!split_sectors)
> 		return bio;
> 
> But shouldn't this check be:
> 
> 	if (split_sectors >= bio_sectors(bio))
> 		return bio;

Please ignore this one. The passed sector count is a limit, which can be 0, so
checking  "if (!split_sectors)" is correct.

-- 
Damien Le Moal
Western Digital Research