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

[David Sterba <dsterba@suse.cz>]

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
* btrfs_map_bio calling btrfs_bio_clone

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

--- 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)
---