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

[Jeff Layton]

On Fri, 2018-05-18 at 08:22 -0700, Matthew Wilcox wrote:
> 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;
> 

I'm not sure I understand what you intend here. If __sync_blockdev
fails, then the error should have already been marked in sb->s_wb_err
(via patch #6). We wouldn't want to record that again at syncfs time.
Note that __sync_blockdev will return errors based on the legacy
AS_EIO/AS_ENOSPC flags.

We really do need to record it in the superblock as soon as possible
after an error occurs. If we want to allow userland to eventually be
able to scrape this value out of the kernel (as we discussed at LSF/MM)
then we can't assume that it'll be doing any sort of syncfs call
beforehand.

The main reason to push this down into the filesystems is to allow them
control over whether to report errors at syncfs time via the superblock
errseq_t or not. If we don't really care about allowing this to be an
opt-in thing, then we could just take the patch that I sent on April
17th:

    [PATCH] fs: track per-sb writeback errors and report them to syncfs

We'd also want patch #6 from this series, I think, but that's more or
less enough to implement this over all filesystems, assuming they use
mapping_set_error to record writeback errors. I'm fine with either
approach.

> (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.