Re: [PATCH 02/10] btrfs: tests: convert test_find_delalloc() to use folios

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/9/7 07:59, Tal Zussman 写道:
> This removes the last btrfs callers of find_or_create_page(),
> find_lock_page(), SetPageDirty(), ClearPageDirty(), and get_page(), and
> 15 calls to compound_head(). The folio lookups return an ERR_PTR instead
> of NULL, so adjust the error handling.
> 
> Update the comments and test messages accordingly.

I think the interface change itself is fine, although still some minor 
concerns inlined below.
> 
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> ---
>   fs/btrfs/tests/extent-io-tests.c | 94 ++++++++++++++++++++--------------------
>   1 file changed, 46 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> index 6eb55bfb2bd4..3056dd934b54 100644
> --- a/fs/btrfs/tests/extent-io-tests.c
> +++ b/fs/btrfs/tests/extent-io-tests.c
> @@ -112,8 +112,8 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	struct btrfs_root *root = NULL;
>   	struct inode *inode = NULL;
>   	struct extent_io_tree *tmp;
> -	struct page *page;
> -	struct page *locked_page = NULL;
> +	struct folio *folio;
> +	struct folio *locked_folio = NULL;
>   	/* In this test we need at least 2 file extents at its maximum size */
>   	u64 max_bytes = BTRFS_MAX_EXTENT_SIZE;
>   	u64 total_dirty = 2 * max_bytes;
> @@ -152,23 +152,24 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	btrfs_extent_io_tree_init(NULL, tmp, IO_TREE_SELFTEST);
>   
>   	/*
> -	 * First go through and create and mark all of our pages dirty, we pin
> -	 * everything to make sure our pages don't get evicted and screw up our
> +	 * First go through and create and mark all of our folios dirty, we pin
> +	 * everything to make sure our folios don't get evicted and screw up our
>   	 * test.
>   	 */
>   	for (pgoff_t index = 0; index < (total_dirty >> PAGE_SHIFT); index++) {
> -		page = find_or_create_page(inode->i_mapping, index, GFP_KERNEL);
> -		if (!page) {
> -			test_err("failed to allocate test page");
> -			ret = -ENOMEM;
> +		folio = __filemap_get_folio(inode->i_mapping, index,
> +				FGP_LOCK | FGP_ACCESSED | FGP_CREAT, GFP_KERNEL);

No matter if it's the older or newer code, it's not exactly following 
the real kernel behavior.

We're relying on the fact that the test inode doesn't have a folio order 
range set, thus we always get page size folios.
> +		if (IS_ERR(folio)) {
> +			test_err("failed to allocate test folio");
> +			ret = PTR_ERR(folio);
>   			goto out;
>   		}
> -		SetPageDirty(page);
> +		folio_set_dirty(folio);

We only need to update the folio flags, no need to bother the possible 
bitmaps for bs < ps cases, or large folio cases exactly because we 
always get page sized folio, and for now the test case only handles bs 
== ps cases.

I'm not pushing for using btrfs_folio_set_dirty() helpers immediately, 
but an "ASSERT(folio_order(folio) == 0);" would be a little safer.

>   		if (index) {
> -			unlock_page(page);
> +			folio_unlock(folio);
>   		} else {
> -			get_page(page);
> -			locked_page = page;
> +			folio_get(folio);
> +			locked_folio = folio;
>   		}
>   	}
>   
> @@ -179,8 +180,7 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	btrfs_set_extent_bit(tmp, 0, sectorsize - 1, EXTENT_DELALLOC, NULL);
>   	start = 0;
>   	end = start + PAGE_SIZE - 1;
> -	found = find_lock_delalloc_range(inode, page_folio(locked_page), &start,
> -					 &end);
> +	found = find_lock_delalloc_range(inode, locked_folio, &start, &end);
>   	if (!found) {
>   		test_err("should have found at least one delalloc");
>   		goto out_bits;
> @@ -191,8 +191,8 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   		goto out_bits;
>   	}
>   	btrfs_unlock_extent(tmp, start, end, NULL);
> -	unlock_page(locked_page);
> -	put_page(locked_page);
> +	folio_unlock(locked_folio);
> +	folio_put(locked_folio);
>   
>   	/*
>   	 * Test this scenario
> @@ -201,17 +201,17 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	 *           |--- search ---|
>   	 */
>   	test_start = SZ_64M;
> -	locked_page = find_lock_page(inode->i_mapping,
> -				     test_start >> PAGE_SHIFT);
> -	if (!locked_page) {
> -		test_err("couldn't find the locked page");
> +	locked_folio = filemap_lock_folio(inode->i_mapping,
> +					  test_start >> PAGE_SHIFT);
> +	if (IS_ERR(locked_folio)) {
> +		test_err("couldn't find the locked folio");
> +		locked_folio = NULL;
>   		goto out_bits;
>   	}
>   	btrfs_set_extent_bit(tmp, sectorsize, max_bytes - 1, EXTENT_DELALLOC, NULL);
>   	start = test_start;
>   	end = start + PAGE_SIZE - 1;

The same here, the range only works if the folio is page sized.

Thus an ASSERT() would be preferred for every range that is still based 
on PAGE_SIZE.

Otherwise looks good to me.

Thanks,
Qu

> -	found = find_lock_delalloc_range(inode, page_folio(locked_page), &start,
> -					 &end);
> +	found = find_lock_delalloc_range(inode, locked_folio, &start, &end);
>   	if (!found) {
>   		test_err("couldn't find delalloc in our range");
>   		goto out_bits;
> @@ -223,12 +223,12 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	}
>   	if (process_folio_range(inode, start, end,
>   				PROCESS_TEST_LOCKED | PROCESS_UNLOCK)) {
> -		test_err("there were unlocked pages in the range");
> +		test_err("there were unlocked folios in the range");
>   		goto out_bits;
>   	}
>   	btrfs_unlock_extent(tmp, start, end, NULL);
> -	/* locked_page was unlocked above */
> -	put_page(locked_page);
> +	/* locked_folio was unlocked above */
> +	folio_put(locked_folio);
>   
>   	/*
>   	 * Test this scenario
> @@ -236,16 +236,16 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	 *                    |--- search ---|
>   	 */
>   	test_start = max_bytes + sectorsize;
> -	locked_page = find_lock_page(inode->i_mapping, test_start >>
> -				     PAGE_SHIFT);
> -	if (!locked_page) {
> -		test_err("couldn't find the locked page");
> +	locked_folio = filemap_lock_folio(inode->i_mapping,
> +					  test_start >> PAGE_SHIFT);
> +	if (IS_ERR(locked_folio)) {
> +		test_err("couldn't find the locked folio");
> +		locked_folio = NULL;
>   		goto out_bits;
>   	}
>   	start = test_start;
>   	end = start + PAGE_SIZE - 1;
> -	found = find_lock_delalloc_range(inode, page_folio(locked_page), &start,
> -					 &end);
> +	found = find_lock_delalloc_range(inode, locked_folio, &start, &end);
>   	if (found) {
>   		test_err("found range when we shouldn't have");
>   		goto out_bits;
> @@ -265,8 +265,7 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	btrfs_set_extent_bit(tmp, max_bytes, total_dirty - 1, EXTENT_DELALLOC, NULL);
>   	start = test_start;
>   	end = start + PAGE_SIZE - 1;
> -	found = find_lock_delalloc_range(inode, page_folio(locked_page), &start,
> -					 &end);
> +	found = find_lock_delalloc_range(inode, locked_folio, &start, &end);
>   	if (!found) {
>   		test_err("didn't find our range");
>   		goto out_bits;
> @@ -278,36 +277,35 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	}
>   	if (process_folio_range(inode, start, end,
>   				PROCESS_TEST_LOCKED | PROCESS_UNLOCK)) {
> -		test_err("pages in range were not all locked");
> +		test_err("folios in range were not all locked");
>   		goto out_bits;
>   	}
>   	btrfs_unlock_extent(tmp, start, end, NULL);
>   
>   	/*
> -	 * Now to test where we run into a page that is no longer dirty in the
> +	 * Now to test where we run into a folio that is no longer dirty in the
>   	 * range we want to find.
>   	 */
> -	page = find_get_page(inode->i_mapping,
> -			     (max_bytes + SZ_1M) >> PAGE_SHIFT);
> -	if (!page) {
> -		test_err("couldn't find our page");
> +	folio = filemap_get_folio(inode->i_mapping,
> +				  (max_bytes + SZ_1M) >> PAGE_SHIFT);
> +	if (IS_ERR(folio)) {
> +		test_err("couldn't find our folio");
>   		goto out_bits;
>   	}
> -	ClearPageDirty(page);
> -	put_page(page);
> +	folio_clear_dirty(folio);
> +	folio_put(folio);
>   
>   	/* We unlocked it in the previous test */
> -	lock_page(locked_page);
> +	folio_lock(locked_folio);
>   	start = test_start;
>   	end = start + PAGE_SIZE - 1;
>   	/*
> -	 * Currently if we fail to find dirty pages in the delalloc range we
> +	 * Currently if we fail to find dirty folios in the delalloc range we
>   	 * will adjust max_bytes down to PAGE_SIZE and then re-search.  If
>   	 * this changes at any point in the future we will need to fix this
>   	 * tests expected behavior.
>   	 */
> -	found = find_lock_delalloc_range(inode, page_folio(locked_page), &start,
> -					 &end);
> +	found = find_lock_delalloc_range(inode, locked_folio, &start, &end);
>   	if (!found) {
>   		test_err("didn't find our range");
>   		goto out_bits;
> @@ -319,7 +317,7 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   	}
>   	if (process_folio_range(inode, start, end, PROCESS_TEST_LOCKED |
>   				PROCESS_UNLOCK)) {
> -		test_err("pages in range were not all locked");
> +		test_err("folios in range were not all locked");
>   		goto out_bits;
>   	}
>   	ret = 0;
> @@ -328,8 +326,8 @@ static int test_find_delalloc(u32 sectorsize, u32 nodesize)
>   		dump_extent_io_tree(tmp);
>   	btrfs_clear_extent_bit(tmp, 0, total_dirty - 1, (unsigned)-1, NULL);
>   out:
> -	if (locked_page)
> -		put_page(locked_page);
> +	if (locked_folio)
> +		folio_put(locked_folio);
>   	process_folio_range(inode, 0, total_dirty - 1,
>   			    PROCESS_UNLOCK | PROCESS_RELEASE);
>   	iput(inode);
>