| Message ID | 1459857443-20611-2-git-send-email-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 6, 2016 at 8:18 AM, Kent Overstreet <kent.overstreet@gmail.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? Thanks, Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.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. 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 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. Instead of adding these low level accessors, it'd be better to convert code to higher level helpers (especially bio_add_page()) where applicable. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 6, 2016 at 9:46 AM, Kent Overstreet <kent.overstreet@gmail.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@gmail.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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.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@gmail.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). > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 6, 2016 at 10:21 AM, Kent Overstreet <kent.overstreet@gmail.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@gmail.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@gmail.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.
diff --git a/include/linux/bio.h b/include/linux/bio.h index 88bc64f..2179bc4 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -310,6 +310,34 @@ static inline void bio_clear_flag(struct bio *bio, unsigned int bit) bio->bi_flags &= ~(1U << bit); } +static inline bool bio_is_full(struct bio *bio) +{ + WARN_ONCE(bio_flagged(bio, BIO_CLONED), "cloned bio"); + + return bio->bi_vcnt >= bio->bi_max_vecs; +} + +static inline struct bio_vec *bio_get_base_vec(struct bio *bio) +{ + return __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter); +} + +/* This helper should be used for setting bvec table on a new bio */ +static inline void bio_set_vec_table(struct bio *bio, struct bio_vec *table, + unsigned max_vecs) +{ + bio->bi_io_vec = table; + bio->bi_max_vecs = max_vecs; +} + +/* For singlepage bvecs, one segment includes one page */ +static inline unsigned bio_pages(struct bio *bio) +{ + if (!bio_flagged(bio, BIO_CLONED)) + return bio->bi_vcnt; + return bio_segments(bio); +} + static inline void bio_get_first_bvec(struct bio *bio, struct bio_vec *bv) { *bv = bio_iovec(bio);
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. So this patches introduces 4 helpers for cleaning up this kind of usage. Both bio_pages() and bio_is_full() can be convertd to support multipage bvecs easily. For bio_get_base_vec() and bio_set_vec_table(), they are often used during initializing a new bio or in case of single bvec bio. With the two new helpers, it becomes easy to audit access of .bi_io_vec and .bi_vcnt. Signed-off-by: Ming Lei <tom.leiming@gmail.com> --- include/linux/bio.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)