Re: [PATCH 2/2] f2fs: quota: use memalloc_nofs_{save,restore} instead of FGP_NOFS

[Chao Yu]

On 7/7/26 17:44, Chao Yu wrote:
> On 7/6/26 22:18, Matthew Wilcox wrote:
>> On Mon, Jul 06, 2026 at 05:59:43PM +0800, Chao Yu wrote:
>>> FGP_NOFS could be removed later, let's use memalloc_nofs_{save,restore}
>>> instead, which is recommended to be used to avoid potential deadlock
>>> when memory allocation in f2fs_quota_write() will call into filesystem
>>> interface again, e.g. .writepages, evict_inode, shrinker due to
>>> complicated lock race condition.
>>
>> I think we need to be clear on why we need the memalloc_nofs_save()
>> call here.  What problem would it cause if we did call into the
>> filesystem to reclaim memory?
> 
> Hmm, after taking another look at the code, I suspect below deadlock may
> occur w/o NOFS flag?
> 
> User Sync (Thread 1)
> - f2fs_do_quota_sync
>  - f2fs_lock_op
>   : down_read(cp_rwsem) [HELD] (1st Read Lock)
>  - f2fs_quota_sync_file
>   - dquot_writeback_dquots
>    - ...

Oh, seems commit 537e11cdc7a6 ("quota: Prevent memory allocation recursion while
holding dq_lock") covers write path (f2fs_quota_write) as well, please ignore this.

Thanks,

>     - f2fs_quota_write
>      - f2fs_write_begin
>       - f2fs_filemap_get_folio
>        - __filemap_get_folio
>         - ... (GFP_FS allocation triggers reclaim)
>          - try_to_free_pages()
>           - shrink_node()
>            - shrink_lruvec()
>             - shrink_page_list()
>              - pageout()  <-- Reclaim decides to write back a dirty F2FS data page
>               - f2fs_writepages()
>                - f2fs_write_cache_pages()
>                 - f2fs_write_single_data_page(allow_balance = true)
>                  - f2fs_balance_fs(need = true)
>                   - f2fs_gc()
>                    - f2fs_write_checkpoint()
>                     - block_operations()
>                      - f2fs_lock_all()
>                       : down_write(cp_rwsem) [BLOCKED] (Self-deadlock)
> 
> To Jaegeuk, please help to double check this. :)
> 
>>
>> I suspect this is the wrong place to insert this call and it should be
>> near the lock that causes the problem.  I've attempted a rewrite of the
>> memalloc_nofs_save documentation; let me know what you think:
>>
>> /**
>>  * memalloc_nofs_save - Prevent recursion into the filesystem.
>>  *
>>  * All memory allocations between calling this function and calling
>>  * memalloc_nofs_restore() will be prevented from calling into filesystems
>>  * to reclaim memory.  Clean page cache memory can still be reclaimed,
>>  * but (for example) inodes will not be.
>>  *
>>  * The primary reason to do this is that the caller has taken a lock
>>  * which would be needed by FS reclaim.  While we could theoretically
>>  * call into a different filesystem in this case, it can be a deep call
>>  * stack so it is better to avoid all filesystems.
>>  *
>>  * Filesystems often choose to incorporate a call to this function as part
>>  * of starting a journal transaction.  While not a lock in the normal
>>  * sense, it has much the same effect as nested journal transactions
>>  * are either prohibited or expensive.
>>  *
>>  * Also call this function if you need to allocate memory while holding
>>  * a file folio locked.  High order allocations (such as those requested
>>  * by slab) can trigger compaction which will attempt to lock the folio.
>>  *
>>  * Context: This function is safe to be used from any context.
>>  * Return: The saved flags to be passed to memalloc_nofs_restore.
>>  */
> 
> It makes sense to me, thanks for updating.
> 
> Thanks,