diff mbox series

[v8,1/5] block: Added folio-ized version of bvec_try_merge_hw_page()

Message ID 20240711050750.17792-2-kundan.kumar@samsung.com (mailing list archive)
State New, archived
Headers show
Series block: add larger order folio instead of pages | expand

Commit Message

Kundan Kumar July 11, 2024, 5:07 a.m. UTC 
Added new function bvec_try_merge_hw_folio(). This is a prep patch.

Signed-off-by: Kundan Kumar <kundan.kumar@samsung.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 27 +++++++++++++++++++++++----
 block/blk.h |  4 ++++
 2 files changed, 27 insertions(+), 4 deletions(-)

Comments

Matthew Wilcox Aug. 17, 2024, 4:23 a.m. UTC | #1 
On Thu, Jul 11, 2024 at 10:37:46AM +0530, Kundan Kumar wrote:
> -bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
> -		struct page *page, unsigned len, unsigned offset,
> +bool bvec_try_merge_hw_folio(struct request_queue *q, struct bio_vec *bv,
> +		struct folio *folio, size_t len, size_t offset,
>  		bool *same_page)
>  {
> +	struct page *page = folio_page(folio, 0);
>  	unsigned long mask = queue_segment_boundary(q);
>  	phys_addr_t addr1 = bvec_phys(bv);
>  	phys_addr_t addr2 = page_to_phys(page) + offset + len - 1;
[...]
> +bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
> +		struct page *page, unsigned int len, unsigned int offset,
> +		bool *same_page)
> +{
> +	struct folio *folio = page_folio(page);
> +
> +	return bvec_try_merge_hw_folio(q, bv, folio, len,
> +			((size_t)folio_page_idx(folio, page) << PAGE_SHIFT) +
> +			offset, same_page);
> +}

This is the wrong way to do it.  bio_add_folio() does it correctly
by being a wrapper around bio_add_page().

The reason is that in the future, not all pages will belong to folios.
For those pages, page_folio() will return NULL, and this will crash.
Kundan Kumar Aug. 20, 2024, 7:43 a.m. UTC | #2 
On 17/08/24 05:23AM, Matthew Wilcox wrote:
>On Thu, Jul 11, 2024 at 10:37:46AM +0530, Kundan Kumar wrote:
>> -bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
>> -		struct page *page, unsigned len, unsigned offset,
>> +bool bvec_try_merge_hw_folio(struct request_queue *q, struct bio_vec *bv,
>> +		struct folio *folio, size_t len, size_t offset,
>>  		bool *same_page)
>>  {
>> +	struct page *page = folio_page(folio, 0);
>>  	unsigned long mask = queue_segment_boundary(q);
>>  	phys_addr_t addr1 = bvec_phys(bv);
>>  	phys_addr_t addr2 = page_to_phys(page) + offset + len - 1;
>[...]
>> +bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
>> +		struct page *page, unsigned int len, unsigned int offset,
>> +		bool *same_page)
>> +{
>> +	struct folio *folio = page_folio(page);
>> +
>> +	return bvec_try_merge_hw_folio(q, bv, folio, len,
>> +			((size_t)folio_page_idx(folio, page) << PAGE_SHIFT) +
>> +			offset, same_page);
>> +}
>
>This is the wrong way to do it.  bio_add_folio() does it correctly
>by being a wrapper around bio_add_page().
>
>The reason is that in the future, not all pages will belong to folios.
>For those pages, page_folio() will return NULL, and this will crash.
>

I can change in this fashion. page_folio is getting used at many other
places in kernel and currently there are no NULL checks. Will every place
need a change?
In this series we use page_folio to fetch folio for pages returned by
iov_iter_extract_pages. Then we iterate on the folios instead of pages.
We were progressing to change all the page related functions to accept
struct folio.
If page_folio may return NULL in future, it will require us to maintain
both page and folio versions. Do you see it differently ?
Kundan Kumar Aug. 27, 2024, 8:22 a.m. UTC | #3 
On Tue, Aug 20, 2024 at 1:28 PM Kundan Kumar <kundan.kumar@samsung.com> wrote:
>
> On 17/08/24 05:23AM, Matthew Wilcox wrote:
> >On Thu, Jul 11, 2024 at 10:37:46AM +0530, Kundan Kumar wrote:
> >> -bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
> >> -            struct page *page, unsigned len, unsigned offset,
> >> +bool bvec_try_merge_hw_folio(struct request_queue *q, struct bio_vec *bv,
> >> +            struct folio *folio, size_t len, size_t offset,
> >>              bool *same_page)
> >>  {
> >> +    struct page *page = folio_page(folio, 0);
> >>      unsigned long mask = queue_segment_boundary(q);
> >>      phys_addr_t addr1 = bvec_phys(bv);
> >>      phys_addr_t addr2 = page_to_phys(page) + offset + len - 1;
> >[...]
> >> +bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
> >> +            struct page *page, unsigned int len, unsigned int offset,
> >> +            bool *same_page)
> >> +{
> >> +    struct folio *folio = page_folio(page);
> >> +
> >> +    return bvec_try_merge_hw_folio(q, bv, folio, len,
> >> +                    ((size_t)folio_page_idx(folio, page) << PAGE_SHIFT) +
> >> +                    offset, same_page);
> >> +}
> >
> >This is the wrong way to do it.  bio_add_folio() does it correctly
> >by being a wrapper around bio_add_page().
> >
> >The reason is that in the future, not all pages will belong to folios.
> >For those pages, page_folio() will return NULL, and this will crash.
> >
>
> I can change in this fashion. page_folio is getting used at many other
> places in kernel and currently there are no NULL checks. Will every place
> need a change?
> In this series we use page_folio to fetch folio for pages returned by
> iov_iter_extract_pages. Then we iterate on the folios instead of pages.
> We were progressing to change all the page related functions to accept
> struct folio.
> If page_folio may return NULL in future, it will require us to maintain
> both page and folio versions. Do you see it differently ?
>

Gentle ping. Any feedback on this ?
diff mbox series

Patch

diff --git a/block/bio.c b/block/bio.c
index c4053d49679a..7018eded8d7b 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -931,7 +931,9 @@  static bool bvec_try_merge_page(struct bio_vec *bv, struct page *page,
 	if (!zone_device_pages_have_same_pgmap(bv->bv_page, page))
 		return false;
 
-	*same_page = ((vec_end_addr & PAGE_MASK) == page_addr);
+	if (off < PAGE_SIZE)
+		*same_page = ((vec_end_addr & PAGE_MASK) == page_addr);
+
 	if (!*same_page) {
 		if (IS_ENABLED(CONFIG_KMSAN))
 			return false;
@@ -944,14 +946,15 @@  static bool bvec_try_merge_page(struct bio_vec *bv, struct page *page,
 }
 
 /*
- * Try to merge a page into a segment, while obeying the hardware segment
+ * Try to merge a folio into a segment, while obeying the hardware segment
  * size limit.  This is not for normal read/write bios, but for passthrough
  * or Zone Append operations that we can't split.
  */
-bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
-		struct page *page, unsigned len, unsigned offset,
+bool bvec_try_merge_hw_folio(struct request_queue *q, struct bio_vec *bv,
+		struct folio *folio, size_t len, size_t offset,
 		bool *same_page)
 {
+	struct page *page = folio_page(folio, 0);
 	unsigned long mask = queue_segment_boundary(q);
 	phys_addr_t addr1 = bvec_phys(bv);
 	phys_addr_t addr2 = page_to_phys(page) + offset + len - 1;
@@ -963,6 +966,22 @@  bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
 	return bvec_try_merge_page(bv, page, len, offset, same_page);
 }
 
+/*
+ * Try to merge a page into a segment, while obeying the hardware segment
+ * size limit.  This is not for normal read/write bios, but for passthrough
+ * or Zone Append operations that we can't split.
+ */
+bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
+		struct page *page, unsigned int len, unsigned int offset,
+		bool *same_page)
+{
+	struct folio *folio = page_folio(page);
+
+	return bvec_try_merge_hw_folio(q, bv, folio, len,
+			((size_t)folio_page_idx(folio, page) << PAGE_SHIFT) +
+			offset, same_page);
+}
+
 /**
  * bio_add_hw_page - attempt to add a page to a bio with hw constraints
  * @q: the target queue
diff --git a/block/blk.h b/block/blk.h
index e180863f918b..6a566b78a0a8 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -91,6 +91,10 @@  struct bio_vec *bvec_alloc(mempool_t *pool, unsigned short *nr_vecs,
 		gfp_t gfp_mask);
 void bvec_free(mempool_t *pool, struct bio_vec *bv, unsigned short nr_vecs);
 
+bool bvec_try_merge_hw_folio(struct request_queue *q, struct bio_vec *bv,
+		struct folio *folio, size_t len, size_t offset,
+		bool *same_page);
+
 bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
 		struct page *page, unsigned len, unsigned offset,
 		bool *same_page);