From: Amir Goldstein <email@example.com>
To: Vivek Goyal <firstname.lastname@example.org>
Cc: Matthew Wilcox <email@example.com>,
Sargun Dhillon <firstname.lastname@example.org>,
Jeff Layton <email@example.com>,
Miklos Szeredi <firstname.lastname@example.org>, Jan Kara <email@example.com>,
NeilBrown <firstname.lastname@example.org>, Al Viro <email@example.com>,
Christoph Hellwig <firstname.lastname@example.org>,
Chengguang Xu <email@example.com>
Subject: Re: [PATCH 3/3] overlayfs: Report writeback errors on upper
Date: Mon, 4 Jan 2021 23:42:51 +0200 [thread overview]
Message-ID: <CAOQ4uxhYXeUt2iggM3oubdgr91QPNhUg2PdN128gRvR3rQoy1Q@mail.gmail.com> (raw)
On Mon, Jan 4, 2021 at 5:40 PM Vivek Goyal <firstname.lastname@example.org> wrote:
> On Mon, Jan 04, 2021 at 05:22:07PM +0200, Amir Goldstein wrote:
> > > > Since Jeff's patch is minimal, I think that it should be the fix applied
> > > > first and proposed for stable (with adaptations for non-volatile overlay).
> > >
> > > Does stable fix has to be same as mainline fix. IOW, I think atleast in
> > > mainline we should first fix it the right way and then think how to fix
> > > it for stable. If fixes taken in mainline are not realistic for stable,
> > > can we push a different small fix just for stable?
> > We can do a lot of things.
> > But if we are able to create a series with minimal (and most critical) fixes
> > followed by other fixes, it would be easier for everyone involved.
> I am not sure this is really critical. writeback error reporting for
> overlayfs are broken since the beginning for regular mounts. There is no
> notion of these errors being reported to user space. If that did not
> create a major issue, then why suddenly volatile mounts make it
> a critical issue.
Volatile mounts didn't make this a critical issue.
But this discussion made us notice a mildly serious issue.
It is not surprising to me that users did not report this issue.
Do you know what it takes for a user to notice that writeback had failed,
but an application did fsync and error did not get reported?
Filesystem durability guaranties are hard to prove especially with so
many subsystem layers and with fsync that does return an error correctly.
I once found a durability bug in fsync of xfs that existed for 12 years.
That fact does not at all make it any less critical.
> To me we should fix the issue properly which is easy to maintain
> down the line and then worry about doing a stable fix if need be.
> > >
> > > IOW, because we have to push a fix in stable, should not determine
> > > what should be problem solution for mainline, IMHO.
> > >
> > I find in this case there is a correlation between the simplest fix and the
> > most relevant fix for stable.
> > > The porblem I have with Jeff's fix is that its only works for volatile
> > > mounts. While I prefer a solution where syncfs() is fixed both for
> > > volatile as well as non-volatile mount and then there is less confusion.
> > >
> > I proposed a variation on Jeff's patch that covers both cases.
> > Sargun is going to work on it.
> What's the problem with my patches which fixes syncfs() error reporting
> for overlayfs both for volatile and non-volatile mount?
- mount 1000 overlays
- 1 writeback error recorded in upper sb
- syncfs (new fd) inside each of the 1000 containers
With your patch 3/3 only one syncfs will report an error for
both volatile and non-volatile cases. Right?
What I would rather see is:
- Non-volatile: first syncfs in every container gets an error (nice to have)
- Volatile: every syncfs and every fsync in every container gets an error
This is why I prefer to sample upper sb error on mount and propagate
new errors to overlayfs sb (Jeff's patch).
I am very much in favor of your patch 1/3 and I am not against the concept
of patches 2-3/3. Just think that ovl_errseq_check_advance() is not the
implementation that gives the most desirable result.
If people do accept my point of view that proxying the stacked error check
is preferred over "passthrough" to upper sb error check, then as a by-product,
the new ->check_error() method is not going to make much of a difference for
overlayfs. Maybe it can be used to fine tune some corner cases.
I am not sure.
If we do agree on the propagate error concept then IMO all other use
cases for not consuming the unseen error from upper fs are nice-to-have.
Before we continue to debate on the implementation, let's first try
to agree on the desired behavior, what is a must vs. what is nice to have.
Without consensus on this, it will be quite hard to converge.
Another thing, to help everyone, I think it is best that any patch on ovl_syncfs
"solutions" will include detailed description of the use cases it solves and
the use cases that it leaves unsolved.
next prev parent reply other threads:[~2021-01-04 23:11 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-21 19:50 [RFC PATCH 0/3][v3] vfs, overlayfs: Fix syncfs() to return correct errors Vivek Goyal
2020-12-21 19:50 ` [PATCH 1/3] vfs: Do not ignore return code from s_op->sync_fs Vivek Goyal
2020-12-22 1:23 ` NeilBrown
2020-12-22 15:17 ` Vivek Goyal
2020-12-21 19:50 ` [PATCH 2/3] vfs: Add a super block operation to check for writeback errors Vivek Goyal
2020-12-22 16:19 ` Matthew Wilcox
2020-12-22 16:25 ` Vivek Goyal
2020-12-23 12:44 ` Jeff Layton
2020-12-23 12:48 ` Jeff Layton
2021-01-04 19:41 ` Vivek Goyal
2020-12-21 19:50 ` [PATCH 3/3] overlayfs: Report writeback errors on upper Vivek Goyal
2020-12-22 16:20 ` Matthew Wilcox
2020-12-22 16:29 ` Vivek Goyal
2020-12-22 17:46 ` Matthew Wilcox
2020-12-22 17:55 ` Vivek Goyal
2020-12-23 12:53 ` Jeff Layton
2020-12-23 18:20 ` Sargun Dhillon
2020-12-23 18:50 ` Matthew Wilcox
2020-12-23 19:29 ` Sargun Dhillon
2020-12-23 20:07 ` Matthew Wilcox
2020-12-23 20:21 ` Sargun Dhillon
2020-12-23 20:44 ` Matthew Wilcox
2020-12-24 9:32 ` Amir Goldstein
2020-12-24 10:12 ` Sargun Dhillon
2020-12-24 12:13 ` Matthew Wilcox
2020-12-25 6:50 ` Amir Goldstein
2020-12-28 13:25 ` Jeff Layton
2020-12-28 15:51 ` Amir Goldstein
2021-01-04 15:51 ` Vivek Goyal
2020-12-28 15:56 ` Matthew Wilcox
2020-12-28 17:26 ` Jeff Layton
2020-12-28 19:25 ` Sargun Dhillon
2020-12-28 19:37 ` Amir Goldstein
2020-12-28 20:48 ` Matthew Wilcox
2021-01-02 13:25 ` Jeff Layton
2021-01-04 16:59 ` Vivek Goyal
2021-01-04 15:14 ` Vivek Goyal
2021-01-04 15:22 ` Amir Goldstein
2021-01-04 15:40 ` Vivek Goyal
2021-01-04 21:42 ` Amir Goldstein [this message]
2021-01-04 22:44 ` Vivek Goyal
2021-01-05 7:11 ` Amir Goldstein
2021-01-05 16:26 ` Vivek Goyal
2021-01-05 16:57 ` Amir Goldstein
2020-12-23 19:00 ` Jeff Layton
2021-01-04 20:00 ` 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).