| Message ID | 20240126065631.3055974-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Convert add_ra_bio_pages() to use a folio | expand |
On 26.01.24 09:53, Matthew Wilcox (Oracle) wrote: > Allocate order-0 folios instead of pages. Saves twelve hidden calls > to compound_head(). Hey Willy, Do you have some kind of "Reviewer Documentation" for folio conversion patches? I'd really like to review this one and the others Qu sent out, but I fear my knowledge of folios is only scratching the surface. Thanks a lot, Johannes > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/btrfs/compression.c | 58 ++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 30 deletions(-) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index 68345f73d429..517f9bc58749 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -421,7 +421,6 @@ static noinline int add_ra_bio_pages(struct inode *inode, > u64 cur = cb->orig_bbio->file_offset + orig_bio->bi_iter.bi_size; > u64 isize = i_size_read(inode); > int ret; > - struct page *page; > struct extent_map *em; > struct address_space *mapping = inode->i_mapping; > struct extent_map_tree *em_tree; > @@ -447,6 +446,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, > end_index = (i_size_read(inode) - 1) >> PAGE_SHIFT; > > while (cur < compressed_end) { > + struct folio *folio; > u64 page_end; > u64 pg_index = cur >> PAGE_SHIFT; > u32 add_size; > @@ -454,8 +454,12 @@ static noinline int add_ra_bio_pages(struct inode *inode, > if (pg_index > end_index) > break; > > - page = xa_load(&mapping->i_pages, pg_index); > - if (page && !xa_is_value(page)) { > + folio = xa_load(&mapping->i_pages, pg_index); > + if (folio && !xa_is_value(folio)) { > + /* > + * We don't have a reference count on the folio, > + * so it is unsafe to refer to folio_size() > + */ > sectors_missed += (PAGE_SIZE - offset_in_page(cur)) >> > fs_info->sectorsize_bits; > > @@ -471,38 +475,38 @@ static noinline int add_ra_bio_pages(struct inode *inode, > continue; > } > > - page = __page_cache_alloc(mapping_gfp_constraint(mapping, > - ~__GFP_FS)); > - if (!page) > + folio = filemap_alloc_folio(mapping_gfp_constraint(mapping, > + ~__GFP_FS), 0); > + if (!folio) > break; > > - if (add_to_page_cache_lru(page, mapping, pg_index, GFP_NOFS)) { > - put_page(page); > + if (filemap_add_folio(mapping, folio, pg_index, GFP_NOFS)) { > + folio_put(folio); > /* There is already a page, skip to page end */ > cur = (pg_index << PAGE_SHIFT) + PAGE_SIZE; > continue; > } > > - if (!*memstall && PageWorkingset(page)) { > + if (!*memstall && folio_test_workingset(folio)) { > psi_memstall_enter(pflags); > *memstall = 1; > } > > - ret = set_page_extent_mapped(page); > + ret = set_folio_extent_mapped(folio); > if (ret < 0) { > - unlock_page(page); > - put_page(page); > + folio_unlock(folio); > + folio_put(folio); > break; > } > > - page_end = (pg_index << PAGE_SHIFT) + PAGE_SIZE - 1; > + page_end = folio_pos(folio) + folio_size(folio) - 1; > lock_extent(tree, cur, page_end, NULL); > read_lock(&em_tree->lock); > em = lookup_extent_mapping(em_tree, cur, page_end + 1 - cur); > read_unlock(&em_tree->lock); > > /* > - * At this point, we have a locked page in the page cache for > + * At this point, we have a locked folio in the page cache for > * these bytes in the file. But, we have to make sure they map > * to this compressed extent on disk. > */ > @@ -511,28 +515,22 @@ static noinline int add_ra_bio_pages(struct inode *inode, > (em->block_start >> SECTOR_SHIFT) != orig_bio->bi_iter.bi_sector) { > free_extent_map(em); > unlock_extent(tree, cur, page_end, NULL); > - unlock_page(page); > - put_page(page); > + folio_unlock(folio); > + folio_put(folio); > break; > } > free_extent_map(em); > > - if (page->index == end_index) { > - size_t zero_offset = offset_in_page(isize); > - > - if (zero_offset) { > - int zeros; > - zeros = PAGE_SIZE - zero_offset; > - memzero_page(page, zero_offset, zeros); > - } > - } > + if (folio->index == end_index) > + folio_zero_segment(folio, offset_in_page(isize), > + folio_size(folio)); > > add_size = min(em->start + em->len, page_end + 1) - cur; > - ret = bio_add_page(orig_bio, page, add_size, offset_in_page(cur)); > + ret = bio_add_folio(orig_bio, folio, add_size, offset_in_page(cur)); > if (ret != add_size) { > unlock_extent(tree, cur, page_end, NULL); > - unlock_page(page); > - put_page(page); > + folio_unlock(folio); > + folio_put(folio); > break; > } > /* > @@ -541,9 +539,9 @@ static noinline int add_ra_bio_pages(struct inode *inode, > * subpage::readers and to unlock the page. > */ > if (fs_info->sectorsize < PAGE_SIZE) > - btrfs_subpage_start_reader(fs_info, page_folio(page), > + btrfs_subpage_start_reader(fs_info, folio, > cur, add_size); > - put_page(page); > + folio_put(folio); > cur += add_size; > } > return 0;
On Mon, Jan 29, 2024 at 02:55:23PM +0000, Johannes Thumshirn wrote: > On 26.01.24 09:53, Matthew Wilcox (Oracle) wrote: > > Allocate order-0 folios instead of pages. Saves twelve hidden calls > > to compound_head(). > > Hey Willy, > > Do you have some kind of "Reviewer Documentation" for folio conversion > patches? > > I'd really like to review this one and the others Qu sent out, but I > fear my knowledge of folios is only scratching the surface. Sorry for the long delay in response. You deserve a better response than this, but ... I don't have one. Partly because the things you need to be concerned about are very different between filesystem patches and mm patches. In filesystems, it's going to depend on whether the filesystem intends to support large folios; obviously btrfs does, but many filesystems (eg adfs, efs, iso9660) will not, because the reward will not be worth the risk. Essentially you're asking for me to predict what mistakes other people are going to make, and that's a tall order. I can barely predict what mistakes I'm going to make ;-) So we've seen the mistake of shifting file position by folio_shift(), which is honestly a new one to me. I've never seen it before. Something that's worth commenting on is if there are unnecessary conversions from folios back to pages, eg lock_page(&folio->page) instead of folio_lock(folio). Really any conversion back from folio to page is an indication of a problem. It might be necessary as a temporary step, but we _should_ have enough infrastructure in place now that filesystems don't need to convert back to pages at any point. Although there's still write_begin/write_end and some of the functions in buffer.c.
On Fri, Jan 26, 2024 at 06:56:29AM +0000, Matthew Wilcox (Oracle) wrote: > Allocate order-0 folios instead of pages. Saves twelve hidden calls > to compound_head(). Ping? This is one of the few remaining callers of add_to_page_cache_lru() and I'd like to get rid of it soon. > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/btrfs/compression.c | 58 ++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 30 deletions(-) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index 68345f73d429..517f9bc58749 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -421,7 +421,6 @@ static noinline int add_ra_bio_pages(struct inode *inode, > u64 cur = cb->orig_bbio->file_offset + orig_bio->bi_iter.bi_size; > u64 isize = i_size_read(inode); > int ret; > - struct page *page; > struct extent_map *em; > struct address_space *mapping = inode->i_mapping; > struct extent_map_tree *em_tree; > @@ -447,6 +446,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, > end_index = (i_size_read(inode) - 1) >> PAGE_SHIFT; > > while (cur < compressed_end) { > + struct folio *folio; > u64 page_end; > u64 pg_index = cur >> PAGE_SHIFT; > u32 add_size; > @@ -454,8 +454,12 @@ static noinline int add_ra_bio_pages(struct inode *inode, > if (pg_index > end_index) > break; > > - page = xa_load(&mapping->i_pages, pg_index); > - if (page && !xa_is_value(page)) { > + folio = xa_load(&mapping->i_pages, pg_index); > + if (folio && !xa_is_value(folio)) { > + /* > + * We don't have a reference count on the folio, > + * so it is unsafe to refer to folio_size() > + */ > sectors_missed += (PAGE_SIZE - offset_in_page(cur)) >> > fs_info->sectorsize_bits; > > @@ -471,38 +475,38 @@ static noinline int add_ra_bio_pages(struct inode *inode, > continue; > } > > - page = __page_cache_alloc(mapping_gfp_constraint(mapping, > - ~__GFP_FS)); > - if (!page) > + folio = filemap_alloc_folio(mapping_gfp_constraint(mapping, > + ~__GFP_FS), 0); > + if (!folio) > break; > > - if (add_to_page_cache_lru(page, mapping, pg_index, GFP_NOFS)) { > - put_page(page); > + if (filemap_add_folio(mapping, folio, pg_index, GFP_NOFS)) { > + folio_put(folio); > /* There is already a page, skip to page end */ > cur = (pg_index << PAGE_SHIFT) + PAGE_SIZE; > continue; > } > > - if (!*memstall && PageWorkingset(page)) { > + if (!*memstall && folio_test_workingset(folio)) { > psi_memstall_enter(pflags); > *memstall = 1; > } > > - ret = set_page_extent_mapped(page); > + ret = set_folio_extent_mapped(folio); > if (ret < 0) { > - unlock_page(page); > - put_page(page); > + folio_unlock(folio); > + folio_put(folio); > break; > } > > - page_end = (pg_index << PAGE_SHIFT) + PAGE_SIZE - 1; > + page_end = folio_pos(folio) + folio_size(folio) - 1; > lock_extent(tree, cur, page_end, NULL); > read_lock(&em_tree->lock); > em = lookup_extent_mapping(em_tree, cur, page_end + 1 - cur); > read_unlock(&em_tree->lock); > > /* > - * At this point, we have a locked page in the page cache for > + * At this point, we have a locked folio in the page cache for > * these bytes in the file. But, we have to make sure they map > * to this compressed extent on disk. > */ > @@ -511,28 +515,22 @@ static noinline int add_ra_bio_pages(struct inode *inode, > (em->block_start >> SECTOR_SHIFT) != orig_bio->bi_iter.bi_sector) { > free_extent_map(em); > unlock_extent(tree, cur, page_end, NULL); > - unlock_page(page); > - put_page(page); > + folio_unlock(folio); > + folio_put(folio); > break; > } > free_extent_map(em); > > - if (page->index == end_index) { > - size_t zero_offset = offset_in_page(isize); > - > - if (zero_offset) { > - int zeros; > - zeros = PAGE_SIZE - zero_offset; > - memzero_page(page, zero_offset, zeros); > - } > - } > + if (folio->index == end_index) > + folio_zero_segment(folio, offset_in_page(isize), > + folio_size(folio)); > > add_size = min(em->start + em->len, page_end + 1) - cur; > - ret = bio_add_page(orig_bio, page, add_size, offset_in_page(cur)); > + ret = bio_add_folio(orig_bio, folio, add_size, offset_in_page(cur)); > if (ret != add_size) { > unlock_extent(tree, cur, page_end, NULL); > - unlock_page(page); > - put_page(page); > + folio_unlock(folio); > + folio_put(folio); > break; > } > /* > @@ -541,9 +539,9 @@ static noinline int add_ra_bio_pages(struct inode *inode, > * subpage::readers and to unlock the page. > */ > if (fs_info->sectorsize < PAGE_SIZE) > - btrfs_subpage_start_reader(fs_info, page_folio(page), > + btrfs_subpage_start_reader(fs_info, folio, > cur, add_size); > - put_page(page); > + folio_put(folio); > cur += add_size; > } > return 0; > -- > 2.43.0 >
在 2024/2/28 08:05, Matthew Wilcox 写道: > On Fri, Jan 26, 2024 at 06:56:29AM +0000, Matthew Wilcox (Oracle) wrote: >> Allocate order-0 folios instead of pages. Saves twelve hidden calls >> to compound_head(). > > Ping? This is one of the few remaining callers of > add_to_page_cache_lru() and I'd like to get rid of it soon. Not an expert thus I can only do some basic reviews here. > >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >> --- >> fs/btrfs/compression.c | 58 ++++++++++++++++++++---------------------- >> 1 file changed, 28 insertions(+), 30 deletions(-) >> >> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c >> index 68345f73d429..517f9bc58749 100644 >> --- a/fs/btrfs/compression.c >> +++ b/fs/btrfs/compression.c >> @@ -421,7 +421,6 @@ static noinline int add_ra_bio_pages(struct inode *inode, >> u64 cur = cb->orig_bbio->file_offset + orig_bio->bi_iter.bi_size; >> u64 isize = i_size_read(inode); >> int ret; >> - struct page *page; >> struct extent_map *em; >> struct address_space *mapping = inode->i_mapping; >> struct extent_map_tree *em_tree; >> @@ -447,6 +446,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, >> end_index = (i_size_read(inode) - 1) >> PAGE_SHIFT; >> >> while (cur < compressed_end) { >> + struct folio *folio; >> u64 page_end; >> u64 pg_index = cur >> PAGE_SHIFT; >> u32 add_size; >> @@ -454,8 +454,12 @@ static noinline int add_ra_bio_pages(struct inode *inode, >> if (pg_index > end_index) >> break; >> >> - page = xa_load(&mapping->i_pages, pg_index); >> - if (page && !xa_is_value(page)) { >> + folio = xa_load(&mapping->i_pages, pg_index); Not familiar with the xa_load usage, does this mean for order-0 pages, folio and page pointers are interchangeable? And what if we go higher order folios in the futuer? Would that still be interchangeable? [...] >> } >> free_extent_map(em); >> >> - if (page->index == end_index) { >> - size_t zero_offset = offset_in_page(isize); >> - >> - if (zero_offset) { >> - int zeros; >> - zeros = PAGE_SIZE - zero_offset; >> - memzero_page(page, zero_offset, zeros); >> - } >> - } >> + if (folio->index == end_index) >> + folio_zero_segment(folio, offset_in_page(isize), >> + folio_size(folio)); This doesn't sound correct to me. If @isize is page aligned, e.g. 4K, and we're the first folio of the page cache, this would zero the first folio, meanwhile the old code would do nothing. Thanks, Qu >> >> add_size = min(em->start + em->len, page_end + 1) - cur; >> - ret = bio_add_page(orig_bio, page, add_size, offset_in_page(cur)); >> + ret = bio_add_folio(orig_bio, folio, add_size, offset_in_page(cur)); >> if (ret != add_size) { >> unlock_extent(tree, cur, page_end, NULL); >> - unlock_page(page); >> - put_page(page); >> + folio_unlock(folio); >> + folio_put(folio); >> break; >> } >> /* >> @@ -541,9 +539,9 @@ static noinline int add_ra_bio_pages(struct inode *inode, >> * subpage::readers and to unlock the page. >> */ >> if (fs_info->sectorsize < PAGE_SIZE) >> - btrfs_subpage_start_reader(fs_info, page_folio(page), >> + btrfs_subpage_start_reader(fs_info, folio, >> cur, add_size); >> - put_page(page); >> + folio_put(folio); >> cur += add_size; >> } >> return 0; >> -- >> 2.43.0 >>
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 68345f73d429..517f9bc58749 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -421,7 +421,6 @@ static noinline int add_ra_bio_pages(struct inode *inode, u64 cur = cb->orig_bbio->file_offset + orig_bio->bi_iter.bi_size; u64 isize = i_size_read(inode); int ret; - struct page *page; struct extent_map *em; struct address_space *mapping = inode->i_mapping; struct extent_map_tree *em_tree; @@ -447,6 +446,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, end_index = (i_size_read(inode) - 1) >> PAGE_SHIFT; while (cur < compressed_end) { + struct folio *folio; u64 page_end; u64 pg_index = cur >> PAGE_SHIFT; u32 add_size; @@ -454,8 +454,12 @@ static noinline int add_ra_bio_pages(struct inode *inode, if (pg_index > end_index) break; - page = xa_load(&mapping->i_pages, pg_index); - if (page && !xa_is_value(page)) { + folio = xa_load(&mapping->i_pages, pg_index); + if (folio && !xa_is_value(folio)) { + /* + * We don't have a reference count on the folio, + * so it is unsafe to refer to folio_size() + */ sectors_missed += (PAGE_SIZE - offset_in_page(cur)) >> fs_info->sectorsize_bits; @@ -471,38 +475,38 @@ static noinline int add_ra_bio_pages(struct inode *inode, continue; } - page = __page_cache_alloc(mapping_gfp_constraint(mapping, - ~__GFP_FS)); - if (!page) + folio = filemap_alloc_folio(mapping_gfp_constraint(mapping, + ~__GFP_FS), 0); + if (!folio) break; - if (add_to_page_cache_lru(page, mapping, pg_index, GFP_NOFS)) { - put_page(page); + if (filemap_add_folio(mapping, folio, pg_index, GFP_NOFS)) { + folio_put(folio); /* There is already a page, skip to page end */ cur = (pg_index << PAGE_SHIFT) + PAGE_SIZE; continue; } - if (!*memstall && PageWorkingset(page)) { + if (!*memstall && folio_test_workingset(folio)) { psi_memstall_enter(pflags); *memstall = 1; } - ret = set_page_extent_mapped(page); + ret = set_folio_extent_mapped(folio); if (ret < 0) { - unlock_page(page); - put_page(page); + folio_unlock(folio); + folio_put(folio); break; } - page_end = (pg_index << PAGE_SHIFT) + PAGE_SIZE - 1; + page_end = folio_pos(folio) + folio_size(folio) - 1; lock_extent(tree, cur, page_end, NULL); read_lock(&em_tree->lock); em = lookup_extent_mapping(em_tree, cur, page_end + 1 - cur); read_unlock(&em_tree->lock); /* - * At this point, we have a locked page in the page cache for + * At this point, we have a locked folio in the page cache for * these bytes in the file. But, we have to make sure they map * to this compressed extent on disk. */ @@ -511,28 +515,22 @@ static noinline int add_ra_bio_pages(struct inode *inode, (em->block_start >> SECTOR_SHIFT) != orig_bio->bi_iter.bi_sector) { free_extent_map(em); unlock_extent(tree, cur, page_end, NULL); - unlock_page(page); - put_page(page); + folio_unlock(folio); + folio_put(folio); break; } free_extent_map(em); - if (page->index == end_index) { - size_t zero_offset = offset_in_page(isize); - - if (zero_offset) { - int zeros; - zeros = PAGE_SIZE - zero_offset; - memzero_page(page, zero_offset, zeros); - } - } + if (folio->index == end_index) + folio_zero_segment(folio, offset_in_page(isize), + folio_size(folio)); add_size = min(em->start + em->len, page_end + 1) - cur; - ret = bio_add_page(orig_bio, page, add_size, offset_in_page(cur)); + ret = bio_add_folio(orig_bio, folio, add_size, offset_in_page(cur)); if (ret != add_size) { unlock_extent(tree, cur, page_end, NULL); - unlock_page(page); - put_page(page); + folio_unlock(folio); + folio_put(folio); break; } /* @@ -541,9 +539,9 @@ static noinline int add_ra_bio_pages(struct inode *inode, * subpage::readers and to unlock the page. */ if (fs_info->sectorsize < PAGE_SIZE) - btrfs_subpage_start_reader(fs_info, page_folio(page), + btrfs_subpage_start_reader(fs_info, folio, cur, add_size); - put_page(page); + folio_put(folio); cur += add_size; } return 0;
Allocate order-0 folios instead of pages. Saves twelve hidden calls to compound_head(). Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/btrfs/compression.c | 58 ++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 30 deletions(-)