Re: [f2fs-dev] [PATCH 05/14] f2fs: support large folio writeback

[Daeho Jeong]

On Mon, Sep 7, 2026 at 3:12 AM Nanzhe Zhao <nzzhao.sigma@xxxxxxxxx> wrote:
>
> On Thu, 27 Aug 2026 15:39:19 -0700, Daeho Jeong wrote:
> > If f2fs_write_single_data_folio() fails on subpage 1, jumping
> > to `out` unconditionally clears the dirty bitmap for the ENTIRE folio
> > (0 to folio_size),
> > and decrements the inode dirty pages.
> >
> > The unwritten dirty subpages lose their dirty tracking and will be
> > discarded cleanly
> > under memory pressure, causing silent data loss.
> >
> > Fix: Only clear dirty bits for successfully submitted subranges (`0,
> > pos`), and redirty
> > the folio if unwritten dirty subpages remain when an error occurs.
>
> Thanks for the review. Agreed.
> My change hunk may like this:
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> @@ f2fs_write_single_data_folio():
> -static int f2fs_write_single_data_folio(struct folio *folio, int *submitted,
> +static int f2fs_write_single_data_folio(struct folio *folio, int *submitted,
>                                         struct writeback_control *wbc,
>                                         enum iostat_type io_type,
> -                                       u64 start, u64 end)
> +                                       u64 *pos, u64 len)
>  {
>         struct inode *inode = folio->mapping->host;
>         struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>         bool atomic_commit = f2fs_is_atomic_file(inode) &&
>                                         folio_test_f2fs_atomic(folio);
>         struct inode *dn_inode = atomic_commit ?
>                                         F2FS_I(inode)->cow_inode : inode;
> -       u64 pos = folio_pos(folio);
> -       pgoff_t start_idx = (start - pos) >> PAGE_SHIFT;
> -       pgoff_t end_idx = (end - 1 - pos) >> PAGE_SHIFT;
> +       pgoff_t start_idx = (*pos - folio_pos(folio)) >> PAGE_SHIFT;
> +       pgoff_t end_idx = (*pos + len - 1 - folio_pos(folio)) >> PAGE_SHIFT;
>         int local_submitted = 0;
>         int err = 0;
> @@ IPU/OPU common write_done:
>                 err = f2fs_inplace_write_data(&fio);
>                 if (err) {
>                         if (fscrypt_inode_uses_fs_layer_crypto(inode))
>                                 fscrypt_finalize_bounce_page(
>                                                 &fio.encrypted_page);
>                         goto rollback_no_dnode;
>                 }
>
>                 local_submitted++;
>                 set_inode_flag(inode, FI_UPDATE_WRITE);
> -               continue;
> +               goto write_done;
>         }
>  ...
>                 f2fs_outplace_write_data(&dn, &fio);
>                 local_submitted++;
>                 set_inode_flag(inode, FI_APPEND_WRITE);
>                 trace_f2fs_do_write_data_page(folio, OPU);
>                 f2fs_put_dnode(&dn);
> -               continue;
> +               goto write_done;
> +
> +write_done:
> +               *pos = (u64)(data_idx + 1) << PAGE_SHIFT;
> +               continue;
> @@ f2fs_write_cache_folios loop + out:
>                 while ((r_len = ffs_find_dirty_range(folio, &pos, end_pos))) {
> -                       err = f2fs_write_single_data_folio(folio, &submitted,
> -                                       wbc, io_type, pos, pos + r_len);
> +                       err = f2fs_write_single_data_folio(folio, &submitted,
> +                                       wbc, io_type, &pos, r_len);
>                         folio_submitted += submitted;
>                         if (err)
>                                 goto out;
>
>                         nwritten += submitted;
> -                       pos += r_len;
>                 }
>  ...
>  out:
> -               f2fs_ffs_clear_subrange_dirty(folio, 0, folio_size(folio));
> -               inode_dec_dirty_pages(inode);
> +               if (f2fs_ffs_clear_subrange_dirty(folio, 0,
> +                                       (err ? pos : end_pos) - folio_pos))
> +                       folio_redirty_for_writepage(wbc, folio);
> +               else
> +                       inode_dec_dirty_pages(inode);
>
> Btw, I think the name f2fs_write_single_data_folio() is misleading now
> that it writes a dirty subrange of a large folio; would
> f2fs_write_folio_dirty_range() sound better?

Hi Nanzhe,

Thanks for the updated hunk. The logic looks good to me. The new name
of the function sounds better.

>
> Thanks,
> Nanzhe