[PATCH 1/2] f2fs: quota: do not use GFP_NOFS in f2fs_quota_read()

[PATCH 1/2] f2fs: quota: do not use GFP_NOFS in f2fs_quota_read()

[Chao Yu]

Commit 02117b8ae9c0 ("f2fs: Set GF_NOFS in read_cache_page_gfp while doing
f2fs_quota_read") adds GFP_NOFS in f2fs_quota_read() to avoid below deadlock:

- do_sys_open
 - vfs_open
  - dquot_file_open
   - dquot_initialize
    - dqget
     - dquot_acquire
      : locks &dqopt->dqio_mutex (VFS Quota Mutex)
      - qtree_read_dquot
       - f2fs_quota_read
        - read_mapping_page (GFP_KERNEL / allows GFP_FS)
         - __alloc_pages_nodemask
          - try_to_free_pages (Direct Reclaim)
           - prune_icache_sb
            - evict
             - f2fs_evict_inode
              - dquot_drop
               - dqput
                - dquot_commit
                 : tries to lock &dqopt->dqio_mutex again
                 ==> DEADLOCK (waiting for itself)

As Jan Kara mentioned, quota system has fixed this issue w/ commit
537e11cdc7a6 ("quota: Prevent memory allocation recursion while holding
dq_lock"), so this GFP_NOFS flag should be relic, let's drop it.

Cc: Jan Kara <jack@suse.cz>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/super.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index da468df058eb..fdfd6a7203dd 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3167,8 +3167,7 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
 		size_t offset;
 
 repeat:
-		folio = mapping_read_folio_gfp(mapping, off >> PAGE_SHIFT,
-				GFP_NOFS);
+		folio = mapping_read_folio_gfp(mapping, off >> PAGE_SHIFT, 0);
 		if (IS_ERR(folio)) {
 			if (PTR_ERR(folio) == -ENOMEM) {
 				memalloc_retry_wait(GFP_NOFS);
-- 
2.49.0

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

[Chao Yu]

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.

Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/data.c  | 2 +-
 fs/f2fs/super.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index ac1cf4de3d62..be4c1d4ed6b2 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3994,7 +3994,7 @@ static int f2fs_write_begin(const struct kiocb *iocb,
 	 * Will wait that below with our IO control.
 	 */
 	folio = f2fs_filemap_get_folio(mapping, index,
-				FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_NOFS,
+				FGP_LOCK | FGP_WRITE | FGP_CREAT,
 				mapping_gfp_mask(mapping));
 	if (IS_ERR(folio)) {
 		err = PTR_ERR(folio);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index fdfd6a7203dd..d28a93657658 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3215,13 +3215,16 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
 	void *fsdata = NULL;
 	int err = 0;
 	int tocopy;
+	unsigned int nofs_flags;
 
 	while (towrite > 0) {
 		tocopy = min_t(unsigned long, sb->s_blocksize - offset,
 								towrite);
 retry:
+		nofs_flags = memalloc_nofs_save();
 		err = a_ops->write_begin(NULL, mapping, off, tocopy,
 							&folio, &fsdata);
+		memalloc_nofs_restore(nofs_flags);
 		if (unlikely(err)) {
 			if (err == -ENOMEM) {
 				memalloc_retry_wait(GFP_NOFS);
-- 
2.49.0

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

[Matthew Wilcox]

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?

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.
 */

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

[Chao Yu]

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

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,

Re: [PATCH 1/2] f2fs: quota: do not use GFP_NOFS in f2fs_quota_read()

[Matthew Wilcox]

On Mon, Jul 06, 2026 at 05:59:42PM +0800, Chao Yu wrote:
> -		folio = mapping_read_folio_gfp(mapping, off >> PAGE_SHIFT,
> -				GFP_NOFS);
> +		folio = mapping_read_folio_gfp(mapping, off >> PAGE_SHIFT, 0);

0?  Surely you mean GFP_KERNEL?

Re: [PATCH 1/2] f2fs: quota: do not use GFP_NOFS in f2fs_quota_read()

[Chao Yu]

On 7/6/26 20:55, Matthew Wilcox wrote:
> On Mon, Jul 06, 2026 at 05:59:42PM +0800, Chao Yu wrote:
>> -		folio = mapping_read_folio_gfp(mapping, off >> PAGE_SHIFT,
>> -				GFP_NOFS);
>> +		folio = mapping_read_folio_gfp(mapping, off >> PAGE_SHIFT, 0);
> 
> 0?  Surely you mean GFP_KERNEL?

Oops, my bad, it should be GFP_KERNEL.

Thanks,