Re: [PATCH v3] overlay: Implement volatile-specific fsync error behaviour

[Sargun Dhillon <sargun@sargun.me>]

On Wed, Jan 6, 2021 at 11:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jan 6, 2021 at 9:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Jan 06, 2021 at 12:35:46AM -0800, Sargun Dhillon wrote:
> > > Overlayfs's volatile option allows the user to bypass all forced sync calls
> > > to the upperdir filesystem. This comes at the cost of safety. We can never
> > > ensure that the user's data is intact, but we can make a best effort to
> > > expose whether or not the data is likely to be in a bad state.
> > >
> > > The best way to handle this in the time being is that if an overlayfs's
> > > upperdir experiences an error after a volatile mount occurs, that error
> > > will be returned on fsync, fdatasync, sync, and syncfs. This is
> > > contradictory to the traditional behaviour of VFS which fails the call
> > > once, and only raises an error if a subsequent fsync error has occurred,
> > > and been raised by the filesystem.
> > >
> > > One awkward aspect of the patch is that we have to manually set the
> > > superblock's errseq_t after the sync_fs callback as opposed to just
> > > returning an error from syncfs. This is because the call chain looks
> > > something like this:
> > >
> > > sys_syncfs ->
> > >       sync_filesystem ->
> > >               __sync_filesystem ->
> > >                       /* The return value is ignored here
> > >                       sb->s_op->sync_fs(sb)
> > >                       _sync_blockdev
> > >               /* Where the VFS fetches the error to raise to userspace */
> > >               errseq_check_and_advance
> > >
> > > Because of this we call errseq_set every time the sync_fs callback occurs.
> >
> > Why not start capturing return code of ->sync_fs and then return error
> > from ovl->sync_fs. And then you don't have to do errseq_set(ovl_sb).
> >
> > I already posted a patch to capture retrun code from ->sync_fs.
> >
> > https://lore.kernel.org/linux-fsdevel/20201221195055.35295-2-vgoyal@redhat.com/
> >
> >
>
> Vivek,
>
> IMO the more important question is "Why not?".
>
> Your patches will undoubtedly get to mainline in the near future and they do
> make the errseq_set(ovl_sb) in this patch a bit redundant, but I really see no
> harm in it. It is very simple for you to remove this line in your patch.
> I do see the big benefit of an independent patch that is easy to apply to fix
> a fresh v5.10 feature.
>
> I think it is easy for people to dismiss the importance of "syncfs on volatile"
> which sounds like a contradiction, but it is not.
> The fact that the current behavior is documented doesn't make it right either.
> It just makes our review wrong.
> The durability guarantee (that volatile does not provide) is very different
> from the "reliability" guarantee that it CAN provide.
> We do not want to have to explain to people that "volatile" provided different
> guarantees depending on the kernel they are running.
> Fixing syncfs/fsync of volatile is much more important IMO than erroring
> on other fs ops post writeback error, because other fs ops are equally
> unreliable on any filesystem in case application did not do fsync.
>
> Ignoring the factor of "backporting cost" when there is no engineering
> justification to do so is just ignoring the pain of others.
> Do you have an engineering argument for objecting this patch is
> applied before your fixes to syncfs vfs API?
>
> Sargun,
>
> Please add Fixes/Stable #v5.10 tags.
>
> Thanks,
> Amir.

I was going to send the patch to stable once it was picked up in
the unionfs tree. I will resend / re-roll with a CC to stable.