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

[Daeho Jeong]

On Thu, Oct 22, 2026 at 8:02 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> On 10/23/26 10:31, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@xxxxxxxxxx>
> >
> > 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?

Hi Chao,

It is not specific to compression. Compressed writes are F2FS_WB_CP_DATA,
but the raw clusters of a compressed file and all other regular file data
are F2FS_WB_DATA, which a checkpoint does not wait for. fsync also goes
through a checkpoint for many other reasons in need_do_checkpoint()
(hardlinks, non-regular files, SBI_NEED_CP, fastboot, ...), and syncfs
always does. Compression just makes every fsync take that path, so
generic/455 hits it often.

I also checked it without compression: generic/455 with -o fastboot, which
makes every fsync go through a checkpoint as well, lost fsynced data in
6 of 100 runs without this patch and none of 100 runs with it.

You're right that a syncfs on a clean filesystem now issues one flush,
where it issued none before. SBI_IS_DIRTY cannot tell whether some data
completed after the last checkpoint flushed the devices, so a skipped
checkpoint cannot know whether the flush is needed. I think the cost is
mitigated, since it occurs, only when a sync checkpoint is
skipped. ext4_sync_fs() does the same: it sends a flush whenever the
journal commit will not, as data writeback can happen without a
transaction.

If you still want to avoid that flush, we can track whether any write
completed after the last flush:
- set a flag in the write end_io, f2fs_write_end_bio() and the direct IO
completion,
- clear it right before the checkpoint issues its flush,
- and flush in a skipped CP_SYNC/CP_FASTBOOT checkpoint only when the flag
is set.

Then a syncfs with nothing written does not flush. The downside is that
every write completion path has to set the flag, and missing one brings
the data loss back silently. Please let me know what you prefer.

>
> >
> > Signed-off-by: Daeho Jeong <daehojeong@xxxxxxxxxx>
> > ---
> >   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?

It only works for multiple devices, skips device 0, which the checkpoint
commit block normally flushes, and relies on the dirty_device bits that
the previous checkpoint has already cleared in this case. But you're right
that the new helper misses the retry and the f2fs_stop_checkpoint() on
failure. I'll extend f2fs_flush_device_cache() to flush all devices for
this case in v2.

Thanks,

>
> 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)
> >   {
>