Re: e827091cb1 "block: merge: get the 1st and last bvec via helpers" broken

From: Ming Lei
Date: Sat Mar 12 2016 - 09:26:37 EST


On Sat, 12 Mar 2016 05:02:56 -0900
Kent Overstreet <kent.overstreet@xxxxxxxxx> wrote:

> Here's the output of the patch below:
>
> generic/036 11s ...run fstests generic/036 at 2016-03-12 13:58:21
> end 4096 0 ffffea0001d611c0 end2 1024 0 ffffea0001d611c0
> len 1024 offset 0 page ffffea0001d611c0
> KGDB: Waiting for remote debugger
>
> Your code gives a biovec with bv_len of 4096, the old code gives a biovec with
> bv_len of 1024 (and then we dump every biovec, we see that the bio had only a
> single biovec that did indeed have bv_len == 1024).

I guess we shouldn't have optimized for the case of non-cloned bio, could you
try the following patch?

--
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1e7248f..4abc129 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -267,11 +267,6 @@ static inline void bio_get_last_bvec(struct bio *bio, struct bio_vec *bv)
struct bvec_iter iter = bio->bi_iter;
int idx;

- if (!bio_flagged(bio, BIO_CLONED)) {
- *bv = bio->bi_io_vec[bio->bi_vcnt - 1];
- return;
- }
-
if (unlikely(!bio_multiple_segments(bio))) {
*bv = bio_iovec(bio);
return;

Thanks,
Ming

>
> bio_get_last_bvec() is broken, please revert your patch for v4.5.
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 261353166d..231aec4fa3 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -318,6 +318,34 @@ static int blk_phys_contig_segment(struct request_queue *q, struct bio *bio,
> bio_get_last_bvec(bio, &end_bv);
> bio_get_first_bvec(nxt, &nxt_bv);
>
> + {
> + struct bvec_iter iter;
> + struct bio_vec end2, bv;
> +
> + bio_for_each_segment(end2, bio, iter)
> + if (end2.bv_len == iter.bi_size)
> + break;
> +
> + if (end_bv.bv_len != end2.bv_len ||
> + end_bv.bv_offset != end2.bv_offset ||
> + end_bv.bv_page != end2.bv_page) {
> + printk(KERN_ERR "end %u %u %p end2 %u %u %p\n",
> + end_bv.bv_len,
> + end_bv.bv_offset,
> + end_bv.bv_page,
> + end2.bv_len,
> + end2.bv_offset,
> + end2.bv_page);
> +
> + bio_for_each_segment(bv, bio, iter)
> + printk(KERN_ERR "len %u offset %u page %p\n",
> + bv.bv_len,
> + bv.bv_offset,
> + bv.bv_page);
> + BUG();
> + }
> + }
> +
> if (!BIOVEC_PHYS_MERGEABLE(&end_bv, &nxt_bv))
> return 0;
>