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

[Kent Overstreet]

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@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.

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).

> 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.