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

Groups > linux.kernel > #1360034 > unrolled thread

Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is  set Keith Busch <keith.busch@intel.com> - 2016-03-17 17:50 +0100
    Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set Ming Lei <tom.leiming@gmail.com> - 2016-03-18 04:00 +0100
      Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set Ming Lei <tom.leiming@gmail.com> - 2016-03-30 15:10 +0200

#1360034 — Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set

On Thu, Mar 17, 2016 at 12:20:28PM +0100, Vitaly Kuznetsov wrote:
> Keith Busch <keith.busch@intel.com> writes:
> > been combined. In any case, I think you can get what you're after just
> > by moving the gap check after BIOVEC_PHYS_MERGABLE. Does the following
> > look ok to you?
> >
> 
> Thanks, it does.

Cool, thanks for confirming.

> Will you send it or would you like me to do that with your Suggested-by?

I'm not confident yet this doesn't break anything, particularly since
we moved the gap check after the length check. Just wanted to confirm
the concept addressed your concern, but still need to take a closer look
and test before submitting.

[toc] | [next] | [standalone]

#1360317 — Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set

On Fri, Mar 18, 2016 at 12:39 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Thu, Mar 17, 2016 at 12:20:28PM +0100, Vitaly Kuznetsov wrote:
>> Keith Busch <keith.busch@intel.com> writes:
>> > been combined. In any case, I think you can get what you're after just
>> > by moving the gap check after BIOVEC_PHYS_MERGABLE. Does the following
>> > look ok to you?
>> >
>>
>> Thanks, it does.
>
> Cool, thanks for confirming.
>
>> Will you send it or would you like me to do that with your Suggested-by?
>
> I'm not confident yet this doesn't break anything, particularly since
> we moved the gap check after the length check. Just wanted to confirm
> the concept addressed your concern, but still need to take a closer look
> and test before submitting.

IMO, the change on blk_bio_segment_split() is correct, because actually it
is a sg gap and the check should have been done between segments
instead of bvecs. So it is reasonable to move the check just before populating
a new segment.

But for the 2nd change in bio_will_gap(), which should fix Vitaly's problem, I
am still not sure if it is completely correct. bio_will_gap() is used
to check if two
bios may be merged. Suppose two bios are continues physically, the last bvec
in 1st bio and the first bvec in 2nd bio might not be in one same segment
because of segment size limit.

The root cause might be from blkdev_writepage(), and I guess these small
bios are from there.

thanks,
Ming Lei

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

#1367190 — Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set

[Multipart message — attachments visible in raw view] — view raw

On Fri, Mar 18, 2016 at 10:59 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Fri, Mar 18, 2016 at 12:39 AM, Keith Busch <keith.busch@intel.com> wrote:
>> On Thu, Mar 17, 2016 at 12:20:28PM +0100, Vitaly Kuznetsov wrote:
>>> Keith Busch <keith.busch@intel.com> writes:
>>> > been combined. In any case, I think you can get what you're after just
>>> > by moving the gap check after BIOVEC_PHYS_MERGABLE. Does the following
>>> > look ok to you?
>>> >
>>>
>>> Thanks, it does.
>>
>> Cool, thanks for confirming.
>>
>>> Will you send it or would you like me to do that with your Suggested-by?
>>
>> I'm not confident yet this doesn't break anything, particularly since
>> we moved the gap check after the length check. Just wanted to confirm
>> the concept addressed your concern, but still need to take a closer look
>> and test before submitting.
>
> IMO, the change on blk_bio_segment_split() is correct, because actually it
> is a sg gap and the check should have been done between segments
> instead of bvecs. So it is reasonable to move the check just before populating
> a new segment.

Thinking of the 1st part change further, looks it is just correct in concept,
but wrong from current implementation. Because of bios/reqs merge,
blk_rq_map_sg() may end one segment in any bvec in theroy, so I guess
that is why each non-1st bvec need the check to make sure no sg gap.
Looks a very crazy limit, :-)

>
> But for the 2nd change in bio_will_gap(), which should fix Vitaly's problem, I
> am still not sure if it is completely correct. bio_will_gap() is used
> to check if two
> bios may be merged. Suppose two bios are continues physically, the last bvec
> in 1st bio and the first bvec in 2nd bio might not be in one same segment
> because of segment size limit.

How about the attached patch?


>
> The root cause might be from blkdev_writepage(), and I guess these small
> bios are from there.
>
> thanks,
> Ming Lei



-- 
Ming Lei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web