Re: [RFC PATCH v2 2/2] overlayfs: propagate errors from upper to overlay sb in sync_fs

[Vivek Goyal <vgoyal@redhat.com>]

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
>