Re: [PATCH 01/27] block: bio: introduce 4 helpers for cleanup

[Ming Lei]

On Wed, Apr 6, 2016 at 9:46 AM, Kent Overstreet
<kent.overstreet@xxxxxxxxx> 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@xxxxxxxxx> 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.

With these helpers, it is easy to audit drivers about their access to
.bi_vcnt & .bi_io_vec.

>
> Sometimes you have cases where the meaning of a member changes significantly
> enough that you really don't want code using it accidentally anymore - like with
> Jens' patches that changed how bi_remaining and bi_cnt work, but after those
> patches it really wasn't correct to use those members directly anymore so he
> renamed them to prevent that.
>
> I don't buy that that's the case for multipage bvecs - the meaning of bi_vcnt
> itself isn't changing (it's just the number of entries in the array!) and it'll

It depends on view, from driver's view, they have changed significantly enough.

> still be possible for code to correctly use it directly.
>
> Same with bio->bi_io_vec, it's still an array of biovecs, that's not changing.
> Your helpers are at the wrong level of abstraction.
>
> Also, there isn't a huge number of bi_vcnt references in the kernel anyways -
> the immutable biovec work required removing most of them.

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.

>
> Instead of adding these low level accessors, it'd be better to convert code to
> higher level helpers (especially bio_add_page()) where applicable.

That is always the better way to use bio_add_page(). but sometimes
both .bi_vcnt and .bi_io_vec is used not for adding page to bio.



Thanks,
Ming Lei