Re: [PATCH v2 2/4] overlay: Document current outstanding shortcoming of volatile

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: Sat, 28 Nov 2020 11:04:37 +0200	[thread overview]
Message-ID: <CAOQ4uxjKRL0Pime7BO9gr_sVfmdhV2XrPhSzOPdDoaCAcRYHBQ@mail.gmail.com> (raw)
In-Reply-To: <20201128085227.GB28230@ircssh-2.c.rugged-nimbus-611.internal>

> > What I suggested was a solution only for the volatile overlay issue
> > where data can vaporise without applications noticing:
> > "...the very minimum is to check for errseq since mount on the fsync
> >  and syncfs calls."
> >
> Yeah, I was confusing the checking that VFS does on our behalf and the checking
> that we can do ourselves in the sync callback. If we return an error prior to
> the vfs checking it short-circuits that entirely.
>
> > Do you get it? there is no pre-file state in the game, not for fsync and not
> > for syncfs.
> >
> > Any single error, no matter how temporary it is and what damage it may
> > or may not have caused to upper layer consistency, permanently
> > invalidates the reliability of the volatile overlay, resulting in:
> > Effective immediately: every fsync/syncfs returns EIO.
> > Going forward: maybe implement overlay shutdown, so every access
> > returns EIO.
> >
> > So now that I hopefully explained myself better, I'll ask again:
> > Am I wrong saying that it is very very simple to fix?
> > Would you mind making that fix at the bottom of the patch series, so it can
> > be easily applied to stable kernels?
> >
> > Thanks,
> > Amir.
>
> I think that this should be easy enough if the semantic is such that volatile
> overlayfs mounts will return EIO on syncfs on every syncfs call if the upperdir's
> super block has experienced errors since the initial mount. I imagine we do not
> want to make it such that if the upperdir has ever experienced errors, return
> EIO on syncfs.
>
> The one caveat that I see is that if the errseq wraps, we can silently begin
> swallowing errors again. Thus, on the first failed syncfs we should just
> store a flag indicating that the volatile fs is bad, and to continue to return
> EIO rather than go through the process of checking errseq_t, but that's easy
> enough to write.

I agree. I sent another reply to your question about testing.
The test I suggested generic/019, only tests that the first fsync
after writeback
error fails and that umount succeeds, so logic is good for volatile overlay.

Thanks,
Amir.