[PATCH] btrfs: Convert add_ra_bio_pages() to use a folio

[PATCH] btrfs: Convert add_ra_bio_pages() to use a folio

[Matthew Wilcox (Oracle)]

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(-)

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

Re: [PATCH] btrfs: Convert add_ra_bio_pages() to use a folio

[Johannes Thumshirn]

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;

Re: [PATCH] btrfs: Convert add_ra_bio_pages() to use a folio

[Matthew Wilcox]

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.

Re: [PATCH] btrfs: Convert add_ra_bio_pages() to use a folio

[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.

> 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
> 

Re: [PATCH] btrfs: Convert add_ra_bio_pages() to use a folio

[Qu Wenruo]

在 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
>>