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

[David Sterba <dsterba@suse.cz>]

On Tue, May 11, 2021 at 07:56:42AM +0800, Qu Wenruo wrote:
> >> -	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()?

Yeah, we want to keep it bisectable.