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

Groups > linux.kernel > #1614272 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-03-31 21:30 +0200
    [RFC PATCH 2/4] dax: set errors in mapping when writeback fails Jeff Layton <jlayton@redhat.com> - 2017-03-31 21:30 +0200
    [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting Jeff Layton <jlayton@redhat.com> - 2017-03-31 21:30 +0200
      Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error  handling and reporting Nikolay Borisov <nborisov@suse.com> - 2017-04-03 09:20 +0200
        Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error  handling and reporting Jeff Layton <jlayton@redhat.com> - 2017-04-03 12:30 +0200
      Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error  handling and reporting Matthew Wilcox <willy@infradead.org> - 2017-04-03 16:50 +0200
        Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error  handling and reporting Jeff Layton <jlayton@redhat.com> - 2017-04-03 17:30 +0200
          Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error  handling and reporting Matthew Wilcox <willy@infradead.org> - 2017-04-03 18:20 +0200
            Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error  handling and reporting Jeff Layton <jlayton@redhat.com> - 2017-04-03 18:40 +0200
    [RFC PATCH 4/4] ext4: wire it up to the new writeback error reporting infrastructure Jeff Layton <jlayton@redhat.com> - 2017-03-31 21:30 +0200
    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-03 06:30 +0200
      Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-03 12:30 +0200
        Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-03 16:40 +0200
          Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-03 19:50 +0200
            Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeremy Allison <jra@samba.org> - 2017-04-03 20:20 +0200
              Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-03 20:20 +0200
                Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeremy Allison <jra@samba.org> - 2017-04-03 20:40 +0200
                  Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeremy Allison <jra@samba.org> - 2017-04-03 20:50 +0200
                    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-03 20:50 +0200
            Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-03 21:20 +0200
              Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-03 22:20 +0200
                Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-04 04:50 +0200
                Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-04 05:10 +0200
                  Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-04 13:50 +0200
                    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-05 00:50 +0200
                  Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-04 14:00 +0200
                    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-04 14:20 +0200
                      Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-04 18:20 +0200
                        Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-04 18:30 +0200
                          Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-04 19:10 +0200
                            Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-04 20:10 +0200
                            Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-05 01:00 +0200
                            Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-05 22:00 +0200
                              Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-05 23:10 +0200
                                Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-06 02:30 +0200
                              Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-06 02:10 +0200
                                Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-06 05:00 +0200
                                  Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-06 07:20 +0200
                                    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-06 15:40 +0200
                                      Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-07 00:00 +0200
                                Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-06 16:10 +0200
                                Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-06 21:20 +0200
                                  Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-06 22:20 +0200
                                    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-07 15:20 +0200
                                      Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-10 01:20 +0200
                                        Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-10 15:30 +0200
                                  Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-07 00:20 +0200
                          Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-05 01:20 +0200
                            Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Jeff Layton <jlayton@redhat.com> - 2017-04-05 13:20 +0200
                              Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-06 02:50 +0200
                    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Theodore Ts'o <tytso@mit.edu> - 2017-04-04 15:40 +0200
                    Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it NeilBrown <neilb@suse.com> - 2017-04-05 00:30 +0200
      Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking  infrastructure and convert ext4 to use it Matthew Wilcox <willy@infradead.org> - 2017-04-03 17:00 +0200

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

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

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?

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

[toc] | [next] | [standalone]

#1614273 — [RFC PATCH 2/4] 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 de622d4282a6..b76b3ffc141a 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -892,8 +892,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) {
+				filemap_set_wb_error(mapping, ret);
 				return ret;
+			}
 		}
 	}
 	return 0;
-- 
2.9.3

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

#1614274 — [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting

Most filesystems currently use mapping_set_error and
filemap_check_errors for setting and reporting/clearing writeback errors
at the mapping level. filemap_check_errors is indirectly called from
most of the filemap_fdatawait_* functions and from
filemap_write_and_wait*. These functions are called from all sorts of
contexts to wait on writeback to finish -- e.g. mostly in fsync, but
also in truncate calls, getattr, etc.

It's those non-fsync callers that are problematic. We should be
reporting writeback errors during fsync, but many places in the code
clear out errors before they can be properly reported, or report errors
at nonsensical times. If I get -EIO on a stat() call, how do I know that
was because writeback failed?

This patch adds a small bit of new infrastructure for setting and
reporting errors during pagecache writeback. While the above was my
original impetus for adding this, I think it's also the case that
current fsync semantics are just problematic for userland. Most
applications that call fsync do so to ensure that the data they wrote
has hit the backing store.

In the case where there are multiple writers to the file at the same
time, this is really hard to determine. The first one to call fsync will
see any stored error, and the rest get back 0. The processes with open
fd may not be associated with one another in any way. They could even be
in different containers, so ensuring coordination between all fsync
callers is not really an option.

One way to remedy this would be to track what file descriptor was used
to dirty the file, but that's rather cumbersome and would likely be
slow. However, there is a simpler way to improve the semantics here
without incurring too much overhead.

This set adds a wb_error field and a sequence counter to the
address_space, and a corresponding sequence counter in the struct file.
When errors are reported during writeback, we set the error field in the
mapping and increment the sequence counter.

When fsync or flush is called, we check the sequence in the file vs. the
one in the mapping. If the file's counter is behind the one in the
mapping, then we update the sequence counter in the file to the value of
the one in the mapping and report the error. If the file is "caught up"
then we just report 0.

This changes the semantics of fsync such that applications can now use
it to determine whether there were any writeback errors since fsync(fd)
was last called (or since the file was opened in the case of fsync
having never been called).

Note that those writeback errors may have occurred when writing data
that was dirtied via an entirely different fd, but that's the case now
with the current mapping_set_error/filemap_check_error infrastructure.
This will at least prevent you from getting a false report of success.

The basic idea here is for filesystems to use filemap_set_wb_error to
set the error in the mapping when there are writeback errors, and then
have the fsync and flush operations call filemap_report_wb_error just
before returning to ensure that those errors get reported properly.

Eventually, it may make sense to move the reporting into the generic
vfs_fsync_range helper, but doing it this way for now makes it simpler
to convert filesystems to the new API individually.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 Documentation/filesystems/vfs.txt | 14 +++++++--
 fs/open.c                         |  3 ++
 include/linux/fs.h                |  5 ++++
 mm/filemap.c                      | 61 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 569211703721..b2b5e411b340 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -577,6 +577,11 @@ should clear PG_Dirty and set PG_Writeback.  It can be actually
 written at any point after PG_Dirty is clear.  Once it is known to be
 safe, PG_Writeback is cleared.
 
+If there is an error during writeback, then the address_space should be
+marked with an error (typically using filemap_set_wb_error), in order to
+ensure that the error can later be reported to the application at fsync
+or close.
+
 Writeback makes use of a writeback_control structure...
 
 struct address_space_operations
@@ -885,11 +890,16 @@ otherwise noted.
 	"private_data" member in the file structure if you want to point
 	to a device structure
 
-  flush: called by the close(2) system call to flush a file
+  flush: called by the close(2) system call to flush a file. Writeback
+	errors not previously reported via fsync should be reported
+	here as you would for fsync.
 
   release: called when the last reference to an open file is closed
 
-  fsync: called by the fsync(2) system call
+  fsync: called by the fsync(2) system call. Filesystems that use the
+	pagecache should call filemap_report_wb_error before returning
+	to ensure that any errors that occurred during writeback are
+	reported and the file's error sequence advanced.
 
   fasync: called by the fcntl(2) system call when asynchronous
 	(non-blocking) mode is enabled for a file
diff --git a/fs/open.c b/fs/open.c
index 949cef29c3bb..26a1483bcad6 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -709,6 +709,9 @@ static int do_dentry_open(struct file *f,
 	f->f_inode = inode;
 	f->f_mapping = inode->i_mapping;
 
+	/* Don't need the i_lock since we're only interested in sequence */
+	f->f_wb_err_seq = inode->i_mapping->wb_err_seq;
+
 	if (unlikely(f->f_flags & O_PATH)) {
 		f->f_mode = FMODE_PATH;
 		f->f_op = &empty_fops;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7251f7bb45e8..88d4577d761a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -394,6 +394,8 @@ struct address_space {
 	gfp_t			gfp_mask;	/* implicit gfp mask for allocations */
 	struct list_head	private_list;	/* ditto */
 	void			*private_data;	/* ditto */
+	u64			wb_err_seq;
+	int			wb_err;
 } __attribute__((aligned(sizeof(long))));
 	/*
 	 * On most architectures that alignment is already the case; but
@@ -868,6 +870,7 @@ struct file {
 	struct list_head	f_tfile_llink;
 #endif /* #ifdef CONFIG_EPOLL */
 	struct address_space	*f_mapping;
+	u64			f_wb_err_seq;
 } __attribute__((aligned(4)));	/* lest something weird decides that 2 is OK */
 
 struct file_handle {
@@ -2521,6 +2524,8 @@ extern int __filemap_fdatawrite_range(struct address_space *mapping,
 extern int filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end);
 extern int filemap_check_errors(struct address_space *mapping);
+extern void filemap_set_wb_error(struct address_space *mapping, int err);
+extern int filemap_report_wb_error(struct file *file);
 
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
 			   int datasync);
diff --git a/mm/filemap.c b/mm/filemap.c
index 1694623a6289..703f069b9812 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -546,6 +546,67 @@ int filemap_write_and_wait_range(struct address_space *mapping,
 EXPORT_SYMBOL(filemap_write_and_wait_range);
 
 /**
+ * filemap_set_wb_error - set the wb error in the mapping for later reporting
+ * @mapping: mapping in which the error should be set
+ * @err: error to set
+ *
+ * When an error occurs during writeback of inode data, we must report that
+ * error during fsync. This function sets the writeback error field in the
+ * mapping, and increments the sequence counter. When fsync or close is later
+ * performed, the caller can then check the sequence in the mapping against
+ * the one in the file to determine whether the error should be reported.
+ *
+ * Note that we always use the latest writeback error, under the assumption
+ * that it doesn't really matter which one gets reported in the case where we
+ * have multiple errors (e.g. -EIO followed by -ENOSPC).
+ */
+void filemap_set_wb_error(struct address_space *mapping, int err)
+{
+	struct inode *inode = mapping->host;
+
+	/*
+	 * 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);
+
+	spin_lock(&inode->i_lock);
+	mapping->wb_err_seq++;
+	mapping->wb_err = err;
+	spin_unlock(&inode->i_lock);
+}
+EXPORT_SYMBOL(filemap_set_wb_error);
+
+/**
+ * filemap_report_wb_error - report wb error (if any) that was previously set
+ * @file: struct file on which the error is being reported
+ *
+ * When userland calls fsync or close (or something like nfsd does the
+ * equivalent), we want to report any writeback errors that occurred since
+ * the last fsync (or since the file was opened if there haven't been any).
+ *
+ * Check the sequence counter in the file and see if it lags the one in the
+ * mapping. If it does, then return whatever error is in the mapping and
+ * set the sequence counter to the value of the one in the mapping. Otherwise,
+ * return 0.
+ */
+int filemap_report_wb_error(struct file *file)
+{
+	int err = 0;
+	struct inode *inode = file_inode(file);
+	struct address_space *mapping = file->f_mapping;
+
+	spin_lock(&inode->i_lock);
+	if (file->f_wb_err_seq < mapping->wb_err_seq) {
+		err = mapping->wb_err;
+		file->f_wb_err_seq = mapping->wb_err_seq;
+	}
+	spin_unlock(&inode->i_lock);
+	return err;
+}
+EXPORT_SYMBOL(filemap_report_wb_error);
+
+/**
  * replace_page_cache_page - replace a pagecache page with a new one
  * @old:	page to be replaced
  * @new:	page to replace with
-- 
2.9.3

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

#1614921 — Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting

On 31.03.2017 22:26, Jeff Layton wrote:
> Most filesystems currently use mapping_set_error and
> filemap_check_errors for setting and reporting/clearing writeback errors
> at the mapping level. filemap_check_errors is indirectly called from
> most of the filemap_fdatawait_* functions and from
> filemap_write_and_wait*. These functions are called from all sorts of
> contexts to wait on writeback to finish -- e.g. mostly in fsync, but
> also in truncate calls, getattr, etc.
> 
> It's those non-fsync callers that are problematic. We should be
> reporting writeback errors during fsync, but many places in the code
> clear out errors before they can be properly reported, or report errors
> at nonsensical times. If I get -EIO on a stat() call, how do I know that
> was because writeback failed?
> 
> This patch adds a small bit of new infrastructure for setting and
> reporting errors during pagecache writeback. While the above was my
> original impetus for adding this, I think it's also the case that
> current fsync semantics are just problematic for userland. Most
> applications that call fsync do so to ensure that the data they wrote
> has hit the backing store.
> 
> In the case where there are multiple writers to the file at the same
> time, this is really hard to determine. The first one to call fsync will
> see any stored error, and the rest get back 0. The processes with open
> fd may not be associated with one another in any way. They could even be
> in different containers, so ensuring coordination between all fsync
> callers is not really an option.
> 
> One way to remedy this would be to track what file descriptor was used
> to dirty the file, but that's rather cumbersome and would likely be
> slow. However, there is a simpler way to improve the semantics here
> without incurring too much overhead.
> 
> This set adds a wb_error field and a sequence counter to the
> address_space, and a corresponding sequence counter in the struct file.
> When errors are reported during writeback, we set the error field in the
> mapping and increment the sequence counter.
> 
> When fsync or flush is called, we check the sequence in the file vs. the
> one in the mapping. If the file's counter is behind the one in the
> mapping, then we update the sequence counter in the file to the value of
> the one in the mapping and report the error. If the file is "caught up"
> then we just report 0.
> 
> This changes the semantics of fsync such that applications can now use
> it to determine whether there were any writeback errors since fsync(fd)
> was last called (or since the file was opened in the case of fsync
> having never been called).
> 
> Note that those writeback errors may have occurred when writing data
> that was dirtied via an entirely different fd, but that's the case now
> with the current mapping_set_error/filemap_check_error infrastructure.
> This will at least prevent you from getting a false report of success.
> 
> The basic idea here is for filesystems to use filemap_set_wb_error to
> set the error in the mapping when there are writeback errors, and then
> have the fsync and flush operations call filemap_report_wb_error just
> before returning to ensure that those errors get reported properly.
> 
> Eventually, it may make sense to move the reporting into the generic
> vfs_fsync_range helper, but doing it this way for now makes it simpler
> to convert filesystems to the new API individually.

There is already a mapping_set_error API which sets flags in
mapping->flags (AS_EIO/AS_ENOSPC). Aren't you essentially duplicating
some of the semantics of that API ?

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

#1615080 — Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting

On Mon, 2017-04-03 at 10:12 +0300, Nikolay Borisov wrote:
> 
> On 31.03.2017 22:26, Jeff Layton wrote:
> > Most filesystems currently use mapping_set_error and
> > filemap_check_errors for setting and reporting/clearing writeback errors
> > at the mapping level. filemap_check_errors is indirectly called from
> > most of the filemap_fdatawait_* functions and from
> > filemap_write_and_wait*. These functions are called from all sorts of
> > contexts to wait on writeback to finish -- e.g. mostly in fsync, but
> > also in truncate calls, getattr, etc.
> > 
> > It's those non-fsync callers that are problematic. We should be
> > reporting writeback errors during fsync, but many places in the code
> > clear out errors before they can be properly reported, or report errors
> > at nonsensical times. If I get -EIO on a stat() call, how do I know that
> > was because writeback failed?
> > 
> > This patch adds a small bit of new infrastructure for setting and
> > reporting errors during pagecache writeback. While the above was my
> > original impetus for adding this, I think it's also the case that
> > current fsync semantics are just problematic for userland. Most
> > applications that call fsync do so to ensure that the data they wrote
> > has hit the backing store.
> > 
> > In the case where there are multiple writers to the file at the same
> > time, this is really hard to determine. The first one to call fsync will
> > see any stored error, and the rest get back 0. The processes with open
> > fd may not be associated with one another in any way. They could even be
> > in different containers, so ensuring coordination between all fsync
> > callers is not really an option.
> > 
> > One way to remedy this would be to track what file descriptor was used
> > to dirty the file, but that's rather cumbersome and would likely be
> > slow. However, there is a simpler way to improve the semantics here
> > without incurring too much overhead.
> > 
> > This set adds a wb_error field and a sequence counter to the
> > address_space, and a corresponding sequence counter in the struct file.
> > When errors are reported during writeback, we set the error field in the
> > mapping and increment the sequence counter.
> > 
> > When fsync or flush is called, we check the sequence in the file vs. the
> > one in the mapping. If the file's counter is behind the one in the
> > mapping, then we update the sequence counter in the file to the value of
> > the one in the mapping and report the error. If the file is "caught up"
> > then we just report 0.
> > 
> > This changes the semantics of fsync such that applications can now use
> > it to determine whether there were any writeback errors since fsync(fd)
> > was last called (or since the file was opened in the case of fsync
> > having never been called).
> > 
> > Note that those writeback errors may have occurred when writing data
> > that was dirtied via an entirely different fd, but that's the case now
> > with the current mapping_set_error/filemap_check_error infrastructure.
> > This will at least prevent you from getting a false report of success.
> > 
> > The basic idea here is for filesystems to use filemap_set_wb_error to
> > set the error in the mapping when there are writeback errors, and then
> > have the fsync and flush operations call filemap_report_wb_error just
> > before returning to ensure that those errors get reported properly.
> > 
> > Eventually, it may make sense to move the reporting into the generic
> > vfs_fsync_range helper, but doing it this way for now makes it simpler
> > to convert filesystems to the new API individually.
> 
> There is already a mapping_set_error API which sets flags in
> mapping->flags (AS_EIO/AS_ENOSPC). Aren't you essentially duplicating
> some of the semantics of that API ?

Yes, more or less for now. The arguments of mapping_set_error and
filemap_set_wb_error are the same, but they do different things with the
error.

The plan is eventually to eliminate mapping_set_error and convert
everything over to use the new infrastructure I'm adding. That's
difficult to do all at once however, so for now some duplication is
necessary.
-- 
Jeff Layton <jlayton@redhat.com>

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

#1615273 — Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting

On Fri, Mar 31, 2017 at 03:26:00PM -0400, Jeff Layton wrote:
> This set adds a wb_error field and a sequence counter to the
> address_space, and a corresponding sequence counter in the struct file.
> When errors are reported during writeback, we set the error field in the
> mapping and increment the sequence counter.

> +++ b/fs/open.c
> @@ -709,6 +709,9 @@ static int do_dentry_open(struct file *f,
>  	f->f_inode = inode;
>  	f->f_mapping = inode->i_mapping;
>  
> +	/* Don't need the i_lock since we're only interested in sequence */
> +	f->f_wb_err_seq = inode->i_mapping->wb_err_seq;
> +

Do we need READ_ONCE() though, to ensure we get a consistent view of
wb_err_seq?  In particular, you made it 64 bit, so 32-bit architectures
are going to have a problem if it's rolling over between 2^32-1 and 2^32.

> +++ b/include/linux/fs.h
> @@ -394,6 +394,8 @@ struct address_space {
>  	gfp_t			gfp_mask;	/* implicit gfp mask for allocations */
>  	struct list_head	private_list;	/* ditto */
>  	void			*private_data;	/* ditto */
> +	u64			wb_err_seq;
> +	int			wb_err;
>  } __attribute__((aligned(sizeof(long))));
>  	/*
>  	 * On most architectures that alignment is already the case; but

I thought we had you convinced to make wb_err_seq an s32 and do clock
arithmetic?

> +int filemap_report_wb_error(struct file *file)
> +{
> +	int err = 0;
> +	struct inode *inode = file_inode(file);
> +	struct address_space *mapping = file->f_mapping;
> +
> +	spin_lock(&inode->i_lock);
> +	if (file->f_wb_err_seq < mapping->wb_err_seq) {
> +		err = mapping->wb_err;
> +		file->f_wb_err_seq = mapping->wb_err_seq;
> +	}
> +	spin_unlock(&inode->i_lock);
> +	return err;
> +}

Now that I think about this some more, I don't think you even need clock
arithmetic -- you just need !=.  And that means there's only a 1 in 2^32
chance that you miss an error.  Good enough, I say!  Particularly since
if errors are occurring that frequently that we wrapped the sequence
counter, the chance that we hit that magic point are really low.

We could even combine the two (I know Dave Chinner has been really
against growing struct address_space in the past):

int decode_wb_err(u32 wb_err)
{
	if (wb_err & 1)
		return -EIO;
	if (wb_err & 2)
		return -ENOSPC;
	return 0;
}

void set_wb_err(struct address_space *mapping, int err)
{
	if (err == -EIO)
		mapping->wb_err |= 1;
	else if (err == -ENOSPC)
		mapping->wb_err |= 2;
	else
		return;
	mapping->wb_err += 4;
}

...
	if (file->f_wb_err != mapping->wb_err) {
		err = decode_wb_err(mapping->wb_err);
		file->f_wb_err = mapping->wb_err;
	}

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

#1615328 — Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting

On Mon, 2017-04-03 at 07:47 -0700, Matthew Wilcox wrote:
> On Fri, Mar 31, 2017 at 03:26:00PM -0400, Jeff Layton wrote:
> > This set adds a wb_error field and a sequence counter to the
> > address_space, and a corresponding sequence counter in the struct file.
> > When errors are reported during writeback, we set the error field in the
> > mapping and increment the sequence counter.
> > +++ b/fs/open.c
> > @@ -709,6 +709,9 @@ static int do_dentry_open(struct file *f,
> >  	f->f_inode = inode;
> >  	f->f_mapping = inode->i_mapping;
> >  
> > +	/* Don't need the i_lock since we're only interested in sequence */
> > +	f->f_wb_err_seq = inode->i_mapping->wb_err_seq;
> > +
> 
> Do we need READ_ONCE() though, to ensure we get a consistent view of
> wb_err_seq?  In particular, you made it 64 bit, so 32-bit architectures
> are going to have a problem if it's rolling over between 2^32-1 and 2^32.
> 

Yeah, I thought about that, and wasn't sure so I left that off. If you
think it's a good idea, then I'm fine with adding it.

> > +++ b/include/linux/fs.h
> > @@ -394,6 +394,8 @@ struct address_space {
> >  	gfp_t			gfp_mask;	/* implicit gfp mask for allocations */
> >  	struct list_head	private_list;	/* ditto */
> >  	void			*private_data;	/* ditto */
> > +	u64			wb_err_seq;
> > +	int			wb_err;
> >  } __attribute__((aligned(sizeof(long))));
> >  	/*
> >  	 * On most architectures that alignment is already the case; but
> 
> I thought we had you convinced to make wb_err_seq an s32 and do clock
> arithmetic?
> 
> > +int filemap_report_wb_error(struct file *file)
> > +{
> > +	int err = 0;
> > +	struct inode *inode = file_inode(file);
> > +	struct address_space *mapping = file->f_mapping;
> > +
> > +	spin_lock(&inode->i_lock);
> > +	if (file->f_wb_err_seq < mapping->wb_err_seq) {
> > +		err = mapping->wb_err;
> > +		file->f_wb_err_seq = mapping->wb_err_seq;
> > +	}
> > +	spin_unlock(&inode->i_lock);
> > +	return err;
> > +}
> 
> Now that I think about this some more, I don't think you even need clock
> arithmetic -- you just need !=.  And that means there's only a 1 in 2^32
> chance that you miss an error.  Good enough, I say!  Particularly since
> if errors are occurring that frequently that we wrapped the sequence
> counter, the chance that we hit that magic point are really low.
> 

> We could even combine the two (I know Dave Chinner has been really
> against growing struct address_space in the past):
> 
> int decode_wb_err(u32 wb_err)
> {
> 	if (wb_err & 1)
> 		return -EIO;
> 	if (wb_err & 2)
> 		return -ENOSPC;
> 	return 0;
> }
> 
> void set_wb_err(struct address_space *mapping, int err)
> {
> 	if (err == -EIO)
> 		mapping->wb_err |= 1;
> 	else if (err == -ENOSPC)
> 		mapping->wb_err |= 2;
> 	else
> 		return;
> 	mapping->wb_err += 4;
> }
> 
> ...
> 	if (file->f_wb_err != mapping->wb_err) {
> 		err = decode_wb_err(mapping->wb_err);
> 		file->f_wb_err = mapping->wb_err;
> 	}

Agreed. I had the same thought about checking for equality just after I
hit send last week. :)

Yes, so just to be clear here if you bump a 32 bit counter every
microsecond you'll end up wrapping in a little over an hour. How fast
can DAX generate I/O errors? :)

I'm fine with a 32 bit counter (and even with using the low order bits
to store error flags) if we're ok with that limitation. The big
question there is whether it's ok to continue reporting -EIO when there
has actually been nothing but -ENOSPC errors since the last fsync. I
think it's a corner case that's not of terribly great concern so I'm
fine with that.

We could try to mitigate it by zeroing out the value when i_writecount
goes to zero though. Then if you close all of the fds on the file, the
error is cleared. Or maybe we could add a new ioctl to explicitly zero
it out?
-- 
Jeff Layton <jlayton@redhat.com>

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

#1615387 — Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting

On Mon, Apr 03, 2017 at 11:19:51AM -0400, Jeff Layton wrote:
> Yes, so just to be clear here if you bump a 32 bit counter every
> microsecond you'll end up wrapping in a little over an hour. How fast
> can DAX generate I/O errors? :)

I admit to not having picked through the code, but how often do we try
to do writebacks?  And how often do we retry writebacks once an -EIO
has happened?  Once we mark a page as PG_error, do we keep trying to
write it back and set the AS error each time?

> I'm fine with a 32 bit counter (and even with using the low order bits
> to store error flags) if we're ok with that limitation. The big
> question there is whether it's ok to continue reporting -EIO when there
> has actually been nothing but -ENOSPC errors since the last fsync. I
> think it's a corner case that's not of terribly great concern so I'm
> fine with that.

Yeah, I was thinking about that, and I'm fine with it too.

> We could try to mitigate it by zeroing out the value when i_writecount
> goes to zero though. Then if you close all of the fds on the file, the
> error is cleared. Or maybe we could add a new ioctl to explicitly zero
> it out?

I'm OK with zeroing the wb_err once i_writecount drops to 0.  Everybody
who cares has already been notified.  The new ioctl feels like overkill.

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

#1615401 — Re: [RFC PATCH 1/4] fs: new infrastructure for writeback error handling and reporting

On Mon, 2017-04-03 at 09:15 -0700, Matthew Wilcox wrote:
> On Mon, Apr 03, 2017 at 11:19:51AM -0400, Jeff Layton wrote:
> > Yes, so just to be clear here if you bump a 32 bit counter every
> > microsecond you'll end up wrapping in a little over an hour. How fast
> > can DAX generate I/O errors? :)
> 
> I admit to not having picked through the code, but how often do we try
> to do writebacks?  And how often do we retry writebacks once an -EIO
> has happened?  Once we mark a page as PG_error, do we keep trying to
> write it back and set the AS error each time?
> 

It depends, but I think it could theoretically happen after trying to
sync out every page in a file. With something like DAX it seems like
you could do that pretty quickly.

One thing we could do is to try and push the filemap_set_wb_error calls
out of writepage ops and allow the callers to do that so we can avoid
bumping the counter unnecessarily. Not sure if that's enough to avoid
wrapping too quickly.

> > I'm fine with a 32 bit counter (and even with using the low order bits
> > to store error flags) if we're ok with that limitation. The big
> > question there is whether it's ok to continue reporting -EIO when there
> > has actually been nothing but -ENOSPC errors since the last fsync. I
> > think it's a corner case that's not of terribly great concern so I'm
> > fine with that.
> 
> Yeah, I was thinking about that, and I'm fine with it too.
> 
> > We could try to mitigate it by zeroing out the value when i_writecount
> > goes to zero though. Then if you close all of the fds on the file, the
> > error is cleared. Or maybe we could add a new ioctl to explicitly zero
> > it out?
> 
> I'm OK with zeroing the wb_err once i_writecount drops to 0.  Everybody
> who cares has already been notified.  The new ioctl feels like overkill.

That's my feeling too.
-- 
Jeff Layton <jlayton@redhat.com>

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

#1614275 — [RFC PATCH 4/4] ext4: wire it up to the new writeback error reporting infrastructure

Convert ext4 to use filemap_set_wb_error for reporting writeback errors
instead of mapping_set_error. Ensure that it calls filemap_report_wb_error
before returning from fsync, and add a new flush operation that ensures
that just calls filemap_report_wb_error before returning.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 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 ++--
 6 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index e8b365000d73..c333b148b5fe 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -656,6 +656,7 @@ const struct file_operations ext4_dir_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= ext4_compat_ioctl,
 #endif
+	.flush		= ext4_flush_file,
 	.fsync		= ext4_sync_file,
 	.open		= ext4_dir_open,
 	.release	= ext4_release_dir,
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index f493af666591..20437bedeed0 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2382,6 +2382,7 @@ extern int ext4_check_all_de(struct inode *dir, struct buffer_head *bh,
 
 /* fsync.c */
 extern int ext4_sync_file(struct file *, loff_t, loff_t, int);
+extern int ext4_flush_file(struct file *, fl_owner_t);
 
 /* hash.c */
 extern int ext4fs_dirhash(const char *name, int len, struct
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 8210c1f43556..cc536a1aa731 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -735,6 +735,7 @@ const struct file_operations ext4_file_operations = {
 	.mmap		= ext4_file_mmap,
 	.open		= ext4_file_open,
 	.release	= ext4_release_file,
+	.flush		= ext4_flush_file,
 	.fsync		= ext4_sync_file,
 	.get_unmapped_area = thp_get_unmapped_area,
 	.splice_read	= generic_file_splice_read,
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 9d549608fd30..e381ab03f1cc 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -79,6 +79,11 @@ static int ext4_sync_parent(struct inode *inode)
 	return ret;
 }
 
+int ext4_flush_file(struct file *file, fl_owner_t id)
+{
+	return filemap_report_wb_error(file);
+}
+
 /*
  * akpm: A new design for ext4_sync_file().
  *
@@ -96,12 +101,12 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	struct inode *inode = file->f_mapping->host;
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
-	int ret = 0, err;
+	int ret = -EIO, err;
 	tid_t commit_tid;
 	bool needs_barrier = false;
 
 	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
-		return -EIO;
+		goto out;
 
 	J_ASSERT(ext4_journal_current_handle() == NULL);
 
@@ -110,6 +115,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (inode->i_sb->s_flags & MS_RDONLY) {
 		/* Make sure that we read updated s_mount_flags value */
 		smp_rmb();
+		ret = 0;
 		if (EXT4_SB(inode->i_sb)->s_mount_flags & EXT4_MF_FS_ABORTED)
 			ret = -EROFS;
 		goto out;
@@ -126,7 +132,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 
 	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
 	if (ret)
-		return ret;
+		goto out;
 	/*
 	 * data=writeback,ordered:
 	 *  The caller's filemap_fdatawrite()/wait will sync the data.
@@ -159,5 +165,6 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	}
 out:
 	trace_ext4_sync_file_exit(inode, ret);
-	return ret;
+	err = filemap_report_wb_error(file);
+	return ret ? : err;
 }
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4247d8d25687..b9c8a73c404b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2634,7 +2634,7 @@ static int __writepage(struct page *page, struct writeback_control *wbc,
 {
 	struct address_space *mapping = data;
 	int ret = ext4_writepage(page, wbc);
-	mapping_set_error(mapping, ret);
+	filemap_set_wb_error(mapping, ret);
 	return ret;
 }
 
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 208241b06662..5adcf5d0ee9a 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -87,7 +87,7 @@ static void ext4_finish_bio(struct bio *bio)
 
 		if (bio->bi_error) {
 			SetPageError(page);
-			mapping_set_error(page->mapping, -EIO);
+			filemap_set_wb_error(page->mapping, -EIO);
 		}
 		bh = head = page_buffers(page);
 		/*
@@ -311,7 +311,7 @@ static void ext4_end_bio(struct bio *bio)
 			     (long) io_end->size,
 			     (unsigned long long)
 			     bi_sector >> (inode->i_blkbits - 9));
-		mapping_set_error(inode->i_mapping, bio->bi_error);
+		filemap_set_wb_error(inode->i_mapping, bio->bi_error);
 	}
 
 	if (io_end->flag & EXT4_IO_END_UNWRITTEN) {
-- 
2.9.3

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

#1614878

[Multipart message — attachments visible in raw view] — view raw

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

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

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

On Mon, 2017-04-03 at 14:25 +1000, NeilBrown wrote:
> 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 ??
> 

It's because I wasn't thinking about all of the places that currently
call mapping_set_error with an error of 0. This worked for ext4 since we
only call this when there is an actual error. You're correct here -- we
should only set the error when it's non-zero. I'll fix 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'm not quite convinced that it's really useful to do anything but
report the latest error.

But...if we did need to prefer one over another, could we get away with
always reporting -EIO once that error occurs? If so, then we'd still
just need a single sequence counter.

> 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() ??
> 

Sure, that's possible.

I'm leery of making too much in the way of changes to the generic VFS
layer code just yet. After making several abortive attempts to try to
fix some of this with large, sweeping changes to the code, I think the
approach of doing this on a per-filesystem basis will be saner.

My concern there for now is that some code (e.g. fs/buffer.c) is shared
between filesystems and will need to call both routines in the interim.
Suppose we have a filesystem (ext2?) that is using the older routines
for now. Making the change to filp_close above might subtly change its
behavior, and I don't think we want to do that.

Once we have everything converted to use the newer API, we should be
able to collapse a lot of the flush routines into the above though.

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

Applications should, but the close(2) manpage does say:

       Not  checking  the return value of close() is a common
       but nevertheless serious  programming  error.   It  is
       quite  possible  that  errors  on  a previous write(2)
       operation are first reported  at  the  final  close().
       Not  checking  the  return value when closing the file
       may lead to silent loss of data.

POSIX seems to say that that behavior is optional, but I think reporting
errors at close is a good idea. There are programs that do check for
that, but whether they do anything useful with the error is a little
less clear.

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

-- 
Jeff Layton <jlayton@redhat.com>

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

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

On Mon, Apr 03, 2017 at 06:28:38AM -0400, Jeff Layton wrote:
> On Mon, 2017-04-03 at 14:25 +1000, NeilBrown wrote:
> > 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'm not quite convinced that it's really useful to do anything but
> report the latest error.
> 
> But...if we did need to prefer one over another, could we get away with
> always reporting -EIO once that error occurs? If so, then we'd still
> just need a single sequence counter.

I wonder whether it's even worth supporting both EIO and ENOSPC for a
writeback problem.  If I understand correctly, at the time of write(),
filesystems check to see if they have enough blocks to satisfy the
request, so ENOSPC only comes up in the writeback context for thinly
provisioned devices.

Programs have basically no use for the distinction.  In either case,
the situation is the same.  The written data is safely in RAM and cannot
be written to the storage.  If one were to make superhuman efforts,
one could mmap the file and write() it to a different device, but that
is incredibly rare.  For most programs, the response is to just die and
let the human deal with the corrupted file.

From a sysadmin point of view, of course the situation is different,
and the remedy is different, but they should be getting that information
through a different mechanism than monitoring the errno from every
system call.

If we do want to continue to support both EIO and ENOSPC from writeback,
then let's have EIO override ENOSPC as an error.  ie if an ENOSPC comes
in after an EIO is set, it only bumps the counter and applications will
see EIO, not ENOSPC on fresh calls to fsync().

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

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

On Mon, 2017-04-03 at 07:32 -0700, Matthew Wilcox wrote:
> On Mon, Apr 03, 2017 at 06:28:38AM -0400, Jeff Layton wrote:
> > On Mon, 2017-04-03 at 14:25 +1000, NeilBrown wrote:
> > > 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'm not quite convinced that it's really useful to do anything but
> > report the latest error.
> > 
> > But...if we did need to prefer one over another, could we get away with
> > always reporting -EIO once that error occurs? If so, then we'd still
> > just need a single sequence counter.
> 
> I wonder whether it's even worth supporting both EIO and ENOSPC for a
> writeback problem.  If I understand correctly, at the time of write(),
> filesystems check to see if they have enough blocks to satisfy the
> request, so ENOSPC only comes up in the writeback context for thinly
> provisioned devices.
> 
> Programs have basically no use for the distinction.  In either case,
> the situation is the same.  The written data is safely in RAM and cannot
> be written to the storage.  If one were to make superhuman efforts,
> one could mmap the file and write() it to a different device, but that
> is incredibly rare.  For most programs, the response is to just die and
> let the human deal with the corrupted file.
> 
> From a sysadmin point of view, of course the situation is different,
> and the remedy is different, but they should be getting that information
> through a different mechanism than monitoring the errno from every
> system call.
> 
> If we do want to continue to support both EIO and ENOSPC from writeback,
> then let's have EIO override ENOSPC as an error.  ie if an ENOSPC comes
> in after an EIO is set, it only bumps the counter and applications will
> see EIO, not ENOSPC on fresh calls to fsync().


No, ENOSPC on writeback can certainly happen with network filesystems.
NFS and CIFS have no way to reserve space. You wouldn't want to have to
do an extra RPC on every buffered write. :)
-- 
Jeff Layton <jlayton@redhat.com>

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

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

On Mon, Apr 03, 2017 at 01:47:37PM -0400, Jeff Layton wrote:
> On Mon, 2017-04-03 at 07:32 -0700, Matthew Wilcox wrote:
> > On Mon, Apr 03, 2017 at 06:28:38AM -0400, Jeff Layton wrote:
> > > On Mon, 2017-04-03 at 14:25 +1000, NeilBrown wrote:
> > > > 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'm not quite convinced that it's really useful to do anything but
> > > report the latest error.
> > > 
> > > But...if we did need to prefer one over another, could we get away with
> > > always reporting -EIO once that error occurs? If so, then we'd still
> > > just need a single sequence counter.
> > 
> > I wonder whether it's even worth supporting both EIO and ENOSPC for a
> > writeback problem.  If I understand correctly, at the time of write(),
> > filesystems check to see if they have enough blocks to satisfy the
> > request, so ENOSPC only comes up in the writeback context for thinly
> > provisioned devices.
> > 
> > Programs have basically no use for the distinction.  In either case,
> > the situation is the same.  The written data is safely in RAM and cannot
> > be written to the storage.  If one were to make superhuman efforts,
> > one could mmap the file and write() it to a different device, but that
> > is incredibly rare.  For most programs, the response is to just die and
> > let the human deal with the corrupted file.
> > 
> > From a sysadmin point of view, of course the situation is different,
> > and the remedy is different, but they should be getting that information
> > through a different mechanism than monitoring the errno from every
> > system call.
> > 
> > If we do want to continue to support both EIO and ENOSPC from writeback,
> > then let's have EIO override ENOSPC as an error.  ie if an ENOSPC comes
> > in after an EIO is set, it only bumps the counter and applications will
> > see EIO, not ENOSPC on fresh calls to fsync().
> 
> 
> No, ENOSPC on writeback can certainly happen with network filesystems.
> NFS and CIFS have no way to reserve space. You wouldn't want to have to
> do an extra RPC on every buffered write. :)

CIFS has a way to reserve space. Look into "allocation size" on create.

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

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

On Mon, 2017-04-03 at 11:09 -0700, Jeremy Allison wrote:
> On Mon, Apr 03, 2017 at 01:47:37PM -0400, Jeff Layton wrote:
> > On Mon, 2017-04-03 at 07:32 -0700, Matthew Wilcox wrote:
> > > On Mon, Apr 03, 2017 at 06:28:38AM -0400, Jeff Layton wrote:
> > > > On Mon, 2017-04-03 at 14:25 +1000, NeilBrown wrote:
> > > > > 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'm not quite convinced that it's really useful to do anything but
> > > > report the latest error.
> > > > 
> > > > But...if we did need to prefer one over another, could we get away with
> > > > always reporting -EIO once that error occurs? If so, then we'd still
> > > > just need a single sequence counter.
> > > 
> > > I wonder whether it's even worth supporting both EIO and ENOSPC for a
> > > writeback problem.  If I understand correctly, at the time of write(),
> > > filesystems check to see if they have enough blocks to satisfy the
> > > request, so ENOSPC only comes up in the writeback context for thinly
> > > provisioned devices.
> > > 
> > > Programs have basically no use for the distinction.  In either case,
> > > the situation is the same.  The written data is safely in RAM and cannot
> > > be written to the storage.  If one were to make superhuman efforts,
> > > one could mmap the file and write() it to a different device, but that
> > > is incredibly rare.  For most programs, the response is to just die and
> > > let the human deal with the corrupted file.
> > > 
> > > From a sysadmin point of view, of course the situation is different,
> > > and the remedy is different, but they should be getting that information
> > > through a different mechanism than monitoring the errno from every
> > > system call.
> > > 
> > > If we do want to continue to support both EIO and ENOSPC from writeback,
> > > then let's have EIO override ENOSPC as an error.  ie if an ENOSPC comes
> > > in after an EIO is set, it only bumps the counter and applications will
> > > see EIO, not ENOSPC on fresh calls to fsync().
> > 
> > 
> > No, ENOSPC on writeback can certainly happen with network filesystems.
> > NFS and CIFS have no way to reserve space. You wouldn't want to have to
> > do an extra RPC on every buffered write. :)
> 
> CIFS has a way to reserve space. Look into "allocation size" on create.

That won't help here as it's done on open().

The problem here is that we might create a file (and not preallocate
anything), then write a bunch of stuff to the cache under an oplock.
Then when we go to write back, we get the CIFS equivalent of -ENOSPC.

What local filesystems do (AIUI) is preallocate so that you can catch
an ENOSPC condition earlier, when you're dirtying new pages in the
cache. That's pretty much impossible to do on a network filesystem
though.

-- 
Jeff Layton <jlayton@redhat.com>

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

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

On Mon, Apr 03, 2017 at 02:18:44PM -0400, Jeff Layton wrote:
> On Mon, 2017-04-03 at 11:09 -0700, Jeremy Allison wrote:
> > On Mon, Apr 03, 2017 at 01:47:37PM -0400, Jeff Layton wrote:
> > > On Mon, 2017-04-03 at 07:32 -0700, Matthew Wilcox wrote:
> > > > On Mon, Apr 03, 2017 at 06:28:38AM -0400, Jeff Layton wrote:
> > > > > On Mon, 2017-04-03 at 14:25 +1000, NeilBrown wrote:
> > > > > > 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'm not quite convinced that it's really useful to do anything but
> > > > > report the latest error.
> > > > > 
> > > > > But...if we did need to prefer one over another, could we get away with
> > > > > always reporting -EIO once that error occurs? If so, then we'd still
> > > > > just need a single sequence counter.
> > > > 
> > > > I wonder whether it's even worth supporting both EIO and ENOSPC for a
> > > > writeback problem.  If I understand correctly, at the time of write(),
> > > > filesystems check to see if they have enough blocks to satisfy the
> > > > request, so ENOSPC only comes up in the writeback context for thinly
> > > > provisioned devices.
> > > > 
> > > > Programs have basically no use for the distinction.  In either case,
> > > > the situation is the same.  The written data is safely in RAM and cannot
> > > > be written to the storage.  If one were to make superhuman efforts,
> > > > one could mmap the file and write() it to a different device, but that
> > > > is incredibly rare.  For most programs, the response is to just die and
> > > > let the human deal with the corrupted file.
> > > > 
> > > > From a sysadmin point of view, of course the situation is different,
> > > > and the remedy is different, but they should be getting that information
> > > > through a different mechanism than monitoring the errno from every
> > > > system call.
> > > > 
> > > > If we do want to continue to support both EIO and ENOSPC from writeback,
> > > > then let's have EIO override ENOSPC as an error.  ie if an ENOSPC comes
> > > > in after an EIO is set, it only bumps the counter and applications will
> > > > see EIO, not ENOSPC on fresh calls to fsync().
> > > 
> > > 
> > > No, ENOSPC on writeback can certainly happen with network filesystems.
> > > NFS and CIFS have no way to reserve space. You wouldn't want to have to
> > > do an extra RPC on every buffered write. :)
> > 
> > CIFS has a way to reserve space. Look into "allocation size" on create.
> 
> That won't help here as it's done on open().
> 
> The problem here is that we might create a file (and not preallocate
> anything), then write a bunch of stuff to the cache under an oplock.
> Then when we go to write back, we get the CIFS equivalent of -ENOSPC.
> 
> What local filesystems do (AIUI) is preallocate so that you can catch
> an ENOSPC condition earlier, when you're dirtying new pages in the
> cache. That's pretty much impossible to do on a network filesystem
> though.

There's also SMB_SET_FILE_ALLOCATION_INFO which can be
done over SMB1/2/3 on an open file handle.

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

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

On Mon, Apr 03, 2017 at 11:36:48AM -0700, Jeremy Allison wrote:
> On Mon, Apr 03, 2017 at 02:18:44PM -0400, Jeff Layton wrote:
> > On Mon, 2017-04-03 at 11:09 -0700, Jeremy Allison wrote:
> > > 
> > > CIFS has a way to reserve space. Look into "allocation size" on create.
> > 
> > That won't help here as it's done on open().
> > 
> > The problem here is that we might create a file (and not preallocate
> > anything), then write a bunch of stuff to the cache under an oplock.
> > Then when we go to write back, we get the CIFS equivalent of -ENOSPC.
> > 
> > What local filesystems do (AIUI) is preallocate so that you can catch
> > an ENOSPC condition earlier, when you're dirtying new pages in the
> > cache. That's pretty much impossible to do on a network filesystem
> > though.
> 
> There's also SMB_SET_FILE_ALLOCATION_INFO which can be
> done over SMB1/2/3 on an open file handle.

There's *always* a way to do something in SMB1/2/3. :-).

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

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

On Mon, 2017-04-03 at 11:40 -0700, Jeremy Allison wrote:
> On Mon, Apr 03, 2017 at 11:36:48AM -0700, Jeremy Allison wrote:
> > On Mon, Apr 03, 2017 at 02:18:44PM -0400, Jeff Layton wrote:
> > > On Mon, 2017-04-03 at 11:09 -0700, Jeremy Allison wrote:
> > > > 
> > > > CIFS has a way to reserve space. Look into "allocation size" on create.
> > > 
> > > That won't help here as it's done on open().
> > > 
> > > The problem here is that we might create a file (and not preallocate
> > > anything), then write a bunch of stuff to the cache under an oplock.
> > > Then when we go to write back, we get the CIFS equivalent of -ENOSPC.
> > > 
> > > What local filesystems do (AIUI) is preallocate so that you can catch
> > > an ENOSPC condition earlier, when you're dirtying new pages in the
> > > cache. That's pretty much impossible to do on a network filesystem
> > > though.
> > 
> > There's also SMB_SET_FILE_ALLOCATION_INFO which can be
> > done over SMB1/2/3 on an open file handle.
> 
> There's *always* a way to do something in SMB1/2/3. :-).

Yes, indeed...Still, I think we'll need to deal with this during
writeback as well. Earlier versions of NFS certainly don't have
anything along those lines, though you could probably do some sort of
speculative preallocation with v4.2.

-- 
Jeff Layton <jlayton@redhat.com>

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

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

On Mon, Apr 03, 2017 at 01:47:37PM -0400, Jeff Layton wrote:
> > I wonder whether it's even worth supporting both EIO and ENOSPC for a
> > writeback problem.  If I understand correctly, at the time of write(),
> > filesystems check to see if they have enough blocks to satisfy the
> > request, so ENOSPC only comes up in the writeback context for thinly
> > provisioned devices.
> 
> No, ENOSPC on writeback can certainly happen with network filesystems.
> NFS and CIFS have no way to reserve space. You wouldn't want to have to
> do an extra RPC on every buffered write. :)

Aaah, yes, network filesystems.  I would indeed not want to do an extra
RPC on every write to a hole (it's a hole vs non-hole question, rather
than a buffered/unbuffered question ... unless you're WAFLing and not
reclaiming quickly enough, I suppose).

So, OK, that makes sense, we should keep allowing filesystems to report
ENOSPC as a writeback error.  But I think much of the argument below
still holds, and we should continue to have a prior EIO to be reported
over a new ENOSPC (even if the program has already consumed the EIO).

If you find that unconvincing, we could do something like this ...

void filemap_set_wb_error(struct address_space *mapping, int err)
{
	struct inode *inode = mapping->host;

	if (!err)
		return;
	/*
	 * 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);

	spin_lock(&inode->i_lock);
	if (err == -EIO)
		mapping->wb_err |= 1;
	else if (err == -ENOSPC)
		mapping->wb_err |= 2;
	mapping->wb_err += 4;
	spin_unlock(&inode->i_lock);
}

int filemap_report_wb_error(struct file *file)
{
	struct inode *inode = file_inode(file);
	struct address_space *mapping = file->f_mapping;
	int err;

	spin_lock(&inode->i_lock);
	if (file->f_wb_err == mapping->wb_err) {
		err = 0;
	} else if (mapping->wb_err & 1) {
		filp->f_wb_err = mapping->wb_err & ~2;
		err = -EIO;
	} else {
		filp->f_wb_err = mapping->wb_err;
		err = -ENOSPC;
	}
	spin_unlock(&inode->i_lock);
	return err;
}

If I got that right, calling fsync() on an inode which has experienced
both errors would first get an EIO.  Calling fsync() on it again would
get an ENOSPC.  Calling fsync() on it a third time would get 0.  When
either error occurs again, the thread will go back through the cycle
(EIO -> ENOSPC -> 0).

> > Programs have basically no use for the distinction.  In either case,
> > the situation is the same.  The written data is safely in RAM and cannot
> > be written to the storage.  If one were to make superhuman efforts,
> > one could mmap the file and write() it to a different device, but that
> > is incredibly rare.  For most programs, the response is to just die and
> > let the human deal with the corrupted file.
> > 
> > From a sysadmin point of view, of course the situation is different,
> > and the remedy is different, but they should be getting that information
> > through a different mechanism than monitoring the errno from every
> > system call.
> > 
> > If we do want to continue to support both EIO and ENOSPC from writeback,
> > then let's have EIO override ENOSPC as an error.  ie if an ENOSPC comes
> > in after an EIO is set, it only bumps the counter and applications will
> > see EIO, not ENOSPC on fresh calls to fsync().

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

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

Back to top | Article view | linux.kernel

csiph-web