Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1337866 > unrolled thread

[PATCH v1 0/4] block: fix bio_will_gap()

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 0/4] block: fix bio_will_gap() Ming Lei <ming.lei@canonical.com> - 2016-02-19 04:30 +0100
    [PATCH v1 1/4] block: bio: introduce helpers to get the 1st and last bvec Ming Lei <ming.lei@canonical.com> - 2016-02-19 04:30 +0100
      Re: [PATCH v1 1/4] block: bio: introduce helpers to get the 1st and  last bvec Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-21 19:20 +0100
      Re: [PATCH v1 1/4] block: bio: introduce helpers to get the 1st and  last bvec Christoph Hellwig <hch@infradead.org> - 2016-02-22 10:00 +0100
    Re: [PATCH v1 0/4] block: fix bio_will_gap() Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-21 19:40 +0100

#1337866 — [PATCH v1 0/4] block: fix bio_will_gap()

Hi Guys,

The bio passed to bio_will_gap() may be fast cloned from upper
layer(dm, md, bcache, fs, ...), or from bio splitting in block
core. Unfortunately bio_will_gap() just figures out the last
bvec via 'bi_io_vec[prev->bi_vcnt - 1]' directly, and this way
is obviously wrong in case of fast-cloned bio.

It is observed that lots of BIOs are still merged even if
the virt boundary limit is violated by the merge, and the issue
was reported from Sagi Grimberg.
    
This patch introduces two helpers for getting the first and last
bvec of one bio and applys them to fix the issue. Sagi tested
the last patchset and confirmed the fix.

V1:
	- get bvec directly for non-cloned bio
	- implement bio_get_last_bvec() with single bio_advance_iter(),
	and avoid to use bio_for_each_segment() which looks a bit inefficient
	- avoid to double check queue_virt_boundary() in bio_will_gap()


 block/blk-merge.c      |  8 ++------
 include/linux/bio.h    | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h | 21 ++++++++++++++++-----
 3 files changed, 59 insertions(+), 11 deletions(-)

Thanks,
Ming

[toc] | [next] | [standalone]

#1337867 — [PATCH v1 1/4] block: bio: introduce helpers to get the 1st and last bvec

The bio passed to bio_will_gap() may be fast cloned from upper
layer(dm, md, bcache, fs, ...), or from bio splitting in block
core.

Unfortunately bio_will_gap() just figures out the last bvec via
'bi_io_vec[prev->bi_vcnt - 1]' directly, and this way is obviously
wrong.

This patch introduces two helpers for getting the first and last
bvec of one bio for fixing the issue.

Cc: stable@vger.kernel.org
Reported-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 include/linux/bio.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index 5349e68..3ce2e45 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -310,6 +310,47 @@ static inline void bio_clear_flag(struct bio *bio, unsigned int bit)
 	bio->bi_flags &= ~(1U << bit);
 }
 
+static inline void bio_get_first_bvec(struct bio *bio, struct bio_vec *bv)
+{
+	*bv = bio_iovec(bio);
+}
+
+/*
+ * bio_get_last_bvec() is introduced to get the last bvec of one
+ * bio for bio_will_gap().
+ */
+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;
+	}
+
+	bio_advance_iter(bio, &iter, iter.bi_size);
+
+	if (!iter.bi_bvec_done)
+		idx = iter.bi_idx - 1;
+	else	/* in the middle of bvec */
+		idx = iter.bi_idx;
+
+	*bv = bio->bi_io_vec[idx];
+
+	/*
+	 * iter.bi_bvec_done records actual length of the last bvec
+	 * if this bio ends in the middle of one io vector
+	 */
+	if (iter.bi_bvec_done)
+		bv->bv_len = iter.bi_bvec_done;
+}
+
 enum bip_flags {
 	BIP_BLOCK_INTEGRITY	= 1 << 0, /* block layer owns integrity data */
 	BIP_MAPPED_INTEGRITY	= 1 << 1, /* ref tag has been remapped */
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1338925 — Re: [PATCH v1 1/4] block: bio: introduce helpers to get the 1st and last bvec

Looks fine,

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

[toc] | [prev] | [next] | [standalone]

#1339229 — Re: [PATCH v1 1/4] block: bio: introduce helpers to get the 1st and last bvec

> +/*
> + * bio_get_last_bvec() is introduced to get the last bvec of one
> + * bio for bio_will_gap().
> + */

I don't think this comment adds any value.

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1338940

> Hi Guys,
>
> The bio passed to bio_will_gap() may be fast cloned from upper
> layer(dm, md, bcache, fs, ...), or from bio splitting in block
> core. Unfortunately bio_will_gap() just figures out the last
> bvec via 'bi_io_vec[prev->bi_vcnt - 1]' directly, and this way
> is obviously wrong in case of fast-cloned bio.
>
> It is observed that lots of BIOs are still merged even if
> the virt boundary limit is violated by the merge, and the issue
> was reported from Sagi Grimberg.
>
> This patch introduces two helpers for getting the first and last
> bvec of one bio and applys them to fix the issue. Sagi tested
> the last patchset and confirmed the fix.
>
> V1:
> 	- get bvec directly for non-cloned bio
> 	- implement bio_get_last_bvec() with single bio_advance_iter(),
> 	and avoid to use bio_for_each_segment() which looks a bit inefficient
> 	- avoid to double check queue_virt_boundary() in bio_will_gap()

Thanks Ming,

Jens, can this make the next 4.5-rc since this regression was detected
in 4.5?

Thanks,
Sagi.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web