| Message ID | 20201214221421.1127423-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | errseq+overlayfs: accomodate the volatile upper layer use-case | expand |
On Mon, Dec 14, 2020 at 05:14:21PM -0500, Jeff Layton wrote: > Peek at the upper layer's errseq_t at mount time for volatile mounts, > and record it in the per-sb info. In sync_fs, check for an error since > the recorded point and set it in the overlayfs superblock if there was > one. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/overlayfs/ovl_entry.h | 1 + > fs/overlayfs/super.c | 19 ++++++++++++++----- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h > index 1b5a2094df8e..f4285da50525 100644 > --- a/fs/overlayfs/ovl_entry.h > +++ b/fs/overlayfs/ovl_entry.h > @@ -79,6 +79,7 @@ struct ovl_fs { > atomic_long_t last_ino; > /* Whiteout dentry cache */ > struct dentry *whiteout; > + errseq_t errseq; > }; > > static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs) > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 290983bcfbb3..3f0cb91915ff 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -264,8 +264,16 @@ static int ovl_sync_fs(struct super_block *sb, int wait) > if (!ovl_upper_mnt(ofs)) > return 0; > > - if (!ovl_should_sync(ofs)) > - return 0; > + upper_sb = ovl_upper_mnt(ofs)->mnt_sb; > + > + if (!ovl_should_sync(ofs)) { > + /* Propagate errors from upper to overlayfs */ > + ret = errseq_check(&upper_sb->s_wb_err, ofs->errseq); > + if (ret) > + errseq_set(&sb->s_wb_err, ret); > + return ret; > + } > + I have few concerns here. I think ovl_sync_fs() should not be different for volatile mounts and non-volatile mounts. IOW, if an overlayfs user calls syncfs(fd), then only difference with non-volatile mount is that we will not call sync_filesystem() on underlying filesystem. But if there is an existing writeback error then that should be reported to syncfs(fd) caller both in case of volatile and non-volatile mounts. Additional requirement in case of non-volatile mount seems to be that as soon as we detect first error, we probably should mark whole file system bad and start returning error for overlay operations so that upper layer can be thrown away and process restarted. And final non-volatile mount requirement seems to that we want to detect writeback errors in non syncfs() paths, for ex. mount(). That's what Sargun is trying to do. Keep a snapshot of upper_sb errseq on disk and upon remount of volatile overlay make sure no writeback errors have happened since then. And that's where I think we should be using new errseq_peek() and errseq_check(&upper_sb->s_wb_err, ofs->errseq) infracture. That way we can detect error on upper without consuming it upon overlay remount. IOW, IMHO, ovl_sync_fs(), should use same mechanism to report error to user space both for volatile and non-volatile mounts. And this new mechanism of peeking at error without consuming it should be used in other paths like remount and possibly other overlay operations(if need be). But creating a special path in ovl_sync_fs() for volatile mounts only will create conflicts with error reporting for non-volatile mounts. And IMHO, these should be same. Is there a good reason that why we should treat volatile and non-volatile mounts differently in ovl_sync_fs() from error detection and reporting point of view. Thanks Vivek > /* > * Not called for sync(2) call or an emergency sync (SB_I_SKIP_SYNC). > * All the super blocks will be iterated, including upper_sb. > @@ -277,8 +285,6 @@ static int ovl_sync_fs(struct super_block *sb, int wait) > if (!wait) > return 0; > > - upper_sb = ovl_upper_mnt(ofs)->mnt_sb; > - > down_read(&upper_sb->s_umount); > ret = sync_filesystem(upper_sb); > up_read(&upper_sb->s_umount); > @@ -1945,8 +1951,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > > sb->s_stack_depth = ovl_upper_mnt(ofs)->mnt_sb->s_stack_depth; > sb->s_time_gran = ovl_upper_mnt(ofs)->mnt_sb->s_time_gran; > - > } > + > + if (ofs->config.ovl_volatile) > + ofs->errseq = errseq_peek(&ovl_upper_mnt(ofs)->mnt_sb->s_wb_err); > + > oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers); > err = PTR_ERR(oe); > if (IS_ERR(oe)) > -- > 2.29.2 >
On Tue, 2020-12-15 at 11:30 -0500, Vivek Goyal wrote: > On Mon, Dec 14, 2020 at 05:14:21PM -0500, Jeff Layton wrote: > > Peek at the upper layer's errseq_t at mount time for volatile mounts, > > and record it in the per-sb info. In sync_fs, check for an error since > > the recorded point and set it in the overlayfs superblock if there was > > one. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/overlayfs/ovl_entry.h | 1 + > > fs/overlayfs/super.c | 19 ++++++++++++++----- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h > > index 1b5a2094df8e..f4285da50525 100644 > > --- a/fs/overlayfs/ovl_entry.h > > +++ b/fs/overlayfs/ovl_entry.h > > @@ -79,6 +79,7 @@ struct ovl_fs { > > atomic_long_t last_ino; > > /* Whiteout dentry cache */ > > struct dentry *whiteout; > > + errseq_t errseq; > > }; > > > > > > > > > > > > > > > > > > static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs) > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > index 290983bcfbb3..3f0cb91915ff 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -264,8 +264,16 @@ static int ovl_sync_fs(struct super_block *sb, int wait) > > if (!ovl_upper_mnt(ofs)) > > return 0; > > > > > > > > > > > > > > > > > > - if (!ovl_should_sync(ofs)) > > - return 0; > > + upper_sb = ovl_upper_mnt(ofs)->mnt_sb; > > + > > + if (!ovl_should_sync(ofs)) { > > + /* Propagate errors from upper to overlayfs */ > > + ret = errseq_check(&upper_sb->s_wb_err, ofs->errseq); > > + if (ret) > > + errseq_set(&sb->s_wb_err, ret); > > + return ret; > > + } > > + > > I have few concerns here. I think ovl_sync_fs() should not be different > for volatile mounts and non-volatile mounts. IOW, if an overlayfs > user calls syncfs(fd), then only difference with non-volatile mount > is that we will not call sync_filesystem() on underlying filesystem. But > if there is an existing writeback error then that should be reported > to syncfs(fd) caller both in case of volatile and non-volatile mounts. > > Additional requirement in case of non-volatile mount seems to be that > as soon as we detect first error, we probably should mark whole file > system bad and start returning error for overlay operations so that > upper layer can be thrown away and process restarted. > That was the reason the patch did the errseq_set on every sync_fs invocation for a volatile mount. That should ensure that syncfs always returns an error. Still, there probably are cleaner ways to do this... > And final non-volatile mount requirement seems to that we want to detect > writeback errors in non syncfs() paths, for ex. mount(). That's what > Sargun is trying to do. Keep a snapshot of upper_sb errseq on disk > and upon remount of volatile overlay make sure no writeback errors > have happened since then. And that's where I think we should be using > new errseq_peek() and errseq_check(&upper_sb->s_wb_err, ofs->errseq) > infracture. That way we can detect error on upper without consuming > it upon overlay remount. > > IOW, IMHO, ovl_sync_fs(), should use same mechanism to report error to > user space both for volatile and non-volatile mounts. And this new > mechanism of peeking at error without consuming it should be used > in other paths like remount and possibly other overlay operations(if need > be). > > But creating a special path in ovl_sync_fs() for volatile mounts > only will create conflicts with error reporting for non-volatile > mounts. And IMHO, these should be same. > > Is there a good reason that why we should treat volatile and non-volatile > mounts differently in ovl_sync_fs() from error detection and reporting > point of view. > Fair enough. I'm not that well-versed in overlayfs, so if you see a better way to do this, then that's fine by me. I just sent this out as a demonstration of how you could do it. Feel free to drop the second patch. I think the simplest solution to most of these issues is to add a new f_op->syncfs vector. You shouldn't need to propagate errors to the ovl sb at all if you add that. You can just operate on the upper sb's s_wb_err, and ignore the one in the ovl sb. > > /* > > * Not called for sync(2) call or an emergency sync (SB_I_SKIP_SYNC). > > * All the super blocks will be iterated, including upper_sb. > > @@ -277,8 +285,6 @@ static int ovl_sync_fs(struct super_block *sb, int wait) > > if (!wait) > > return 0; > > > > > > > > > > - upper_sb = ovl_upper_mnt(ofs)->mnt_sb; > > - > > down_read(&upper_sb->s_umount); > > ret = sync_filesystem(upper_sb); > > up_read(&upper_sb->s_umount); > > @@ -1945,8 +1951,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > > > > > > > > > > sb->s_stack_depth = ovl_upper_mnt(ofs)->mnt_sb->s_stack_depth; > > sb->s_time_gran = ovl_upper_mnt(ofs)->mnt_sb->s_time_gran; > > - > > } > > + > > + if (ofs->config.ovl_volatile) > > + ofs->errseq = errseq_peek(&ovl_upper_mnt(ofs)->mnt_sb->s_wb_err); > > + > > oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers); > > err = PTR_ERR(oe); > > if (IS_ERR(oe)) > > -- > > 2.29.2 > > >
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index 1b5a2094df8e..f4285da50525 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -79,6 +79,7 @@ struct ovl_fs { atomic_long_t last_ino; /* Whiteout dentry cache */ struct dentry *whiteout; + errseq_t errseq; }; static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 290983bcfbb3..3f0cb91915ff 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -264,8 +264,16 @@ static int ovl_sync_fs(struct super_block *sb, int wait) if (!ovl_upper_mnt(ofs)) return 0; - if (!ovl_should_sync(ofs)) - return 0; + upper_sb = ovl_upper_mnt(ofs)->mnt_sb; + + if (!ovl_should_sync(ofs)) { + /* Propagate errors from upper to overlayfs */ + ret = errseq_check(&upper_sb->s_wb_err, ofs->errseq); + if (ret) + errseq_set(&sb->s_wb_err, ret); + return ret; + } + /* * Not called for sync(2) call or an emergency sync (SB_I_SKIP_SYNC). * All the super blocks will be iterated, including upper_sb. @@ -277,8 +285,6 @@ static int ovl_sync_fs(struct super_block *sb, int wait) if (!wait) return 0; - upper_sb = ovl_upper_mnt(ofs)->mnt_sb; - down_read(&upper_sb->s_umount); ret = sync_filesystem(upper_sb); up_read(&upper_sb->s_umount); @@ -1945,8 +1951,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) sb->s_stack_depth = ovl_upper_mnt(ofs)->mnt_sb->s_stack_depth; sb->s_time_gran = ovl_upper_mnt(ofs)->mnt_sb->s_time_gran; - } + + if (ofs->config.ovl_volatile) + ofs->errseq = errseq_peek(&ovl_upper_mnt(ofs)->mnt_sb->s_wb_err); + oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers); err = PTR_ERR(oe); if (IS_ERR(oe))
Peek at the upper layer's errseq_t at mount time for volatile mounts, and record it in the per-sb info. In sync_fs, check for an error since the recorded point and set it in the overlayfs superblock if there was one. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/overlayfs/ovl_entry.h | 1 + fs/overlayfs/super.c | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)