Re: [f2fs-dev] [PATCH 07/14] f2fs: make GC migration large-folio aware

[Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

On 8/26/26 16:26, Nanzhe Zhao wrote:
> 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(), f2fs_find_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.
> 
> Signed-off-by: Nanzhe Zhao <zhaonanzhe@xiaomi.com>
> ---
>   fs/f2fs/data.c | 96 ++++++++++++++++++++++++++++++++++----------------
>   fs/f2fs/f2fs.h |  6 ++--
>   fs/f2fs/gc.c   | 30 ++++++++++++++--
>   3 files changed, 97 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 608d8ea8e607..0e54b1e25893 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1241,19 +1241,31 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode,
>   
>   /* This can handle encryption stuffs */
>   static void f2fs_submit_page_read(struct inode *inode, struct fsverity_info *vi,
> -				  struct folio *folio, block_t blkaddr,
> -				  blk_opf_t op_flags, bool for_write)
> +				  struct folio *folio, pgoff_t index,
> +				  block_t blkaddr, blk_opf_t op_flags,
> +				  bool for_write)
>   {
>   	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   	struct bio *bio;
> +	size_t offset = 0;
>   
> -	bio = f2fs_grab_read_bio(inode, vi, blkaddr, 1, op_flags, folio->index,
> +	if (f2fs_folio_has_ffs(folio)) {
> +		struct f2fs_folio_state *ffs = folio->private;
> +		unsigned long flags;
> +
> +		offset = offset_in_folio(folio, (loff_t)index << PAGE_SHIFT);



> +		spin_lock_irqsave(&ffs->state_lock, flags);
> +		ffs->read_pages_pending++;
> +		spin_unlock_irqrestore(&ffs->state_lock, flags);
> +	}

How about wrapping this to a function, and use it to clean up the codes.

f2fs_update_read_folio_pending(, pages)

> +
> +	bio = f2fs_grab_read_bio(inode, vi, blkaddr, 1, op_flags, index,
>   				 for_write);
>   
>   	/* wait for GCed page writeback via META_MAPPING */
>   	f2fs_wait_on_block_writeback(inode, blkaddr);
>   
> -	if (!bio_add_folio(bio, folio, PAGE_SIZE, 0))
> +	if (!bio_add_folio(bio, folio, PAGE_SIZE, offset))
>   		f2fs_bug_on(sbi, 1);
>   
>   	inc_page_count(sbi, F2FS_RD_DATA);
> @@ -1399,21 +1411,13 @@ 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 (folio_test_large(folio))
> +		f2fs_ffs_find_or_alloc(folio);
>   
>   	if (f2fs_lookup_read_extent_cache_block(inode, index,
>   						&dn.data_blkaddr)) {
> @@ -1448,7 +1452,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;
>   	}
> @@ -1461,15 +1465,17 @@ 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;
>   	}
>   
> -	f2fs_submit_page_read(inode, f2fs_need_verity(inode, folio->index),
> -			      folio, dn.data_blkaddr, op_flags, for_write);
> +	f2fs_submit_page_read(inode, f2fs_need_verity(inode, index),
> +			      folio, index, dn.data_blkaddr, op_flags, for_write);
>   	return folio;
>   
>   put_err:
> @@ -1486,7 +1492,7 @@ struct folio *f2fs_find_data_folio(struct inode *inode, pgoff_t index,
>   	folio = f2fs_filemap_get_folio(mapping, index, FGP_ACCESSED, 0);
>   	if (IS_ERR(folio))
>   		goto read;
> -	if (folio_test_uptodate(folio))
> +	if (f2fs_ffs_test_blk_uptodate(folio, index))
>   		return folio;
>   	f2fs_folio_put(folio, false);
>   
> @@ -1495,11 +1501,11 @@ struct folio *f2fs_find_data_folio(struct inode *inode, pgoff_t index,
>   	if (IS_ERR(folio))
>   		return folio;
>   
> -	if (folio_test_uptodate(folio))
> +	if (f2fs_ffs_test_blk_uptodate(folio, index))
>   		return folio;
>   
>   	folio_wait_locked(folio);
> -	if (unlikely(!folio_test_uptodate(folio))) {
> +	if (unlikely(!f2fs_ffs_test_blk_uptodate(folio, index))) {
>   		f2fs_folio_put(folio, false);
>   		return ERR_PTR(-EIO);
>   	}
> @@ -1523,7 +1529,8 @@ 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 || !folio_test_uptodate(folio))) {
> +	if (unlikely(folio->mapping != mapping ||
> +		    !f2fs_ffs_test_blk_uptodate(folio, index))) {
>   		f2fs_folio_put(folio, true);
>   		return ERR_PTR(-EIO);
>   	}
> @@ -2681,6 +2688,23 @@ 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;
> +
> +	if (!f2fs_folio_has_ffs(folio))
> +		return folio_test_dirty(folio);
> +
> +	if (!folio_contains(folio, index))
> +		return false;

Can we change this to a sanity check?

> +
> +	ffs = folio->private;
> +	nr_subpages = folio_nr_pages(folio);
> +	idx = index - folio->index;
> +	return test_bit(nr_subpages + idx, ffs->state);
> +}
> +
>   void f2fs_ffs_mark_subrange_dirty(struct folio *folio,
>   				    size_t offset, size_t len)
>   {
> @@ -2719,18 +2743,30 @@ static bool __ffs_clear_subrange_dirty(struct folio *folio,
>   			2 * nr_subpages;
>   }
>   
> -void f2fs_ffs_clear_subrange_dirty(struct folio *folio, size_t offset, size_t len)
> +/**
> + * f2fs_ffs_clear_subrange_dirty - clear the dirty bits of a folio subrange
> + * @folio: the large folio
> + * @offset: start byte offset within the folio
> + * @len: byte length of the subrange
> + *
> + * Clear the dirty bits of the 4K subpages covered by [offset, offset + len),
> + * and return whether the folio still has any dirty subpage left.
> + */
> +bool f2fs_ffs_clear_subrange_dirty(struct folio *folio, size_t offset,
> +				   size_t len)
>   {
>   	struct f2fs_folio_state *ffs;
>   	unsigned long flags;
> +	bool dirty;
>   
>   	if (!f2fs_folio_has_ffs(folio))
> -		return;
> +		return false;
>   
> -	ffs = (struct f2fs_folio_state *)folio->private;
> +	ffs = folio->private;
>   	spin_lock_irqsave(&ffs->state_lock, flags);
> -	__ffs_clear_subrange_dirty(folio, ffs, offset, len);
> +	dirty = __ffs_clear_subrange_dirty(folio, ffs, offset, len);
>   	spin_unlock_irqrestore(&ffs->state_lock, flags);
> +	return dirty;
>   }
>   
>   static unsigned int ffs_next_dirty_subpage(struct f2fs_folio_state *ffs,
> @@ -4853,7 +4889,7 @@ static int f2fs_write_begin(const struct kiocb *iocb,
>   		 */
>   		f2fs_submit_page_read(inode,
>   				      NULL, /* can't write to fsverity files */
> -				      folio, blkaddr, 0, true);
> +				      folio, index, blkaddr, 0, true);
>   
>   		folio_lock(folio);
>   		if (unlikely(folio->mapping != mapping)) {
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 71e6d7e34c7b..0ffbe2bd04c8 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4344,10 +4344,10 @@ 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);
> -void f2fs_ffs_clear_subrange_dirty(struct folio *folio, size_t offset, size_t len);
> +bool f2fs_ffs_clear_subrange_dirty(struct folio *folio, size_t offset,
> +				   size_t len);
>   void f2fs_write_failed(struct inode *inode, loff_t to);
>   void f2fs_invalidate_folio(struct folio *folio, size_t offset, size_t length);
>   bool f2fs_release_folio(struct folio *folio, gfp_t wait);
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 0c17038fcfd7..42e6dfcbf98d 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1504,12 +1504,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;
> @@ -1524,6 +1531,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 {
> @@ -1536,32 +1545,49 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>   			.op_flags = REQ_SYNC,
>   			.old_blkaddr = NULL_ADDR,
>   			.folio = folio,
> +			.folio_offset = bidx - folio->index,
> +			.folio_blkcnt = 1,
>   			.encrypted_page = NULL,
>   			.need_lock = LOCK_REQ,
>   			.io_type = FS_GC_DATA_IO,
>   		};
> -		bool is_dirty = folio_test_dirty(folio);
> +		struct f2fs_folio_state *ffs = NULL;
> +		bool is_dirty = f2fs_ffs_test_blk_dirty(folio, bidx);
>   
>   retry:
>   		f2fs_folio_wait_writeback(folio, DATA, true, true);
>   
> +		if (large) {
> +			ffs = folio->private;
> +			f2fs_ffs_mark_subrange_dirty(folio, foff, PAGE_SIZE);
> +		}
>   		folio_mark_dirty(folio);
>   		if (folio_clear_dirty_for_io(folio)) {
>   			inode_dec_dirty_pages(inode);
>   			f2fs_remove_dirty_inode(inode);
> +			if (large &&
> +			    f2fs_ffs_clear_subrange_dirty(folio, foff, PAGE_SIZE))
> +				folio_mark_dirty(folio);

Why do we need to mark folio dirty again after clearing dirty?

Thanks,

>   		}
>   
> +		if (large)
> +			atomic_inc(&ffs->write_pages_pending);
>   		folio_set_f2fs_gcing(folio);
>   
>   		err = f2fs_do_write_data_page(&fio);
>   		if (err) {
>   			folio_clear_f2fs_gcing(folio);
> +			if (large)
> +				atomic_dec(&ffs->write_pages_pending);
>   			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:



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel