Re: [PATCH v3 1/4] btrfs: remove the dead branch in btrfs_io_needs_validation()

[Qu Wenruo <wqu@suse.com>]

On 2021/5/11 上午4:14, David Sterba wrote:
> On Mon, May 03, 2021 at 10:08:53AM +0800, Qu Wenruo wrote:
>>   	/*
>> -	 * We need to validate each sector individually if the failed I/O was
>> -	 * for multiple sectors.
>> -	 *
>> -	 * There are a few possible bios that can end up here:
>> -	 * 1. A buffered read bio, which is not cloned.
>> -	 * 2. A direct I/O read bio, which is cloned.
>> -	 * 3. A (buffered or direct) repair bio, which is not cloned.
>> -	 *
>> -	 * For cloned bios (case 2), we can get the size from
>> -	 * btrfs_io_bio->iter; for non-cloned bios (cases 1 and 3), we can get
>> -	 * it from the bvecs.
>> +	 * We're ensured we won't get cloned bio in end_bio_extent_readpage(),
>> +	 * thus we can get the length from the bvecs.
>>   	 */
>> -	if (bio_flagged(bio, BIO_CLONED)) {
>> -		if (btrfs_io_bio(bio)->iter.bi_size > blocksize)
>> +	bio_for_each_bvec_all(bvec, bio, i) {
>> +		len += bvec->bv_len;
>> +		if (len > blocksize)
>>   			return true;
> 
> I've looked if the bio cloning is used at all so we could potentially
> get rid of all the BIO_CLONED assertions. There are still two cases:
> 
> * btrfs_submit_direct calling btrfs_bio_clone_partial

This is what I missed.

In fact for DIO read repair we can still hit a cloned bio.

But in that case, we still don't need any validation since the DIO read 
repair is ensured to only submit sector sized repair.
So the patchset is still fine, but will break the bisect.

> * btrfs_map_bio calling btrfs_bio_clone

This is not a problem, as the generated bio are for real device, not for 
the inode pages, and read repair only happens for inode pages, we're 
completely fine.

> 
> So in this case I'd rather add an assertion before
> bio_for_each_bvec_all, as this fits the usecase "this never happens".
> The original assertions were added everywhere once the bio iteration
> behaviour changed without much notice, so we need to be cautious.
> 
> Applied with the following fixup

Then bisect will be broken.

If one is testing just this patch, DIO read repair will trigger the 
ASSERT().

Is it possible to discard this patch and completely rely on the last 
patch to remove btrfs_io_needs_validation()?

Thanks,
Qu

> 
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2616,6 +2616,7 @@ static bool btrfs_io_needs_validation(struct inode *inode, struct bio *bio)
>           * We're ensured we won't get cloned bio in end_bio_extent_readpage(),
>           * thus we can get the length from the bvecs.
>           */
> +       ASSERT(!bio_flagged(bio, BIO_CLONED));
>          bio_for_each_bvec_all(bvec, bio, i) {
>                  len += bvec->bv_len;
>                  if (len > blocksize)
> ---
>