From: Amir Goldstein <amir73il@gmail.com>
To: Sargun Dhillon <sargun@sargun.me>
Cc: Jeff Layton <jlayton@redhat.com>,
overlayfs <linux-unionfs@vger.kernel.org>,
Miklos Szeredi <miklos@szeredi.hu>,
Alexander Viro <viro@zeniv.linux.org.uk>,
Giuseppe Scrivano <gscrivan@redhat.com>,
Vivek Goyal <vgoyal@redhat.com>,
Daniel J Walsh <dwalsh@redhat.com>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
David Howells <dhowells@redhat.com>
Subject: Re: [PATCH v2 2/4] overlay: Document current outstanding shortcoming of volatile
Date: Tue, 1 Dec 2020 13:29:21 +0200 [thread overview]
Message-ID: <CAOQ4uxiVLY14=3zDwcRysw69=EN=2vVq5y8JP0Q72Cz95qJ6ng@mail.gmail.com> (raw)
In-Reply-To: <20201201110928.GA24837@ircssh-2.c.rugged-nimbus-611.internal>
> syncfs seems to enforce the semantics laid out by VFS[1]. Specifically the
> statement:
> When there is an error during writeback, they expect that error to be reported
> when a file sync request is made. After an error has been reported on one
> request, subsequent requests on the same file descriptor should return 0, unless
> further writeback errors have occurred since the previous file syncronization.
>
> This is enforced by the errseq_check_and_advance logic. We can hack around this
> logic by resetting the errset (setting the error on it) every time we get the
> sync_fs callback, but that to me seems wrong. FWIW, implementing this behaviour
> for fdatasync, and fsync is easier, because the error is bubbled up from the
> filesystem to the VFS. I don't actually think this is a good idea because
> it seems like this sync_fs behaviour is a bit...not neccessarily what all
> filesystems expect. For example, btrfs_sync_fs returns an error if it
> is unable to finish the current transaction. Nowhere in the btrfs code
> does it set the errseq on the superblock if this fails.
>
> I think we have a couple paths forward:
> 1. Change the semantic of sync_fs so the error is always bubbled up if
> it returns a non-zero value. If we do this, we have to decide whether
> or not we would _also_ call errseq_check_and_advance on the SB,
> or leave that to a subsequent call.
> 2. Have overlayfs forcefully set an error on the superblock on every
> callback to sync_fs. This seems ugly, but I wrote a little patch,
> and it seems to solve the problem for all the fsync / fdatasync /
> sync / syncfs variants without having to do plumbing in VFS.
> 3. Choose a different set of semantics for how we want to handle
> errors in volatile mounts.
>
IMO it is best if you post your patch to fix volatile overlayfs, because
it seems to me that the volatile overlayfs issue is worse than generic
sync_fs issues and it's good if we had a small backportable patch
even if a bit ugly.
Later you can pursue the sync_fs semantic fixes if you wish they
do not contradict the fix in overlayfs, just are just a way to remove
a hack.
Thanks,
Amir.
next prev parent reply other threads:[~2020-12-01 11:30 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-27 9:20 [PATCH v2 0/4] Make overlayfs volatile mounts reusable Sargun Dhillon
2020-11-27 9:20 ` [PATCH v2 1/4] fs: Add s_instance_id field to superblock for unique identification Sargun Dhillon
2020-11-27 9:20 ` [PATCH v2 2/4] overlay: Document current outstanding shortcoming of volatile Sargun Dhillon
2020-11-27 12:52 ` Amir Goldstein
2020-11-27 22:11 ` Sargun Dhillon
2020-11-28 2:01 ` Jeff Layton
2020-11-28 4:45 ` Sargun Dhillon
2020-11-28 7:12 ` Amir Goldstein
2020-11-28 8:52 ` Sargun Dhillon
2020-11-28 9:04 ` Amir Goldstein
2020-12-01 11:09 ` Sargun Dhillon
2020-12-01 11:29 ` Amir Goldstein [this message]
2020-12-01 13:01 ` Jeff Layton
2020-12-01 15:24 ` Vivek Goyal
2020-12-01 16:10 ` Jeff Layton
2020-11-28 12:04 ` Jeff Layton
2020-11-28 8:56 ` Amir Goldstein
2020-11-28 9:06 ` Amir Goldstein
2020-11-27 9:20 ` [PATCH v2 3/4] overlay: Add the ability to remount volatile directories when safe Sargun Dhillon
2020-11-27 11:09 ` kernel test robot
2020-11-27 13:04 ` Amir Goldstein
2020-12-07 11:39 ` Dan Carpenter
2020-11-27 9:20 ` [PATCH v2 4/4] overlay: Add rudimentary checking of writeback errseq on volatile remount Sargun Dhillon
2020-11-30 18:43 ` Vivek Goyal
2020-11-30 19:15 ` Vivek Goyal
2020-12-05 9:13 ` Amir Goldstein
2020-12-05 13:51 ` Jeff Layton
2020-12-05 14:51 ` Amir Goldstein
2020-11-30 19:33 ` Vivek Goyal
2020-12-01 11:56 ` Sargun Dhillon
2020-12-01 12:45 ` Jeff Layton
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOQ4uxiVLY14=3zDwcRysw69=EN=2vVq5y8JP0Q72Cz95qJ6ng@mail.gmail.com' \
--to=amir73il@gmail.com \
--cc=dhowells@redhat.com \
--cc=dwalsh@redhat.com \
--cc=gscrivan@redhat.com \
--cc=jlayton@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=sargun@sargun.me \
--cc=vgoyal@redhat.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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).