Re: [RFC PATCH 07/11] ext4: have sync_fs op report writeback errors when passed a since pointer

[Matthew Wilcox]

On Fri, May 18, 2018 at 08:34:11AM -0400, Jeff Layton wrote:
> From: Jeff Layton <jlayton@xxxxxxxxxx>
> 
> When ext4_sync_fs gets a non-NULL since pointer, use it to report
> errors vs. the errseq_t in the super_block. This allows us to
> properly report an error to sync_fs when any inode has failed writeback
> since we last checked for it.
> 
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
>  fs/ext4/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 896ddf8c3421..a5f41d31611f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4898,6 +4898,8 @@ static int ext4_sync_fs(struct super_block *sb, int wait, errseq_t *since)
>  	}
>  out:
>  	ret2 = __sync_blockdev(sb->s_bdev, wait);
> +	if (since)
> +		ret2 = errseq_check_and_advance(&sb->s_wb_err, since);
>  	return ret ? ret : ret2;
>  }

This is inconsistent with the ext2 and xfs implementations ...

I'm worried this is too much complexity to push down to the filesystems.
When should errors get reported through the return value; when should they
be reported through the errseq_t?

Can we hide all of this?  Maybe ext4 could do:

	errseq_set(&sb->s_wb_err, __sync_blockdev(sb->s_bdev, wait));
	return ret;

(we'd need to make calling errseq_set(x, 0) be a no-op instead of an error)

and then the caller is the one who takes care of calling
errseq_check_and_advance() so we don't have to pass 'since' into each
filesystem.