[PATCH v2 08/17] fs: retrofit old error reporting API onto new infrastructure

From: Jeff Layton
Date: Wed Apr 12 2017 - 08:07:38 EST

Now that we have a better way to store and report errors that occur
during writeback, we need to convert the existing codebase to use it. We
could just adapt all of the filesystem code and related infrastructure
to the new API, but that's a lot of churn.

When it comes to setting errors in the mapping, filemap_set_wb_error is
a drop-in replacement for mapping_set_error. Turn that function into a
simple wrapper around the new one.

Because we want to ensure that writeback errors are always reported at
fsync time, inject filemap_report_wb_error calls much closer to the
syscall boundary. For fsync calls (and things like the nfsd equivalent),
we either return the error that the fsync operation returns, or the one
returned by filemap_report_wb_error. In both cases, we advance the
file->f_wb_err to the latest value.

The final piece of the puzzle is what to do about filemap_check_errors
calls that are being called directly or via filemap_* functions. Here,
we must take a little "creative license".

Since we now handle advancing the file->f_wb_err value at the generic
filesystem layer, we no longer need those callers to do anything like
that, or clear errors out of the mapping. A lot of the existing codebase
relies on being getting an error back from those functions when there is
a writeback problem, so we do still want to have them report writeback
errors somehow.

When reporting writeback errors, we will always report errors that have
occurred since a particular point in time. With the old writeback error
reporting, the time we used was "since it was last tested/cleared" which
is entirely arbitrary and potentially racy. Now, we can at least report
the latest error that has occurred since an arbitrary point in time
(represented as a sampled wb_err_t value).

In the case where we don't have a struct file to work with, this patch
just has the wrappers sample the mapping->wb_err value, and use that as
an arbitrary point from which to check for errors.

That's probably not ideal, particularly in the case of something like
filemap_fdatawait, but I'm not sure it's any worse than what we
already have, and this gives us a basis from which to work.

A lot of those callers will likely want to change to a model where they
sample a wb_err_t much earlier (perhaps when starting a transaction) and
then use that when checking to see if an error occurred later. I'm open
to adding new functions to the API to enable that in later patches.

Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
---
Documentation/filesystems/vfs.txt | 9 ++++-----
fs/btrfs/file.c | 10 ++--------
fs/btrfs/tree-log.c | 9 ++-------
fs/ext2/file.c | 3 ++-
fs/f2fs/file.c | 3 +++
fs/f2fs/node.c | 6 +-----
fs/fuse/file.c | 7 +++++--
fs/sync.c | 5 ++++-
include/linux/fs.h | 1 -
include/linux/pagemap.h | 7 +------
ipc/shm.c | 5 ++++-
mm/filemap.c | 32 ++++++++++----------------------
12 files changed, 38 insertions(+), 59 deletions(-)

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 1e0119f1c46a..493bf052fa33 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -577,7 +577,7 @@ 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
+marked with an error (typically using mapping_set_error), in order to
ensure that the error can later be reported to the application when an
fsync is issued.

@@ -895,10 +895,9 @@ otherwise noted.

release: called when the last reference to an open file is closed

- 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.
+ fsync: called by the fsync(2) system call. Errors that were previously
+ recorded using mapping_set_error will automatically be returned to
+ the application 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/btrfs/file.c b/fs/btrfs/file.c
index 520cb7230b2d..8ae0884350dd 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1962,6 +1962,7 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
int ret = 0;
bool full_sync = 0;
u64 len;
+ wb_err_t wb_since = READ_ONCE(file->f_wb_err);

/*
* The range length can be represented by u64, we have to do the typecasts
@@ -2079,14 +2080,7 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
*/
clear_bit(BTRFS_INODE_NEEDS_FULL_SYNC,
&BTRFS_I(inode)->runtime_flags);
- /*
- * An ordered extent might have started before and completed
- * already with io errors, in which case the inode was not
- * updated and we end up here. So check the inode's mapping
- * flags for any errors that might have happened while doing
- * writeback of file data.
- */
- ret = filemap_check_errors(inode->i_mapping);
+ ret = filemap_check_wb_error(inode->i_mapping, wb_since);
inode_unlock(inode);
goto out;
}
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index a59674c3e69e..b7f5dabae999 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3972,12 +3972,6 @@ static int wait_ordered_extents(struct btrfs_trans_handle *trans,
test_bit(BTRFS_ORDERED_IOERR, &ordered->flags)));

if (test_bit(BTRFS_ORDERED_IOERR, &ordered->flags)) {
- /*
- * Clear the AS_EIO/AS_ENOSPC flags from the inode's
- * i_mapping flags, so that the next fsync won't get
- * an outdated io error too.
- */
- filemap_check_errors(inode->i_mapping);
*ordered_io_error = true;
break;
}
@@ -4171,6 +4165,7 @@ static int btrfs_log_changed_extents(struct btrfs_trans_handle *trans,
u64 test_gen;
int ret = 0;
int num = 0;
+ wb_err_t since = filemap_sample_wb_error(inode->vfs_inode.i_mapping);

INIT_LIST_HEAD(&extents);

@@ -4214,7 +4209,7 @@ static int btrfs_log_changed_extents(struct btrfs_trans_handle *trans,
* without writing to the log tree and the fsync must report the
* file data write error and not commit the current transaction.
*/
- ret = filemap_check_errors(inode->vfs_inode.i_mapping);
+ ret = filemap_check_wb_error(inode->vfs_inode.i_mapping, since);
if (ret)
ctx->io_err = ret;
process:
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index 0ca77d337c94..bfb87969ad9b 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -175,12 +175,13 @@ int ext2_fsync(struct file *file, loff_t start, loff_t end, int datasync)
int ret;
struct super_block *sb = file->f_mapping->host->i_sb;
struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
+ wb_err_t since = READ_ONCE(file->f_wb_err);

ret = generic_file_fsync(file, start, end, datasync);
if (ret == -EIO)
ret = -EIO;
else
- ret = filemap_check_errors(mapping);
+ ret = filemap_check_wb_error(mapping, since);

if (ret) {
/* We don't really know where the IO error happened... */
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5f7317875a67..2ab4a28aa933 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -187,6 +187,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
.nr_to_write = LONG_MAX,
.for_reclaim = 0,
};
+ wb_err_t since = READ_ONCE(file->f_wb_err);

if (unlikely(f2fs_readonly(inode->i_sb)))
return 0;
@@ -265,6 +266,8 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
}

ret = wait_on_node_pages_writeback(sbi, ino);
+ if (ret == 0)
+ ret = filemap_check_wb_error(NODE_MAPPING(sbi), since);
if (ret)
goto out;

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 481aa8dc79f4..b3ef9504fd8b 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1630,7 +1630,7 @@ int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
{
pgoff_t index = 0, end = ULONG_MAX;
struct pagevec pvec;
- int ret2, ret = 0;
+ int ret = 0;

pagevec_init(&pvec, 0);

@@ -1658,10 +1658,6 @@ int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
pagevec_release(&pvec);
cond_resched();
}
-
- ret2 = filemap_check_errors(NODE_MAPPING(sbi));
- if (!ret)
- ret = ret2;
return ret;
}

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index ec238fb5a584..7972fdb189bf 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -398,6 +398,7 @@ static int fuse_flush(struct file *file, fl_owner_t id)
struct fuse_req *req;
struct fuse_flush_in inarg;
int err;
+ wb_err_t since = READ_ONCE(file->f_wb_err);

if (is_bad_inode(inode))
return -EIO;
@@ -413,7 +414,7 @@ static int fuse_flush(struct file *file, fl_owner_t id)
fuse_sync_writes(inode);
inode_unlock(inode);

- err = filemap_check_errors(file->f_mapping);
+ err = filemap_check_wb_error(file->f_mapping, since);
if (err)
return err;

@@ -446,6 +447,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
FUSE_ARGS(args);
struct fuse_fsync_in inarg;
int err;
+ wb_err_t since;

if (is_bad_inode(inode))
return -EIO;
@@ -461,6 +463,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
if (err)
goto out;

+ since = READ_ONCE(file->f_wb_err);
fuse_sync_writes(inode);

/*
@@ -468,7 +471,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
* filemap_write_and_wait_range() does not catch errors.
* We have to do this directly after fuse_sync_writes()
*/
- err = filemap_check_errors(file->f_mapping);
+ err = filemap_check_wb_error(file->f_mapping, since);
if (err)
goto out;

diff --git a/fs/sync.c b/fs/sync.c
index 11ba023434b1..cfde7a2b88ec 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -182,6 +182,7 @@ SYSCALL_DEFINE1(syncfs, int, fd)
*/
int vfs_fsync_range(struct file *file, loff_t start, loff_t end, int datasync)
{
+ int ret, ret2;
struct inode *inode = file->f_mapping->host;

if (!file->f_op->fsync)
@@ -192,7 +193,9 @@ int vfs_fsync_range(struct file *file, loff_t start, loff_t end, int datasync)
spin_unlock(&inode->i_lock);
mark_inode_dirty_sync(inode);
}
- return call_fsync(file, start, end, datasync);
+ ret = call_fsync(file, start, end, datasync);
+ ret2 = filemap_report_wb_error(file);
+ return ret ? : ret2;
}
EXPORT_SYMBOL(vfs_fsync_range);

diff --git a/include/linux/fs.h b/include/linux/fs.h
index bcc791d43c6e..bd2cfee92b4f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2532,7 +2532,6 @@ extern int __filemap_fdatawrite_range(struct address_space *mapping,
loff_t start, loff_t end, int sync_mode);
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 wb_err_t filemap_sample_wb_error(struct address_space *mapping);
extern int filemap_report_wb_error(struct file *file);
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 84943e8057ef..458caa916244 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -30,12 +30,7 @@ enum mapping_flags {

static inline void mapping_set_error(struct address_space *mapping, int error)
{
- if (unlikely(error)) {
- if (error == -ENOSPC)
- set_bit(AS_ENOSPC, &mapping->flags);
- else
- set_bit(AS_EIO, &mapping->flags);
- }
+ return filemap_set_wb_error(mapping, error);
}

static inline void mapping_set_unevictable(struct address_space *mapping)
diff --git a/ipc/shm.c b/ipc/shm.c
index 481d2a9c298a..5d8df57cfc76 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -448,11 +448,14 @@ static int shm_release(struct inode *ino, struct file *file)

static int shm_fsync(struct file *file, loff_t start, loff_t end, int datasync)
{
+ int ret, ret2;
struct shm_file_data *sfd = shm_file_data(file);

if (!sfd->file->f_op->fsync)
return -EINVAL;
- return call_fsync(sfd->file, start, end, datasync);
+ ret = call_fsync(sfd->file, start, end, datasync);
+ ret2 = filemap_report_wb_error(file);
+ return ret ? : ret2;
}

static long shm_fallocate(struct file *file, int mode, loff_t offset,
diff --git a/mm/filemap.c b/mm/filemap.c
index 4966e9dea945..525dddc15abb 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -295,20 +295,6 @@ void delete_from_page_cache(struct page *page)
}
EXPORT_SYMBOL(delete_from_page_cache);

-int filemap_check_errors(struct address_space *mapping)
-{
- int ret = 0;
- /* Check for outstanding write errors */
- if (test_bit(AS_ENOSPC, &mapping->flags) &&
- test_and_clear_bit(AS_ENOSPC, &mapping->flags))
- ret = -ENOSPC;
- if (test_bit(AS_EIO, &mapping->flags) &&
- test_and_clear_bit(AS_EIO, &mapping->flags))
- ret = -EIO;
- return ret;
-}
-EXPORT_SYMBOL(filemap_check_errors);
-
/**
* __filemap_fdatawrite_range - start writeback on mapping dirty pages in range
* @mapping: address space structure to write
@@ -431,9 +417,10 @@ int filemap_fdatawait_range(struct address_space *mapping, loff_t start_byte,
loff_t end_byte)
{
int ret, ret2;
+ wb_err_t since = READ_ONCE(mapping->wb_err);

ret = __filemap_fdatawait_range(mapping, start_byte, end_byte);
- ret2 = filemap_check_errors(mapping);
+ ret2 = filemap_check_wb_error(mapping, since);
if (!ret)
ret = ret2;

@@ -489,6 +476,7 @@ EXPORT_SYMBOL(filemap_fdatawait);
int filemap_write_and_wait(struct address_space *mapping)
{
int err = 0;
+ wb_err_t since = READ_ONCE(mapping->wb_err);

if ((!dax_mapping(mapping) && mapping->nrpages) ||
(dax_mapping(mapping) && mapping->nrexceptional)) {
@@ -500,12 +488,12 @@ int filemap_write_and_wait(struct address_space *mapping)
* thing (e.g. bug) happened, so we avoid waiting for it.
*/
if (err != -EIO) {
- int err2 = filemap_fdatawait(mapping);
+ filemap_fdatawait_keep_errors(mapping);
if (!err)
- err = err2;
+ err = filemap_check_wb_error(mapping, since);
}
} else {
- err = filemap_check_errors(mapping);
+ err = filemap_check_wb_error(mapping, since);
}
return err;
}
@@ -526,6 +514,7 @@ int filemap_write_and_wait_range(struct address_space *mapping,
loff_t lstart, loff_t lend)
{
int err = 0;
+ wb_err_t since = READ_ONCE(mapping->wb_err);

if ((!dax_mapping(mapping) && mapping->nrpages) ||
(dax_mapping(mapping) && mapping->nrexceptional)) {
@@ -533,13 +522,12 @@ int filemap_write_and_wait_range(struct address_space *mapping,
WB_SYNC_ALL);
/* See comment of filemap_write_and_wait() */
if (err != -EIO) {
- int err2 = filemap_fdatawait_range(mapping,
- lstart, lend);
+ __filemap_fdatawait_range(mapping, lstart, lend);
if (!err)
- err = err2;
+ err = filemap_check_wb_error(mapping, since);
}
} else {
- err = filemap_check_errors(mapping);
+ err = filemap_check_wb_error(mapping, since);
}
return err;
}
--
2.9.3