Re: [PATCH v1 3/3] overlay: Add rudimentary checking of writeback errseq on volatile remount

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Nov 25, 2020 at 04:03:06PM +0200, Amir Goldstein wrote:
> On Wed, Nov 25, 2020 at 12:46 PM Sargun Dhillon <sargun@sargun.me> wrote:
> >
> > Volatile remounts validate the following at the moment:
> >  * Has the module been reloaded / the system rebooted
> >  * Has the workdir been remounted
> >
> > This adds a new check for errors detected via the superblock's
> > errseq_t. At mount time, the errseq_t is snapshotted to disk,
> > and upon remount it's re-verified. This allows for kernel-level
> > detection of errors without forcing userspace to perform a
> > sync and allows for the hidden detection of writeback errors.
> >
> 
> Looks fine as long as you verify that the reuse is also volatile.
> 
> Care to also add the alleged issues that Vivek pointed out with existing
> volatile mount to the documentation? (unless Vivek intends to do fix those)

I thought current writeback error issue with volatile mounts needs to
be fixed with shutting down filesystem. (And mere documentation is not
enough).

Amir, are you planning to improve your ovl-shutdown patches to detect
writeback errors for volatile mounts. Or you want somebody else to
look at it.

W.r.t this patch set, I still think that first we should have patches
to shutdown filesystem on writeback errors (for volatile mount), and
then detecting writeback errors on remount makes more sense.

Vivek

> 
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > ---
> >  fs/overlayfs/overlayfs.h | 1 +
> >  fs/overlayfs/readdir.c   | 6 ++++++
> >  fs/overlayfs/super.c     | 1 +
> >  3 files changed, 8 insertions(+)
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index de694ee99d7c..e8a711953b64 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -85,6 +85,7 @@ struct ovl_volatile_info {
> >          */
> >         uuid_t          ovl_boot_id;    /* Must stay first member */
> >         u64             s_instance_id;
> > +       errseq_t        errseq; /* Implemented as a u32 */
> >  } __packed;
> >
> >  /*
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index 4e3e2bc3ea43..2bb0641ecbbd 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -1109,6 +1109,12 @@ static int ovl_verify_volatile_info(struct ovl_fs *ofs,
> >                 return -EINVAL;
> >         }
> >
> > +       err = errseq_check(&volatiledir->d_sb->s_wb_err, info.errseq);
> > +       if (err) {
> > +               pr_debug("Workdir filesystem reports errors: %d\n", err);
> > +               return -EINVAL;
> > +       }
> > +
> >         return 1;
> >  }
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 9a1b07907662..49dee41ec125 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1248,6 +1248,7 @@ static int ovl_set_volatile_info(struct ovl_fs *ofs, struct dentry *volatiledir)
> >         int err;
> >         struct ovl_volatile_info info = {
> >                 .s_instance_id = volatiledir->d_sb->s_instance_id,
> > +               .errseq = errseq_sample(&volatiledir->d_sb->s_wb_err),
> >         };
> >
> >         uuid_copy(&info.ovl_boot_id, &ovl_boot_id);
> > --
> > 2.25.1
> >
>