[PATCH 1/2] btrfs: Convert defrag_prepare_one_page() to use a folio

[PATCH 1/2] btrfs: Convert defrag_prepare_one_page() to use a folio

[Matthew Wilcox (Oracle)]

Use a folio throughout defrag_prepare_one_page() to remove dozens of
hidden calls to compound_head().  There is no support here for large
folios; indeed, turn the existing check for PageCompound into a check
for large folios.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/defrag.c | 53 ++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index a9a068af8d6e..17a13d3ed131 100644
--- a/fs/btrfs/defrag.c
+++ b/fs/btrfs/defrag.c
@@ -868,13 +868,14 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	u64 page_start = (u64)index << PAGE_SHIFT;
 	u64 page_end = page_start + PAGE_SIZE - 1;
 	struct extent_state *cached_state = NULL;
-	struct page *page;
+	struct folio *folio;
 	int ret;
 
 again:
-	page = find_or_create_page(mapping, index, mask);
-	if (!page)
-		return ERR_PTR(-ENOMEM);
+	folio = __filemap_get_folio(mapping, index,
+			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
+	if (IS_ERR(folio))
+		return &folio->page;
 
 	/*
 	 * Since we can defragment files opened read-only, we can encounter
@@ -884,16 +885,16 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	 * executables that explicitly enable them, so this isn't very
 	 * restrictive.
 	 */
-	if (PageCompound(page)) {
-		unlock_page(page);
-		put_page(page);
+	if (folio_test_large(folio)) {
+		folio_unlock(folio);
+		folio_put(folio);
 		return ERR_PTR(-ETXTBSY);
 	}
 
-	ret = set_page_extent_mapped(page);
+	ret = set_page_extent_mapped(&folio->page);
 	if (ret < 0) {
-		unlock_page(page);
-		put_page(page);
+		folio_unlock(folio);
+		folio_put(folio);
 		return ERR_PTR(ret);
 	}
 
@@ -908,17 +909,17 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 		if (!ordered)
 			break;
 
-		unlock_page(page);
+		folio_unlock(folio);
 		btrfs_start_ordered_extent(ordered);
 		btrfs_put_ordered_extent(ordered);
-		lock_page(page);
+		folio_lock(folio);
 		/*
-		 * We unlocked the page above, so we need check if it was
+		 * We unlocked the folio above, so we need check if it was
 		 * released or not.
 		 */
-		if (page->mapping != mapping || !PagePrivate(page)) {
-			unlock_page(page);
-			put_page(page);
+		if (folio->mapping != mapping || !folio->private) {
+			folio_unlock(folio);
+			folio_put(folio);
 			goto again;
 		}
 	}
@@ -927,21 +928,21 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	 * Now the page range has no ordered extent any more.  Read the page to
 	 * make it uptodate.
 	 */
-	if (!PageUptodate(page)) {
-		btrfs_read_folio(NULL, page_folio(page));
-		lock_page(page);
-		if (page->mapping != mapping || !PagePrivate(page)) {
-			unlock_page(page);
-			put_page(page);
+	if (!folio_test_uptodate(folio)) {
+		btrfs_read_folio(NULL, folio);
+		folio_lock(folio);
+		if (folio->mapping != mapping || !folio->private) {
+			folio_unlock(folio);
+			folio_put(folio);
 			goto again;
 		}
-		if (!PageUptodate(page)) {
-			unlock_page(page);
-			put_page(page);
+		if (!folio_test_uptodate(folio)) {
+			folio_unlock(folio);
+			folio_put(folio);
 			return ERR_PTR(-EIO);
 		}
 	}
-	return page;
+	return &folio->page;
 }
 
 struct defrag_target_range {
-- 
2.42.0

[PATCH 2/2] btrfs: Use a folio array throughout the defrag process

[Matthew Wilcox (Oracle)]

Remove more hidden calls to compound_head() by using an array of folios
instead of pages.  Also neaten the error path in defrag_one_range() by
adjusting the length of the array instead of checking for NULL.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/defrag.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index 17a13d3ed131..4b94362779fe 100644
--- a/fs/btrfs/defrag.c
+++ b/fs/btrfs/defrag.c
@@ -861,7 +861,7 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
  * NOTE: Caller should also wait for page writeback after the cluster is
  * prepared, here we don't do writeback wait for each page.
  */
-static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t index)
+static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t index)
 {
 	struct address_space *mapping = inode->vfs_inode.i_mapping;
 	gfp_t mask = btrfs_alloc_write_mask(mapping);
@@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	folio = __filemap_get_folio(mapping, index,
 			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
 	if (IS_ERR(folio))
-		return &folio->page;
+		return folio;
 
 	/*
 	 * Since we can defragment files opened read-only, we can encounter
@@ -942,7 +942,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 			return ERR_PTR(-EIO);
 		}
 	}
-	return &folio->page;
+	return folio;
 }
 
 struct defrag_target_range {
@@ -1163,7 +1163,7 @@ static_assert(PAGE_ALIGNED(CLUSTER_SIZE));
  */
 static int defrag_one_locked_target(struct btrfs_inode *inode,
 				    struct defrag_target_range *target,
-				    struct page **pages, int nr_pages,
+				    struct folio **folios, int nr_pages,
 				    struct extent_state **cached_state)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
@@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
 	const u64 len = target->len;
 	unsigned long last_index = (start + len - 1) >> PAGE_SHIFT;
 	unsigned long start_index = start >> PAGE_SHIFT;
-	unsigned long first_index = page_index(pages[0]);
+	unsigned long first_index = folios[0]->index;
 	int ret = 0;
 	int i;
 
@@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
 
 	/* Update the page status */
 	for (i = start_index - first_index; i <= last_index - first_index; i++) {
-		ClearPageChecked(pages[i]);
-		btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len);
+		folio_clear_checked(folios[i]);
+		btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len);
 	}
 	btrfs_delalloc_release_extents(inode, len);
 	extent_changeset_free(data_reserved);
@@ -1206,7 +1206,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 	struct defrag_target_range *entry;
 	struct defrag_target_range *tmp;
 	LIST_HEAD(target_list);
-	struct page **pages;
+	struct folio **folios;
 	const u32 sectorsize = inode->root->fs_info->sectorsize;
 	u64 last_index = (start + len - 1) >> PAGE_SHIFT;
 	u64 start_index = start >> PAGE_SHIFT;
@@ -1217,21 +1217,21 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 	ASSERT(nr_pages <= CLUSTER_SIZE / PAGE_SIZE);
 	ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(len, sectorsize));
 
-	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
-	if (!pages)
+	folios = kcalloc(nr_pages, sizeof(struct folio *), GFP_NOFS);
+	if (!folios)
 		return -ENOMEM;
 
 	/* Prepare all pages */
 	for (i = 0; i < nr_pages; i++) {
-		pages[i] = defrag_prepare_one_page(inode, start_index + i);
-		if (IS_ERR(pages[i])) {
-			ret = PTR_ERR(pages[i]);
-			pages[i] = NULL;
-			goto free_pages;
+		folios[i] = defrag_prepare_one_folio(inode, start_index + i);
+		if (IS_ERR(folios[i])) {
+			ret = PTR_ERR(folios[i]);
+			nr_pages = i;
+			goto free_folios;
 		}
 	}
 	for (i = 0; i < nr_pages; i++)
-		wait_on_page_writeback(pages[i]);
+		folio_wait_writeback(folios[i]);
 
 	/* Lock the pages range */
 	lock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
@@ -1251,7 +1251,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 		goto unlock_extent;
 
 	list_for_each_entry(entry, &target_list, list) {
-		ret = defrag_one_locked_target(inode, entry, pages, nr_pages,
+		ret = defrag_one_locked_target(inode, entry, folios, nr_pages,
 					       &cached_state);
 		if (ret < 0)
 			break;
@@ -1265,14 +1265,12 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 	unlock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
 		      (last_index << PAGE_SHIFT) + PAGE_SIZE - 1,
 		      &cached_state);
-free_pages:
+free_folios:
 	for (i = 0; i < nr_pages; i++) {
-		if (pages[i]) {
-			unlock_page(pages[i]);
-			put_page(pages[i]);
-		}
+		folio_unlock(folios[i]);
+		folio_put(folios[i]);
 	}
-	kfree(pages);
+	kfree(folios);
 	return ret;
 }
 
-- 
2.42.0

Re: [PATCH 2/2] btrfs: Use a folio array throughout the defrag process

[Qu Wenruo]

[-- Attachment #1.1.1: Type: text/plain, Size: 6207 bytes --]



On 2023/12/9 05:57, Matthew Wilcox (Oracle) wrote:
> Remove more hidden calls to compound_head() by using an array of folios
> instead of pages.  Also neaten the error path in defrag_one_range() by
> adjusting the length of the array instead of checking for NULL.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

This time only some unrelated questions below.
> ---
>   fs/btrfs/defrag.c | 44 +++++++++++++++++++++-----------------------
>   1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index 17a13d3ed131..4b94362779fe 100644
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -861,7 +861,7 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>    * NOTE: Caller should also wait for page writeback after the cluster is
>    * prepared, here we don't do writeback wait for each page.
>    */
> -static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t index)
> +static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t index)
>   {
>   	struct address_space *mapping = inode->vfs_inode.i_mapping;
>   	gfp_t mask = btrfs_alloc_write_mask(mapping);
> @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>   	folio = __filemap_get_folio(mapping, index,
>   			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
>   	if (IS_ERR(folio))
> -		return &folio->page;

What's the proper way to grab the first page of a folio?

For now, I'm going folio_page() and it's the same wrapper using 
folio->page, but I'm wondering would there be any recommendation for it.

> +		return folio;
>   
>   	/*
>   	 * Since we can defragment files opened read-only, we can encounter
> @@ -942,7 +942,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>   			return ERR_PTR(-EIO);
>   		}
>   	}
> -	return &folio->page;
> +	return folio;
>   }
>   
>   struct defrag_target_range {
> @@ -1163,7 +1163,7 @@ static_assert(PAGE_ALIGNED(CLUSTER_SIZE));
>    */
>   static int defrag_one_locked_target(struct btrfs_inode *inode,
>   				    struct defrag_target_range *target,
> -				    struct page **pages, int nr_pages,
> +				    struct folio **folios, int nr_pages,
>   				    struct extent_state **cached_state)
>   {
>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>   	const u64 len = target->len;
>   	unsigned long last_index = (start + len - 1) >> PAGE_SHIFT;
>   	unsigned long start_index = start >> PAGE_SHIFT;
> -	unsigned long first_index = page_index(pages[0]);
> +	unsigned long first_index = folios[0]->index;

The same for index, there is a folio_index() wrapper.

So should we go the wrapper or would the wrapper be gone in the future?
>   	int ret = 0;
>   	int i;
>   
> @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>   
>   	/* Update the page status */
>   	for (i = start_index - first_index; i <= last_index - first_index; i++) {
> -		ClearPageChecked(pages[i]);
> -		btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len);
> +		folio_clear_checked(folios[i]);
> +		btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len);

After my patch "2/3 btrfs: migrate subpage code to folio interfaces", 
btrfs_page_*() helpers accept folio parameter directly, thus this may 
lead to a conflicts.

Thanks,
Qu
>   	}
>   	btrfs_delalloc_release_extents(inode, len);
>   	extent_changeset_free(data_reserved);
> @@ -1206,7 +1206,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>   	struct defrag_target_range *entry;
>   	struct defrag_target_range *tmp;
>   	LIST_HEAD(target_list);
> -	struct page **pages;
> +	struct folio **folios;
>   	const u32 sectorsize = inode->root->fs_info->sectorsize;
>   	u64 last_index = (start + len - 1) >> PAGE_SHIFT;
>   	u64 start_index = start >> PAGE_SHIFT;
> @@ -1217,21 +1217,21 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>   	ASSERT(nr_pages <= CLUSTER_SIZE / PAGE_SIZE);
>   	ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(len, sectorsize));
>   
> -	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
> -	if (!pages)
> +	folios = kcalloc(nr_pages, sizeof(struct folio *), GFP_NOFS);
> +	if (!folios)
>   		return -ENOMEM;
>   
>   	/* Prepare all pages */
>   	for (i = 0; i < nr_pages; i++) {
> -		pages[i] = defrag_prepare_one_page(inode, start_index + i);
> -		if (IS_ERR(pages[i])) {
> -			ret = PTR_ERR(pages[i]);
> -			pages[i] = NULL;
> -			goto free_pages;
> +		folios[i] = defrag_prepare_one_folio(inode, start_index + i);
> +		if (IS_ERR(folios[i])) {
> +			ret = PTR_ERR(folios[i]);
> +			nr_pages = i;
> +			goto free_folios;
>   		}
>   	}
>   	for (i = 0; i < nr_pages; i++)
> -		wait_on_page_writeback(pages[i]);
> +		folio_wait_writeback(folios[i]);
>   
>   	/* Lock the pages range */
>   	lock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
> @@ -1251,7 +1251,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>   		goto unlock_extent;
>   
>   	list_for_each_entry(entry, &target_list, list) {
> -		ret = defrag_one_locked_target(inode, entry, pages, nr_pages,
> +		ret = defrag_one_locked_target(inode, entry, folios, nr_pages,
>   					       &cached_state);
>   		if (ret < 0)
>   			break;
> @@ -1265,14 +1265,12 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>   	unlock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
>   		      (last_index << PAGE_SHIFT) + PAGE_SIZE - 1,
>   		      &cached_state);
> -free_pages:
> +free_folios:
>   	for (i = 0; i < nr_pages; i++) {
> -		if (pages[i]) {
> -			unlock_page(pages[i]);
> -			put_page(pages[i]);
> -		}
> +		folio_unlock(folios[i]);
> +		folio_put(folios[i]);
>   	}
> -	kfree(pages);
> +	kfree(folios);
>   	return ret;
>   }
>   

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7027 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] btrfs: Use a folio array throughout the defrag process

[Matthew Wilcox]

On Sat, Dec 09, 2023 at 07:44:34AM +1030, Qu Wenruo wrote:
> > @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
> >   	folio = __filemap_get_folio(mapping, index,
> >   			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
> >   	if (IS_ERR(folio))
> > -		return &folio->page;
> 
> What's the proper way to grab the first page of a folio?

It depends why you're doing it.

If you're just doing it temporarily to convert back from folio to page
so you can make progress on the conversion, use &folio->page.  That
is a "bad code smell", but acceptable while you're doing the conversion.
It's a good signal "there is more work here to be done".

> For now, I'm going folio_page() and it's the same wrapper using folio->page,
> but I'm wondering would there be any recommendation for it.

folio_page() is good when you actually need a particular page.  eg you're
going to call kmap().  When we get to finally divorcing folio from page,
folio_page() will still work, and &folio->page won't.

> > @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
> >   	const u64 len = target->len;
> >   	unsigned long last_index = (start + len - 1) >> PAGE_SHIFT;
> >   	unsigned long start_index = start >> PAGE_SHIFT;
> > -	unsigned long first_index = page_index(pages[0]);
> > +	unsigned long first_index = folios[0]->index;
> 
> The same for index, there is a folio_index() wrapper.
> 
> So should we go the wrapper or would the wrapper be gone in the future?

folio_index() has a specific purpose (just like page_index() did, but it
seems like a lot of filesystem people never read the kernel-doc on it).
In short, filesystems never need to call folio_index(), nor page_index().
You can always just use folio->index unless you're in the direct-io path
(and you shouldn't be looking at folio_index() then either!).

> > @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
> >   	/* Update the page status */
> >   	for (i = start_index - first_index; i <= last_index - first_index; i++) {
> > -		ClearPageChecked(pages[i]);
> > -		btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len);
> > +		folio_clear_checked(folios[i]);
> > +		btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len);
> 
> After my patch "2/3 btrfs: migrate subpage code to folio interfaces",
> btrfs_page_*() helpers accept folio parameter directly, thus this may lead
> to a conflicts.

I built against linux-next 20231207; I thought all your folio changes
had landed there?  If you want to adopt these patches and land them
after yours, I am more than OK with that!

Re: [PATCH 2/2] btrfs: Use a folio array throughout the defrag process

[Qu Wenruo]

On 2023/12/9 07:54, Matthew Wilcox wrote:
> On Sat, Dec 09, 2023 at 07:44:34AM +1030, Qu Wenruo wrote:
>>> @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>>>    	folio = __filemap_get_folio(mapping, index,
>>>    			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
>>>    	if (IS_ERR(folio))
>>> -		return &folio->page;
>>
>> What's the proper way to grab the first page of a folio?
>
> It depends why you're doing it.
>
> If you're just doing it temporarily to convert back from folio to page
> so you can make progress on the conversion, use &folio->page.  That
> is a "bad code smell", but acceptable while you're doing the conversion.
> It's a good signal "there is more work here to be done".
>
>> For now, I'm going folio_page() and it's the same wrapper using folio->page,
>> but I'm wondering would there be any recommendation for it.
>
> folio_page() is good when you actually need a particular page.  eg you're
> going to call kmap().  When we get to finally divorcing folio from page,
> folio_page() will still work, and &folio->page won't.
>
>>> @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>>>    	const u64 len = target->len;
>>>    	unsigned long last_index = (start + len - 1) >> PAGE_SHIFT;
>>>    	unsigned long start_index = start >> PAGE_SHIFT;
>>> -	unsigned long first_index = page_index(pages[0]);
>>> +	unsigned long first_index = folios[0]->index;
>>
>> The same for index, there is a folio_index() wrapper.
>>
>> So should we go the wrapper or would the wrapper be gone in the future?
>
> folio_index() has a specific purpose (just like page_index() did, but it
> seems like a lot of filesystem people never read the kernel-doc on it).

Indeed I should check the code, it says very clearly:

  * If you know
  * the page is definitely in the page cache, you can look at the folio's
  * index directly.


> In short, filesystems never need to call folio_index(), nor page_index().

Yep, as when fs got the page they are mostly already mapped.

> You can always just use folio->index unless you're in the direct-io path
> (and you shouldn't be looking at folio_index() then either!).

And DIO pages are never mapped thus never call that.

>
>>> @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>>>    	/* Update the page status */
>>>    	for (i = start_index - first_index; i <= last_index - first_index; i++) {
>>> -		ClearPageChecked(pages[i]);
>>> -		btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len);
>>> +		folio_clear_checked(folios[i]);
>>> +		btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len);
>>
>> After my patch "2/3 btrfs: migrate subpage code to folio interfaces",
>> btrfs_page_*() helpers accept folio parameter directly, thus this may lead
>> to a conflicts.
>
> I built against linux-next 20231207; I thought all your folio changes
> had landed there?  If you want to adopt these patches and land them
> after yours, I am more than OK with that!

No big deal, David and I can definitely handle the conflicts, no need to
bother you.

Thanks a lot for the lesson on the folio interface best practice!
Qu

>

Re: [PATCH 1/2] btrfs: Convert defrag_prepare_one_page() to use a folio

[Qu Wenruo]

On 2023/12/9 05:57, Matthew Wilcox (Oracle) wrote:
> Use a folio throughout defrag_prepare_one_page() to remove dozens of
> hidden calls to compound_head().  There is no support here for large
> folios; indeed, turn the existing check for PageCompound into a check
> for large folios.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good to me.

Although some comments inlined below
> ---
>   fs/btrfs/defrag.c | 53 ++++++++++++++++++++++++-----------------------
>   1 file changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index a9a068af8d6e..17a13d3ed131 100644
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -868,13 +868,14 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>   	u64 page_start = (u64)index << PAGE_SHIFT;
>   	u64 page_end = page_start + PAGE_SIZE - 1;
>   	struct extent_state *cached_state = NULL;
> -	struct page *page;
> +	struct folio *folio;
>   	int ret;
>
>   again:
> -	page = find_or_create_page(mapping, index, mask);
> -	if (!page)
> -		return ERR_PTR(-ENOMEM);
> +	folio = __filemap_get_folio(mapping, index,
> +			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);

When I was (and still am) a newbie to the folio interfaces, the "__"
prefix is driving me away to use it.

Mind to change it in the future? Like adding a new
filemap_get_or_create_folio()?



> +	if (IS_ERR(folio))
> +		return &folio->page;
>
>   	/*
>   	 * Since we can defragment files opened read-only, we can encounter
> @@ -884,16 +885,16 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>   	 * executables that explicitly enable them, so this isn't very
>   	 * restrictive.
>   	 */
> -	if (PageCompound(page)) {
> -		unlock_page(page);
> -		put_page(page);
> +	if (folio_test_large(folio)) {
> +		folio_unlock(folio);
> +		folio_put(folio);
>   		return ERR_PTR(-ETXTBSY);
>   	}
>
> -	ret = set_page_extent_mapped(page);
> +	ret = set_page_extent_mapped(&folio->page);

With my recent patches, set_page_extent_mapped() is already using folio
interfaces, I guess it's time to finally change it to accept a folio.

>   	if (ret < 0) {
> -		unlock_page(page);
> -		put_page(page);
> +		folio_unlock(folio);
> +		folio_put(folio);
>   		return ERR_PTR(ret);
>   	}
>
> @@ -908,17 +909,17 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>   		if (!ordered)
>   			break;
>
> -		unlock_page(page);
> +		folio_unlock(folio);
>   		btrfs_start_ordered_extent(ordered);
>   		btrfs_put_ordered_extent(ordered);
> -		lock_page(page);
> +		folio_lock(folio);
>   		/*
> -		 * We unlocked the page above, so we need check if it was
> +		 * We unlocked the folio above, so we need check if it was
>   		 * released or not.
>   		 */
> -		if (page->mapping != mapping || !PagePrivate(page)) {
> -			unlock_page(page);
> -			put_page(page);
> +		if (folio->mapping != mapping || !folio->private) {

This folio->private check is not the same as PagePrivate(page) IIRC.
Isn't folio_test_private() more suitable here?

Thanks,
Qu

> +			folio_unlock(folio);
> +			folio_put(folio);
>   			goto again;
>   		}
>   	}
> @@ -927,21 +928,21 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>   	 * Now the page range has no ordered extent any more.  Read the page to
>   	 * make it uptodate.
>   	 */
> -	if (!PageUptodate(page)) {
> -		btrfs_read_folio(NULL, page_folio(page));
> -		lock_page(page);
> -		if (page->mapping != mapping || !PagePrivate(page)) {
> -			unlock_page(page);
> -			put_page(page);
> +	if (!folio_test_uptodate(folio)) {
> +		btrfs_read_folio(NULL, folio);
> +		folio_lock(folio);
> +		if (folio->mapping != mapping || !folio->private) {
> +			folio_unlock(folio);
> +			folio_put(folio);
>   			goto again;
>   		}
> -		if (!PageUptodate(page)) {
> -			unlock_page(page);
> -			put_page(page);
> +		if (!folio_test_uptodate(folio)) {
> +			folio_unlock(folio);
> +			folio_put(folio);
>   			return ERR_PTR(-EIO);
>   		}
>   	}
> -	return page;
> +	return &folio->page;
>   }
>
>   struct defrag_target_range {

Re: [PATCH 1/2] btrfs: Convert defrag_prepare_one_page() to use a folio

[Matthew Wilcox]

On Sat, Dec 09, 2023 at 07:26:45AM +1030, Qu Wenruo wrote:
> >   again:
> > -	page = find_or_create_page(mapping, index, mask);
> > -	if (!page)
> > -		return ERR_PTR(-ENOMEM);
> > +	folio = __filemap_get_folio(mapping, index,
> > +			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
> 
> When I was (and still am) a newbie to the folio interfaces, the "__"
> prefix is driving me away to use it.
> 
> Mind to change it in the future? Like adding a new
> filemap_get_or_create_folio()?

I'm always open to better naming ideas.  We definitely have too many
inline functions that call pagecache_get_page():

find_get_page()
find_get_page_flags()
find_lock_page()
find_or_create_page()
grab_cache_page_nowait()

... and I don't think anyone could tell you when to use which one
without looking at the implementations or cribbing from another
filesystem.

So I've tried to keep it simple for the folio replacements and so
far we only have:

filemap_get_folio()
filemap_lock_folio()
filemap_grab_folio()

and honestly, I don't love filemap_grab_folio() and I'm regretting
its creation.

What I do like is the creation of FGP_WRITEBEGIN, but that doesn't
address your concern about the leading __.  Would you be happier if
we renamed __filemap_get_folio() to filemap_get_folio_flags()?

This would all be much better if C allowed you to specify default
arguments to functions :-P

> > -	ret = set_page_extent_mapped(page);
> > +	ret = set_page_extent_mapped(&folio->page);
> 
> With my recent patches, set_page_extent_mapped() is already using folio
> interfaces, I guess it's time to finally change it to accept a folio.

I'll add this as a prep patch:

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b5a2399ed420..99ae54ab004c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -909,18 +909,22 @@ static int attach_extent_buffer_page(struct extent_buffer *eb,
 
 int set_page_extent_mapped(struct page *page)
 {
-	struct folio *folio = page_folio(page);
+	return set_folio_extent_mapped(page_folio(page));
+}
+
+int set_folio_extent_mapped(struct folio *folio)
+{
 	struct btrfs_fs_info *fs_info;
 
-	ASSERT(page->mapping);
+	ASSERT(folio->mapping);
 
 	if (folio_test_private(folio))
 		return 0;
 
-	fs_info = btrfs_sb(page->mapping->host->i_sb);
+	fs_info = btrfs_sb(folio->mapping->host->i_sb);
 
-	if (btrfs_is_subpage(fs_info, page))
-		return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA);
+	if (btrfs_is_subpage(fs_info, &folio->page))
+		return btrfs_attach_subpage(fs_info, &folio->page, BTRFS_SUBPAGE_DATA);
 
 	folio_attach_private(folio, (void *)EXTENT_FOLIO_PRIVATE);
 	return 0;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index c73d53c22ec5..b6b31fb41bdf 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -202,6 +202,7 @@ int btree_write_cache_pages(struct address_space *mapping,
 void extent_readahead(struct readahead_control *rac);
 int extent_fiemap(struct btrfs_inode *inode, struct fiemap_extent_info *fieinfo,
 		  u64 start, u64 len);
+int set_folio_extent_mapped(struct folio *folio);
 int set_page_extent_mapped(struct page *page);
 void clear_page_extent_mapped(struct page *page);
 

> > -		if (page->mapping != mapping || !PagePrivate(page)) {
> > -			unlock_page(page);
> > -			put_page(page);
> > +		if (folio->mapping != mapping || !folio->private) {
> 
> This folio->private check is not the same as PagePrivate(page) IIRC.
> Isn't folio_test_private() more suitable here?

One of the projects I'm pursuing on the side is getting rid of PG_private.
There's really no need to burn a very precious page flag telling us
whether the filesystem has something stored in folio->private when we
could just look at folio->private instead.

So yes, generates different code, but the two should be the same.

Re: [PATCH 1/2] btrfs: Convert defrag_prepare_one_page() to use a folio

[Qu Wenruo]

On 2023/12/9 07:46, Matthew Wilcox wrote:
> On Sat, Dec 09, 2023 at 07:26:45AM +1030, Qu Wenruo wrote:
>>>    again:
>>> -	page = find_or_create_page(mapping, index, mask);
>>> -	if (!page)
>>> -		return ERR_PTR(-ENOMEM);
>>> +	folio = __filemap_get_folio(mapping, index,
>>> +			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
>>
>> When I was (and still am) a newbie to the folio interfaces, the "__"
>> prefix is driving me away to use it.
>>
>> Mind to change it in the future? Like adding a new
>> filemap_get_or_create_folio()?
>
> I'm always open to better naming ideas.  We definitely have too many
> inline functions that call pagecache_get_page():
>
> find_get_page()
> find_get_page_flags()
> find_lock_page()
> find_or_create_page()
> grab_cache_page_nowait()
>
> ... and I don't think anyone could tell you when to use which one
> without looking at the implementations or cribbing from another
> filesystem.
>
> So I've tried to keep it simple for the folio replacements and so
> far we only have:
>
> filemap_get_folio()
> filemap_lock_folio()
> filemap_grab_folio()
>
> and honestly, I don't love filemap_grab_folio() and I'm regretting
> its creation.

In that case, I'd prefer just a single filemap_get_folio(), passing all
the GFP and FGP flags just like __filemap_get_folio(), and get rid of
any wrappers.

In fact, I'd say the __filemap_get_folio() with all the FGP flags is
easier to read to me at least.
(And with direct FGP control for callers, we can make it easier to
allocate higher order folios if the caller wants)

>
> What I do like is the creation of FGP_WRITEBEGIN, but that doesn't
> address your concern about the leading __.  Would you be happier if
> we renamed __filemap_get_folio() to filemap_get_folio_flags()?

Oh, with a _flags() suffix it would be also a good idea.
>
> This would all be much better if C allowed you to specify default
> arguments to functions :-P
>
>>> -	ret = set_page_extent_mapped(page);
>>> +	ret = set_page_extent_mapped(&folio->page);
>>
>> With my recent patches, set_page_extent_mapped() is already using folio
>> interfaces, I guess it's time to finally change it to accept a folio.
>
> I'll add this as a prep patch:
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index b5a2399ed420..99ae54ab004c 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -909,18 +909,22 @@ static int attach_extent_buffer_page(struct extent_buffer *eb,
>
>   int set_page_extent_mapped(struct page *page)
>   {
> -	struct folio *folio = page_folio(page);
> +	return set_folio_extent_mapped(page_folio(page));
> +}
> +
> +int set_folio_extent_mapped(struct folio *folio)
> +{
>   	struct btrfs_fs_info *fs_info;
>
> -	ASSERT(page->mapping);
> +	ASSERT(folio->mapping);
>
>   	if (folio_test_private(folio))
>   		return 0;
>
> -	fs_info = btrfs_sb(page->mapping->host->i_sb);
> +	fs_info = btrfs_sb(folio->mapping->host->i_sb);
>
> -	if (btrfs_is_subpage(fs_info, page))
> -		return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA);
> +	if (btrfs_is_subpage(fs_info, &folio->page))
> +		return btrfs_attach_subpage(fs_info, &folio->page, BTRFS_SUBPAGE_DATA);
>
>   	folio_attach_private(folio, (void *)EXTENT_FOLIO_PRIVATE);
>   	return 0;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index c73d53c22ec5..b6b31fb41bdf 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -202,6 +202,7 @@ int btree_write_cache_pages(struct address_space *mapping,
>   void extent_readahead(struct readahead_control *rac);
>   int extent_fiemap(struct btrfs_inode *inode, struct fiemap_extent_info *fieinfo,
>   		  u64 start, u64 len);
> +int set_folio_extent_mapped(struct folio *folio);
>   int set_page_extent_mapped(struct page *page);
>   void clear_page_extent_mapped(struct page *page);
>
>
>>> -		if (page->mapping != mapping || !PagePrivate(page)) {
>>> -			unlock_page(page);
>>> -			put_page(page);
>>> +		if (folio->mapping != mapping || !folio->private) {
>>
>> This folio->private check is not the same as PagePrivate(page) IIRC.
>> Isn't folio_test_private() more suitable here?
>
> One of the projects I'm pursuing on the side is getting rid of PG_private.
> There's really no need to burn a very precious page flag telling us
> whether the filesystem has something stored in folio->private when we
> could just look at folio->private instead.
>
> So yes, generates different code, but the two should be the same.

OK, got it. And hoping before we can get rid of PG_private, we can get
rid of page usage inside btrfs first.

Thanks,
Qu

[PATCH 1/2] btrfs: Convert defrag_prepare_one_page() to use a folio

[Matthew Wilcox (Oracle)]

Use a folio throughout defrag_prepare_one_page() to remove dozens of
hidden calls to compound_head().  There is no support here for large
folios; indeed, turn the existing check for PageCompound into a check
for large folios.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/defrag.c | 53 ++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index f2ff4cbe8656..4392a09d2bb1 100644
--- a/fs/btrfs/defrag.c
+++ b/fs/btrfs/defrag.c
@@ -724,13 +724,14 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	u64 page_start = (u64)index << PAGE_SHIFT;
 	u64 page_end = page_start + PAGE_SIZE - 1;
 	struct extent_state *cached_state = NULL;
-	struct page *page;
+	struct folio *folio;
 	int ret;
 
 again:
-	page = find_or_create_page(mapping, index, mask);
-	if (!page)
-		return ERR_PTR(-ENOMEM);
+	folio = __filemap_get_folio(mapping, index,
+			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
+	if (IS_ERR(folio))
+		return &folio->page;
 
 	/*
 	 * Since we can defragment files opened read-only, we can encounter
@@ -740,16 +741,16 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	 * executables that explicitly enable them, so this isn't very
 	 * restrictive.
 	 */
-	if (PageCompound(page)) {
-		unlock_page(page);
-		put_page(page);
+	if (folio_test_large(folio)) {
+		folio_unlock(folio);
+		folio_put(folio);
 		return ERR_PTR(-ETXTBSY);
 	}
 
-	ret = set_page_extent_mapped(page);
+	ret = set_page_extent_mapped(&folio->page);
 	if (ret < 0) {
-		unlock_page(page);
-		put_page(page);
+		folio_unlock(folio);
+		folio_put(folio);
 		return ERR_PTR(ret);
 	}
 
@@ -764,17 +765,17 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 		if (!ordered)
 			break;
 
-		unlock_page(page);
+		folio_unlock(folio);
 		btrfs_start_ordered_extent(ordered);
 		btrfs_put_ordered_extent(ordered);
-		lock_page(page);
+		folio_lock(folio);
 		/*
-		 * We unlocked the page above, so we need check if it was
+		 * We unlocked the folio above, so we need check if it was
 		 * released or not.
 		 */
-		if (page->mapping != mapping || !PagePrivate(page)) {
-			unlock_page(page);
-			put_page(page);
+		if (folio->mapping != mapping || !folio->private) {
+			folio_unlock(folio);
+			folio_put(folio);
 			goto again;
 		}
 	}
@@ -783,21 +784,21 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	 * Now the page range has no ordered extent any more.  Read the page to
 	 * make it uptodate.
 	 */
-	if (!PageUptodate(page)) {
-		btrfs_read_folio(NULL, page_folio(page));
-		lock_page(page);
-		if (page->mapping != mapping || !PagePrivate(page)) {
-			unlock_page(page);
-			put_page(page);
+	if (!folio_test_uptodate(folio)) {
+		btrfs_read_folio(NULL, folio);
+		folio_lock(folio);
+		if (folio->mapping != mapping || !folio->private) {
+			folio_unlock(folio);
+			folio_put(folio);
 			goto again;
 		}
-		if (!PageUptodate(page)) {
-			unlock_page(page);
-			put_page(page);
+		if (!folio_test_uptodate(folio)) {
+			folio_unlock(folio);
+			folio_put(folio);
 			return ERR_PTR(-EIO);
 		}
 	}
-	return page;
+	return &folio->page;
 }
 
 struct defrag_target_range {
-- 
2.40.1

[PATCH 2/2] btrfs: Use a folio array throughout the defrag process

[Matthew Wilcox (Oracle)]

Remove more hidden calls to compound_head() by using an array of folios
instead of pages.  Also neaten the error path in defrag_one_range() by
adjusting the length of the array instead of checking for NULL.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/defrag.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index 4392a09d2bb1..065cb613e3b7 100644
--- a/fs/btrfs/defrag.c
+++ b/fs/btrfs/defrag.c
@@ -717,7 +717,7 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
  * NOTE: Caller should also wait for page writeback after the cluster is
  * prepared, here we don't do writeback wait for each page.
  */
-static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t index)
+static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t index)
 {
 	struct address_space *mapping = inode->vfs_inode.i_mapping;
 	gfp_t mask = btrfs_alloc_write_mask(mapping);
@@ -731,7 +731,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 	folio = __filemap_get_folio(mapping, index,
 			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
 	if (IS_ERR(folio))
-		return &folio->page;
+		return folio;
 
 	/*
 	 * Since we can defragment files opened read-only, we can encounter
@@ -798,7 +798,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
 			return ERR_PTR(-EIO);
 		}
 	}
-	return &folio->page;
+	return folio;
 }
 
 struct defrag_target_range {
@@ -1020,7 +1020,7 @@ static_assert(PAGE_ALIGNED(CLUSTER_SIZE));
  */
 static int defrag_one_locked_target(struct btrfs_inode *inode,
 				    struct defrag_target_range *target,
-				    struct page **pages, int nr_pages,
+				    struct folio **folios, int nr_pages,
 				    struct extent_state **cached_state)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
@@ -1029,7 +1029,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
 	const u64 len = target->len;
 	unsigned long last_index = (start + len - 1) >> PAGE_SHIFT;
 	unsigned long start_index = start >> PAGE_SHIFT;
-	unsigned long first_index = page_index(pages[0]);
+	unsigned long first_index = folios[0]->index;
 	int ret = 0;
 	int i;
 
@@ -1046,8 +1046,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
 
 	/* Update the page status */
 	for (i = start_index - first_index; i <= last_index - first_index; i++) {
-		ClearPageChecked(pages[i]);
-		btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len);
+		folio_clear_checked(folios[i]);
+		btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len);
 	}
 	btrfs_delalloc_release_extents(inode, len);
 	extent_changeset_free(data_reserved);
@@ -1063,7 +1063,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 	struct defrag_target_range *entry;
 	struct defrag_target_range *tmp;
 	LIST_HEAD(target_list);
-	struct page **pages;
+	struct folio **folios;
 	const u32 sectorsize = inode->root->fs_info->sectorsize;
 	u64 last_index = (start + len - 1) >> PAGE_SHIFT;
 	u64 start_index = start >> PAGE_SHIFT;
@@ -1074,21 +1074,21 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 	ASSERT(nr_pages <= CLUSTER_SIZE / PAGE_SIZE);
 	ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(len, sectorsize));
 
-	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
-	if (!pages)
+	folios = kcalloc(nr_pages, sizeof(struct folio *), GFP_NOFS);
+	if (!folios)
 		return -ENOMEM;
 
 	/* Prepare all pages */
 	for (i = 0; i < nr_pages; i++) {
-		pages[i] = defrag_prepare_one_page(inode, start_index + i);
-		if (IS_ERR(pages[i])) {
-			ret = PTR_ERR(pages[i]);
-			pages[i] = NULL;
-			goto free_pages;
+		folios[i] = defrag_prepare_one_folio(inode, start_index + i);
+		if (IS_ERR(folios[i])) {
+			ret = PTR_ERR(folios[i]);
+			nr_pages = i;
+			goto free_folios;
 		}
 	}
 	for (i = 0; i < nr_pages; i++)
-		wait_on_page_writeback(pages[i]);
+		folio_wait_writeback(folios[i]);
 
 	/* Lock the pages range */
 	lock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
@@ -1108,7 +1108,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 		goto unlock_extent;
 
 	list_for_each_entry(entry, &target_list, list) {
-		ret = defrag_one_locked_target(inode, entry, pages, nr_pages,
+		ret = defrag_one_locked_target(inode, entry, folios, nr_pages,
 					       &cached_state);
 		if (ret < 0)
 			break;
@@ -1122,14 +1122,12 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
 	unlock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
 		      (last_index << PAGE_SHIFT) + PAGE_SIZE - 1,
 		      &cached_state);
-free_pages:
+free_folios:
 	for (i = 0; i < nr_pages; i++) {
-		if (pages[i]) {
-			unlock_page(pages[i]);
-			put_page(pages[i]);
-		}
+		folio_unlock(folios[i]);
+		folio_put(folios[i]);
 	}
-	kfree(pages);
+	kfree(folios);
 	return ret;
 }
 
-- 
2.40.1

Re: [PATCH 2/2] btrfs: Use a folio array throughout the defrag process

[David Sterba]

On Mon, Aug 14, 2023 at 06:03:50PM +0100, Matthew Wilcox (Oracle) wrote:
> @@ -1020,7 +1020,7 @@ static_assert(PAGE_ALIGNED(CLUSTER_SIZE));
>   */
>  static int defrag_one_locked_target(struct btrfs_inode *inode,
>  				    struct defrag_target_range *target,
> -				    struct page **pages, int nr_pages,
> +				    struct folio **folios, int nr_pages,

The nr_pages can be renamed to nr_folios here so it's in sync. I can fix
that in the commit unless there's a reason to resend the patch.

>  				    struct extent_state **cached_state)
>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> @@ -1029,7 +1029,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>  	const u64 len = target->len;
>  	unsigned long last_index = (start + len - 1) >> PAGE_SHIFT;
>  	unsigned long start_index = start >> PAGE_SHIFT;
> -	unsigned long first_index = page_index(pages[0]);
> +	unsigned long first_index = folios[0]->index;
>  	int ret = 0;
>  	int i;
>  
> @@ -1046,8 +1046,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>  
>  	/* Update the page status */
>  	for (i = start_index - first_index; i <= last_index - first_index; i++) {
> -		ClearPageChecked(pages[i]);
> -		btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len);
> +		folio_clear_checked(folios[i]);
> +		btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len);
>  	}
>  	btrfs_delalloc_release_extents(inode, len);
>  	extent_changeset_free(data_reserved);
> @@ -1063,7 +1063,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>  	struct defrag_target_range *entry;
>  	struct defrag_target_range *tmp;
>  	LIST_HEAD(target_list);
> -	struct page **pages;
> +	struct folio **folios;
>  	const u32 sectorsize = inode->root->fs_info->sectorsize;
>  	u64 last_index = (start + len - 1) >> PAGE_SHIFT;
>  	u64 start_index = start >> PAGE_SHIFT;
> @@ -1074,21 +1074,21 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>  	ASSERT(nr_pages <= CLUSTER_SIZE / PAGE_SIZE);
>  	ASSERT(IS_ALIGNED(start, sectorsize) && IS_ALIGNED(len, sectorsize));
>  
> -	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
> -	if (!pages)
> +	folios = kcalloc(nr_pages, sizeof(struct folio *), GFP_NOFS);
> +	if (!folios)
>  		return -ENOMEM;
>  
>  	/* Prepare all pages */
>  	for (i = 0; i < nr_pages; i++) {
> -		pages[i] = defrag_prepare_one_page(inode, start_index + i);
> -		if (IS_ERR(pages[i])) {
> -			ret = PTR_ERR(pages[i]);
> -			pages[i] = NULL;
> -			goto free_pages;
> +		folios[i] = defrag_prepare_one_folio(inode, start_index + i);
> +		if (IS_ERR(folios[i])) {
> +			ret = PTR_ERR(folios[i]);
> +			nr_pages = i;
> +			goto free_folios;
>  		}
>  	}
>  	for (i = 0; i < nr_pages; i++)
> -		wait_on_page_writeback(pages[i]);
> +		folio_wait_writeback(folios[i]);
>  
>  	/* Lock the pages range */
>  	lock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
> @@ -1108,7 +1108,7 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>  		goto unlock_extent;
>  
>  	list_for_each_entry(entry, &target_list, list) {
> -		ret = defrag_one_locked_target(inode, entry, pages, nr_pages,
> +		ret = defrag_one_locked_target(inode, entry, folios, nr_pages,
>  					       &cached_state);
>  		if (ret < 0)
>  			break;
> @@ -1122,14 +1122,12 @@ static int defrag_one_range(struct btrfs_inode *inode, u64 start, u32 len,
>  	unlock_extent(&inode->io_tree, start_index << PAGE_SHIFT,
>  		      (last_index << PAGE_SHIFT) + PAGE_SIZE - 1,
>  		      &cached_state);
> -free_pages:
> +free_folios:
>  	for (i = 0; i < nr_pages; i++) {
> -		if (pages[i]) {
> -			unlock_page(pages[i]);
> -			put_page(pages[i]);
> -		}
> +		folio_unlock(folios[i]);
> +		folio_put(folios[i]);
>  	}
> -	kfree(pages);
> +	kfree(folios);
>  	return ret;
>  }
>  
> -- 
> 2.40.1