[PATCH 1/3] ext4: avoid kernel warning when writing the superblock to a dead device

[PATCH 1/3] ext4: avoid kernel warning when writing the superblock to a dead device

[Theodore Ts'o]

The xfstests generic/475 test switches the underlying device with
dm-error while running a stress test.  This results in a large number
of file system errors, and since we can't lock the buffer head when
marking the superblock dirty in the ext4_grp_locked_error() case, it's
possible the superblock to be !buffer_uptodate() without
buffer_write_io_error() being true.

We need to set buffer_uptodate() before we call mark_buffer_dirty() or
this will trigger a WARN_ON.  It's safe to do this since the
superblock must have been properly read into memory or the mount would
have been successful.  So if buffer_uptodate() is not set, we can
safely assume that this happened due to a failed attempt to write the
superblock.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d6c142d73d99..fb12d3c17c1b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4902,7 +4902,7 @@ static int ext4_commit_super(struct super_block *sb, int sync)
 	ext4_superblock_csum_set(sb);
 	if (sync)
 		lock_buffer(sbh);
-	if (buffer_write_io_error(sbh)) {
+	if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) {
 		/*
 		 * Oh, dear.  A previous attempt to write the
 		 * superblock failed.  This could happen because the
-- 
2.19.1

[PATCH 2/3] ext4: use ext4_write_inode() when fsyncing w/o a journal

[Theodore Ts'o]

In no-journal mode, we previously used __generic_file_fsync() in
no-journal mode.  This triggers a lockdep warning, and in addition,
it's not safe to depend on the inode writeback mechanism in the case
ext4.  We can solve both problems by calling ext4_write_inode()
directly.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@kernel.org
---
 fs/ext4/fsync.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 26a7fe5c4fd3..87a7ff00ef62 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -116,8 +116,16 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		goto out;
 	}
 
+	ret = file_write_and_wait_range(file, start, end);
+	if (ret)
+		return ret;
+
 	if (!journal) {
-		ret = __generic_file_fsync(file, start, end, datasync);
+		struct writeback_control wbc = {
+			.sync_mode = WB_SYNC_ALL
+		};
+
+		ret = ext4_write_inode(inode, &wbc);
 		if (!ret)
 			ret = ext4_sync_parent(inode);
 		if (test_opt(inode->i_sb, BARRIER))
@@ -125,9 +133,6 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		goto out;
 	}
 
-	ret = file_write_and_wait_range(file, start, end);
-	if (ret)
-		return ret;
 	/*
 	 * data=writeback,ordered:
 	 *  The caller's filemap_fdatawrite()/wait will sync the data.
-- 
2.19.1

[PATCH 3/3] ext4: track writeback errors using the generic tracking infrastructure

[Theodore Ts'o]

We already using mapping_set_error() in fs/ext4/page_io.c, so all we
need to do is to use file_check_and_advance_wb_err() when handling
fsync() requests in ext4_sync_file().

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@kernel.org
---
 fs/ext4/fsync.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 87a7ff00ef62..712f00995390 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -164,6 +164,9 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 			ret = err;
 	}
 out:
+	err = file_check_and_advance_wb_err(file);
+	if (ret == 0)
+		ret = err;
 	trace_ext4_sync_file_exit(inode, ret);
 	return ret;
 }
-- 
2.19.1

Re: [PATCH 1/3] ext4: avoid kernel warning when writing the superblock to a dead device

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v4.20.0, v4.19.13, v4.14.91, v4.9.148, v4.4.169, v3.18.131, 

v4.20.0: Build OK!
v4.19.13: Build OK!
v4.14.91: Build OK!
v4.9.148: Build OK!
v4.4.169: Failed to apply! Possible dependencies:
    1566a48aaa10 ("ext4: don't lock buffer in ext4_commit_super if holding spinlock")
    4743f8399061 ("ext4: Fix WARN_ON_ONCE in ext4_commit_super()")

v3.18.131: Failed to apply! Possible dependencies:
    1566a48aaa10 ("ext4: don't lock buffer in ext4_commit_super if holding spinlock")
    4743f8399061 ("ext4: Fix WARN_ON_ONCE in ext4_commit_super()")
    564bc402526e ("ext4, jbd2: add REQ_FUA flag when recording an error in the superblock")


How should we proceed with this patch?

--
Thanks,
Sasha

Re: [PATCH 2/3] ext4: use ext4_write_inode() when fsyncing w/o a journal

[Jan Kara]

On Mon 31-12-18 14:48:35, Theodore Ts'o wrote:
> In no-journal mode, we previously used __generic_file_fsync() in
> no-journal mode.  This triggers a lockdep warning, and in addition,
> it's not safe to depend on the inode writeback mechanism in the case
> ext4.  We can solve both problems by calling ext4_write_inode()
> directly.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@kernel.org
> ---
>  fs/ext4/fsync.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> index 26a7fe5c4fd3..87a7ff00ef62 100644
> --- a/fs/ext4/fsync.c
> +++ b/fs/ext4/fsync.c
> @@ -116,8 +116,16 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  		goto out;
>  	}
>  
> +	ret = file_write_and_wait_range(file, start, end);
> +	if (ret)
> +		return ret;
> +
>  	if (!journal) {
> -		ret = __generic_file_fsync(file, start, end, datasync);
> +		struct writeback_control wbc = {
> +			.sync_mode = WB_SYNC_ALL
> +		};
> +
> +		ret = ext4_write_inode(inode, &wbc);

Going through some older email... How come this is safe Ted?
ext4_write_inode() will not write out metadata buffers associated with the
inode (unlike __generic_file_fsync() which calls sync_mapping_buffers()).
So you probably need to call sync_mapping_buffers() before calling
ext4_write_inode() here?

								Honza

> @@ -125,9 +133,6 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  		goto out;
>  	}
>  
> -	ret = file_write_and_wait_range(file, start, end);
> -	if (ret)
> -		return ret;
>  	/*
>  	 * data=writeback,ordered:
>  	 *  The caller's filemap_fdatawrite()/wait will sync the data.
> -- 
> 2.19.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR