On Wed, Apr 6, 2016 at 10:21 AM, Kent Overstreet <kent.overstreet@xxxxxxxxx> 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@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). 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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
