| Message ID | 20201125104621.18838-4-sargun@sargun.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make overlayfs volatile mounts reusable | expand |
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) > 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 >
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 > > >
On Wed, Nov 25, 2020 at 5:36 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > 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). > Documentation is the bare minimum. If someone implements the shutdown approach that would be best. > 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. I did not intend to work on this. Whoever wants to pick this up doesn't need to actually implement the shutdown ioctl, may implement only an "internal shutdown" on error. > > 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. > I agree that would be very nice, but I can also understand the argument that volatile mount has an issue, which does not get any better or any worse as a result of Sargun's patches. If anything, they improve the situation: Currently, the user does have a way to know if any data was lost on a volatile mount. After a successful mount cycle, the user knows that no data was lost during the last volatile mount period. Thanks, Amir.
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);
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. 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(+)