Re: [PATCH] block/bio.c: update bi_iter.bi_size before recounting segments
Hi, I don't see this patch in linux-next yet nor a review. Jens, Andrew; did you notice it? On 05/29/2014 09:59 AM, Ming Lei wrote: > The patch of "bio: modify __bio_add_page() to accept pages that > don't start a new segment" changes the way for adding one page > to bio: > > - previously by adding page after checking successfully > - now by trying to add page and recover if it fails > > Unfortunately the patch forgets to update bio->bi_iter.bi_size > before trying to add page, then the last vector for holding > the added page may not be covered if recouning segments is needed, > so bio->bi_phys_segments may become not consistent with the > actual bio page buffers after the page is added successfully > to the bio(after bi_iter.bi_size is added by 'len') > > Suppose the page in the last vector can't be merged to bio, tragedy > will happen when __bio_add_page() is called to add another page: > > - blk_recount_segments() is called and the actual segments get > figured out correctly > > - the actual segments may become queue_max_segments(q) plus one > in failure path > > - driver will find the segment count is too big to handle. > > The patch fixes the virtio-blk oops bug reported from Jet Chen in > below link: > > http://marc.info/?l=linux-kernel&m=140113053817095&w=2 > > Cc: Jens Axboe > Cc: Maurizio Lombardi > Cc: Dongsu Park > Cc: Christoph Hellwig > Cc: Kent Overstreet > Cc: Andrew Morton > Reported-by: Jet Chen > Tested-by: Jet Chen > Signed-off-by: Ming Lei > --- > Andrew, could you put the patch in your -mm tree > because the previous two patches were routed from > your tree? > > block/bio.c |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/bio.c b/block/bio.c > index 0443694..f9bae56 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -744,6 +744,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > } > } > > + bio->bi_iter.bi_size += len; > goto done; > } > } > @@ -761,6 +762,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_offset = offset; > bio->bi_vcnt++; > bio->bi_phys_segments++; > + bio->bi_iter.bi_size += len; > > /* >* Perform a recount if the number of segments is greater > @@ -802,7 +804,6 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bio->bi_flags &= ~(1 << BIO_SEG_VALID); > > done: > - bio->bi_iter.bi_size += len; > return len; > > failed: > @@ -810,6 +811,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_len = 0; > bvec->bv_offset = 0; > bio->bi_vcnt--; > + bio->bi_iter.bi_size -= len; > blk_recount_segments(q, bio); > return 0; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block/bio.c: update bi_iter.bi_size before recounting segments
Hi Ming, On 29.05.2014 15:59, Ming Lei wrote: > The patch of "bio: modify __bio_add_page() to accept pages that > don't start a new segment" changes the way for adding one page > to bio: > > - previously by adding page after checking successfully > - now by trying to add page and recover if it fails > > Unfortunately the patch forgets to update bio->bi_iter.bi_size > before trying to add page, then the last vector for holding > the added page may not be covered if recouning segments is needed, > so bio->bi_phys_segments may become not consistent with the > actual bio page buffers after the page is added successfully > to the bio(after bi_iter.bi_size is added by 'len') > > Suppose the page in the last vector can't be merged to bio, tragedy > will happen when __bio_add_page() is called to add another page: > > - blk_recount_segments() is called and the actual segments get > figured out correctly > > - the actual segments may become queue_max_segments(q) plus one > in failure path > > - driver will find the segment count is too big to handle. > > The patch fixes the virtio-blk oops bug reported from Jet Chen in > below link: > > http://marc.info/?l=linux-kernel&m=140113053817095&w=2 A great patch description, thanks! :-) You can add: Tested-by: Dongsu Park > Cc: Jens Axboe > Cc: Maurizio Lombardi > Cc: Dongsu Park > Cc: Christoph Hellwig > Cc: Kent Overstreet > Cc: Andrew Morton > Reported-by: Jet Chen > Tested-by: Jet Chen > Signed-off-by: Ming Lei > --- > Andrew, could you put the patch in your -mm tree > because the previous two patches were routed from > your tree? > > block/bio.c |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/bio.c b/block/bio.c > index 0443694..f9bae56 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -744,6 +744,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > } > } > > + bio->bi_iter.bi_size += len; > goto done; > } > } > @@ -761,6 +762,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_offset = offset; > bio->bi_vcnt++; > bio->bi_phys_segments++; > + bio->bi_iter.bi_size += len; > > /* >* Perform a recount if the number of segments is greater > @@ -802,7 +804,6 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bio->bi_flags &= ~(1 << BIO_SEG_VALID); > > done: > - bio->bi_iter.bi_size += len; > return len; > > failed: > @@ -810,6 +811,7 @@ static int __bio_add_page(struct request_queue *q, struct > bio *bio, struct page > bvec->bv_len = 0; > bvec->bv_offset = 0; > bio->bi_vcnt--; > + bio->bi_iter.bi_size -= len; > blk_recount_segments(q, bio); > return 0; > } > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] block/bio.c: update bi_iter.bi_size before recounting segments
The patch of "bio: modify __bio_add_page() to accept pages that don't start a new segment" changes the way for adding one page to bio: - previously by adding page after checking successfully - now by trying to add page and recover if it fails Unfortunately the patch forgets to update bio->bi_iter.bi_size before trying to add page, then the last vector for holding the added page may not be covered if recouning segments is needed, so bio->bi_phys_segments may become not consistent with the actual bio page buffers after the page is added successfully to the bio(after bi_iter.bi_size is added by 'len') Suppose the page in the last vector can't be merged to bio, tragedy will happen when __bio_add_page() is called to add another page: - blk_recount_segments() is called and the actual segments get figured out correctly - the actual segments may become queue_max_segments(q) plus one in failure path - driver will find the segment count is too big to handle. The patch fixes the virtio-blk oops bug reported from Jet Chen in below link: http://marc.info/?l=linux-kernel&m=140113053817095&w=2 Cc: Jens Axboe Cc: Maurizio Lombardi Cc: Dongsu Park Cc: Christoph Hellwig Cc: Kent Overstreet Cc: Andrew Morton Reported-by: Jet Chen Tested-by: Jet Chen Signed-off-by: Ming Lei --- Andrew, could you put the patch in your -mm tree because the previous two patches were routed from your tree? block/bio.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/bio.c b/block/bio.c index 0443694..f9bae56 100644 --- a/block/bio.c +++ b/block/bio.c @@ -744,6 +744,7 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page } } + bio->bi_iter.bi_size += len; goto done; } } @@ -761,6 +762,7 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bvec->bv_offset = offset; bio->bi_vcnt++; bio->bi_phys_segments++; + bio->bi_iter.bi_size += len; /* * Perform a recount if the number of segments is greater @@ -802,7 +804,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bio->bi_flags &= ~(1 << BIO_SEG_VALID); done: - bio->bi_iter.bi_size += len; return len; failed: @@ -810,6 +811,7 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bvec->bv_len = 0; bvec->bv_offset = 0; bio->bi_vcnt--; + bio->bi_iter.bi_size -= len; blk_recount_segments(q, bio); return 0; } -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
