On Fri, Jul 14, 2017 at 10:04:34AM +0100, Filipe Manana wrote: > On Thu, Jul 13, 2017 at 11:38 PM, Liu Bo <bo.li....@oracle.com> wrote: > > On Thu, Jul 13, 2017 at 09:36:19AM -0700, Liu Bo wrote: > >> On Thu, Jul 13, 2017 at 03:09:54PM +0100, fdman...@kernel.org wrote: > >> > @@ -1423,11 +1430,14 @@ static int fail_bio_stripe(struct btrfs_raid_bio > >> > *rbio, > >> > */ > >> > static void set_bio_pages_uptodate(struct bio *bio) > >> > { > >> > - struct bio_vec *bvec; > >> > - int i; > >> > + struct bio_vec bvec; > >> > + struct bvec_iter iter; > >> > + > >> > + if (bio_flagged(bio, BIO_CLONED)) > >> > + bio->bi_iter = btrfs_io_bio(bio)->iter; > >> > > > > > It is not necessary to update set_bio_pages_uptodate() because the bio > > here is for reading pages in RMW and no way it could be a cloned one. > > >From my testing I've never seen this function getting cloned bios as > well. But I'm afraid we don't have full test coverage and it scares me > to have code around that can cause silent corruptions or crashes due > to such subtle details (bi_vcnt can't be used for cloned bios and it's > not obvious which APIs from the block layer make use of it without > reading their code). Plus a quick look I had at btrfs_map_bio() > suggested it can clone bios used for read operations through > btrfs_submit_bio_hook() for example. So that's why I changed that > function too to be able to deal both with cloned and non-cloned bios.
I agree with that approach. My first reaction was to switch to bio_for_each_segment everywhere, but this would increase stack consumption by 32 bytes, so with the asserts in place I hope we'd catch potential cloned/bi_vcnt violations. As you point out, the API is not clear on that. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html