Re: [PATCH 2/3] btrfs: Convert test_find_delalloc() to use a folio, part two

[Qu Wenruo <wqu@suse.com>]

在 2025/6/14 09:05, Qu Wenruo 写道:
> 
> 
> 在 2025/6/14 04:37, Matthew Wilcox (Oracle) 写道:
>> Replace the 'page' variable with 'folio'.  Removes six calls to
>> compound_head().
>>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>   fs/btrfs/tests/extent-io-tests.c | 32 +++++++++++++++++---------------
>>   1 file changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent- 
>> io-tests.c
>> index 8bdf742d90fd..36720b77b440 100644
>> --- a/fs/btrfs/tests/extent-io-tests.c
>> +++ b/fs/btrfs/tests/extent-io-tests.c
>> @@ -111,7 +111,7 @@ 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 folio *folio;
>>       struct folio *locked_folio = NULL;
>>       unsigned long index = 0;
>>       /* In this test we need at least 2 file extents at its maximum 
>> size */
>> @@ -152,23 +152,25 @@ 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 (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");
>> +        folio = __filemap_get_folio(inode->i_mapping, index,
>> +                FGP_LOCK | FGP_ACCESSED | FGP_CREAT,
>> +                GFP_KERNEL);
>> +        if (!folio) {
>> +            test_err("failed to allocate test folio");
>>               ret = -ENOMEM;
>>               goto out;
>>           }
>> -        SetPageDirty(page);
>> +        folio_mark_dirty(folio);
> 
> Crashing immediately when loading the module.
> (Need CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y)
> 
> [   20.626710] BUG: kernel NULL pointer dereference, address: 
> 0000000000000000

This is from the a_ops, which is NULL for testing inodes.

So here we should not call folio_mark_dirty(), as it will call 
dirty_folio() callbacks unconditionally no matter if the mapping has 
such call backs.

Instead we should call noop_dirty_folio() instead, which is the 
equivalent of SetPageDirty().

Thanks,
Qu

> [   20.628812] #PF: supervisor instruction fetch in kernel mode
> [   20.630648] #PF: error_code(0x0010) - not-present page
> [   20.632156] PGD 0 P4D 0
> [   20.632893] Oops: Oops: 0010 [#1] SMP NOPTI
> [   20.634052] CPU: 6 UID: 0 PID: 622 Comm: insmod Tainted: G OE       
> 6.16.0-rc1-custom+ #253 PREEMPT(full)
> [   20.636879] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> [   20.638321] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
> unknown 02/02/2022
> [   20.640524] RIP: 0010:0x0
> [   20.641290] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
> [   20.643075] RSP: 0018:ffffc90001587c88 EFLAGS: 00010246
> [   20.644519] RAX: ffff88810b144670 RBX: 0000000000000000 RCX: 
> 0000000000000001
> [   20.646490] RDX: 0000000000000000 RSI: ffffea0004128200 RDI: 
> ffff88810b144670
> [   20.648496] RBP: ffff88810b144500 R08: 0000000000000000 R09: 
> ffffffff83549b20
> [   20.650524] R10: 00000000000002c0 R11: 0000000000000000 R12: 
> 0000000000000000
> [   20.652642] R13: ffff88810b1443a8 R14: ffffea0004128200 R15: 
> 0000000000001000
> [   20.654778] FS:  00007f2e60e99740(0000) GS:ffff8882f45e2000(0000) 
> knlGS:0000000000000000
> [   20.657158] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   20.658980] CR2: ffffffffffffffd6 CR3: 00000001040b9000 CR4: 
> 00000000000006f0
> [   20.661219] Call Trace:
> [   20.662057]  <TASK>
> [   20.662757]  btrfs_test_extent_io+0x17a/0xf40 [btrfs]
> [   20.664266]  btrfs_run_sanity_tests.cold+0x84/0x11e [btrfs]
> [   20.665839]  init_btrfs_fs+0x4d/0xb0 [btrfs]
> [   20.667380]  ? __pfx_init_btrfs_fs+0x10/0x10 [btrfs]
> [   20.668883]  do_one_initcall+0x76/0x250
> [   20.670098]  do_init_module+0x62/0x250
> [   20.671359]  init_module_from_file+0x85/0xc0
> [   20.672586]  idempotent_init_module+0x148/0x340
> [   20.673900]  __x64_sys_finit_module+0x6d/0xd0
> [   20.675074]  do_syscall_64+0x54/0x1d0
> [   20.676167]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> 
> Furthermore, the error handling of __filemap_get_folio() is incorrect.
> That function returns either a valid folio, or an ERR_PTR(), no more NULL.
> 
> This applies to all folio calls like filemap_lock_folio() too.
> 
> Thanks,
> Qu
> 
>>           if (index) {
>> -            unlock_page(page);
>> +            folio_unlock(folio);
>>           } else {
>> -            get_page(page);
>> -            locked_folio = page_folio(page);
>> +            folio_get(folio);
>> +            locked_folio = folio;
>>           }
>>       }
>> @@ -283,14 +285,14 @@ static int test_find_delalloc(u32 sectorsize, 
>> u32 nodesize)
>>        * Now to test where we run into a page that is no longer dirty 
>> in the
>>        * range we want to find.
>>        */
>> -    page = find_get_page(inode->i_mapping,
>> +    folio = filemap_get_folio(inode->i_mapping,
>>                    (max_bytes + SZ_1M) >> PAGE_SHIFT);
>> -    if (!page) {
>> -        test_err("couldn't find our page");
>> +    if (!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 */
>>       folio_lock(locked_folio);
>