Re: [PATCH v7 08/15] f2fs: make GC migration large-folio aware

[Chao Yu]

On 10/26/26 11:37, Nanzhe Zhao wrote:
> From: Nanzhe Zhao <zhaonanzhe@xxxxxxxxxx>
> 
> GC can operate on a 4K block that is cached inside a large folio.
> The data lookup helpers therefore need to test and update uptodate
> state for the addressed subpage instead of rejecting large folios or
> treating the whole folio as the target block.
> 
> Let f2fs_get_read_data_folio() and f2fs_get_lock_data_folio() to use
> subpage uptodate state. Submit single-block reads at the requested
> folio offset and zero only the addressed 4K range for NEW_ADDR.
> 
> Also update `move_data_page` to mark, clear, and restore dirty
> state for the target subpage, and submit write I/O with the subpage
> offset recorded in f2fs_io_info.
> 
> The write_pages_pending reference is taken by f2fs_do_write_data_page()
> and dropped from its common error path, so a block that is truncated
> concurrently no longer leaks the counter.
> 
> Reported-by: kernel test robot <yi1.lai@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-lkp/202610081027.b13dcf10-lkp@xxxxxxxxx
> Signed-off-by: Nanzhe Zhao <zhaonanzhe@xxxxxxxxxx>
> ---
>  fs/f2fs/data.c    | 70 +++++++++++++++++++++++++++++++++--------------
>  fs/f2fs/f2fs.h    |  1 +
>  fs/f2fs/gc.c      | 28 +++++++++++++++----
>  fs/f2fs/segment.c |  2 +-
>  4 files changed, 75 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index d027c949823b..b3ada22cb0cb 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1679,20 +1679,16 @@ struct folio *f2fs_get_read_data_folio(struct inode *inode, pgoff_t index,
>  	struct dnode_of_data dn;
>  	struct folio *folio;
>  	int err;
> -retry:
> +
>  	folio = f2fs_grab_cache_folio(mapping, index, for_write);
>  	if (IS_ERR(folio))
>  		return folio;
>  
>  	if (folio_test_large(folio)) {
> -		pgoff_t folio_index = mapping_align_index(mapping, index);
> -		unsigned long nr_pages = folio_nr_pages(folio);
> -
> -		f2fs_folio_put(folio, true);
> -		invalidate_inode_pages2_range(mapping, folio_index,
> -				folio_index + nr_pages - 1);
> -		f2fs_schedule_timeout(DEFAULT_SCHEDULE_TIMEOUT);
> -		goto retry;
> +		if (!f2fs_ffs_find_or_alloc(folio)) {
> +			err = -ENOMEM;
> +			goto put_err;
> +		}
>  	}
>  
>  	if (f2fs_lookup_read_extent_cache_block(inode, index,
> @@ -1728,7 +1724,7 @@ struct folio *f2fs_get_read_data_folio(struct inode *inode, pgoff_t index,
>  		goto put_err;
>  	}
>  got_it:
> -	if (folio_test_uptodate(folio)) {
> +	if (f2fs_ffs_test_blk_uptodate(folio, index)) {
>  		folio_unlock(folio);
>  		return folio;
>  	}
> @@ -1741,9 +1737,11 @@ struct folio *f2fs_get_read_data_folio(struct inode *inode, pgoff_t index,
>  	 * f2fs_init_inode_metadata.
>  	 */
>  	if (dn.data_blkaddr == NEW_ADDR) {
> -		folio_zero_segment(folio, 0, folio_size(folio));
> -		if (!folio_test_uptodate(folio))
> -			folio_mark_uptodate(folio);
> +		size_t offset = offset_in_folio(folio,
> +						(loff_t)index << PAGE_SHIFT);
> +
> +		folio_zero_segment(folio, offset, offset + PAGE_SIZE);
> +		f2fs_ffs_mark_subrange_uptodate(folio, offset, PAGE_SIZE);
>  		folio_unlock(folio);
>  		return folio;
>  	}
> @@ -1803,7 +1801,6 @@ struct folio *f2fs_get_lock_data_folio(struct inode *inode, pgoff_t index,
>  
>  	/* wait for read completion */
>  	folio_lock(folio);
> -
>  	if (unlikely(folio->mapping != mapping)) {
>  		f2fs_folio_put(folio, true);
>  		return ERR_PTR(-EIO);
> @@ -1812,7 +1809,7 @@ struct folio *f2fs_get_lock_data_folio(struct inode *inode, pgoff_t index,
>  	/*
>  	 * ffs can be released while the folio was unlocked above.  Rebuild it
>  	 * for a fully uptodate folio; a partially uptodate one cannot be
> -	 * rebuilt and will fail the uptodate check below.
> +	 * rebuilt and will fail the subpage check below.
>  	 */
>  	if (folio_test_large(folio) && !f2fs_folio_has_ffs(folio)) {
>  		if (!f2fs_ffs_find_or_alloc(folio)) {
> @@ -1821,7 +1818,7 @@ struct folio *f2fs_get_lock_data_folio(struct inode *inode, pgoff_t index,
>  		}
>  	}
>  
> -	if (unlikely(!folio_test_uptodate(folio))) {
> +	if (unlikely(!f2fs_ffs_test_blk_uptodate(folio, index))) {
>  		f2fs_folio_put(folio, true);
>  		return ERR_PTR(-EIO);
>  	}
> @@ -2970,6 +2967,22 @@ static void f2fs_ffs_mark_subrange_uptodate(struct folio *folio, size_t offset,
>  		folio_mark_uptodate(folio);
>  }
>  
> +bool f2fs_ffs_test_blk_dirty(const struct folio *folio, pgoff_t index)
> +{
> +	struct f2fs_folio_state *ffs;
> +	unsigned int idx, nr_subpages;
> +
> +	f2fs_bug_on(F2FS_F_SB(folio), !folio_contains(folio, index));
> +
> +	if (!f2fs_folio_has_ffs(folio))
> +		return folio_test_dirty(folio);
> +
> +	ffs = (struct f2fs_folio_state *)folio->private;
> +	nr_subpages = folio_nr_pages(folio);
> +	idx = index - folio->index;
> +	return test_bit(nr_subpages + idx, ffs->state);
> +}
> +
>  static unsigned int __ffs_mark_subrange_dirty(struct folio *folio,
>  			struct f2fs_folio_state *ffs, size_t offset, size_t len)
>  {
> @@ -3063,7 +3076,6 @@ bool f2fs_ffs_clear_subrange_dirty_and_test(struct folio *folio, size_t offset,
>  
>  	return dirty;
>  }
> -
>  static unsigned int ffs_next_dirty_subpage(struct f2fs_folio_state *ffs,
>  			const struct folio *folio, unsigned int start,
>  			unsigned int end)
> @@ -3561,7 +3573,7 @@ bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio)
>  		return true;
>  
>  	if (fio) {
> -		if (page_private_gcing(fio->page))
> +		if (folio_test_f2fs_gcing(fio->folio))
>  			return true;
>  		if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED) &&
>  			f2fs_is_checkpointed_data(sbi, fio->old_blkaddr)))
> @@ -3587,8 +3599,11 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  	struct dnode_of_data dn;
>  	struct node_info ni;
>  	struct f2fs_lock_context lc;
> +	struct f2fs_folio_state *ffs;
>  	bool ipu_force = false;
>  	bool atomic_commit;
> +	bool pending_added = false;
> +	bool submitted = false;
>  	int err = 0;
>  
>  	/* Use COW inode to make dnode_of_data for atomic write */
> @@ -3617,8 +3632,11 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  		return -EINVAL;
>  
>  	/* Deadlock due to between page->lock and f2fs_lock_op */
> -	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi, &lc))
> +	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi, &lc)) {
> +		if (!f2fs_ffs_has_dirty_subpage(folio))
> +			folio_clear_f2fs_gcing(folio);
>  		return -EAGAIN;
> +	}
>  
>  	err = f2fs_get_dnode_of_data(&dn, F2FS_FIO_LBLK(fio),
>  				     LOOKUP_NODE);
> @@ -3630,7 +3648,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  	/* This page is already truncated */
>  	if (fio->old_blkaddr == NULL_ADDR) {
>  		folio_clear_uptodate(folio);
> -		folio_clear_f2fs_gcing(folio);
>  		goto out_writepage;
>  	}
>  got_it:
> @@ -3680,6 +3697,11 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  	fio->version = ni.version;
>  
>  	__folio_start_writeback(folio, folio_test_dirty(folio));
> +	if (f2fs_folio_has_ffs(folio)) {
> +		ffs = (struct f2fs_folio_state *)folio->private;
> +		atomic_inc(&ffs->write_pages_pending);
> +		pending_added = true;
> +	}
>  
>  	if (fio->compr_blocks && fio->old_blkaddr == COMPRESS_ADDR)
>  		f2fs_i_compr_blocks_update(inode, fio->compr_blocks - 1, false);
> @@ -3694,6 +3716,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  		err = -EIO;
>  		goto out_writepage;
>  	}
> +	submitted = true;
>  	set_inode_flag(inode, FI_APPEND_WRITE);
>  	if (atomic_commit)
>  		folio_clear_f2fs_atomic(folio);
> @@ -3702,6 +3725,13 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  out:
>  	if (fio->need_lock == LOCK_REQ)
>  		f2fs_unlock_op(fio->sbi, &lc);
> +	if (!submitted) {
> +		if (pending_added) {
> +			f2fs_ffs_put_write_pending(folio, 1);
> +		} else if (!f2fs_ffs_has_dirty_subpage(folio)) {
> +			folio_clear_f2fs_gcing(folio);

If f2fs_trylock_op() failed:

	if (fio->need_lock == LOCK_RETRY) {
		if (!f2fs_trylock_op(fio->sbi, &lc)) {
			err = -EAGAIN;
			goto out_writepage;
		}
		fio->need_lock = LOCK_REQ;
	}

For order-0 folio, we may loss gcing flag becuase f2fs_ffs_has_dirty_subpage()
return false?

Two migration issues:

In f2fs_migrate_blocks() and f2fs_defragment_range(), we missed to mark subpage dirty?

			folio = f2fs_get_lock_data_folio(inode, blkidx, true);
			if (IS_ERR(folio)) {
				f2fs_up_write(&sbi->pin_sem);
				ret = PTR_ERR(folio);
				goto done;
			}

			folio_mark_dirty(folio);
			f2fs_folio_put(folio, true);

> +		}
> +	}
>  	return err;
>  }
>  
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 915ca72cf276..1b63dd961f15 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4493,6 +4493,7 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted,
>  				int compr_blocks, bool allow_balance);
>  bool f2fs_ffs_test_blk_uptodate(const struct folio *folio, pgoff_t index);
>  struct f2fs_folio_state *f2fs_ffs_find_or_alloc(struct folio *folio);
> +bool f2fs_ffs_test_blk_dirty(const struct folio *folio, pgoff_t index);
>  void f2fs_ffs_mark_subrange_dirty(struct folio *folio, size_t offset, size_t len);
>  bool f2fs_ffs_clear_subrange_dirty_and_test(struct folio *folio, size_t offset,
>  					    size_t len);
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index c66ca03d0d5a..c3e01f6811b4 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1520,12 +1520,19 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  						unsigned int segno, int off)
>  {
>  	struct folio *folio;
> +	size_t foff = 0;
> +	bool large = false;
>  	int err = 0;
>  
>  	folio = f2fs_get_lock_data_folio(inode, bidx, true);
>  	if (IS_ERR(folio))
>  		return PTR_ERR(folio);
>  
> +	if (f2fs_folio_has_ffs(folio)) {
> +		large = true;
> +		foff = offset_in_folio(folio, (loff_t)bidx << PAGE_SHIFT);
> +	}
> +
>  	if (!check_valid_map(F2FS_I_SB(inode), segno, off)) {
>  		err = -ENOENT;
>  		goto out;
> @@ -1540,6 +1547,8 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  			err = -EAGAIN;
>  			goto out;
>  		}
> +		if (large)
> +			f2fs_ffs_mark_subrange_dirty(folio, foff, PAGE_SIZE);
>  		folio_mark_dirty(folio);
>  		folio_set_f2fs_gcing(folio);
>  	} else {
> @@ -1558,28 +1567,37 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  			.need_lock = LOCK_REQ,
>  			.io_type = FS_GC_DATA_IO,
>  		};
> -		bool is_dirty = folio_test_dirty(folio);
> +		bool is_dirty = f2fs_ffs_test_blk_dirty(folio, bidx);
>  
>  retry:
>  		f2fs_folio_wait_writeback(folio, true, true);
>  
> +		if (large)
> +			f2fs_ffs_mark_subrange_dirty(folio, foff, PAGE_SIZE);
>  		folio_mark_dirty(folio);
>  		if (folio_clear_dirty_for_io(folio)) {
> -			inode_dec_dirty_block(inode);
> -			f2fs_remove_dirty_inode(inode);
> +			if (!large) {
> +				inode_dec_dirty_block(inode);
> +				f2fs_remove_dirty_inode(inode);
> +			} else if (f2fs_ffs_clear_subrange_dirty_and_test(folio,
> +							foff, PAGE_SIZE)) {
> +				folio_mark_dirty(folio);
> +			}
>  		}
>  
>  		folio_set_f2fs_gcing(folio);
>  
>  		err = f2fs_do_write_data_page(&fio);

Another race issue my local bots report, please take a look:

  Where:  move_data_page()  (gc.c:1564–1601) and the generic  writeback_iter()  used by  f2fs_write_cache_folios() .

  The cycle:

  1. FG_GC writes one subpage. Its bio waits in  write_io[DATA][COLD]  until  do_garbage_collect()  submits it later.
  2. If another subpage is still dirty, the folio is left both dirty and under writeback and is then unlocked.
  3. A writer doing fsync, sync or checkpoint calls  writeback_get_folio()  →  folio_prepare_writeback() . It locks the folio and sleeps in the generic  folio_wait_writeback() , which does not flush f2fs's merged bio.
  4. GC moves on to the next block of the same folio and blocks on  folio_lock . The bio is never submitted.

  Reachable today? Yes, on immutable large folios: BG_GC dirties subpage A, FG_GC migrates subpage B, and an fsync runs at the same time.

  Fix: in  move_data_page() , when  large && folio_test_dirty(folio)  after the write, call  f2fs_submit_merged_write_cond(sbi, NULL, folio, 0, DATA)  before unlocking. Optionally do the same in  f2fs_write_cache_folios()   out:  when it redirties a folio that is under writeback.

Thanks,

>  		if (err) {
> -			folio_clear_f2fs_gcing(folio);
>  			if (err == -ENOMEM) {
>  				memalloc_retry_wait(GFP_NOFS);
>  				goto retry;
>  			}
> -			if (is_dirty)
> +			if (is_dirty) {
> +				if (large)
> +					f2fs_ffs_mark_subrange_dirty(folio, foff, PAGE_SIZE);
>  				folio_mark_dirty(folio);
> +			}
>  		}
>  	}
>  out:
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index cbb65e0343c8..e017a5f2a67f 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -3863,7 +3863,7 @@ static int __get_segment_type_6(struct f2fs_io_info *fio)
>  		if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
>  			return CURSEG_COLD_DATA_PINNED;
>  
> -		if (page_private_gcing(fio->page)) {
> +		if (folio_test_f2fs_gcing(fio->folio)) {
>  			if (fio->sbi->am.atgc_enabled &&
>  				(fio->io_type == FS_DATA_IO) &&
>  				(fio->sbi->gc_mode != GC_URGENT_HIGH) &&