lkml.org 
[lkml]   [2016]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 01/27] block: bio: introduce 4 helpers for cleanup
From
On Wed, Apr 6, 2016 at 10:21 AM, Kent Overstreet
<kent.overstreet@gmail.com> wrote:
> On Wed, Apr 06, 2016 at 10:11:27AM +0800, Ming Lei wrote:
>> On Wed, Apr 6, 2016 at 9:46 AM, Kent Overstreet
>> <kent.overstreet@gmail.com> wrote:
>> > On Wed, Apr 06, 2016 at 09:34:34AM +0800, Ming Lei wrote:
>> >> On Wed, Apr 6, 2016 at 8:18 AM, Kent Overstreet
>> >> <kent.overstreet@gmail.com> wrote:
>> >> > On Tue, Apr 05, 2016 at 07:56:46PM +0800, Ming Lei wrote:
>> >> >> Some drivers access bio->bi_vcnt and bio->bi_io_vec directly,
>> >> >> firstly it isn't a good practice, secondly it may cause trouble
>> >> >> for converting to multipage bvecs.
>> >> >
>> >> > "not good practice" is OO bullshit snake oil without more justification. We
>> >> > don't plaster accessors everywhere without an actual reason.
>> >> >
>> >> > How would it cause trouble with multipage bvecs?
>> >>
>> >> Simply speaking, the current drivers may depend on .bi_vcnt for
>> >> computing how many page there are in one bio. After multipage bvecs,
>> >> it is not true any more. Isn't it a actual reason?
>> >
>> > But it's completely valid to use bi_vcnt for segments, which is what it's always
>> > _really_ meant anyways.
>>
>> Previously drivers may be confused with segment and page, so they just thought
>> segment is same with page. The situation will change after multipage bvecs
>> is introduced.
>>
>> Drivers may loop over .bi_io_vec and .bi_vcnt for accessing each pages.
>> (pktcdvd, staging: lustre, raid,...)
>>
>> It isn't practical to fix all these drivers before introducing multipage bvecs.
>> Meantime we can't cause regressions with multipage bvecs. But we can
>> disable multipage bvecs for some insane drivers if they insist on their
>> misusing.
>
> No - it is both practical and IMO _required_ to convert those drivers to
> bio_for_each_segment() or bio_for_each_page() as appropriate, before multipage
> bvecs.
>
> Especially code that needs pages and segments _has_ to be converted before
> multipage bvecs.
>
> If you'll recall looking at my various patch series from way back, especially
> around immutable biovecs - most of the work was in converting drivers, not the
> actual implementation (and I got rid of a more bi_io_vec/bi_vcnt uses than you
> have left, so honestly there's no excuse for not doing it right).

Looks your style for new featue is the following way:
- convert all drivers to new interface
- convert core code to new feature and enable it

My style is:
- if driver is easy to convert, then take new interface; othewise just leave it
alone without using new feature
- convert core code to new feature and enable it

I don't want to discuss which way is better.

But my way just introduces change to driver as few as possible, and
I try to avoid regression becasue I don't want to change code hugely
without detailed test.

That is why you can see the change to driver in this patchset is just
a little.

Thanks,

>
>> With these helpers, it is easy to audit drivers about their access to
>> .bi_vcnt & .bi_io_vec.
>
> It's easy to grep for those uses now!
>
>> After this ptach is applied, only btrfs and md are left with these references.
>>
>> For btrfs, we still need to audit each usage and try to clean them up.
>> For md, we can't enable multipage bvecs for them until all these usage
>> are cleaned up or audited.
>
> Cleaning up those should be your focus now, not adding these helpers. You don't
> need these patches to go in to tell you what needs to be cleaned up, we already
> know wha thas to be done.




--
Ming Lei

\
 
 \ /
  Last update: 2016-04-06 06:41    [W:0.113 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site