| 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 |
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.
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 ?
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 --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);