Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752787AbcDFELr (ORCPT ); Wed, 6 Apr 2016 00:11:47 -0400 Received: from mail-yw0-f196.google.com ([209.85.161.196]:36434 "EHLO mail-yw0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750821AbcDFELn (ORCPT ); Wed, 6 Apr 2016 00:11:43 -0400 MIME-Version: 1.0 In-Reply-To: <20160406022137.GA7452@kmo-pixel> References: <1459857443-20611-1-git-send-email-tom.leiming@gmail.com> <1459857443-20611-2-git-send-email-tom.leiming@gmail.com> <20160406001841.GA31161@kmo-pixel> <20160406014636.GB32713@kmo-pixel> <20160406022137.GA7452@kmo-pixel> Date: Wed, 6 Apr 2016 12:11:42 +0800 Message-ID: Subject: Re: [PATCH 01/27] block: bio: introduce 4 helpers for cleanup From: Ming Lei To: Kent Overstreet Cc: Jens Axboe , Linux Kernel Mailing List , linux-block@vger.kernel.org, Christoph Hellwig , Boaz Harrosh , Jan Kara , Keith Busch , Tejun Heo , Mike Snitzer Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3556 Lines: 90 On Wed, Apr 6, 2016 at 10:21 AM, Kent Overstreet 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 >> 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 >> >> 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