Re: [RFC PATCH 3/3] overlay: Add the ability to remount volatile directories when safe

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Nov 18, 2020 at 10:27 AM Sargun Dhillon <sargun@sargun.me> wrote:
>
> On Wed, Nov 18, 2020 at 09:24:04AM +0200, Amir Goldstein wrote:
> > On Tue, Nov 17, 2020 at 8:29 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Tue, Nov 17, 2020 at 08:03:16PM +0200, Amir Goldstein wrote:
> > > > > > C. "shutdown" the filesystem if writeback errors happened and return
> > > > > >      EIO from any read, like some blockdev filesystems will do in face
> > > > > >      of metadata write errors
> > > > > >
> > > > > > I happen to have a branch ready for that ;-)
> > > > > > https://github.com/amir73il/linux/commits/ovl-shutdown
> > > > >
> > > > >
> > > > > This branch seems to implement shutdown ioctl. So it will still need
> > > > > glue code to detect writeback failure in upper/ and trigger shutdown
> > > > > internally?
> > > > >
> > > >
> > > > Yes.
> > > > ovl_get_acess() can check both the administrative ofs->goingdown
> > > > command and the upper writeback error condition for volatile ovl
> > > > or something like that.
> > >
> > > This approach will not help mmaped() pages though, if I do.
> > >
> > > - Store to addr
> > > - msync
> > > - Load from addr
> > >
> > > There is a chance that I can still read back old data.
> > >
> >
> > msync does not go through overlay. It goes directly to upper fs,
> > so it will sync pages and return error on volatile overlay as well.
> >
> > Maybe there will still be weird corner cases, but the shutdown approach
> > should cover most or all of the interesting cases.
> When would we check the errseq_t of the upperdir? Only when the user
> calls fsync, or upon close? Periodically?
>

Ideally, if it is not too costly, on every "access".

The ovl-shutdown branch adds a ovl_get_access() call before access to any
overlay object.

> >
> > Thanks,
> > Amir.
>
> We can tackle this later, but I suggest the following semantics, which
> follow how ext4 works:
>
> https://www.kernel.org/doc/Documentation/filesystems/ext4.txt
> errors=remount-ro       Remount the filesystem read-only on an error.
> errors=continue         Keep going on a filesystem error.
> [Sargun: We probably don't want this one]
> errors=panic            Panic and halt the machine if an error occurs.
>                         (These mount options override the errors behavior
>                         specified in the superblock, which can be configured
>                         using tune2fs)

None of these modes seem relevant to volatile overlay IMO.

>
> ----
> We can potentially add a fourth option, which is shutdown -- that would
> return something like EIO or ESHUTDOWN for all calls.
>

FWIW, that's the only mode XFS supports.

> In addition to that, we should pass through the right errseqs to make
> the errseq helpers work:
> int filemap_check_wb_err(struct address_space *mapping, errseq_t since) [1]
> errseq_t filemap_sample_wb_err(struct address_space *mapping) [2]
> errseq_t file_sample_sb_err(struct file *file)
>

Are you referring to volatile or non-valatile overlayfs?

For fsync, because every overlay file has a "shadow" real file,
I think errseq of overlayfs file should already reflect the correct state
of the errseq of the real file.

For syncfs, we should record the errseq of upper fs on mount, as your
patch did.

For volatile overlay, syncfs should fail permanently if there was a writeback
error since mount, not only once, so there is no reason to update the
errseq on the overlay sb? It is not like one syncfs can observe an error and
in the next it will be gone.

For non-volatile overlay, we probably need to report syncfs error once if
upper fs errseq is bigger than ovl sb errseq and advance ovl sb errseq.

Thanks,
Amir.