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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACVXFVPrYDrG6w+c=a6uBVzOLY2hY8UwQmi_McSfUErfBwaDYw@mail.gmail.com>
Date:	Wed, 6 Apr 2016 12:11:42 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Kent Overstreet <kent.overstreet@...il.com>
Cc:	Jens Axboe <axboe@...com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-block@...r.kernel.org, Christoph Hellwig <hch@...radead.org>,
	Boaz Harrosh <boaz@...xistor.com>, Jan Kara <jack@...e.cz>,
	Keith Busch <keith.busch@...el.com>, Tejun Heo <tj@...nel.org>,
	Mike Snitzer <snitzer@...hat.com>
Subject: Re: [PATCH 01/27] block: bio: introduce 4 helpers for cleanup

On Wed, Apr 6, 2016 at 10:21 AM, Kent Overstreet
<kent.overstreet@...il.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@...il.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@...il.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