Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629543 > unrolled thread

[PATCH v3 00/20] fs: introduce new writeback error reporting and convert existing API as a wrapper around it

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/20] fs: introduce new writeback error reporting and convert existing API as a wrapper around it Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 09/20] 9p: set mapping error when writeback fails in launder_page Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 09/20] 9p: set mapping error when writeback fails in  launder_page Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 09/20] 9p: set mapping error when writeback fails in  launder_page Jan Kara <jack@suse.cz> - 2017-04-24 18:00 +0200
    [PATCH v3 08/20] mm: ensure that we set mapping error if writeout() fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 08/20] mm: ensure that we set mapping error if  writeout() fails Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 08/20] mm: ensure that we set mapping error if  writeout() fails Jan Kara <jack@suse.cz> - 2017-04-24 18:00 +0200
    [PATCH v3 04/20] fs: check for writeback errors after syncing out buffers in generic_file_fsync Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 04/20] fs: check for writeback errors after syncing  out buffers in generic_file_fsync Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 04/20] fs: check for writeback errors after syncing  out buffers in generic_file_fsync Jan Kara <jack@suse.cz> - 2017-04-24 17:50 +0200
    [PATCH v3 06/20] dax: set errors in mapping when writeback fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails Jan Kara <jack@suse.cz> - 2017-04-24 18:00 +0200
      Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-04-24 21:20 +0200
    [PATCH v3 12/20] lib: add errseq_t type and infrastructure for handling it Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 03/20] buffer: use mapping_set_error instead of setting the flag Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jan Kara <jack@suse.cz> - 2017-04-24 18:10 +0200
        Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 19:20 +0200
          Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jan Kara <jack@suse.cz> - 2017-04-25 11:50 +0200
            Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-25 12:40 +0200
              Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jan Kara <jack@suse.cz> - 2017-04-25 13:30 +0200
                Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-25 18:50 +0200
    [PATCH v3 01/20] mm: drop "wait" parameter from write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 01/20] mm: drop "wait" parameter from write_one_page Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
    [PATCH v3 15/20] mm: remove AS_EIO and AS_ENOSPC flags Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 18/20] mm: clean up error handling in write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 16/20] mm: don't TestClearPageError in __filemap_fdatawait_range Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 20/20] gfs2: clean up some filemap_* calls Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Bob Peterson <rpeterso@redhat.com> - 2017-04-24 16:20 +0200
        Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Jeff Layton <jlayton@redhat.com> - 2017-04-24 19:00 +0200
          Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Bob Peterson <rpeterso@redhat.com> - 2017-04-24 19:50 +0200
            Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Jeff Layton <jlayton@redhat.com> - 2017-04-24 20:00 +0200
    [PATCH v3 07/20] nilfs2: set the mapping error when calling SetPageError on writeback Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 07/20] nilfs2: set the mapping error when calling  SetPageError on writeback Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
    [PATCH v3 13/20] fs: new infrastructure for writeback error handling and reporting Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 17/20] cifs: cleanup writeback handling errors and comments Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 19/20] jbd2: don't reset error in journal_finish_inode_data_buffers Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 02/20] mm: fix mapping_set_error call in me_pagecache_dirty Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
    [PATCH v3 05/20] orangefs: don't call filemap_write_and_wait from fsync Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
      Re: [PATCH v3 05/20] orangefs: don't call filemap_write_and_wait  from fsync Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
        Re: [PATCH v3 05/20] orangefs: don't call filemap_write_and_wait from fsync Mike Marshall <hubcap@omnibond.com> - 2017-04-24 20:30 +0200
    [PATCH v3 14/20] fs: retrofit old error reporting API onto new infrastructure Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
    [PATCH v3 11/20] cifs: set mapping error when page writeback fails in writepage or launder_pages Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
      Re: [PATCH v3 11/20] cifs: set mapping error when page writeback  fails in writepage or launder_pages Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
        Re: [PATCH v3 11/20] cifs: set mapping error when page writeback  fails in writepage or launder_pages Jeff Layton <jlayton@redhat.com> - 2017-04-24 19:20 +0200

Page 1 of 3  [1] 2 3  Next page →

#1629543 — [PATCH v3 00/20] fs: introduce new writeback error reporting and convert existing API as a wrapper around it

v3: wb_err_t -> errseq_t
    clean up places that re-set errors after calling filemap_* functions

v2: introduce wb_err_t, use atomics

Apologies for the wide posting here, but this touches a lot of areas.
This is v3 of the patchset to improve how we're tracking and reporting
errors that occur during pagecache writeback.

There are several situations where the kernel can "lose" errors that
occur during writeback, such that fsync will return success even
though it failed to write back some data previously. The basic idea
here is to have the kernel be more deliberate about the point from
which errors are checked to ensure that that doesn't happen.

Additionally, this set changes the behavior of fsync in Linux to report
writeback errors on all fds instead of just the first one. This allows
writers to reliably tell whether their data made it to the backing
device without having to coordinate fsync calls with other writers.

This set sprawls over a large swath of kernel code. I think the first 12
patches in the series are pretty straightforward and are more or less
ready for merge.

The real changes start with patch 13. That adds support for errseq_t,
builds a new writeback error tracking API on top of that, and converts
the existing code to use it. After that, there are a few cleanup patches
to eliminate some unneeded error re-setting, etc.

Unfortunately, testing this across so many filesystems is rather
difficult. I have a xfstest for block-based filesystems that uses
dm_error that I'll post soon. That works well with ext4, but btrfs and
xfs seem to go r/o soon after the first error. I also don't have a good
general method for testing this on network filesystems (yet!).

I'd like to see better testing here and am open to suggestions. I will
note that the POSIX fsync spec says this:

"It is reasonable to assert that the key aspects of fsync() are
unreasonable to test in a test suite. That does not make the function
any less valuable, just more difficult to test. [...] It would also not
be unreasonable to omit testing for fsync(), allowing it to be treated
as a quality-of-implementation issue."

Of course, they're talking about a POSIX conformance test, but I
think the same point applies here.

At this point, I'd like to start getting some of the preliminary patches
merged (the first 12 or so). Most of those aren't terribly controversial
and seem like reasonable bugfixes and cleanups. If any subsystem
maintainers want to pick those up, then please do.

After that, I'd like to get the larger changes into linux-next with an
aim for merge in v4.13 or v4.14 (depending on how testing goes).

Feedback is of course welcome!

Jeff Layton (20):
  mm: drop "wait" parameter from write_one_page
  mm: fix mapping_set_error call in me_pagecache_dirty
  buffer: use mapping_set_error instead of setting the flag
  fs: check for writeback errors after syncing out buffers in
    generic_file_fsync
  orangefs: don't call filemap_write_and_wait from fsync
  dax: set errors in mapping when writeback fails
  nilfs2: set the mapping error when calling SetPageError on writeback
  mm: ensure that we set mapping error if writeout() fails
  9p: set mapping error when writeback fails in launder_page
  fuse: set mapping error in writepage_locked when it fails
  cifs: set mapping error when page writeback fails in writepage or
    launder_pages
  lib: add errseq_t type and infrastructure for handling it
  fs: new infrastructure for writeback error handling and reporting
  fs: retrofit old error reporting API onto new infrastructure
  mm: remove AS_EIO and AS_ENOSPC flags
  mm: don't TestClearPageError in __filemap_fdatawait_range
  cifs: cleanup writeback handling errors and comments
  mm: clean up error handling in write_one_page
  jbd2: don't reset error in journal_finish_inode_data_buffers
  gfs2: clean up some filemap_* calls

 Documentation/filesystems/vfs.txt |   9 +-
 fs/9p/vfs_addr.c                  |   5 +-
 fs/btrfs/file.c                   |  10 +-
 fs/btrfs/tree-log.c               |   9 +-
 fs/buffer.c                       |   2 +-
 fs/cifs/cifsfs.c                  |   4 +-
 fs/cifs/file.c                    |  17 ++--
 fs/cifs/inode.c                   |  22 ++---
 fs/dax.c                          |   4 +-
 fs/exofs/dir.c                    |   2 +-
 fs/ext2/dir.c                     |   2 +-
 fs/ext2/file.c                    |   2 +-
 fs/f2fs/file.c                    |   3 +
 fs/f2fs/node.c                    |   6 +-
 fs/fuse/file.c                    |   8 +-
 fs/gfs2/glops.c                   |  12 +--
 fs/gfs2/lops.c                    |   4 +-
 fs/gfs2/super.c                   |   6 +-
 fs/jbd2/commit.c                  |  13 +--
 fs/jfs/jfs_metapage.c             |   4 +-
 fs/libfs.c                        |   3 +
 fs/minix/dir.c                    |   2 +-
 fs/nilfs2/segment.c               |   1 +
 fs/open.c                         |   3 +
 fs/orangefs/file.c                |   5 +-
 fs/sysv/dir.c                     |   2 +-
 fs/ufs/dir.c                      |   2 +-
 include/linux/errseq.h            |  16 +++
 include/linux/fs.h                |  41 ++++++--
 include/linux/mm.h                |   2 +-
 include/linux/pagemap.h           |  18 ++--
 lib/Makefile                      |   2 +-
 lib/errseq.c                      | 199 ++++++++++++++++++++++++++++++++++++++
 mm/filemap.c                      |  88 ++++++++++-------
 mm/memory-failure.c               |   2 +-
 mm/migrate.c                      |   6 +-
 mm/page-writeback.c               |  23 +++--
 37 files changed, 398 insertions(+), 161 deletions(-)
 create mode 100644 include/linux/errseq.h
 create mode 100644 lib/errseq.c

-- 
2.9.3

[toc] | [next] | [standalone]

#1629544 — [PATCH v3 09/20] 9p: set mapping error when writeback fails in launder_page

launder_page is just writeback under the page lock. We still need to
mark the mapping for errors there when they occur.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/9p/vfs_addr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
index adaf6f6dd858..7af6e6501698 100644
--- a/fs/9p/vfs_addr.c
+++ b/fs/9p/vfs_addr.c
@@ -223,8 +223,11 @@ static int v9fs_launder_page(struct page *page)
 	v9fs_fscache_wait_on_page_write(inode, page);
 	if (clear_page_dirty_for_io(page)) {
 		retval = v9fs_vfs_writepage_locked(page);
-		if (retval)
+		if (retval) {
+			if (retval != -EAGAIN)
+				mapping_set_error(page->mapping, retval);
 			return retval;
+		}
 	}
 	return 0;
 }
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629676 — Re: [PATCH v3 09/20] 9p: set mapping error when writeback fails in launder_page

On Mon, Apr 24, 2017 at 09:22:48AM -0400, Jeff Layton wrote:
> launder_page is just writeback under the page lock. We still need to
> mark the mapping for errors there when they occur.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1629705 — Re: [PATCH v3 09/20] 9p: set mapping error when writeback fails in launder_page

On Mon 24-04-17 09:22:48, Jeff Layton wrote:
> launder_page is just writeback under the page lock. We still need to
> mark the mapping for errors there when they occur.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/9p/vfs_addr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
> index adaf6f6dd858..7af6e6501698 100644
> --- a/fs/9p/vfs_addr.c
> +++ b/fs/9p/vfs_addr.c
> @@ -223,8 +223,11 @@ static int v9fs_launder_page(struct page *page)
>  	v9fs_fscache_wait_on_page_write(inode, page);
>  	if (clear_page_dirty_for_io(page)) {
>  		retval = v9fs_vfs_writepage_locked(page);
> -		if (retval)
> +		if (retval) {
> +			if (retval != -EAGAIN)
> +				mapping_set_error(page->mapping, retval);
>  			return retval;
> +		}
>  	}
>  	return 0;
>  }
> -- 
> 2.9.3
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1629545 — [PATCH v3 08/20] mm: ensure that we set mapping error if writeout() fails

If writepage fails during a page migration, then we need to ensure that
fsync will see it by flagging the mapping.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 mm/migrate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 738f1d5f8350..3a59830bdae2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -792,7 +792,11 @@ static int writeout(struct address_space *mapping, struct page *page)
 		/* unlocked. Relock */
 		lock_page(page);
 
-	return (rc < 0) ? -EIO : -EAGAIN;
+	if (rc < 0) {
+		mapping_set_error(mapping, rc);
+		return -EIO;
+	}
+	return -EAGAIN;
 }
 
 /*
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629674 — Re: [PATCH v3 08/20] mm: ensure that we set mapping error if writeout() fails

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1629712 — Re: [PATCH v3 08/20] mm: ensure that we set mapping error if writeout() fails

On Mon 24-04-17 09:22:47, Jeff Layton wrote:
> If writepage fails during a page migration, then we need to ensure that
> fsync will see it by flagging the mapping.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  mm/migrate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 738f1d5f8350..3a59830bdae2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -792,7 +792,11 @@ static int writeout(struct address_space *mapping, struct page *page)
>  		/* unlocked. Relock */
>  		lock_page(page);
>  
> -	return (rc < 0) ? -EIO : -EAGAIN;
> +	if (rc < 0) {
> +		mapping_set_error(mapping, rc);
> +		return -EIO;
> +	}
> +	return -EAGAIN;
>  }
>  
>  /*
> -- 
> 2.9.3
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1629546 — [PATCH v3 04/20] fs: check for writeback errors after syncing out buffers in generic_file_fsync

ext2 currently does a test+clear of the AS_EIO flag, which is
is problematic for some coming changes.

What we really need to do instead is call filemap_check_errors
in __generic_file_fsync after syncing out the buffers. That
will be sufficient for this case, and help other callers detect
these errors properly as well.

With that, we don't need to twiddle it in ext2.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ext2/file.c | 2 +-
 fs/libfs.c     | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index b21891a6bfca..ed00e7ae0ef3 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -177,7 +177,7 @@ int ext2_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
 
 	ret = generic_file_fsync(file, start, end, datasync);
-	if (ret == -EIO || test_and_clear_bit(AS_EIO, &mapping->flags)) {
+	if (ret == -EIO) {
 		/* We don't really know where the IO error happened... */
 		ext2_error(sb, __func__,
 			   "detected IO error when writing metadata buffers");
diff --git a/fs/libfs.c b/fs/libfs.c
index a8b62e5d43a9..12a48ee442d3 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -991,7 +991,8 @@ int __generic_file_fsync(struct file *file, loff_t start, loff_t end,
 
 out:
 	inode_unlock(inode);
-	return ret;
+	err = filemap_check_errors(inode->i_mapping);
+	return ret ? : err;
 }
 EXPORT_SYMBOL(__generic_file_fsync);
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629672 — Re: [PATCH v3 04/20] fs: check for writeback errors after syncing out buffers in generic_file_fsync

>  out:
>  	inode_unlock(inode);
> -	return ret;
> +	err = filemap_check_errors(inode->i_mapping);
> +	return ret ? : err;

Can you spell out the whole unary operation instead of this weird GCC
extension?

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1629688 — Re: [PATCH v3 04/20] fs: check for writeback errors after syncing out buffers in generic_file_fsync

On Mon 24-04-17 09:22:43, Jeff Layton wrote:
> ext2 currently does a test+clear of the AS_EIO flag, which is
> is problematic for some coming changes.
> 
> What we really need to do instead is call filemap_check_errors
> in __generic_file_fsync after syncing out the buffers. That
> will be sufficient for this case, and help other callers detect
> these errors properly as well.
> 
> With that, we don't need to twiddle it in ext2.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/ext2/file.c | 2 +-
>  fs/libfs.c     | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> index b21891a6bfca..ed00e7ae0ef3 100644
> --- a/fs/ext2/file.c
> +++ b/fs/ext2/file.c
> @@ -177,7 +177,7 @@ int ext2_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  	struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
>  
>  	ret = generic_file_fsync(file, start, end, datasync);
> -	if (ret == -EIO || test_and_clear_bit(AS_EIO, &mapping->flags)) {
> +	if (ret == -EIO) {
>  		/* We don't really know where the IO error happened... */
>  		ext2_error(sb, __func__,
>  			   "detected IO error when writing metadata buffers");
> diff --git a/fs/libfs.c b/fs/libfs.c
> index a8b62e5d43a9..12a48ee442d3 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -991,7 +991,8 @@ int __generic_file_fsync(struct file *file, loff_t start, loff_t end,
>  
>  out:
>  	inode_unlock(inode);
> -	return ret;
> +	err = filemap_check_errors(inode->i_mapping);
> +	return ret ? : err;
>  }
>  EXPORT_SYMBOL(__generic_file_fsync);
>  
> -- 
> 2.9.3
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1629549 — [PATCH v3 06/20] dax: set errors in mapping when writeback fails

In order to get proper error codes from fsync, we must set an error in
the mapping range when writeback fails.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/dax.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/dax.c b/fs/dax.c
index 85abd741253d..9b6b04030c3f 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -901,8 +901,10 @@ int dax_writeback_mapping_range(struct address_space *mapping,
 
 			ret = dax_writeback_one(bdev, mapping, indices[i],
 					pvec.pages[i]);
-			if (ret < 0)
+			if (ret < 0) {
+				mapping_set_error(mapping, ret);
 				return ret;
+			}
 		}
 	}
 	return 0;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629673 — Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1629714 — Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails

On Mon 24-04-17 09:22:45, Jeff Layton wrote:
> In order to get proper error codes from fsync, we must set an error in
> the mapping range when writeback fails.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

So I'm fine with the change but please expand the changelog to something
like:

DAX currently doesn't set errors in the mapping when cache flushing fails
in dax_writeback_mapping_range(). Since this function can get called only
from fsync(2) or sync(2), this is actually as good as it can currently get
since we correctly propagate the error up from dax_writeback_mapping_range()
to filemap_fdatawrite(). However in the future better writeback error
handling will enable us to properly report these errors on fsync(2) even if
there are multiple file descriptors open against the file or if sync(2)
gets called before fsync(2). So convert DAX to using standard error
reporting through the mapping.

After improving the changelog you can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/dax.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 85abd741253d..9b6b04030c3f 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -901,8 +901,10 @@ int dax_writeback_mapping_range(struct address_space *mapping,
>  
>  			ret = dax_writeback_one(bdev, mapping, indices[i],
>  					pvec.pages[i]);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				mapping_set_error(mapping, ret);
>  				return ret;
> +			}
>  		}
>  	}
>  	return 0;
> -- 
> 2.9.3
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1629903 — Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails

On Mon, Apr 24, 2017 at 09:22:45AM -0400, Jeff Layton wrote:
> In order to get proper error codes from fsync, we must set an error in
> the mapping range when writeback fails.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Works fine in some error injection testing.

Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

> ---
>  fs/dax.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 85abd741253d..9b6b04030c3f 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -901,8 +901,10 @@ int dax_writeback_mapping_range(struct address_space *mapping,
>  
>  			ret = dax_writeback_one(bdev, mapping, indices[i],
>  					pvec.pages[i]);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				mapping_set_error(mapping, ret);
>  				return ret;
> +			}
>  		}
>  	}
>  	return 0;
> -- 
> 2.9.3
> 

[toc] | [prev] | [next] | [standalone]

#1629550 — [PATCH v3 12/20] lib: add errseq_t type and infrastructure for handling it

An errseq_t is a way of recording errors in one place, and allowing any
number of "subscribers" to tell whether an error has been set again
since a previous time.

It's implemented as an unsigned 32-bit value that is managed with atomic
operations. The low order bits are designated to hold an error code
(max size of MAX_ERRNO). The upper bits are used as a counter.

The API works with consumers sampling an errseq_t value at a particular
point in time. Later, that value can be used to tell whether new errors
have been set since that time.

Note that there is a 1 in 512k risk of collisions here if new errors
are being recorded frequently, since we have so few bits to use as a
counter. To mitigate this, one bit is used as a flag to tell whether the
value has been sampled since a new value was recorded. That allows
us to avoid bumping the counter if no one has sampled it since it
was last bumped.

Later patches will build on this infrastructure to change how writeback
errors are tracked in the kernel.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/errseq.h |  16 ++++
 lib/Makefile           |   2 +-
 lib/errseq.c           | 199 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 216 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/errseq.h
 create mode 100644 lib/errseq.c

diff --git a/include/linux/errseq.h b/include/linux/errseq.h
new file mode 100644
index 000000000000..e2d374dbf683
--- /dev/null
+++ b/include/linux/errseq.h
@@ -0,0 +1,16 @@
+#ifndef _LINUX_ERRSEQ_H
+#define _LINUX_ERRSEQ_H
+typedef u32	errseq_t;
+
+void __errseq_set(errseq_t *eseq, int err);
+static inline void errseq_set(errseq_t *eseq, int err)
+{
+	/* Optimize for the common case of no error */
+	if (unlikely(err))
+		__errseq_set(eseq, err);
+}
+
+errseq_t errseq_sample(errseq_t *eseq);
+int errseq_check(errseq_t *eseq, errseq_t since);
+int errseq_check_and_advance(errseq_t *eseq, errseq_t *since);
+#endif
diff --git a/lib/Makefile b/lib/Makefile
index 320ac46a8725..2423afef40f7 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -41,7 +41,7 @@ obj-y += bcd.o div64.o sort.o parser.o debug_locks.o random32.o \
 	 gcd.o lcm.o list_sort.o uuid.o flex_array.o iov_iter.o clz_ctz.o \
 	 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
 	 percpu-refcount.o percpu_ida.o rhashtable.o reciprocal_div.o \
-	 once.o refcount.o
+	 once.o refcount.o errseq.o
 obj-y += string_helpers.o
 obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
 obj-y += hexdump.o
diff --git a/lib/errseq.c b/lib/errseq.c
new file mode 100644
index 000000000000..c783d0a39cb0
--- /dev/null
+++ b/lib/errseq.c
@@ -0,0 +1,199 @@
+#include <linux/err.h>
+#include <linux/bug.h>
+#include <linux/atomic.h>
+#include <linux/errseq.h>
+
+/*
+ * An errseq_t is a way of recording errors in one place, and allowing any
+ * number of "subscribers" to tell whether it has changed since an arbitrary
+ * time of their choosing.
+ *
+ * It's implemented as an unsigned 32-bit value. The low order bits are
+ * designated to hold an error code (between 0 and -MAX_ERRNO). The upper bits
+ * are used as a counter. This is done with atomics instead of locking so that
+ * these functions can be called from any context.
+ *
+ * The general idea is for consumers to sample an errseq_t value at a
+ * particular point in time. Later, that value can be used to tell whether any
+ * new errors have occurred since that time.
+ *
+ * Note that there is a risk of collisions, if new errors are being recorded
+ * frequently, since we have so few bits to use as a counter.
+ *
+ * To mitigate this, one bit is used as a flag to tell whether the value has
+ * been sampled since a new value was recorded. That allows us to avoid bumping
+ * the counter if no one has sampled it since the last time an error was
+ * recorded.
+ *
+ * A new errseq_t should always be zeroed out.  A errseq_t value of all zeroes
+ * is the special (but common) case where there has never been an error. An all
+ * zero value thus serves as the "epoch" if one wishes to know whether there
+ * has ever been an error set since it was first initialized.
+ */
+
+/* The low bits are designated for error code (max of MAX_ERRNO) */
+#define ERRSEQ_SHIFT		ilog2(MAX_ERRNO + 1)
+
+/* This bit is used as a flag to indicate whether the value has been seen */
+#define ERRSEQ_SEEN		(1 << ERRSEQ_SHIFT)
+
+/* The "ones" bit for the counter */
+#define ERRSEQ_CTR_INC		(1 << (ERRSEQ_SHIFT + 1))
+
+/**
+ * __errseq_set - set a errseq_t for later reporting
+ * @eseq: errseq_t field that should be set
+ * @err: error to set
+ *
+ * This function sets the error in *eseq, and increments the sequence counter
+ * if the last sequence was sampled at some point in the past.
+ *
+ * Any error set will always overwrite an existing error.
+ *
+ * Most callers will want to use the errseq_set inline wrapper to efficiently
+ * handle the common case where err is 0.
+ */
+void __errseq_set(errseq_t *eseq, int err)
+{
+	errseq_t old;
+
+	/* MAX_ERRNO must be able to serve as a mask */
+	BUILD_BUG_ON_NOT_POWER_OF_2(MAX_ERRNO + 1);
+
+	/*
+	 * Ensure the error code actually fits where we want it to go. If it
+	 * doesn't then just throw a warning and don't record anything. We
+	 * also don't accept zero here as that would effectively clear a
+	 * previous error.
+	 */
+	if (WARN(unlikely(err == 0 || (unsigned int)-err > MAX_ERRNO),
+				"err = %d\n", err))
+		return;
+
+	old = READ_ONCE(*eseq);
+	for (;;) {
+		errseq_t new, cur;
+
+		/* Clear out error bits and set new error */
+		new = (old & ~(MAX_ERRNO|ERRSEQ_SEEN)) | -err;
+
+		/* Only increment if someone has looked at it */
+		if (old & ERRSEQ_SEEN)
+			new += ERRSEQ_CTR_INC;
+
+		/* If there would be no change, then call it done */
+		if (new == old)
+			break;
+
+		/* Try to swap the new value into place */
+		cur = cmpxchg(eseq, old, new);
+
+		/*
+		 * Call it success if we did the swap or someone else beat us
+		 * to it for the same value.
+		 */
+		if (likely(cur == old || cur == new))
+			break;
+
+		/* Raced with an update, try again */
+		old = cur;
+	}
+}
+EXPORT_SYMBOL(__errseq_set);
+
+/**
+ * errseq_sample - grab current errseq_t value
+ * @eseq: pointer to errseq_t to be sampled
+ *
+ * This function allows callers to sample an errseq_t value, marking it as
+ * "seen" if required.
+ */
+errseq_t errseq_sample(errseq_t *eseq)
+{
+	errseq_t old = READ_ONCE(*eseq);
+	errseq_t new = old;
+
+	/*
+	 * For the common case of no errors ever having been set, we can skip
+	 * marking the SEEN bit. Once an error has been set, the value will
+	 * never go back to zero.
+	 */
+	if (old != 0) {
+		new |= ERRSEQ_SEEN;
+		if (old != new)
+			cmpxchg(eseq, old, new);
+	}
+	return new;
+}
+EXPORT_SYMBOL(errseq_sample);
+
+/**
+ * errseq_check - has an error occurred since a particular point in time?
+ * @eseq: pointer to errseq_t value to be checked
+ * @since: previously-sampled errseq_t from which to check
+ *
+ * Grab the value that eseq points to, and see if it has changed "since"
+ * the given value was sampled. The "since" value is not advanced, so there
+ * is no need to mark the value as seen.
+ *
+ * Returns the latest error set in the errseq_t or 0 if it hasn't changed.
+ */
+int errseq_check(errseq_t *eseq, errseq_t since)
+{
+	errseq_t cur = READ_ONCE(*eseq);
+
+	if (likely(cur == since))
+		return 0;
+	return -(cur & MAX_ERRNO);
+}
+EXPORT_SYMBOL(errseq_check);
+
+/**
+ * errseq_report - report wb error (if any) that was previously set
+ * @eseq: pointer to value being checked reported
+ * @since: pointer to previously-sampled errseq_t to check against and advance
+ *
+ * Grab the eseq value, and see whether it matches the value that since
+ * points to. If it does, then just return 0.
+ *
+ * If it doesn't, then the value has changed. Set the "seen" flag, and try to
+ * swap it into place as the new eseq value. Then, set that value as the new
+ * "since" value, and return whatever the error portion is set to.
+ *
+ * Note that no locking is provided here for concurrent updates to the "since"
+ * value. The caller must provide that if necessary. Because of this, callers
+ * may want to do a lockless errseq_check before taking the lock and calling
+ * this.
+ */
+int errseq_check_and_advance(errseq_t *eseq, errseq_t *since)
+{
+	int err = 0;
+	errseq_t old, new;
+
+	/*
+	 * Most callers will want to use the inline wrapper to check this,
+	 * so that the common case of no error is handled without needing
+	 * to lock.
+	 */
+	old = READ_ONCE(*eseq);
+	if (likely(old != *since)) {
+		/*
+		 * Set the flag and try to swap it into place if it has
+		 * changed.
+		 *
+		 * We don't care about the outcome of the swap here. If the
+		 * swap doesn't occur, then it has either been updated by a
+		 * writer who is bumping the seq count anyway, or another
+		 * reader who is just setting the "seen" flag. Either outcome
+		 * is OK here, and we can advance since and return an error
+		 * based on what we have.
+		 */
+		new = old | ERRSEQ_SEEN;
+		if (new != old)
+			cmpxchg(eseq, old, new);
+		*since = new;
+		err = -(new & MAX_ERRNO);
+	}
+	return err;
+}
+EXPORT_SYMBOL(errseq_check_and_advance);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629551 — [PATCH v3 03/20] buffer: use mapping_set_error instead of setting the flag

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 9196f2a270da..70638941066d 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -483,7 +483,7 @@ static void __remove_assoc_queue(struct buffer_head *bh)
 	list_del_init(&bh->b_assoc_buffers);
 	WARN_ON(!bh->b_assoc_map);
 	if (buffer_write_io_error(bh))
-		set_bit(AS_EIO, &bh->b_assoc_map->flags);
+		mapping_set_error(bh->b_assoc_map, -EIO);
 	bh->b_assoc_map = NULL;
 }
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629552 — [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails

This ensures that we see errors on fsync when writeback fails.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/fuse/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index ec238fb5a584..07d0efcb050c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
 err_free:
 	fuse_request_free(req);
 err:
+	mapping_set_error(page->mapping, error);
 	end_page_writeback(page);
 	return error;
 }
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629735 — Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails

On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> This ensures that we see errors on fsync when writeback fails.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Hum, but do we really want to clobber mapping errors with temporary stuff
like ENOMEM? Or do you want to handle that in mapping_set_error?

								Honza

> ---
>  fs/fuse/file.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index ec238fb5a584..07d0efcb050c 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
>  err_free:
>  	fuse_request_free(req);
>  err:
> +	mapping_set_error(page->mapping, error);
>  	end_page_writeback(page);
>  	return error;
>  }
> -- 
> 2.9.3
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1629806 — Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails

On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > This ensures that we see errors on fsync when writeback fails.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> Hum, but do we really want to clobber mapping errors with temporary stuff
> like ENOMEM? Or do you want to handle that in mapping_set_error?
> 

Right now we don't really have such a thing as temporary errors in the
writeback codepath. If you return an error here, the data doesn't stay
dirty or anything, and I think we want to ensure that that gets reported
via fsync.

I'd like to see us add better handling for retryable errors for stuff
like ENOMEM or EAGAIN. I think this is the first step toward that
though. Once we have more consistent handling of writeback errors in
general, then we can start doing more interesting things with retryable
errors.

So yeah, I this is the right thing to do for now.

> 
> > ---
> >  fs/fuse/file.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index ec238fb5a584..07d0efcb050c 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
> >  err_free:
> >  	fuse_request_free(req);
> >  err:
> > +	mapping_set_error(page->mapping, error);
> >  	end_page_writeback(page);
> >  	return error;
> >  }
> > -- 
> > 2.9.3
> > 
> > 

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1630346 — Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails

On Mon 24-04-17 13:14:36, Jeff Layton wrote:
> On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> > On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > > This ensures that we see errors on fsync when writeback fails.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > 
> > Hum, but do we really want to clobber mapping errors with temporary stuff
> > like ENOMEM? Or do you want to handle that in mapping_set_error?
> > 
> 
> Right now we don't really have such a thing as temporary errors in the
> writeback codepath. If you return an error here, the data doesn't stay
> dirty or anything, and I think we want to ensure that that gets reported
> via fsync.
> 
> I'd like to see us add better handling for retryable errors for stuff
> like ENOMEM or EAGAIN. I think this is the first step toward that
> though. Once we have more consistent handling of writeback errors in
> general, then we can start doing more interesting things with retryable
> errors.
> 
> So yeah, I this is the right thing to do for now.

OK, fair enough. And question number 2):

Who is actually responsible for setting the error in the mapping when error
happens inside ->writepage()? Is it the ->writepage() callback or the
caller of ->writepage()? Or something else? Currently it seems to be a
strange mix (e.g. mm/page-writeback.c: __writepage() calls
mapping_set_error() when ->writepage() returns error) so I'd like to
understand what's the plan and have that recorded in the changelogs.

								Honza

> 
> > 
> > > ---
> > >  fs/fuse/file.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > index ec238fb5a584..07d0efcb050c 100644
> > > --- a/fs/fuse/file.c
> > > +++ b/fs/fuse/file.c
> > > @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
> > >  err_free:
> > >  	fuse_request_free(req);
> > >  err:
> > > +	mapping_set_error(page->mapping, error);
> > >  	end_page_writeback(page);
> > >  	return error;
> > >  }
> > > -- 
> > > 2.9.3
> > > 
> > > 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web