Re: [PATCH 3/3] overlayfs: Report writeback errors on upper

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Dec 28, 2020 at 03:56:18PM +0000, Matthew Wilcox wrote:
> On Mon, Dec 28, 2020 at 08:25:50AM -0500, Jeff Layton wrote:
> > To be clear, the main thing you'll lose with the method above is the
> > ability to see an unseen error on a newly opened fd, if there was an
> > overlayfs mount using the same upper sb before your open occurred.
> > 
> > IOW, consider two overlayfs mounts using the same upper layer sb:
> > 
> > ovlfs1				ovlfs2
> > ----------------------------------------------------------------------
> > mount
> > open fd1
> > write to fd1
> > <writeback fails>
> > 				mount (upper errseq_t SEEN flag marked)
> > open fd2
> > syncfs(fd2)
> > syncfs(fd1)
> > 
> > 
> > On a "normal" (non-overlay) fs, you'd get an error back on both syncfs
> > calls. The first one has a sample from before the error occurred, and
> > the second one has a sample of 0, due to the fact that the error was
> > unseen at open time.
> > 
> > On overlayfs, with the intervening mount of ovlfs2, syncfs(fd1) will
> > return an error and syncfs(fd2) will not. If we split the SEEN flag into
> > two, then we can ensure that they both still get an error in this
> > situation.
> 
> But do we need to?  If the inode has been evicted we also lose the errno.

That's for the case of fsync(), right? For the case of syncfs() we will
not lose error as its stored in super_block.

Even for the case of fsync(), inode can be evicted only if no other
fd is opened for the file. So in above example, fd1 is opened so
inode can't be evicted, that means we will see error on syncfs(fd2)
and not lose it.

So if we start consuming upper fs on overlay mount(), it will be a
change of behavior for applications using same upper fs. So far
overlay mount() does not consume unseen error and even if an fd
is opened after the error, application will see error on super
block. If we consume error on mount(), we change behavior.

I am not saying that's necessarily bad, I am just trying to point
out that its a user space visible behavior change and worried
if somebody starts calling it a regression.

Anyway, I looks like two problems got mixed into same thread. One
problem we need to solve is that syncfs() on overlayfs should
report back writeback errors (as well as other errors) to applications.
And that's what this patch series is solving.

And then second issue is detecting writeback errors over remount
for volatile mounts. And that's where this question comes whether
we should split seen flag or we should simply consume error on
mount. So this can be further discussed when patches for this
changes are posted again.

For now, I will focus on trying to fix first issue and post patches
for that again after more testing.

Vivek


> The guarantee we provide is that a fd that was open before the error
> occurred will see the error.  An fd that's opened after the error occurred
> may or may not see the error.