On Wed, Dec 23, 2020 at 06:20:27PM +0000, Sargun Dhillon wrote:
> On Mon, Dec 21, 2020 at 02:50:55PM -0500, Vivek Goyal wrote:
> > Currently syncfs() and fsync() seem to be two interfaces which check and
> > return writeback errors on superblock to user space. fsync() should
> > work fine with overlayfs as it relies on underlying filesystem to
> > do the check and return error. For example, if ext4 is on upper filesystem,
> > then ext4_sync_file() calls file_check_and_advance_wb_err(file) on
> > upper file and returns error. So overlayfs does not have to do anything
> > special.
> >
> > But with syncfs(), error check happens in vfs in syncfs() w.r.t
> > overlay_sb->s_wb_err. Given overlayfs is stacked filesystem, it
> > does not do actual writeback and all writeback errors are recorded
> > on underlying filesystem. So sb->s_wb_err is never updated hence
> > syncfs() does not work with overlay.
> >
> > Jeff suggested that instead of trying to propagate errors to overlay
> > super block, why not simply check for errors against upper filesystem
> > super block. I implemented this idea.
> >
> > Overlay file has "since" value which needs to be initialized at open
> > time. Overlay overrides VFS initialization and re-initializes
> > f->f_sb_err w.r.t upper super block. Later when
> > ovl_sb->errseq_check_advance() is called, f->f_sb_err is used as
> > since value to figure out if any error on upper sb has happened since
> > then.
> >
> > Note, Right now this patch only deals with regular file and directories.
> > Yet to deal with special files like device inodes, socket, fifo etc.
> >
> > Suggested-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > ---
> > fs/overlayfs/file.c | 1 +
> > fs/overlayfs/overlayfs.h | 1 +
> > fs/overlayfs/readdir.c | 1 +
> > fs/overlayfs/super.c | 23 +++++++++++++++++++++++
> > fs/overlayfs/util.c | 13 +++++++++++++
> > 5 files changed, 39 insertions(+)
> >
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index efccb7c1f9bc..7b58a44dcb71 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -163,6 +163,7 @@ static int ovl_open(struct inode *inode, struct file *file)
> > return PTR_ERR(realfile);
> >
> > file->private_data = realfile;
> > + ovl_init_file_errseq(file);
> >
> > return 0;
> > }
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index f8880aa2ba0e..47838abbfb3d 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -322,6 +322,7 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, struct dentry *dentry);
> > bool ovl_is_metacopy_dentry(struct dentry *dentry);
> > char *ovl_get_redirect_xattr(struct ovl_fs *ofs, struct dentry *dentry,
> > int padding);
> > +void ovl_init_file_errseq(struct file *file);
> >
> > static inline bool ovl_is_impuredir(struct super_block *sb,
> > struct dentry *dentry)
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index 01620ebae1bd..0c48f1545483 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -960,6 +960,7 @@ static int ovl_dir_open(struct inode *inode, struct file *file)
> > od->is_real = ovl_dir_is_real(file->f_path.dentry);
> > od->is_upper = OVL_TYPE_UPPER(type);
> > file->private_data = od;
> > + ovl_init_file_errseq(file);
> >
> > return 0;
> > }
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 290983bcfbb3..d99867983722 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -390,6 +390,28 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
> > return ret;
> > }
> >
> > +static int ovl_errseq_check_advance(struct super_block *sb, struct file *file)
> > +{
> > + struct ovl_fs *ofs = sb->s_fs_info;
> > + struct super_block *upper_sb;
> > + int ret;
> > +
> > + if (!ovl_upper_mnt(ofs))
> > + return 0;
> > +
> > + upper_sb = ovl_upper_mnt(ofs)->mnt_sb;
> > +
> > + if (!errseq_check(&upper_sb->s_wb_err, file->f_sb_err))
> > + return 0;
> > +
> > + /* Something changed, must use slow path */
> > + spin_lock(&file->f_lock);
> > + ret = errseq_check_and_advance(&upper_sb->s_wb_err, &file->f_sb_err);
> > + spin_unlock(&file->f_lock);
> > +
> > + return ret;
> > +}
> > +
> > static const struct super_operations ovl_super_operations = {
> > .alloc_inode = ovl_alloc_inode,
> > .free_inode = ovl_free_inode,
> > @@ -400,6 +422,7 @@ static const struct super_operations ovl_super_operations = {
> > .statfs = ovl_statfs,
> > .show_options = ovl_show_options,
> > .remount_fs = ovl_remount,
> > + .errseq_check_advance = ovl_errseq_check_advance,
> > };
> >
> > enum {
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 23f475627d07..a1742847f3a8 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -950,3 +950,16 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, struct dentry *dentry,
> > kfree(buf);
> > return ERR_PTR(res);
> > }
> > +
> > +void ovl_init_file_errseq(struct file *file)
> > +{
> > + struct super_block *sb = file_dentry(file)->d_sb;
> > + struct ovl_fs *ofs = sb->s_fs_info;
> > + struct super_block *upper_sb;
> > +
> > + if (!ovl_upper_mnt(ofs))
> > + return;
> > +
> > + upper_sb = ovl_upper_mnt(ofs)->mnt_sb;
> > + file->f_sb_err = errseq_sample(&upper_sb->s_wb_err);
> > +}
> > --
> > 2.25.4
> >
>
> I fail to see why this is neccessary if you incorporate error reporting into the
> sync_fs callback. Why is this separate from that callback?
- Writeback error should be checked after __sync_blockdev() has been
called. And that's called by VFS and not in ->sync_fs. This can be
changed by moving __sync_blockdev() insde ->sync_fs() for all
filesystems which need it. And that's what Jeff had done in the
past. That attempt did not make it upstream.
https://lore.kernel.org/linux-fsdevel/20180518123415.28181-1-jlayton@xxxxxxxxxx/
Given this triggered changes all over the place, I started looking for
alternatives. And thought adding a super operation to check for errors
solves the problem and keeps the changes to minimum.
> If you pickup Jeff's
> patch that adds the 2nd flag to errseq for "observed", you should be able to
> stash the first errseq seen in the ovl_fs struct, and do the check-and-return
> in there instead instead of adding this new infrastructure.
Why to stash errseq in ovl_fs struct when we can directly compare it
with upper sb? "since" value is in "struct file" and current errseq
value is in upper_sb. Atleast to solve this problem it should not
be mandatory to stash errseq value in ovl_fs.
>
> IMHO, if we're going to fix this, sync_fs should be replaced, and there should
> be a generic_sync_fs wrapper which does the errseq, callback, and sync blockdev,
> but then filesystems should be able to override it and do the requisite work.
Not exactly sure what you mean. But I guess you are falling back to the
idea of moving some of the vfs functionality of calling __sync_blockdev()
into filesystem ->sync_fs handler. That leads back to patches Jeff
Layton had posted in the past and is much more invasive change.
I am not against that approach but so far I have not seen any strong
interest from other in favor of that approach instead. So I find
this approach simpler and much less intrusive.
Vivek
