On 02/16/2017 02:59 PM, Ming Lei wrote: > On Thu, Feb 16, 2017 at 9:40 PM, Johannes Thumshirn <jthumshirn@xxxxxxx> wrote: >> On 02/16/2017 02:30 PM, Ming Lei wrote: >>> On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@xxxxxxx> wrote: >>>> On 02/16/2017 12:45 PM, Ming Lei wrote: >>>>> MD need this helper to remove the last added page, so introduce >>>>> it. >>>>> >>>>> Signed-off-by: Ming Lei <tom.leiming@xxxxxxxxx> >>>>> --- >>>>> block/bio.c | 23 +++++++++++++++++++++++ >>>>> include/linux/bio.h | 1 + >>>>> 2 files changed, 24 insertions(+) >>>>> >>>>> diff --git a/block/bio.c b/block/bio.c >>>>> index 5eec5e08417f..0ce7ffcd7939 100644 >>>>> --- a/block/bio.c >>>>> +++ b/block/bio.c >>>>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page >>>>> EXPORT_SYMBOL(bio_add_pc_page); >>>>> >>>>> /** >>>>> + * bio_remove_last_page - remove the last added page >>>>> + * @bio: destination bio >>>>> + * >>>>> + * Attempt to remove the last added page from the bio_vec maplist. >>>>> + */ >>>>> +void bio_remove_last_page(struct bio *bio) >>>>> +{ >>>>> + /* >>>>> + * cloned bio must not modify vec list >>>>> + */ >>>>> + if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED))) >>>>> + return; >>>>> + >>>>> + if (bio->bi_vcnt > 0) { >>>> >>>> In patch 1 you introduce bio_segments_all() with the log message 'So >>>> that we can replace the direct access to .bi_vcnt.' Here you introduce a >>>> new direct access to it (plus the duplicated WARN_ON_ONCE()). >>>> >>>> Maybe use the helper directly here (I admit I haven't gone through the >>>> whole series yet, so I can't see if the change is made later). >>> >>> Firstly MD does need one helper to remove the last added page, as you >>> can see there are three such uses in patch3. >>> >>> Secondly both the two helpers will be changed once multipage bvec >>> is supported, that means we have to change MD too after multipage bvec >>> if just using bio_segments_all() to replace .bi_vcnt for removing >>> the last added page. >> >> I'm not sure if we're talking past each other here, I assumed you'd do >> something like: >> >> void bio_remove_last_page(struct bio *bio) >> { >> int vcnt = bio_segments_all(bio); >> >> if (bio_flagged(bio, BIO_CLONED)) >> return; >> >> if (vcnt > 0) { >> struct bio_vec *bv = &bio->bi_io_vec[vcnt - 1]; >> >> bio->bi_iter.bi_size -= bv->bv_len; >> bio->bi_vcnt--; >> } >> } > > What we are doing is to remove the external direct access to bvec > table in drivers or filesystems because they may misuse that, for > example, drivers often use .bi_vcnt to get the page count in the bio, > but the actual meaning is just bvec's count. > > And the implementation in block layer still need to play the table directly, > especially for sake of efficiency, cause we understand the details and won't > misuse that. Ah OK, so bio_segments_all() is intended for drivers. Thanks for the clarification, Johannes -- Johannes Thumshirn Storage jthumshirn@xxxxxxx +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850