Re: [PATCH v3 0/8] ovl: only sync what was written through the overlay

[Amir Goldstein]

On Fri, Nov 6, 2026 at 5:53 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> On Fri, Nov 06, 2026 at 04:32:50PM +0100, Christian Brauner wrote:
> > The thing that's a bit ugly is that overlayfs open-codes the tail of
> > sync_filesystems(): ovl_sync_fs() calling upper_sb->s_op->sync_fs() and
> > sync_blockdev() itself is one more place that needs to know what
> > sync_filesystem() does.
> >
> > So we should probably add a helper to fs/sync.c for the ->sync_fs() and
> > block device part and use that.
>
> v4 went out a couple of hours before this, it only swaps in v2 of Amir's
> patch, so all of this applies to it as well.
>
> I've added sync_fs_and_bdev(sb, wait) for v5, sync_filesystem() calls it
> twice and ovl_sync_fs() once.
>
> > Eheeeem, isn't that kind of a bad proxy for this? What ->sync_fs() +
> > block device do is a property of the upper filesystems and how is that
> > related to RENAME_WHITEOUT? There's a few counter examples you even
> > give:
>
> [...]
>
> > So on ubifs a syncfs() in relaxed mode returns with metadata still in
> > memory. Meh. I know the flag got droppped but that was an overlayfs
> > speicfic thing. A generic property of ->sync_fs() is a different thing?
>
> I dropped the flag after v1 because nobody liked it, me included, and I
> don't want to touch every file system for this.
>
> What relaxed leaves to ->sync_fs() and the block device is only the
> upper inodes that have no overlay inode, that's whiteouts and the work
> directories. With creation fixed (below) everything else is on the dirty
> list and gets written.
>
> For the file systems that pass the probe: ext4, xfs and btrfs log all
> metadata. ext4 without a journal has it in the buffers of the block
> device, which we write. f2fs writes its dirty inodes in the checkpoint,
> f2fs_sync_inode_meta(). ubifs writes names, whiteouts and links to its
> journal when they are made, only setattr leaves an inode dirty in the
> VFS. The only setattr overlayfs does on an inode it doesn't track is the
> chmod of the work directory to 0 at mount time, right after it was
> created with mode 0. That's the counter example from the cover letter,
> and it's the only one I could find.
>
> Anybody who needs more than that has fsync=on and fsync=strict, which
> sync the whole upper file system like before.
>

Another way of putting this is that this entire sub-thread revolves around
what should be the default value of OVERLAY_FS_FSYNC_AUTO
for upstream kernel, because the distros are likely to change it anyway.

AFAIK, the only upper fs which is going to be getting the wrong default
with the current OVERLAY_FS_FSYNC_AUTO=y is ubifs.

My rationale regarding ubifs is that it is a very specific and unique user
base for embedded/IoT/mobile devices.

The durability of overlayfs is already wrong on ubifs without
fsync=strict. We tried to fix this without opt-in, but we failed,
so using ubifs requires fsync=strict anyway for correct durability.

I admit that this is a weak excuse for regressing syncfs for ovl+ubifs,
but I am still leaning towards leaving OVERLAY_FS_FSYNC_AUTO=y
as the upstream default.

> > Afaict, creation isn't covered. ovl_instantiate() ends up in
> > ovl_inode_init() which now uses __ovl_copyattr(). So a new file,
> > directory, symlink or hardlink only dirties the parent but not the new
> > overlay inode.
> >
> > Fine for metadata, but a long symlink isn't metadata. We should mark the
> > new inode dirty where it's instantiated.
>
> Yup, ovl_instantiate() marks it in v5. Nothing gets lost today, ext4
> journals the block of a long symlink, xfs logs it, btrfs has it in the
> tree and f2fs writes it in f2fs_symlink(), but we shouldn't depend on
> that. I've added a long symlink to the fstest.
>
> > * ovl_park_inode() calls mark_inode_dirty_sync(). Jan's deferred inode
> >   reclaim (cf. [1]) removes exactly that from iput() because it does IO
> >   from dentry reclaim.
> >
> >   He replaces it with queue_dirtytime_writeback(). That moves the inode
> >   to the head of b_dirty_time and wakes the flusher.
> >
> > So it would be nice if we could base your thing on top of Jan's series
> > (I hope he'll send v2 soon!) and call queue_dirtytime_writeback() thing.
>
> The one in ovl_park_inode() is called from ->write_inode(), not from
> iput(), and it's on the overlay inode, which has no ->dirty_inode(), so
> it only moves it to another list.
>
> The one that does IO from dentry reclaim is iput() of an overlay inode
> with I_DIRTY_TIME, that's sync_lazytime() -> ovl_sync_lazytime() ->
> mark_inode_dirty_sync(upper). Jan's patch 2 takes that out of iput(), so
> it goes away without any change on my side, and then using
> queue_dirtytime_writeback() in ovl_park_inode() makes sense too. It
> would have to be exported for that.
>
> I only need patches 1 and 2 of that series, and they don't depend on the
> rest of it. Jan, could those two go in ahead of the deferred reclaim
> part? Then I'll rebase on top of them. Otherwise I'd rather do this as a
> follow up than wait for the whole thing.
>
> > > An overlay inode that loses its last reference while it is dirty is
> > > kept until it has been written and then sits on the LRU, like with
> > > any other file system.
> >
> > Hm, Jan's series fixes that as well afaict. :)
>
> I don't see how, what am I missing? overlayfs has inode_just_drop() for
> ->drop_inode(), so a dirty overlay inode is evicted on the last iput()
> and we forget that the upper inode has to be written. Patch 6 keeps it
> around until it's clean.
>
> > There's some Sashiko thing btw you should look into?
>
> The only mails I got from it are on v1, those are fixed since v2, except
> for dirtied_time_when of a parked inode, which I left alone on purpose,
> it's in the v2 changelog.

I am not sure what the current policy is for Sashiko and emails for ovl reviews.

There are 3 high severity comments on v3 that are still in v4:
https://sashiko.dev/#/patchset/20261106-b4-ovl-containerized-sync-v4-0-6ddde0b7dd8a%40toxicpanda.com

Thanks,
Amir.