Re: [f2fs-dev] [PATCH] f2fs: flush devices when a sync checkpoint is skipped

[Chao Yu <chao@kernel.org>]

On 10/23/26 10:31, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> When fsync needs a checkpoint, f2fs_do_sync_file() writes and waits for
> the file data and then relies on the checkpoint to flush the device
> write caches. syncfs works the same way through f2fs_sync_fs(). But
> f2fs_write_checkpoint() skips a CP_SYNC or CP_FASTBOOT checkpoint
> without any flush when SBI_IS_DIRTY is clear.
> 
> That can happen even though the data was just written. A checkpoint
> does not wait for F2FS_WB_DATA writes, so another checkpoint can commit
> the new block addresses of the file while its data writes are still in
> flight, flush the devices before they complete, and clear SBI_IS_DIRTY.
> The checkpoint requested by fsync is then skipped, and fsync returns
> while the data may still be in a volatile write cache. After a power
> cut, the file points to blocks that were never persisted.
> 
> Flush the write caches of all devices when a sync checkpoint is skipped.
> 
> This was found by generic/455 with compression enabled for all files,
> where every fsync goes through a checkpoint. On a 4KB page kernel, 3 of
> 100 runs lost fsynced blocks without this patch and none of 100 runs did
> with it.

Seems we only find this issue on compressed file, can we fix compression
case only? If compression is disable, it will issue unnecessary preflush
during syncfs?

Maybe we can add a reference count to track writebacked compressed page.

- increase it during writeback
- decrease it in write_end_io
- in f2fs_write_checkpoint
  - skip if it's zero
  - otherwise, wait it to be zero & issue flush

Thoughts?

> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>   fs/f2fs/checkpoint.c | 18 ++++++++++++++----
>   fs/f2fs/f2fs.h       |  1 +
>   fs/f2fs/segment.c    | 17 +++++++++++++++++
>   3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 1ed38c668fc..158d97fc349 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1973,10 +1973,20 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>   
>   	stat_cp_time(cpc, CP_TIME_LOCK);
>   
> -	if (!is_sbi_flag_set(sbi, SBI_IS_DIRTY) &&
> -		((cpc->reason & CP_FASTBOOT) || (cpc->reason & CP_SYNC) ||
> -		((cpc->reason & CP_DISCARD) && !sbi->discard_blks)))
> -		goto out;
> +	if (!is_sbi_flag_set(sbi, SBI_IS_DIRTY)) {
> +		if (cpc->reason & (CP_FASTBOOT | CP_SYNC)) {
> +			/*
> +			 * The data that fsync or syncfs waited for may have
> +			 * completed after the last checkpoint flushed the
> +			 * devices, and that checkpoint may already point to
> +			 * it, so flush the devices.
> +			 */
> +			err = f2fs_flush_all_devices(sbi);
> +			goto out;
> +		}
> +		if ((cpc->reason & CP_DISCARD) && !sbi->discard_blks)
> +			goto out;
> +	}
>   	if (unlikely(f2fs_cp_error(sbi))) {
>   		err = -EIO;
>   		goto out;
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 1f9bd8e3b52..a4a38818988 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4265,6 +4265,7 @@ void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi, bool from_bg);
>   int f2fs_issue_flush(struct f2fs_sb_info *sbi, nid_t ino);
>   int f2fs_create_flush_cmd_control(struct f2fs_sb_info *sbi);
>   int f2fs_flush_device_cache(struct f2fs_sb_info *sbi);
> +int f2fs_flush_all_devices(struct f2fs_sb_info *sbi);
>   void f2fs_destroy_flush_cmd_control(struct f2fs_sb_info *sbi, bool free);
>   void f2fs_invalidate_blocks(struct f2fs_sb_info *sbi, block_t addr,
>   						unsigned int len);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 06fb92fc847..5b015cf9071 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -822,6 +822,23 @@ int f2fs_flush_device_cache(struct f2fs_sb_info *sbi)
>   	return ret;
>   }
>   
> +/*
> + * Flush the write caches of all devices, regardless of which ones received
> + * writes since the last checkpoint.
> + */
> +int f2fs_flush_all_devices(struct f2fs_sb_info *sbi)

I guess there is an existing function f2fs_flush_device_cache(), I feel we'd
better reuse it to avoid missing any functionality inside it, right?

Thanks,

> +{
> +	int ret, i;
> +
> +	if (test_opt(sbi, NOBARRIER))
> +		return 0;
> +
> +	ret = __submit_flush_wait(sbi, sbi->sb->s_bdev);
> +	for (i = 1; !ret && i < sbi->s_ndevs; i++)
> +		ret = __submit_flush_wait(sbi, FDEV(i).bdev);
> +	return ret;
> +}
> +
>   static void __locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno,
>   		enum dirty_type dirty_type)
>   {