FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 09-07-2012, 08:58 PM
Kent Overstreet
 
Default block: Add bio_reset()

On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
> On 2012-09-06 16:34, Kent Overstreet wrote:
> > Reusing bios is something that's been highly frowned upon in the past,
> > but driver code keeps doing it anyways. If it's going to happen anyways,
> > we should provide a generic method.
> >
> > This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> > was open coding it, by doing a bio_init() and resetting bi_destructor.
> >
> > This required reordering struct bio, but the block layer is not yet
> > nearly fast enough for any cacheline effects to matter here.
>
> That's an odd and misplaced comment. Was just doing testing today at 5M
> IOPS, and even years back we've had cache effects for O_DIRECT in higher
> speed setups.

Ah, I wasn't aware that you were pushing that many iops through the
block layer - most I've tested myself was around 1M. It wouldn't
surprise me if cache effects in struct bio mattered around 5M...

> That said, we haven't done cache analysis in a long time. So moving
> members around isn't necessarily a huge deal.

Ok, good to know. I've got another patch coming later that reorders
struct bio a bit more, for immutable bvecs (bi_sector, bi_size, bi_idx
go into a struct bvec_iter together).

> Lastly, this isn't a great commit message for other reasons. Anyone can
> see that it moves members around. It'd be a lot better to explain _why_
> it is reordering the struct.

Yeah, I suppose so. Will keep that in mind for the next patch.

>
> BTW, I looked over the rest of the patches, and it looks OK to me.

Resent them. Thanks!

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-07-2012, 09:55 PM
Jens Axboe
 
Default block: Add bio_reset()

On 2012-09-07 14:58, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
>> On 2012-09-06 16:34, Kent Overstreet wrote:
>>> Reusing bios is something that's been highly frowned upon in the past,
>>> but driver code keeps doing it anyways. If it's going to happen anyways,
>>> we should provide a generic method.
>>>
>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
>>>
>>> This required reordering struct bio, but the block layer is not yet
>>> nearly fast enough for any cacheline effects to matter here.
>>
>> That's an odd and misplaced comment. Was just doing testing today at 5M
>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
>> speed setups.
>
> Ah, I wasn't aware that you were pushing that many iops through the
> block layer - most I've tested myself was around 1M. It wouldn't
> surprise me if cache effects in struct bio mattered around 5M...

5M is nothing, just did 13.5M :-)

But we can reshuffle for now. As mentioned, we're way overdue for a
decent look at cache profiling in any case.

>> That said, we haven't done cache analysis in a long time. So moving
>> members around isn't necessarily a huge deal.
>
> Ok, good to know. I've got another patch coming later that reorders
> struct bio a bit more, for immutable bvecs (bi_sector, bi_size, bi_idx
> go into a struct bvec_iter together).

OK

>> Lastly, this isn't a great commit message for other reasons. Anyone can
>> see that it moves members around. It'd be a lot better to explain _why_
>> it is reordering the struct.
>
> Yeah, I suppose so. Will keep that in mind for the next patch.

Thanks.

>> BTW, I looked over the rest of the patches, and it looks OK to me.
>
> Resent them. Thanks!

Got it.

--
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-07-2012, 10:06 PM
Jens Axboe
 
Default block: Add bio_reset()

On 2012-09-07 15:55, Jens Axboe wrote:
> On 2012-09-07 14:58, Kent Overstreet wrote:
>> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
>>> On 2012-09-06 16:34, Kent Overstreet wrote:
>>>> Reusing bios is something that's been highly frowned upon in the past,
>>>> but driver code keeps doing it anyways. If it's going to happen anyways,
>>>> we should provide a generic method.
>>>>
>>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
>>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
>>>>
>>>> This required reordering struct bio, but the block layer is not yet
>>>> nearly fast enough for any cacheline effects to matter here.
>>>
>>> That's an odd and misplaced comment. Was just doing testing today at 5M
>>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
>>> speed setups.
>>
>> Ah, I wasn't aware that you were pushing that many iops through the
>> block layer - most I've tested myself was around 1M. It wouldn't
>> surprise me if cache effects in struct bio mattered around 5M...
>
> 5M is nothing, just did 13.5M :-)
>
> But we can reshuffle for now. As mentioned, we're way overdue for a
> decent look at cache profiling in any case.

No ill effects seen so far, fwiw:

read : io=1735.8GB, bw=53690MB/s, iops=13745K, runt= 33104msec

--
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-07-2012, 10:25 PM
Kent Overstreet
 
Default block: Add bio_reset()

On Fri, Sep 07, 2012 at 04:06:45PM -0600, Jens Axboe wrote:
> On 2012-09-07 15:55, Jens Axboe wrote:
> > On 2012-09-07 14:58, Kent Overstreet wrote:
> >> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
> >>> On 2012-09-06 16:34, Kent Overstreet wrote:
> >>>> Reusing bios is something that's been highly frowned upon in the past,
> >>>> but driver code keeps doing it anyways. If it's going to happen anyways,
> >>>> we should provide a generic method.
> >>>>
> >>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> >>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
> >>>>
> >>>> This required reordering struct bio, but the block layer is not yet
> >>>> nearly fast enough for any cacheline effects to matter here.
> >>>
> >>> That's an odd and misplaced comment. Was just doing testing today at 5M
> >>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
> >>> speed setups.
> >>
> >> Ah, I wasn't aware that you were pushing that many iops through the
> >> block layer - most I've tested myself was around 1M. It wouldn't
> >> surprise me if cache effects in struct bio mattered around 5M...
> >
> > 5M is nothing, just did 13.5M :-)
> >
> > But we can reshuffle for now. As mentioned, we're way overdue for a
> > decent look at cache profiling in any case.
>
> No ill effects seen so far, fwiw:
>
> read : io=1735.8GB, bw=53690MB/s, iops=13745K, runt= 33104msec

Cool!

I'd be really curious to see a profile. Of the patches I've got queued
up I don't think anything's going to significantly affect performance
yet, but I'm hoping the cleanups/immutable bvec stuff/efficient bio
splitting enables some performance gains.

Well, it certainly will for stacking drivers, but I'm less sure what
it's going to look like running on just a raw flash device.

My end goal is making generic_make_request handle arbitrary sized bios,
and have (efficient) splitting happen as required. This'll get rid of a
bunch of code and complexity in the upper layers, in bio_add_page() and
elsewhere. More in the stacking drivers - merge_bvec_fn is horrendous to
support.

I think I might be able to efficiently get rid of the
segments-after-merging precalculating, and just have segments merged
once. That'd get rid of a couple fields in struct bio, and get it under
2 cachelines last I counted.

Course, all this doesn't matter as much for 4k bios so it may just be a
wash for you.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-07-2012, 10:44 PM
Jens Axboe
 
Default block: Add bio_reset()

On 2012-09-07 16:25, Kent Overstreet wrote:
> On Fri, Sep 07, 2012 at 04:06:45PM -0600, Jens Axboe wrote:
>> On 2012-09-07 15:55, Jens Axboe wrote:
>>> On 2012-09-07 14:58, Kent Overstreet wrote:
>>>> On Thu, Sep 06, 2012 at 07:34:18PM -0600, Jens Axboe wrote:
>>>>> On 2012-09-06 16:34, Kent Overstreet wrote:
>>>>>> Reusing bios is something that's been highly frowned upon in the past,
>>>>>> but driver code keeps doing it anyways. If it's going to happen anyways,
>>>>>> we should provide a generic method.
>>>>>>
>>>>>> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
>>>>>> was open coding it, by doing a bio_init() and resetting bi_destructor.
>>>>>>
>>>>>> This required reordering struct bio, but the block layer is not yet
>>>>>> nearly fast enough for any cacheline effects to matter here.
>>>>>
>>>>> That's an odd and misplaced comment. Was just doing testing today at 5M
>>>>> IOPS, and even years back we've had cache effects for O_DIRECT in higher
>>>>> speed setups.
>>>>
>>>> Ah, I wasn't aware that you were pushing that many iops through the
>>>> block layer - most I've tested myself was around 1M. It wouldn't
>>>> surprise me if cache effects in struct bio mattered around 5M...
>>>
>>> 5M is nothing, just did 13.5M :-)
>>>
>>> But we can reshuffle for now. As mentioned, we're way overdue for a
>>> decent look at cache profiling in any case.
>>
>> No ill effects seen so far, fwiw:
>>
>> read : io=1735.8GB, bw=53690MB/s, iops=13745K, runt= 33104msec
>
> Cool!
>
> I'd be really curious to see a profile. Of the patches I've got queued
> up I don't think anything's going to significantly affect performance
> yet, but I'm hoping the cleanups/immutable bvec stuff/efficient bio
> splitting enables some performance gains.

Got more work to do, but certainly not a problem sharing.

> Well, it certainly will for stacking drivers, but I'm less sure what
> it's going to look like running on just a raw flash device.
>
> My end goal is making generic_make_request handle arbitrary sized bios,
> and have (efficient) splitting happen as required. This'll get rid of a
> bunch of code and complexity in the upper layers, in bio_add_page() and
> elsewhere. More in the stacking drivers - merge_bvec_fn is horrendous to
> support.

It is a nasty interface, in retrospect probably a mistake. As long as we
don't split ever on non-stacking drivers, I don't care too much. And it
would get rid of complexity in those drivers, so that's a nice win.
merge_bvec_fn not only a bad interface, it's also pretty slow...

> I think I might be able to efficiently get rid of the
> segments-after-merging precalculating, and just have segments merged
> once. That'd get rid of a couple fields in struct bio, and get it under
> 2 cachelines last I counted.

It's 2 cachelines now, but reducing is always a great thing. Getting rid
of the repeated recalculate after merge would be a nice win.

> Course, all this doesn't matter as much for 4k bios so it may just be a
> wash for you.

Right, for me it doesn't matter. As long as you don't slow me down :-)

--
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-07-2012, 11:14 PM
Alasdair G Kergon
 
Default block: Add bio_reset()

As I indicated already in this discussion, dm started to use
merge_bvec_fn as a cheap way of avoiding splitting and this improved
overall efficiency. Often it's better to pay the small price of calling
that function to ensure the bio is created the right size in the first
place so it won't have to get split later.

I'm as yet unconvinced that removing merge_bvec_fn would be an overall
win. Some of Kent's other changes that make splitting cheaper will
improve the balance in some situations, but that might be handled by
simplifying the merge_bvec_fn calculations in those situations.
(Or changing the mechanism to avoid repeating performing the mapping
when it hasn't changed.)

IOW Any proposal to remove merge_bvec_fn from dm needs careful
evaluation to ensure it doesn't introduce any significant
performance regressions for some sets of users.

Alasdair

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-07-2012, 11:25 PM
Kent Overstreet
 
Default block: Add bio_reset()

On Sat, Sep 08, 2012 at 12:14:33AM +0100, Alasdair G Kergon wrote:
> As I indicated already in this discussion, dm started to use
> merge_bvec_fn as a cheap way of avoiding splitting and this improved
> overall efficiency. Often it's better to pay the small price of calling
> that function to ensure the bio is created the right size in the first
> place so it won't have to get split later.

When I say cheap, I mean _cheap_:

split = bio_clone_bioset(bio, gfp_flags, bs);

bio_advance(bio, sectors << 9);
split->bi_iter.bi_size = sectors << 9;

And the clone doesn't copy the bvecs - split->bi_io_vec ==
bio->bi_io_vec.

> I'm as yet unconvinced that removing merge_bvec_fn would be an overall
> win. Some of Kent's other changes that make splitting cheaper will
> improve the balance in some situations, but that might be handled by
> simplifying the merge_bvec_fn calculations in those situations.
> (Or changing the mechanism to avoid repeating performing the mapping
> when it hasn't changed.)

The current situation is what causes you to repeatedly do the mapping
lookup, since you'll often get contiguous bios that don't need to be
split at the mapping level (because of other requirements of the
underlying devices or because implementing merge_bvec_fn correctly was
too hard).

Splitting only when required is going to _improve_ that.

> IOW Any proposal to remove merge_bvec_fn from dm needs careful
> evaluation to ensure it doesn't introduce any significant
> performance regressions for some sets of users.

There's also the 1000+ lines of deleted code to consider. In my
immutable bvec branch I've deleted over 400 lines of code, and that's
without actually trying to delete code. Getting rid of merge_bvec_fn
deletes another 800 lines of code on top of that.

CPU wise, there won't be any performance regressions. The only cause for
concern I can think of is where the upper layer could've made use of
partial completions - i.e. it submitted a 1 mb bio instead of a bunch of
128k bios, but it could've made use of that first 128k if it went to a
different device and completed sooner.

Only thing I know of that'd be affected by that though is readahead, and
I have a couple ideas for easily solving that if it actually becomes an
issue.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 06:19 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org