From: Sargun Dhillon <firstname.lastname@example.org>
To: Amir Goldstein <email@example.com>
Cc: Vivek Goyal <firstname.lastname@example.org>,
Miklos Szeredi <email@example.com>,
Alexander Viro <firstname.lastname@example.org>,
Giuseppe Scrivano <email@example.com>,
Daniel J Walsh <firstname.lastname@example.org>,
David Howells <email@example.com>,
Chengguang Xu <firstname.lastname@example.org>,
Christoph Hellwig <email@example.com>, NeilBrown <firstname.lastname@example.org>,
Jan Kara <email@example.com>, Jeff Layton <firstname.lastname@example.org>,
Matthew Wilcox <email@example.com>
Subject: Re: [PATCH v3] overlay: Implement volatile-specific fsync error behaviour
Date: Thu, 7 Jan 2021 00:02:10 -0800 [thread overview]
Message-ID: <CAMp4zn_fgpsDjObTpXk9=sDEdQf3dhod+54ZvwogNLcQwR--UQ@mail.gmail.com> (raw)
On Wed, Jan 6, 2021 at 11:02 PM Amir Goldstein <firstname.lastname@example.org> wrote:
> On Wed, Jan 6, 2021 at 9:47 PM Vivek Goyal <email@example.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://firstname.lastname@example.org/
> 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.
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.
next prev parent reply other threads:[~2021-01-07 8: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
2021-01-07 8:02 ` Sargun Dhillon [this message]
2021-01-07 13:44 ` Vivek Goyal
2021-01-07 14:44 ` Amir Goldstein
2021-01-07 14:57 ` Vivek Goyal
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 \
* 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).