From: Vivek Goyal <email@example.com>
To: Amir Goldstein <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: Tue, 5 Jan 2021 11:26:46 -0500 [thread overview]
Message-ID: <20210105162646.GD3200@redhat.com> (raw)
On Tue, Jan 05, 2021 at 09:11:23AM +0200, Amir Goldstein wrote:
> > >
> > > What I would rather see is:
> > > - Non-volatile: first syncfs in every container gets an error (nice to have)
> > I am not sure why are we making this behavior per container. This should
> > be no different from current semantics we have for syncfs() on regular
> > filesystem. And that will provide what you are looking for. If you
> > want single error to be reported in all ovleray mounts, then make
> > sure you have one fd open in each mount after mount, then call syncfs()
> > on that fd.
> > Not sure why overlayfs behavior/semantics should be any differnt
> > than what regular filessytems like ext4/xfs are offering. Once we
> > get page cache sharing sorted out with xfs reflink, then people
> > will not even need overlayfs and be able to launch containers
> > just using xfs reflink and share base image. In that case also
> > they will need to keep an fd open per container they want to
> > see an error in.
> > So my patches exactly provide that. syncfs() behavior is same with
> > overlayfs as application gets it on other filesystems. And to me
> > its important to keep behavior same.
> > > - Volatile: every syncfs and every fsync in every container gets an error
> > > (important IMO)
> > For volatile mounts, I agree that we need to fail overlayfs instance
> > as soon as first error is detected since mount. And this applies to
> > not only syncfs()/fsync() but to read/write and other operations too.
> > For that we will need additional patches which are floating around
> > to keep errseq sample in overlay and check for errors in all
> > paths syncfs/fsync/read/write/.... and fail fs.
> > But these patches build on top of my patches.
> Here we disagree.
> I don't see how Jeff's patch is "building on top of your patches"
> seeing that it is perfectly well contained and does not in fact depend
> on your patches.
Jeff's patches are solving problem only for volatile mounts and they
are propagating error to overlayfs sb.
My patches are solving the issue both for volatile mount as well as
non-volatile mounts and solve it using same method so there is no
So there are multiple pieces to this puzzle and IMHO, it probably
should be fixed in this order.
A. First fix the syncfs() path to return error both for volatile as
as well non-volatile mounts.
B. And then add patches to fail filesystem for volatile mount as soon
as first error is detected (either in syncfs path or in other paths
like read/write/...). This probably will require to save errseq
in ovl_fs, and then compare with upper_sb in critical paths and fail
filesystem as soon as error is detected.
C. Finally fix the issues related to mount/remount error detection which
Sargun is wanting to fix. This will be largerly solved by B except
saving errseq on disk.
My patches should fix the first problem. And more patches can be
applied on top to fix issue B and issue C.
Now if we agree with this, in this context I see that fixing problem
B and C is building on top of my patches which fixes problem A.
> And I do insist that the fix for volatile mounts syncfs/fsync error
> reporting should be applied before your patches or at the very least
> not heavily depend on them.
I still don't understand that why volatile syncfs() error reporting
is more important than non-volatile syncfs(). But I will stop harping
on this point now.
My issue with Jeff's patches is that syncfs() error reporting should
be dealt in same way both for volatile and non-volatile mount. That
is compare file->f_sb_err and upper_sb->s_wb_err to figure out if
there is an error to report to user space. Currently this patches
only solve the problem for volatile mounts and use propagation to
overlay sb which is conflicting for non-volatile mounts.
IIUC, your primary concern with volatile mount is that you want to
detect as soon as writeback error happens, and flag it to container
manager so that container manager can stop container, throw away
upper layer and restart from scratch. If yes, what you want can
be solved by solving problem B and backporting it to LTS kernel.
I think patches for that will be well contained within overlayfs
(And no VFS) changes and should be relatively easy to backport.
IOW, backportability to LTS kernel should not be a concern/blocker
for my patch series which fixes syncfs() issue for overlayfs.
> volatile mount was introduced in fresh new v5.10, which is also an
> LTS kernel. It would be inconsiderate of volatile mount users and developers
> to make backporting that fix to v5.10.y any harder than it should be.
> > My patches don't solve this problem of failing overlay mount for
> > the volatile mount case.
> Here we agree.
> > >
> > > This is why I prefer to sample upper sb error on mount and propagate
> > > new errors to overlayfs sb (Jeff's patch).
> > Ok, I think this is one of the key points of the whole discussion. What
> > mechanism should be used to propagate writeback errors through overlayfs.
> > A. Propagate errors from upper sb to overlay sb.
> > B. Leave overlay sb alone and use upper sb for error checks.
> > We don't have good model to propagate errors between super blocks,
> > so Jeff preferred not to do error propagation between super blocks
> > for regular mounts.
> > https://firstname.lastname@example.org/
> > If we are not defining new semantics for syncfs() for overlayfs, then
> > I can't see what's the advantage of coming up with new mechanism to
> > propagate errors to overlay sb. Approach B should work just fine and
> > provide the syncfs() semantics we want for overlayfs (Same semantics
> > as other filesystems).
> Ok. I am on board with B.
> Philosophically. overlayfs model is somewhere between "passthrough"
> and "proxy" when handling pure upper files and as overlayfs evolves,
> it steadily moves towards the "proxy" model, with page cache and
> writeback being the largest remaining piece to convert.
> So I concede that as long as overlayfs writeback is mostly passthrough,
> syncfs might as well be passthrough to upper fs as well.
next prev parent reply other threads:[~2021-01-05 16:28 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
2021-01-04 22:44 ` Vivek Goyal
2021-01-05 7:11 ` Amir Goldstein
2021-01-05 16:26 ` Vivek Goyal [this message]
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).