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

Groups > linux.kernel > #1334195 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] block: fix bio_will_gap() Ming Lei <ming.lei@canonical.com> - 2016-02-15 08:10 +0100
    [PATCH 2/4] block: check virt boundary in bio_will_gap() Ming Lei <ming.lei@canonical.com> - 2016-02-15 08:10 +0100
      Re: [PATCH 2/4] block: check virt boundary in bio_will_gap() Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-15 09:30 +0100
        Re: [PATCH 2/4] block: check virt boundary in bio_will_gap() Ming Lei <ming.lei@canonical.com> - 2016-02-15 11:30 +0100
          Re: [PATCH 2/4] block: check virt boundary in bio_will_gap() Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-15 21:30 +0100
            Re: [PATCH 2/4] block: check virt boundary in bio_will_gap() Ming Lei <ming.lei@canonical.com> - 2016-02-16 14:10 +0100
            Re: [PATCH 2/4] block: check virt boundary in bio_will_gap() Ming Lei <ming.lei@canonical.com> - 2016-02-16 14:10 +0100
    Re: [PATCH 0/4] block: fix bio_will_gap() Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-15 09:40 +0100

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

Hi,

After bio splitting is introduced, the splitted bio can be fast-cloned,
which is correct because biovecs has become immutable since v3.13.
    
Unfortunately bio_will_gap() isn't ready for this kind of change,
because it figures out the last bvec via 'bi_io_vec[prev->bi_vcnt - 1]'
directly.

It is observed that lots of BIOs are merges even the virt boundary
limit is violated, and the issue is reported from Sagi Grimberg.
    
This patchset try to fix the issue by introducing two helpers for getting
the first and last bvec of one bio by the bio iterator helpers.

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

Thanks,
Ming

[toc] | [next] | [standalone]

#1334200 — [PATCH 2/4] block: check virt boundary in bio_will_gap()

In the following patch, the way for figuring out
the last bvec will be changed with a bit cost introduced,
so return immediately if the queue doesn't have virt
boundary limit. Actually most of devices have not
this limit.

Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 include/linux/blkdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4571ef1..b8ff6a3 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1388,7 +1388,7 @@ static inline bool bvec_gap_to_prev(struct request_queue *q,
 static inline bool bio_will_gap(struct request_queue *q, struct bio *prev,
 			 struct bio *next)
 {
-	if (!bio_has_data(prev))
+	if (!bio_has_data(prev) || !queue_virt_boundary(q))
 		return false;
 
 	return bvec_gap_to_prev(q, &prev->bi_io_vec[prev->bi_vcnt - 1],
-- 
1.9.1

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

#1334234 — Re: [PATCH 2/4] block: check virt boundary in bio_will_gap()

> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 4571ef1..b8ff6a3 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1388,7 +1388,7 @@ static inline bool bvec_gap_to_prev(struct request_queue *q,
>   static inline bool bio_will_gap(struct request_queue *q, struct bio *prev,
>   			 struct bio *next)
>   {
> -	if (!bio_has_data(prev))
> +	if (!bio_has_data(prev) || !queue_virt_boundary(q))
>   		return false;

Can we not do that?

bvec_gap_to_prev is already checking the virt_boundary and I'd sorta
like to keep the motivation to optimize bio_get_last_bvec() to be O(1).

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

#1334356 — Re: [PATCH 2/4] block: check virt boundary in bio_will_gap()

On Mon, Feb 15, 2016 at 4:22 PM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 4571ef1..b8ff6a3 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -1388,7 +1388,7 @@ static inline bool bvec_gap_to_prev(struct
>> request_queue *q,
>>   static inline bool bio_will_gap(struct request_queue *q, struct bio
>> *prev,
>>                          struct bio *next)
>>   {
>> -       if (!bio_has_data(prev))
>> +       if (!bio_has_data(prev) || !queue_virt_boundary(q))
>>                 return false;
>
>
> Can we not do that?

Given there are only 3 drivers which set virt boundary, I think
it is reasonable to do that.

>
> bvec_gap_to_prev is already checking the virt_boundary and I'd sorta
> like to keep the motivation to optimize bio_get_last_bvec() to be O(1).

Currently the approaches I thought of still need to iterate bvec by bvec,
not sure if O(1) can be reached easily, but I am happy to discuss the
optimized implementation.

Thanks,
Ming

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

#1334796 — Re: [PATCH 2/4] block: check virt boundary in bio_will_gap()

>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index 4571ef1..b8ff6a3 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -1388,7 +1388,7 @@ static inline bool bvec_gap_to_prev(struct
>>> request_queue *q,
>>>    static inline bool bio_will_gap(struct request_queue *q, struct bio
>>> *prev,
>>>                           struct bio *next)
>>>    {
>>> -       if (!bio_has_data(prev))
>>> +       if (!bio_has_data(prev) || !queue_virt_boundary(q))
>>>    bio_integrity_add_page              return false;
>>
>>
>> Can we not do that?
>
> Given there are only 3 drivers which set virt boundary, I think
> it is reasonable to do that.

3 drivers that are really performance critical. I don't think we
should add optimized branching for some of the drivers especially
when the drivers that do set virt_boundary *really* care about latency.

>> bvec_gap_to_prev is already checking the virt_boundary and I'd sorta
>> like to keep the motivation to optimize bio_get_last_bvec() to be O(1).
>
> Currently the approaches I thought of still need to iterate bvec by bvec,
> not sure if O(1) can be reached easily, but I am happy to discuss the
> optimized implementation.

Me too. Note that I don't mind if the bio split code won't be optimized,
but I do want req_gap_back_merge/req_gap_front_merge to be...

Also, are the bvec_gap_to_prev usages in bio_add_pc_page and
bio_integrity_add_page safe? I didn't test this stuff with integrity
payloads...

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

#1335350 — Re: [PATCH 2/4] block: check virt boundary in bio_will_gap()

On Tue, Feb 16, 2016 at 4:27 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>
>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>> index 4571ef1..b8ff6a3 100644
>>>> --- a/include/linux/blkdev.h
>>>> +++ b/include/linux/blkdev.h
>>>> @@ -1388,7 +1388,7 @@ static inline bool bvec_gap_to_prev(struct
>>>> request_queue *q,
>>>>    static inline bool bio_will_gap(struct request_queue *q, struct bio
>>>> *prev,
>>>>                           struct bio *next)
>>>>    {
>>>> -       if (!bio_has_data(prev))
>>>> +       if (!bio_has_data(prev) || !queue_virt_boundary(q))
>>>>    bio_integrity_add_page              return false;
>>>
>>>
>>>
>>> Can we not do that?
>>
>>
>> Given there are only 3 drivers which set virt boundary, I think
>> it is reasonable to do that.
>
>
> 3 drivers that are really performance critical. I don't think we
> should add optimized branching for some of the drivers especially
> when the drivers that do set virt_boundary *really* care about latency.
>
>>> bvec_gap_to_prev is already checking the virt_boundary and I'd sorta
>>> like to keep the motivation to optimize bio_get_last_bvec() to be O(1).
>>
>>
>> Currently the approaches I thought of still need to iterate bvec by bvec,
>> not sure if O(1) can be reached easily, but I am happy to discuss the
>> optimized implementation.
>
>
> Me too. Note that I don't mind if the bio split code won't be optimized,
> but I do want req_gap_back_merge/req_gap_front_merge to be...
>
> Also, are the bvec_gap_to_prev usages in bio_add_pc_page and
> bio_integrity_add_page safe? I didn't test this stuff with integrity

Yes, because both are non-cloned bvec table.

> payloads...

Thanks,

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

#1335351 — Re: [PATCH 2/4] block: check virt boundary in bio_will_gap()

On Tue, Feb 16, 2016 at 4:27 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>
>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>> index 4571ef1..b8ff6a3 100644
>>>> --- a/include/linux/blkdev.h
>>>> +++ b/include/linux/blkdev.h
>>>> @@ -1388,7 +1388,7 @@ static inline bool bvec_gap_to_prev(struct
>>>> request_queue *q,
>>>>    static inline bool bio_will_gap(struct request_queue *q, struct bio
>>>> *prev,
>>>>                           struct bio *next)
>>>>    {
>>>> -       if (!bio_has_data(prev))
>>>> +       if (!bio_has_data(prev) || !queue_virt_boundary(q))
>>>>    bio_integrity_add_page              return false;
>>>
>>>
>>>
>>> Can we not do that?
>>
>>
>> Given there are only 3 drivers which set virt boundary, I think
>> it is reasonable to do that.
>
>
> 3 drivers that are really performance critical. I don't think we
> should add optimized branching for some of the drivers especially
> when the drivers that do set virt_boundary *really* care about latency.

I don't think the extra check on bvec_gap_to_prev() can make any
difference, but if you do care we can introduce __bvec_gap_to_prev()
in which the check is moved into bio_will_gap().

Thanks,

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

#1334262

> Hi,
>
> After bio splitting is introduced, the splitted bio can be fast-cloned,
> which is correct because biovecs has become immutable since v3.13.
>
> Unfortunately bio_will_gap() isn't ready for this kind of change,
> because it figures out the last bvec via 'bi_io_vec[prev->bi_vcnt - 1]'
> directly.
>
> It is observed that lots of BIOs are merges even the virt boundary
> limit is violated, and the issue is reported from Sagi Grimberg.

This set makes the virt_boundary violation go away...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web