Re: [PATCH V4 13/45] block: blk-merge: try to make front segments in full size
From: Ming Lei
Date: Mon Jan 08 2018 - 21:35:05 EST
On Tue, Jan 09, 2018 at 12:09:27AM +0300, Dmitry Osipenko wrote:
> On 18.12.2017 15:22, Ming Lei wrote:
> > When merging one bvec into segment, if the bvec is too big
> > to merge, current policy is to move the whole bvec into another
> > new segment.
> >
> > This patchset changes the policy into trying to maximize size of
> > front segments, that means in above situation, part of bvec
> > is merged into current segment, and the remainder is put
> > into next segment.
> >
> > This patch prepares for support multipage bvec because
> > it can be quite common to see this case and we should try
> > to make front segments in full size.
> >
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > block/blk-merge.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 49 insertions(+), 5 deletions(-)
> >
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index a476337a8ff4..42ceb89bc566 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -109,6 +109,7 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
> > bool do_split = true;
> > struct bio *new = NULL;
> > const unsigned max_sectors = get_max_io_size(q, bio);
> > + unsigned advance = 0;
> >
> > bio_for_each_segment(bv, bio, iter) {
> > /*
> > @@ -134,12 +135,32 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
> > }
> >
> > if (bvprvp && blk_queue_cluster(q)) {
> > - if (seg_size + bv.bv_len > queue_max_segment_size(q))
> > - goto new_segment;
> > if (!BIOVEC_PHYS_MERGEABLE(bvprvp, &bv))
> > goto new_segment;
> > if (!BIOVEC_SEG_BOUNDARY(q, bvprvp, &bv))
> > goto new_segment;
> > + if (seg_size + bv.bv_len > queue_max_segment_size(q)) {
> > + /*
> > + * On assumption is that initial value of
> > + * @seg_size(equals to bv.bv_len) won't be
> > + * bigger than max segment size, but will
> > + * becomes false after multipage bvec comes.
> > + */
> > + advance = queue_max_segment_size(q) - seg_size;
> > +
> > + if (advance > 0) {
> > + seg_size += advance;
> > + sectors += advance >> 9;
> > + bv.bv_len -= advance;
> > + bv.bv_offset += advance;
> > + }
> > +
> > + /*
> > + * Still need to put remainder of current
> > + * bvec into a new segment.
> > + */
> > + goto new_segment;
> > + }
> >
> > seg_size += bv.bv_len;
> > bvprv = bv;
> > @@ -161,6 +182,12 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
> > seg_size = bv.bv_len;
> > sectors += bv.bv_len >> 9;
> >
> > + /* restore the bvec for iterator */
> > + if (advance) {
> > + bv.bv_len += advance;
> > + bv.bv_offset -= advance;
> > + advance = 0;
> > + }
> > }
> >
> > do_split = false;
> > @@ -361,16 +388,29 @@ __blk_segment_map_sg(struct request_queue *q, struct bio_vec *bvec,
> > {
> >
> > int nbytes = bvec->bv_len;
> > + unsigned advance = 0;
> >
> > if (*sg && *cluster) {
> > - if ((*sg)->length + nbytes > queue_max_segment_size(q))
> > - goto new_segment;
> > -
> > if (!BIOVEC_PHYS_MERGEABLE(bvprv, bvec))
> > goto new_segment;
> > if (!BIOVEC_SEG_BOUNDARY(q, bvprv, bvec))
> > goto new_segment;
> >
> > + /*
> > + * try best to merge part of the bvec into previous
> > + * segment and follow same policy with
> > + * blk_bio_segment_split()
> > + */
> > + if ((*sg)->length + nbytes > queue_max_segment_size(q)) {
> > + advance = queue_max_segment_size(q) - (*sg)->length;
> > + if (advance) {
> > + (*sg)->length += advance;
> > + bvec->bv_offset += advance;
> > + bvec->bv_len -= advance;
> > + }
> > + goto new_segment;
> > + }
> > +
> > (*sg)->length += nbytes;
> > } else {
> > new_segment:
> > @@ -393,6 +433,10 @@ __blk_segment_map_sg(struct request_queue *q, struct bio_vec *bvec,
> >
> > sg_set_page(*sg, bvec->bv_page, nbytes, bvec->bv_offset);
> > (*nsegs)++;
> > +
> > + /* for making iterator happy */
> > + bvec->bv_offset -= advance;
> > + bvec->bv_len += advance;
> > }
> > *bvprv = *bvec;
> > }
> >
>
> Hello,
>
> This patch breaks MMC on next-20180108, in particular MMC doesn't work anymore
> with this patch on NVIDIA Tegra20:
>
> <3>[ 36.622253] print_req_error: I/O error, dev mmcblk1, sector 512
> <3>[ 36.671233] print_req_error: I/O error, dev mmcblk2, sector 128
> <3>[ 36.711308] print_req_error: I/O error, dev mmcblk1, sector 31325304
> <3>[ 36.749232] print_req_error: I/O error, dev mmcblk2, sector 512
> <3>[ 36.761235] print_req_error: I/O error, dev mmcblk1, sector 31325816
> <3>[ 36.832039] print_req_error: I/O error, dev mmcblk2, sector 31259768
> <3>[ 99.793248] print_req_error: I/O error, dev mmcblk1, sector 31323136
> <3>[ 99.982043] print_req_error: I/O error, dev mmcblk1, sector 929792
> <3>[ 99.986301] print_req_error: I/O error, dev mmcblk1, sector 930816
> <3>[ 100.293624] print_req_error: I/O error, dev mmcblk1, sector 932864
> <3>[ 100.466839] print_req_error: I/O error, dev mmcblk1, sector 947200
> <3>[ 100.642955] print_req_error: I/O error, dev mmcblk1, sector 949248
> <3>[ 100.818838] print_req_error: I/O error, dev mmcblk1, sector 230400
>
> Any attempt of mounting MMC block dev ends with a kernel crash. Reverting this
> patch fixes the issue.
Hi Dmitry,
Thanks for your report!
Could you share us what the segment limits are on your MMC?
cat /sys/block/mmcN/queue/max_segment_size
cat /sys/block/mmcN/queue/max_segments
Please test the following patch to see if your issue can be fixed?
---
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 446f63e076aa..cfab36c26608 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -431,12 +431,14 @@ __blk_segment_map_sg(struct request_queue *q, struct bio_vec *bvec,
sg_set_page(*sg, bvec->bv_page, nbytes, bvec->bv_offset);
(*nsegs)++;
+ }
+ *bvprv = *bvec;
+ if (advance) {
/* for making iterator happy */
bvec->bv_offset -= advance;
bvec->bv_len += advance;
}
- *bvprv = *bvec;
}
static inline int __blk_bvec_map_sg(struct request_queue *q, struct bio_vec bv,
Thanks,
Ming