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.liu@xxxxxxxxxx> wrote:
> > On Thu, Jul 13, 2017 at 09:36:19AM -0700, Liu Bo wrote:
> >> On Thu, Jul 13, 2017 at 03:09:54PM +0100, fdmanana@xxxxxxxxxx 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
