Re: [PATCH] btrfs: don't merge pages into bio if their page offset is not continuous

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

On 2022/8/13 15:39, Christoph Hellwig wrote:
>> +	if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE) {
>>   		contig = bio->bi_iter.bi_sector == sector;
>> -	else
>> -		contig = bio_end_sector(bio) == sector;
>> +	} else {
>
> I don't tink we can lose the bio_end_sector check here.
>
> Also if you touch this, invrting the check so that it is
>
> 	if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE) {
> 		/* main version here */
> 	} else {
> 		/* compressed special case */
> 	}
>
> might make it a bit readable.

Indeed.

>
>> +		struct bio_vec *bvec;
>> +
>> +		/* Empty bio, we can always add page into it. */
>> +		if (bio->bi_iter.bi_size == 0) {
>
> This is also true for the compressed case, isn't it?

That is already implied in the "contig = bio->bi_iter.bi_sector ==
sector;" check.

Remember for compressed IO, we always set the the disk_bytenr to
whatever the bytenr of the extent start.

>
> So maybe:
>
> 	if (bio->bi_iter.bi_size == 0) {
> 		contig = true;
> 	} else if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE) {
> 		struct bio_vec *bvec = bio_last_bvec_all(bio);
>
> 		/*
> 		 * The contig check requires the following conditions to be met:
> 		 * 1) The pages are belonging to the same inode
> 		 * 2) The range has adjacent logical bytenr
> 		 * 3) The range has adjacent file offset (NEW)
> 		 *    This is required for the usage of btrfs_bio->file_offset.
> 		 */
> 		contig = bio_end_sector(bio) == sector &&
> 			    page_offset(bvec->bv_page) + bvec->bv_offset +
> 			    bvec->bv_len == page_offset(page) + pg_offset);
> 	} else {
> 		contig = bio->bi_iter.bi_sector == sector;
> 	}

This indeed looks better.

>
> (we don't need the mapping check, as all the submit_extent_page
> always loops over the same mapping)

Yep, that mapping check is already implied by the call chain.

BTW, I'll also update the commit message to mention some future
cleanups, as now a btrfs_bio is completely a continous range, thus
things like endio_readpage_release_extent() related infrastructure can
also be removed.

Thanks,
Qu