bio_add_page() and __bio_add_page() are capable of adding pages into bio, and now we have at least two such usages alreay: - __bio_iov_bvec_add_pages() - nvmet_bdev_execute_rw(). So update comments on these two helpers. The thing is a bit special for __bio_try_merge_page(), given the caller needs to know if the new added page is same with the last added page, then it isn't safe to pass multi-page in case that 'same_page' is true, so adds warning on potential misuse, and updates comment on __bio_try_merge_page(). Cc: linux-xfs@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: Christoph Hellwig <hch@infradead.org> Reviewed-by: Christoph Hellwig <hch@infradead.org> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- V2: - add Reviewed-by tag - rebase on latest for-5.2/block block/bio.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/block/bio.c b/block/bio.c index 5959141d4e46..e92c37ce20a6 100644 --- a/block/bio.c +++ b/block/bio.c @@ -688,6 +688,8 @@ static bool can_add_page_to_seg(struct request_queue *q, if (bv->bv_len + len > queue_max_segment_size(q)) return false; + WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE); + return true; } @@ -786,9 +788,9 @@ EXPORT_SYMBOL(bio_add_pc_page); /** * __bio_try_merge_page - try appending data to an existing bvec. * @bio: destination bio - * @page: page to add + * @page: start page to add * @len: length of the data to add - * @off: offset of the data in @page + * @off: offset of the data relative to @page * @same_page: if %true only merge if the new data is in the same physical * page as the last segment of the bio. * @@ -796,6 +798,8 @@ EXPORT_SYMBOL(bio_add_pc_page); * a useful optimisation for file systems with a block size smaller than the * page size. * + * Warn if (@len, @off) crosses pages in case that @same_page is true. + * * Return %true on success or %false on failure. */ bool __bio_try_merge_page(struct bio *bio, struct page *page, @@ -818,11 +822,11 @@ bool __bio_try_merge_page(struct bio *bio, struct page *page, EXPORT_SYMBOL_GPL(__bio_try_merge_page); /** - * __bio_add_page - add page to a bio in a new segment + * __bio_add_page - add page(s) 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 + * @page: start page to add + * @len: length of the data to add, may cross pages + * @off: offset of the data relative to @page, may cross pages * * Add the data at @page + @off to @bio as a new bvec. The caller must ensure * that @bio has space for another bvec. @@ -845,13 +849,13 @@ void __bio_add_page(struct bio *bio, struct page *page, EXPORT_SYMBOL_GPL(__bio_add_page); /** - * bio_add_page - attempt to add page to bio + * bio_add_page - attempt to add page(s) to bio * @bio: destination bio - * @page: page to add - * @len: vec entry length - * @offset: vec entry offset + * @page: start page to add + * @len: vec entry length, may cross pages + * @offset: vec entry offset relative to @page, may cross pages * - * Attempt to add a page to the bio_vec maplist. This will only fail + * Attempt to add page(s) to the bio_vec maplist. This will only fail * if either bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio. */ int bio_add_page(struct bio *bio, struct page *page, -- 2.9.5
[-- Attachment #1: Type: text/plain, Size: 3163 bytes --] Hi Ming, Thank you for the patch! Yet something to improve: [auto build test ERROR on block/for-next] [also build test ERROR on next-20190418] [cannot apply to v5.1-rc6] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Ming-Lei/block-clarify-that-bio_add_page-and-related-helpers-can-add-multi-pages/20190423-010505 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next config: x86_64-randconfig-x015-201916 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from arch/x86/include/asm/bug.h:83:0, from include/linux/bug.h:5, from include/linux/mmdebug.h:5, from include/linux/mm.h:9, from block/bio.c:18: block/bio.c: In function 'can_add_page_to_seg': >> block/bio.c:691:15: error: 'same_page' undeclared (first use in this function); did you mean 'shake_page'? WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE); ^ include/asm-generic/bug.h:69:25: note: in definition of macro 'WARN_ON_ONCE' int __ret_warn_on = !!(condition); \ ^~~~~~~~~ block/bio.c:691:15: note: each undeclared identifier is reported only once for each function it appears in WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE); ^ include/asm-generic/bug.h:69:25: note: in definition of macro 'WARN_ON_ONCE' int __ret_warn_on = !!(condition); \ ^~~~~~~~~ >> block/bio.c:691:35: error: 'off' undeclared (first use in this function) WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE); ^ include/asm-generic/bug.h:69:25: note: in definition of macro 'WARN_ON_ONCE' int __ret_warn_on = !!(condition); \ ^~~~~~~~~ vim +691 block/bio.c 672 673 /* 674 * Check if the @page can be added to the current segment(@bv), and make 675 * sure to call it only if page_is_mergeable(@bv, @page) is true 676 */ 677 static bool can_add_page_to_seg(struct request_queue *q, 678 struct bio_vec *bv, struct page *page, unsigned len, 679 unsigned offset) 680 { 681 unsigned long mask = queue_segment_boundary(q); 682 phys_addr_t addr1 = page_to_phys(bv->bv_page) + bv->bv_offset; 683 phys_addr_t addr2 = page_to_phys(page) + offset + len - 1; 684 685 if ((addr1 | mask) != (addr2 | mask)) 686 return false; 687 688 if (bv->bv_len + len > queue_max_segment_size(q)) 689 return false; 690 > 691 WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE); 692 693 return true; 694 } 695 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 30526 bytes --]
On Mon, Apr 22, 2019 at 06:02:07PM +0800, Ming Lei wrote:
> bio_add_page() and __bio_add_page() are capable of adding pages into
> bio, and now we have at least two such usages alreay:
>
> - __bio_iov_bvec_add_pages()
> - nvmet_bdev_execute_rw().
>
> So update comments on these two helpers.
>
> The thing is a bit special for __bio_try_merge_page(), given the caller
> needs to know if the new added page is same with the last added page,
> then it isn't safe to pass multi-page in case that 'same_page' is true,
> so adds warning on potential misuse, and updates comment on
> __bio_try_merge_page().
>
> Cc: linux-xfs@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: Christoph Hellwig <hch@infradead.org>
> Reviewed-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> V2:
> - add Reviewed-by tag
> - rebase on latest for-5.2/block
>
> block/bio.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 5959141d4e46..e92c37ce20a6 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -688,6 +688,8 @@ static bool can_add_page_to_seg(struct request_queue *q,
> if (bv->bv_len + len > queue_max_segment_size(q))
> return false;
>
> + WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE);
No need for the inner braces.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>