archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <>
To: Vivek Goyal <>
Cc: Sargun Dhillon <>,
	overlayfs <>,
	Miklos Szeredi <>,
	Alexander Viro <>,
	Giuseppe Scrivano <>,
	Daniel J Walsh <>,
	linux-fsdevel <>,
	David Howells <>,
	Chengguang Xu <>,
	Christoph Hellwig <>, NeilBrown <>,
	Jan Kara <>, Jeff Layton <>,
	Matthew Wilcox <>
Subject: Re: [PATCH v3] overlay: Implement volatile-specific fsync error behaviour
Date: Thu, 7 Jan 2021 09:02:00 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Wed, Jan 6, 2021 at 9:47 PM Vivek Goyal <> 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.


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?


Please add Fixes/Stable #v5.10 tags.


  parent reply	other threads:[~2021-01-07  7:03 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-06  8:35 [PATCH v3] overlay: Implement volatile-specific fsync error behaviour Sargun Dhillon
2021-01-06  9:34 ` Amir Goldstein
2021-01-06 19:46 ` Vivek Goyal
2021-01-07  3:51   ` Sargun Dhillon
2021-01-07  7:02   ` Amir Goldstein [this message]
2021-01-07  8:02     ` Sargun Dhillon
2021-01-07 13:44     ` Vivek Goyal
2021-01-07 14:44       ` Amir Goldstein
2021-01-07 14:57         ` Vivek Goyal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).