| Message ID | 20180509074830.16196-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 09, 2018 at 09:47:58AM +0200, Christoph Hellwig wrote: > +/** > + * __bio_try_merge_page - try adding data to an existing bvec > + * @bio: destination bio > + * @page: page to add > + * @len: length of the range to add > + * @off: offset into @page > + * > + * Try adding the data described at @page + @offset to the last bvec of @bio. > + * Return %true on success or %false on failure. This can happen frequently > + * for file systems with a block size smaller than the page size. > + */ Could we make this: /** * __bio_try_merge_page() - Try appending data to an existing bvec. * @bio: Destination bio. * @page: Page to add. * @len: Length of the data to add. * @off: Offset of the data in @page. * * Try to add the data at @page + @off to the last bvec of @bio. This is * a useful optimisation for file systems with a block size smaller than * the page size. * * Context: Any context. * Return: %true on success or %false on failure. */ (page, len, off) is a bit weird to me. Usually we do (page, off, len). > +/** > + * __bio_add_page - add page to a bio in a new segment > + * @bio: destination bio > + * @page: page to add > + * @len: length of the range to add > + * @off: offset into @page > + * > + * Add the data at @page + @offset to @bio as a new bvec. The caller must > + * ensure that @bio has space for another bvec. > + */ /** * __bio_add_page - Add page to a bio in a new segment. * @bio: Destination bio. * @page: Page to add. * @len: Length of the data to add. * @off: Offset of the data in @page. * * Add the data at @page + @off to @bio as a new bvec. The caller must * ensure that @bio has space for another bvec. * * Context: Any context. */ > +static inline bool bio_full(struct bio *bio) > +{ > + return bio->bi_vcnt >= bio->bi_max_vecs; > +} I really like this helper.
On Wed, May 09, 2018 at 08:12:43AM -0700, Matthew Wilcox wrote:
> (page, len, off) is a bit weird to me. Usually we do (page, off, len).
That's what I'd usually do, too. But this odd convention is what
bio_add_page uses, so I decided to stick to that instead of having two
different conventions in one family of functions.
On Wed, May 9, 2018 at 3:47 PM, Christoph Hellwig <hch@lst.de> wrote: > For the upcoming removal of buffer heads in XFS we need to keep track of > the number of outstanding writeback requests per page. For this we need > to know if bio_add_page merged a region with the previous bvec or not. > Instead of adding additional arguments this refactors bio_add_page to > be implemented using three lower level helpers which users like XFS can > use directly if they care about the merge decisions. The merge policy may be transparent to fs, such as multipage bvec. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/bio.c | 87 ++++++++++++++++++++++++++++++--------------- > include/linux/bio.h | 9 +++++ > 2 files changed, 67 insertions(+), 29 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 53e0f0a1ed94..6ceba6adbf42 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -773,7 +773,7 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page > return 0; > } > > - if (bio->bi_vcnt >= bio->bi_max_vecs) > + if (bio_full(bio)) > return 0; > > /* > @@ -820,6 +820,59 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page > } > EXPORT_SYMBOL(bio_add_pc_page); > > +/** > + * __bio_try_merge_page - try adding data to an existing bvec > + * @bio: destination bio > + * @page: page to add > + * @len: length of the range to add > + * @off: offset into @page > + * > + * Try adding the data described at @page + @offset to the last bvec of @bio. > + * Return %true on success or %false on failure. This can happen frequently > + * for file systems with a block size smaller than the page size. > + */ > +bool __bio_try_merge_page(struct bio *bio, struct page *page, > + unsigned int len, unsigned int off) > +{ > + if (bio->bi_vcnt > 0) { > + struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1]; > + > + if (page == bv->bv_page && off == bv->bv_offset + bv->bv_len) { > + bv->bv_len += len; > + bio->bi_iter.bi_size += len; > + return true; > + } > + } > + return false; > +} > +EXPORT_SYMBOL_GPL(__bio_try_merge_page); > + > +/** > + * __bio_add_page - add page to a bio in a new segment > + * @bio: destination bio > + * @page: page to add > + * @len: length of the range to add > + * @off: offset into @page > + * > + * Add the data at @page + @offset to @bio as a new bvec. The caller must > + * ensure that @bio has space for another bvec. > + */ > +void __bio_add_page(struct bio *bio, struct page *page, > + unsigned int len, unsigned int off) > +{ > + struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt]; > + > + WARN_ON_ONCE(bio_full(bio)); > + > + bv->bv_page = page; > + bv->bv_offset = off; > + bv->bv_len = len; > + > + bio->bi_iter.bi_size += len; > + bio->bi_vcnt++; > +} > +EXPORT_SYMBOL_GPL(__bio_add_page); Given both __bio_try_merge_page() and __bio_add_page() are exported, please add WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)), otherwise both may be misused by external users.
On May 10, 2018, at 12:40 AM, Christoph Hellwig <hch@lst.de> wrote: > > On Wed, May 09, 2018 at 08:12:43AM -0700, Matthew Wilcox wrote: >> (page, len, off) is a bit weird to me. Usually we do (page, off, len). > > That's what I'd usually do, too. But this odd convention is what > bio_add_page uses, so I decided to stick to that instead of having two > different conventions in one family of functions. Would it make sense to change the bio_add_page() and bio_add_pc_page() to use the more common convention instead of continuing the spread of this non-standard calling convention? This is doubly problematic since "off" and "len" are both unsigned int values so it is easy to get them mixed up, and just reordering the bio_add_page() arguments would not generate any errors. One option would be to rename this function bio_page_add() so there are build errors or first add bio_page_add() and mark bio_add_page() deprecated and allow some short time for transition? There are about 50 uses under drivers/ and 50 uses under fs/. Cheers, Andreas
On Thu, May 10, 2018 at 04:52:00PM +0800, Ming Lei wrote: > On Wed, May 9, 2018 at 3:47 PM, Christoph Hellwig <hch@lst.de> wrote: > > For the upcoming removal of buffer heads in XFS we need to keep track of > > the number of outstanding writeback requests per page. For this we need > > to know if bio_add_page merged a region with the previous bvec or not. > > Instead of adding additional arguments this refactors bio_add_page to > > be implemented using three lower level helpers which users like XFS can > > use directly if they care about the merge decisions. > > The merge policy may be transparent to fs, such as multipage bvec. The whole point of this series it to make it explicit. That will have to be carried over into a multipage bvec world. That means the current __bio_try_merge_page will have to remain as-is in that new world order, but we'd also add a new __bio_try_merge_segment which merges beyond th e page. For the iomap and xfs code we'd first call __bio_try_merge_page, if that fails increment the read/write count, and only after that call __bio_try_merge_segment.
On Thu, May 10, 2018 at 03:49:53PM -0600, Andreas Dilger wrote: > Would it make sense to change the bio_add_page() and bio_add_pc_page() > to use the more common convention instead of continuing the spread of > this non-standard calling convention? This is doubly problematic since > "off" and "len" are both unsigned int values so it is easy to get them > mixed up, and just reordering the bio_add_page() arguments would not > generate any errors. We have more than hundred callers. I don't think we want to create so much churn just to clean things up a bit without any meaѕurable benefit. And even if you want to clean it up I'd rather keep it away from my iomap/xfs buffered I/O series :) > One option would be to rename this function bio_page_add() so there are > build errors or first add bio_page_add() and mark bio_add_page() > deprecated and allow some short time for transition? There are about > 50 uses under drivers/ and 50 uses under fs/. If you think the churn is worthwhile send a separate series for that. My two new functions should have very few callers even by then, so feel free to just update them as well.
On 5/11/18 12:29 AM, Christoph Hellwig wrote: > On Thu, May 10, 2018 at 03:49:53PM -0600, Andreas Dilger wrote: >> Would it make sense to change the bio_add_page() and bio_add_pc_page() >> to use the more common convention instead of continuing the spread of >> this non-standard calling convention? This is doubly problematic since >> "off" and "len" are both unsigned int values so it is easy to get them >> mixed up, and just reordering the bio_add_page() arguments would not >> generate any errors. > > We have more than hundred callers. I don't think we want to create > so much churn just to clean things up a bit without any meaѕurable > benefit. And even if you want to clean it up I'd rather keep it > away from my iomap/xfs buffered I/O series :) Yeah let's not do that, I know someone that always gets really grumpy when changes like that are made. So given that, I think we should retain the argument order for that we already have for __bio_try_merge_page() as well.
On 5/9/2018 1:17 PM, Christoph Hellwig wrote: > For the upcoming removal of buffer heads in XFS we need to keep track of > the number of outstanding writeback requests per page. For this we need > to know if bio_add_page merged a region with the previous bvec or not. > Instead of adding additional arguments this refactors bio_add_page to > be implemented using three lower level helpers which users like XFS can > use directly if they care about the merge decisions. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/bio.c | 87 ++++++++++++++++++++++++++++++--------------- > include/linux/bio.h | 9 +++++ > 2 files changed, 67 insertions(+), 29 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 53e0f0a1ed94..6ceba6adbf42 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -773,7 +773,7 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page > return 0; > } > > - if (bio->bi_vcnt >= bio->bi_max_vecs) > + if (bio_full(bio)) > return 0; > > /* > @@ -820,6 +820,59 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page > } > EXPORT_SYMBOL(bio_add_pc_page); > > +/** > + * __bio_try_merge_page - try adding data to an existing bvec > + * @bio: destination bio > + * @page: page to add > + * @len: length of the range to add > + * @off: offset into @page > + * > + * Try adding the data described at @page + @offset to the last bvec of @bio. > + * Return %true on success or %false on failure. This can happen frequently > + * for file systems with a block size smaller than the page size. > + */ > +bool __bio_try_merge_page(struct bio *bio, struct page *page, > + unsigned int len, unsigned int off) > +{ > + if (bio->bi_vcnt > 0) { > + struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1]; > + > + if (page == bv->bv_page && off == bv->bv_offset + bv->bv_len) { > + bv->bv_len += len; > + bio->bi_iter.bi_size += len; > + return true; > + } > + } > + return false; > +} > +EXPORT_SYMBOL_GPL(__bio_try_merge_page); > + > +/** > + * __bio_add_page - add page to a bio in a new segment > + * @bio: destination bio > + * @page: page to add > + * @len: length of the range to add > + * @off: offset into @page > + * > + * Add the data at @page + @offset to @bio as a new bvec. The caller must > + * ensure that @bio has space for another bvec. > + */ > +void __bio_add_page(struct bio *bio, struct page *page, > + unsigned int len, unsigned int off) > +{ > + struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt]; > + > + WARN_ON_ONCE(bio_full(bio)); Please correct my understanding here. I am still new at understanding this. 1. if bio_full is true that means no space in bio->bio_io_vec[] no? Than how come we are still proceeding ahead with only warning? While originally in bio_add_page we used to return after checking bio_full. Callers can still call __bio_add_page directly right. 2. Also the bio_io_vec size allocated will only be upto bio->bi_max_vecs right? I could not follow up very well with the bvec_alloc function, mainly when nr_iovec > inline_vecs. So how and where it is getting sure that we are getting _nr_iovecs_ allocated from the bvec_pool? hmm.. tricky. Please help me understand this. 1. So we have defined different slabs of different sizes in bvec_slabs. and when the allocation request of nr_iovecs come we try to grab the predefined(in terms of size) slab of bvec_slabs and return. In case if that allocation does not succeed from slab, we go for mempool_alloc. 2. IF above is correct why don't we set the bio->bi_max_vecs to the size of the slab instead of keeeping it to nr_iovecs which user requested? (in bio_alloc_bioset) 3. Could you please help understand why for cloned bio we still allow __bio_add_page to work? why not WARN and return like in original code? 4. Ok, I see that in patch 32 you are first checking bio_full and calling for xfs_chain_bio. But there also I think you are making sure that new ioend->io_bio is the new chained bio which is not full. Apologies if above doesn't make any sense. > + > + bv->bv_page = page; > + bv->bv_offset = off; > + bv->bv_len = len; > + > + bio->bi_iter.bi_size += len; > + bio->bi_vcnt++; > +} > +EXPORT_SYMBOL_GPL(__bio_add_page); > + > /** > * bio_add_page - attempt to add page to bio > * @bio: destination bio > @@ -833,40 +886,16 @@ EXPORT_SYMBOL(bio_add_pc_page); > int bio_add_page(struct bio *bio, struct page *page, > unsigned int len, unsigned int offset) > { > - struct bio_vec *bv; > - > /* > * cloned bio must not modify vec list > */ > if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED))) > return 0; > - > - /* > - * For filesystems with a blocksize smaller than the pagesize > - * we will often be called with the same page as last time and > - * a consecutive offset. Optimize this special case. > - */ > - if (bio->bi_vcnt > 0) { > - bv = &bio->bi_io_vec[bio->bi_vcnt - 1]; > - > - if (page == bv->bv_page && > - offset == bv->bv_offset + bv->bv_len) { > - bv->bv_len += len; > - goto done; > - } > + if (!__bio_try_merge_page(bio, page, len, offset)) { > + if (bio_full(bio)) > + return 0; > + __bio_add_page(bio, page, len, offset); > } > - > - if (bio->bi_vcnt >= bio->bi_max_vecs) > - return 0; Originally here we were supposed to return and not proceed further. Should __bio_add_page not have similar checks to safeguard crossing the bio_io_vec[] boundary? > - > - bv = &bio->bi_io_vec[bio->bi_vcnt]; > - bv->bv_page = page; > - bv->bv_len = len; > - bv->bv_offset = offset; > - > - bio->bi_vcnt++; > -done: > - bio->bi_iter.bi_size += len; > return len; > } > EXPORT_SYMBOL(bio_add_page); > diff --git a/include/linux/bio.h b/include/linux/bio.h > index ce547a25e8ae..3e73c8bc25ea 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -123,6 +123,11 @@ static inline void *bio_data(struct bio *bio) > return NULL; > } > > +static inline bool bio_full(struct bio *bio) > +{ > + return bio->bi_vcnt >= bio->bi_max_vecs; > +} > + > /* > * will die > */ > @@ -470,6 +475,10 @@ void bio_chain(struct bio *, struct bio *); > extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int); > extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *, > unsigned int, unsigned int); > +bool __bio_try_merge_page(struct bio *bio, struct page *page, > + unsigned int len, unsigned int off); > +void __bio_add_page(struct bio *bio, struct page *page, > + unsigned int len, unsigned int off); > int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter); > struct rq_map_data; > extern struct bio *bio_map_user_iov(struct request_queue *, >
On Wed, May 16, 2018 at 10:36:14AM +0530, Ritesh Harjani wrote: > 1. if bio_full is true that means no space in bio->bio_io_vec[] no? > Than how come we are still proceeding ahead with only warning? > While originally in bio_add_page we used to return after checking > bio_full. Callers can still call __bio_add_page directly right. I you don't know if the bio is full or not don't use __bio_add_page, keep using bio_add_page. The WARN_ON is just a debug tool to catch cases where the developer did use it incorrectly. > 2. IF above is correct why don't we set the bio->bi_max_vecs to the size > of the slab instead of keeeping it to nr_iovecs which user requested? > (in bio_alloc_bioset) Because we limit the user to the number that the user requested. Not that this patch changes anything about that. > 3. Could you please help understand why for cloned bio we still allow > __bio_add_page to work? why not WARN and return like in original code? It doesn't work, and I have now added the WARN_ON to deal with any incorrect usage. >> - if (bio->bi_vcnt >= bio->bi_max_vecs) >> - return 0; > Originally here we were supposed to return and not proceed further. > Should __bio_add_page not have similar checks to safeguard crossing > the bio_io_vec[] boundary? No, __bio_add_page is the "I know what I am doing" interface.
On 5/16/2018 11:35 PM, Christoph Hellwig wrote: > On Wed, May 16, 2018 at 10:36:14AM +0530, Ritesh Harjani wrote: >> 1. if bio_full is true that means no space in bio->bio_io_vec[] no? >> Than how come we are still proceeding ahead with only warning? >> While originally in bio_add_page we used to return after checking >> bio_full. Callers can still call __bio_add_page directly right. > > I you don't know if the bio is full or not don't use __bio_add_page, > keep using bio_add_page. The WARN_ON is just a debug tool to catch > cases where the developer did use it incorrectly. > >> 2. IF above is correct why don't we set the bio->bi_max_vecs to the size >> of the slab instead of keeeping it to nr_iovecs which user requested? >> (in bio_alloc_bioset) > > Because we limit the user to the number that the user requested. Not > that this patch changes anything about that. > >> 3. Could you please help understand why for cloned bio we still allow >> __bio_add_page to work? why not WARN and return like in original code? > > It doesn't work, and I have now added the WARN_ON to deal with any > incorrect usage. > >>> - if (bio->bi_vcnt >= bio->bi_max_vecs) >>> - return 0; >> Originally here we were supposed to return and not proceed further. >> Should __bio_add_page not have similar checks to safeguard crossing >> the bio_io_vec[] boundary? > > No, __bio_add_page is the "I know what I am doing" interface. > Thanks for explaining. I guess I missed reading the comment made on top of function __bio_add_page. "The caller must ensure that @bio has space for another bvec" This discussion helped me understand a bit about bios & bio_vec. Thanks!! Ritesh
diff --git a/block/bio.c b/block/bio.c index 53e0f0a1ed94..6ceba6adbf42 100644 --- a/block/bio.c +++ b/block/bio.c @@ -773,7 +773,7 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page return 0; } - if (bio->bi_vcnt >= bio->bi_max_vecs) + if (bio_full(bio)) return 0; /* @@ -820,6 +820,59 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page } EXPORT_SYMBOL(bio_add_pc_page); +/** + * __bio_try_merge_page - try adding data to an existing bvec + * @bio: destination bio + * @page: page to add + * @len: length of the range to add + * @off: offset into @page + * + * Try adding the data described at @page + @offset to the last bvec of @bio. + * Return %true on success or %false on failure. This can happen frequently + * for file systems with a block size smaller than the page size. + */ +bool __bio_try_merge_page(struct bio *bio, struct page *page, + unsigned int len, unsigned int off) +{ + if (bio->bi_vcnt > 0) { + struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1]; + + if (page == bv->bv_page && off == bv->bv_offset + bv->bv_len) { + bv->bv_len += len; + bio->bi_iter.bi_size += len; + return true; + } + } + return false; +} +EXPORT_SYMBOL_GPL(__bio_try_merge_page); + +/** + * __bio_add_page - add page to a bio in a new segment + * @bio: destination bio + * @page: page to add + * @len: length of the range to add + * @off: offset into @page + * + * Add the data at @page + @offset to @bio as a new bvec. The caller must + * ensure that @bio has space for another bvec. + */ +void __bio_add_page(struct bio *bio, struct page *page, + unsigned int len, unsigned int off) +{ + struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt]; + + WARN_ON_ONCE(bio_full(bio)); + + bv->bv_page = page; + bv->bv_offset = off; + bv->bv_len = len; + + bio->bi_iter.bi_size += len; + bio->bi_vcnt++; +} +EXPORT_SYMBOL_GPL(__bio_add_page); + /** * bio_add_page - attempt to add page to bio * @bio: destination bio @@ -833,40 +886,16 @@ EXPORT_SYMBOL(bio_add_pc_page); int bio_add_page(struct bio *bio, struct page *page, unsigned int len, unsigned int offset) { - struct bio_vec *bv; - /* * cloned bio must not modify vec list */ if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED))) return 0; - - /* - * For filesystems with a blocksize smaller than the pagesize - * we will often be called with the same page as last time and - * a consecutive offset. Optimize this special case. - */ - if (bio->bi_vcnt > 0) { - bv = &bio->bi_io_vec[bio->bi_vcnt - 1]; - - if (page == bv->bv_page && - offset == bv->bv_offset + bv->bv_len) { - bv->bv_len += len; - goto done; - } + if (!__bio_try_merge_page(bio, page, len, offset)) { + if (bio_full(bio)) + return 0; + __bio_add_page(bio, page, len, offset); } - - if (bio->bi_vcnt >= bio->bi_max_vecs) - return 0; - - bv = &bio->bi_io_vec[bio->bi_vcnt]; - bv->bv_page = page; - bv->bv_len = len; - bv->bv_offset = offset; - - bio->bi_vcnt++; -done: - bio->bi_iter.bi_size += len; return len; } EXPORT_SYMBOL(bio_add_page); diff --git a/include/linux/bio.h b/include/linux/bio.h index ce547a25e8ae..3e73c8bc25ea 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -123,6 +123,11 @@ static inline void *bio_data(struct bio *bio) return NULL; } +static inline bool bio_full(struct bio *bio) +{ + return bio->bi_vcnt >= bio->bi_max_vecs; +} + /* * will die */ @@ -470,6 +475,10 @@ void bio_chain(struct bio *, struct bio *); extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int); extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *, unsigned int, unsigned int); +bool __bio_try_merge_page(struct bio *bio, struct page *page, + unsigned int len, unsigned int off); +void __bio_add_page(struct bio *bio, struct page *page, + unsigned int len, unsigned int off); int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter); struct rq_map_data; extern struct bio *bio_map_user_iov(struct request_queue *,
For the upcoming removal of buffer heads in XFS we need to keep track of the number of outstanding writeback requests per page. For this we need to know if bio_add_page merged a region with the previous bvec or not. Instead of adding additional arguments this refactors bio_add_page to be implemented using three lower level helpers which users like XFS can use directly if they care about the merge decisions. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bio.c | 87 ++++++++++++++++++++++++++++++--------------- include/linux/bio.h | 9 +++++ 2 files changed, 67 insertions(+), 29 deletions(-)