Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]

On Fri, Mar 31 2017, Jeff Layton wrote:

> During LSF/MM this year, we had a discussion about the current sorry
> state of writeback error reporting, and what could be done to improve
> the situation. This patchset represents a first pass at the proposal
> I made there.
>
> It first adds a new set of writeback error tracking infrastructure to
> ensure that errors are properly stored and reported at fsync time. It
> also makes a small but significant change to ensure that writeback
> errors are reported on all file descriptors, not just on the first one
> where fsync is called.
>
> Note that this is a _very_ rough draft at this point. I did some by-hand
> testing with dm-error to ensure that it does the right thing there.
> Mostly I'm interested in early feedback at this point -- does this basic
> approach make sense?

I think that having ->wb_err_seq and returning errors to all file
descriptors is a good idea.
I don't like ->wb_err, particularly that you allow it to be set
to zero:
 +	/*
 +	 * This should be called with the error code that we want to return
 +	 * on fsync. Thus, it should always be <= 0.
 +	 */
 +	WARN_ON(err > 0);

Why is that ??

Also I think that EIO should always over-ride ENOSPC as the possible
responses are different.  That probably means you need a separate seq
number for each, which isn't ideal.

I don't like that you need to add a 'flush' handler to every filesystem,
most of which just call
 +	return filemap_report_wb_error(file);

Could we just have
	if (filp->f_op->flush)
		retval = filp->f_op->flush(filp, id);
+	else
+		retval = filemap_report_wb_error(filp);
in flip_close() ??

... or maybe it is wrong to return this error on close().
After all, the file actually does get closed, so no error occurred.
If an application cares about EIO, it should always call fsync() before
close().

Thanks,
NeilBrown


>
> Jeff Layton (4):
>   fs: new infrastructure for writeback error handling and reporting
>   dax: set errors in mapping when writeback fails
>   buffer: set wb errors using both new and old infrastructure for now
>   ext4: wire it up to the new writeback error reporting infrastructure
>
>  Documentation/filesystems/vfs.txt | 14 +++++++--
>  fs/buffer.c                       |  6 +++-
>  fs/dax.c                          |  4 ++-
>  fs/ext4/dir.c                     |  1 +
>  fs/ext4/ext4.h                    |  1 +
>  fs/ext4/file.c                    |  1 +
>  fs/ext4/fsync.c                   | 15 +++++++---
>  fs/ext4/inode.c                   |  2 +-
>  fs/ext4/page-io.c                 |  4 +--
>  fs/open.c                         |  3 ++
>  include/linux/fs.h                |  5 ++++
>  mm/filemap.c                      | 61 +++++++++++++++++++++++++++++++++++++++
>  12 files changed, 106 insertions(+), 11 deletions(-)
>
> -- 
> 2.9.3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]