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

[Ming Lei]

Hi Guys,

On Thu, Feb 18, 2016 at 2:16 PM, Ming Lei <ming.lei@xxxxxxxxxxxxx> wrote:
> Hi Kent,
>
> Thanks for your review.
>
> On Thu, Feb 18, 2016 at 12:24 PM, Kent Overstreet
> <kent.overstreet@xxxxxxxxx> wrote:
>> On Mon, Feb 15, 2016 at 05:42:12PM +0800, Ming Lei wrote:
>>> Cc Kent and Keith.
>>>
>>> Follows another version which should be more efficient.
>>> Kent and Keith, I appreciate much if you may give a review on it.
>>>
>>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>>> index 56d2db8..ef45fec 100644
>>> --- a/include/linux/bio.h
>>> +++ b/include/linux/bio.h
>>> @@ -278,11 +278,21 @@ static inline void bio_get_first_bvec(struct bio *bio, struct bio_vec *bv)
>>>   */
>>>  static inline void bio_get_last_bvec(struct bio *bio, struct bio_vec *bv)
>>>  {
>>> -     struct bvec_iter iter;
>>> +     struct bvec_iter iter = bio->bi_iter;
>>> +     int idx;
>>> +
>>> +     bio_advance_iter(bio, &iter, iter.bi_size);
>>> +
>>> +     WARN_ON(!iter.bi_idx && !iter.bi_bvec_done);
>>> +
>>> +     if (!iter.bi_bvec_done)
>>> +             idx = iter.bi_idx - 1;
>>> +     else    /* in the middle of bvec */
>>> +             idx = iter.bi_idx;
>>>
>>> -     bio_for_each_segment(*bv, bio, iter)
>>> -             if (bv->bv_len == iter.bi_size)
>>> -                     break;
>>> +     *bv = bio->bi_io_vec[idx];
>>> +     if (iter.bi_bvec_done)
>>> +             bv->bv_len = iter.bi_bvec_done;
>>>  }
>>
>> It can't be done correctly without a loop.
>
> As we discussed in gtalk, the only case this patch can't cope with
> is that one single bvec doesn't use up the remained io vector,
> but it can be handled by putting the following code at the
> function entry:
>
> if (!bio_multiple_segments(bio)) {
>        *bv = bio_iovec(bio);
>        return;
> }
>
>>
>> The reason is that if the bio was split in the middle of a segment, bv->bv_len
>> on the last biovec will be larger than what's actually used by the bio (it's
>> being shared between the two splits!).
>
> The last two lines in this helper should handle the situation.
>
>>
>> You have to iterate over all the biovecs so that you can see where
>> bi_iter->bi_size ends.
>
> I understand your concern is that this patch may not be much more
> efficient than bio_for_each_segment().
>
> IMO, one win of the patch is that 16bytes bvec copy is saved for all
> vectors, and another 'win' is to just run bvec_iter_advance() once(
> like move the outside for loop inside).
>
> I will run some benchmark to see if there is any performance
> difference between the two patches.

When I call bench_last_bvec()(see below) from null_queue_rq():
drivers/block/null_blk.c, IOPS with the 2nd patch in fio test(libaio,
randread, null_blk with default mod parameter) is better than the
1st one by > ~2%:

-------------------------
|BS      | IOPS          |
------------------------
|64K    |  +2%           |
-----------------------
|512K  |  +3%          |
------------------------

the 1st patch: use bio_for_each_segment()
the 2nd patch: use single bio_advance_iter()

static void bench_last_bvec(struct request *rq)
{
        static unsigned long total = 0;
        struct bio *bio;
        struct bio_vec bv = {0};

        __rq_for_each_bio(bio, rq) {
                bio_get_last_bvec(bio, &bv);
                total += bv.bv_len;
        }
}

Thanks
Ming