Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 17 Dec 2020 15:08:56 -0500
From:   Jeffrey Layton <jlayton@poochiereds.net>
To:     Vivek Goyal <vgoyal@redhat.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-unionfs@vger.kernel.org, jlayton@kernel.org,
        amir73il@gmail.com, sargun@sargun.me, miklos@szeredi.hu,
        willy@infradead.org, jack@suse.cz, neilb@suse.com,
        viro@zeniv.linux.org.uk
Subject: Re: [PATCH 3/3] overlayfs: Check writeback errors w.r.t upper in
 ->syncfs()
Message-ID: <20201217200856.GA707519@tleilax.poochiereds.net>
References: <20201216233149.39025-1-vgoyal@redhat.com>
 <20201216233149.39025-4-vgoyal@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201216233149.39025-4-vgoyal@redhat.com>
Precedence: bulk

On Wed, Dec 16, 2020 at 06:31:49PM -0500, Vivek Goyal wrote:
> Check for writeback error on overlay super block w.r.t "struct file"
> passed in ->syncfs().
> 
> As of now real error happens on upper sb. So this patch first propagates
> error from upper sb to overlay sb and then checks error w.r.t struct
> file passed in.
> 
> Jeff, I know you prefer that I should rather file upper file and check
> error directly on on upper sb w.r.t this real upper file.  While I was
> implementing that I thought what if file is on lower (and has not been
> copied up yet). In that case shall we not check writeback errors and
> return back to user space? That does not sound right though because,
> we are not checking for writeback errors on this file. Rather we
> are checking for any error on superblock. Upper might have an error
> and we should report it to user even if file in question is a lower
> file. And that's why I fell back to this approach. But I am open to
> change it if there are issues in this method.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/ovl_entry.h |  2 ++
>  fs/overlayfs/super.c     | 15 ++++++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 1b5a2094df8e..a08fd719ee7b 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -79,6 +79,8 @@ struct ovl_fs {
>  	atomic_long_t last_ino;
>  	/* Whiteout dentry cache */
>  	struct dentry *whiteout;
> +	/* Protects multiple sb->s_wb_err update from upper_sb . */
> +	spinlock_t errseq_lock;
>  };
>  
>  static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs)
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index b4d92e6fa5ce..e7bc4492205e 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -291,7 +291,7 @@ int ovl_syncfs(struct file *file)
>  	struct super_block *sb = file->f_path.dentry->d_sb;
>  	struct ovl_fs *ofs = sb->s_fs_info;
>  	struct super_block *upper_sb;
> -	int ret;
> +	int ret, ret2;
>  
>  	ret = 0;
>  	down_read(&sb->s_umount);
> @@ -310,10 +310,18 @@ int ovl_syncfs(struct file *file)
>  	ret = sync_filesystem(upper_sb);
>  	up_read(&upper_sb->s_umount);
>  
> +	/* Update overlay sb->s_wb_err */
> +	if (errseq_check(&upper_sb->s_wb_err, sb->s_wb_err)) {
> +		/* Upper sb has errors since last time */
> +		spin_lock(&ofs->errseq_lock);
> +		errseq_check_and_advance(&upper_sb->s_wb_err, &sb->s_wb_err);
> +		spin_unlock(&ofs->errseq_lock);
> +	}

So, the problem here is that the resulting value in sb->s_wb_err is
going to end up with the REPORTED flag set (using the naming in my
latest set). So, a later opener of a file on sb->s_wb_err won't see it.

For instance, suppose you call sync() on the box and does the above
check and advance. Then, you open the file and call syncfs() and get
back no error because REPORTED flag was set when you opened. That error
will then be lost.

>  
> +	ret2 = errseq_check_and_advance(&sb->s_wb_err, &file->f_sb_err);
>  out:
>  	up_read(&sb->s_umount);
> -	return ret;
> +	return ret ? ret : ret2;
>  }
>  
>  /**
> @@ -1903,6 +1911,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!cred)
>  		goto out_err;
>  
> +	spin_lock_init(&ofs->errseq_lock);
>  	/* Is there a reason anyone would want not to share whiteouts? */
>  	ofs->share_whiteout = true;
>  
> @@ -1975,7 +1984,7 @@ 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;
> -
> +		sb->s_wb_err = errseq_sample(&ovl_upper_mnt(ofs)->mnt_sb->s_wb_err);

This will mark the error on the upper_sb as REPORTED, and that's not
really that's the case if you're just using it set s_wb_err in the
overlay. You might want to use errseq_peek in this situation.

>  	}
>  	oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers);
>  	err = PTR_ERR(oe);
> -- 
> 2.25.4
>