fstests generic/441 failure on ext2

Hi all,

With ext2 driven by ext4 module (or ext4 without journal, I haven't tested ext2 module, but I guess the result is the same), v4.14-rc1 kernel starts to fail fstests generic/441 as:

+First fsync after reopen of fd[0] failed: Input/output error

git bisect shows that this is uncovered by commit ffb959bbdf92 ("mm:
remove optimizations based on i_size in mapping writeback waits"), which removed (i_size == 0) check in filemap_fdatawait().

I say "uncovered" because test fails with 4.13 kernel too if we re-open the test file without O_TRUNC flag in src/fsync-err.c (so file size is not zero, and fails the i_size == 0 check).

The EIO was returned by sync_inode_metadata() in __generic_file_fsync(), the call trace is like:

do_fsync
vfs_fsync_range
ext4_sync_file
__generic_file_fsync
sync_inode_metadata
writeback_single_inode
__writeback_single_inode
filemap_fdatawait => EIO here

Thanks,
Eryu

Quoted 28 lines · Eryu Guan

Hi all,

With ext2 driven by ext4 module (or ext4 without journal, I haven't tested ext2 module, but I guess the result is the same), v4.14-rc1 kernel starts to fail fstests generic/441 as:

+First fsync after reopen of fd[0] failed: Input/output error

git bisect shows that this is uncovered by commit ffb959bbdf92 ("mm:
remove optimizations based on i_size in mapping writeback waits"), which removed (i_size == 0) check in filemap_fdatawait().

I say "uncovered" because test fails with 4.13 kernel too if we re-open the test file without O_TRUNC flag in src/fsync-err.c (so file size is not zero, and fails the i_size == 0 check).

The EIO was returned by sync_inode_metadata() in __generic_file_fsync(), the call trace is like:

do_fsync
vfs_fsync_range
ext4_sync_file
__generic_file_fsync
sync_inode_metadata
writeback_single_inode
__writeback_single_inode
filemap_fdatawait => EIO here

Quoted 2 lines

Thanks,
Eryu

(cc'ing Jan and linux-fsdevel)

Thanks for the bug report. The analysis looks spot-on.

So yeah...we have this "legacy" filemap_fdatawait call in __writeback_single_inode, and that is returning -EIO, likely because AS_EIO was set on the inode from the earlier wb errors.

That error return is pretty sketchy since it could be cleared at any time, and pretty much everything we care about here is now using errseq_t for error reporting at fsync. I don't think we really care too much about that flag in this codepath anymore.

Quoted 36 lines · Jeff Layton

Quoted 28 lines · Eryu Guan

Hi all,

With ext2 driven by ext4 module (or ext4 without journal, I haven't tested ext2 module, but I guess the result is the same), v4.14-rc1 kernel starts to fail fstests generic/441 as:

+First fsync after reopen of fd[0] failed: Input/output error

git bisect shows that this is uncovered by commit ffb959bbdf92 ("mm:
remove optimizations based on i_size in mapping writeback waits"), which removed (i_size == 0) check in filemap_fdatawait().

I say "uncovered" because test fails with 4.13 kernel too if we re-open the test file without O_TRUNC flag in src/fsync-err.c (so file size is not zero, and fails the i_size == 0 check).

The EIO was returned by sync_inode_metadata() in __generic_file_fsync(), the call trace is like:

do_fsync
vfs_fsync_range
ext4_sync_file
__generic_file_fsync
sync_inode_metadata
writeback_single_inode
__writeback_single_inode
filemap_fdatawait => EIO here

Quoted 2 lines

Thanks,
Eryu

(cc'ing Jan and linux-fsdevel)

Thanks for the bug report. The analysis looks spot-on.

Quoted 8 lines

So yeah...we have this "legacy" filemap_fdatawait call in __writeback_single_inode, and that is returning -EIO, likely because AS_EIO was set on the inode from the earlier wb errors.

That error return is pretty sketchy since it could be cleared at any time, and pretty much everything we care about here is now using errseq_t for error reporting at fsync. I don't think we really care too much about that flag in this codepath anymore.

So I agree fsync(2) path is covered but that fdatawait() call is also responsible for reporting error e.g. for write_inode_now() calls and there we still have some unconverted users. So for now I don't have a better solution than to live with this additional somewhat stale EIO error. Or possibly we can have truncate to 0 clear writeback error which would mask the problem again and kind of makes sense...

Quoted 46 lines · Jan Kara

Quoted 36 lines · Jeff Layton

Quoted 28 lines · Eryu Guan

Hi all,

With ext2 driven by ext4 module (or ext4 without journal, I haven't tested ext2 module, but I guess the result is the same), v4.14-rc1 kernel starts to fail fstests generic/441 as:

+First fsync after reopen of fd[0] failed: Input/output error

git bisect shows that this is uncovered by commit ffb959bbdf92 ("mm:
remove optimizations based on i_size in mapping writeback waits"), which removed (i_size == 0) check in filemap_fdatawait().

I say "uncovered" because test fails with 4.13 kernel too if we re-open the test file without O_TRUNC flag in src/fsync-err.c (so file size is not zero, and fails the i_size == 0 check).

The EIO was returned by sync_inode_metadata() in __generic_file_fsync(), the call trace is like:

do_fsync
vfs_fsync_range
ext4_sync_file
__generic_file_fsync
sync_inode_metadata
writeback_single_inode
__writeback_single_inode
filemap_fdatawait => EIO here

Quoted 2 lines

Thanks,
Eryu

(cc'ing Jan and linux-fsdevel)

Thanks for the bug report. The analysis looks spot-on.

Quoted 8 lines

So yeah...we have this "legacy" filemap_fdatawait call in __writeback_single_inode, and that is returning -EIO, likely because AS_EIO was set on the inode from the earlier wb errors.

That error return is pretty sketchy since it could be cleared at any time, and pretty much everything we care about here is now using errseq_t for error reporting at fsync. I don't think we really care too much about that flag in this codepath anymore.

Quoted 8 lines

So I agree fsync(2) path is covered but that fdatawait() call is also responsible for reporting error e.g. for write_inode_now() calls and there we still have some unconverted users. So for now I don't have a better solution than to live with this additional somewhat stale EIO error. Or possibly we can have truncate to 0 clear writeback error which would mask the problem again and kind of makes sense...

Another thought would be to have file_check_and_advance_wb_err (and maybe filemap_check_wb_err) also clear AS_EIO and AS_ENOSPC. That sort of makes sense since legacy users would have cleared those flags.

I had a patch that did that at one point, but dropped it since it didn't seem necessary. That might be the best fix for this though.

From: Jeff Layton <[email protected]>

Eryu noticed that he could sometimes get a leftover error reported when it shouldn't be on fsync with ext2 and non-journalled ext4. The problem is that writeback_single_inode still uses filemap_fdatawait. That picks up a previously set AS_EIO flag, which would ordinarily have been cleared before.

Since we're mostly using this function as a replacement for filemap_check_errors, have filemap_check_and_advance_wb_err clear AS_EIO and AS_ENOSPC when reporting an error. That should allow the new function to better emulate the behavior of the old with respect to these flags.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 mm/filemap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

mm/filemap.c +8 -0

index 870971e20967..404722ea0fdd 100644
@@ -620,6 +620,14 @@ int file_check_and_advance_wb_err(struct file *file)
 		trace_file_check_and_advance_wb_err(file, old);
 		spin_unlock(&file->f_lock);
 	}
+
+	/*
+	 * We're mostly using this function as a drop in replacement for
+	 * filemap_check_errors. Clear AS_EIO/AS_ENOSPC to emulate the effect
+	 * that the legacy code would have had on these flags.
+	 */
+	clear_bit(AS_EIO, &mapping->flags);
+	clear_bit(AS_ENOSPC, &mapping->flags);
 	return err;
 }
 EXPORT_SYMBOL(file_check_and_advance_wb_err);

Signature · 1 line

2.13.5

From: Jeff Layton <[email protected]>

Eryu noticed that he could sometimes get a leftover error reported when it shouldn't be on fsync with ext2 and non-journalled ext4. The problem is that writeback_single_inode still uses filemap_fdatawait. That picks up a previously set AS_EIO flag, which would ordinarily have been cleared before.

Since we're mostly using this function as a replacement for filemap_check_errors, have filemap_check_and_advance_wb_err clear AS_EIO and AS_ENOSPC when reporting an error. That should allow the new function to better emulate the behavior of the old with respect to these flags.

Reported-by Eryu Guan <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 mm/filemap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

mm/filemap.c +1 -0

index 870971e20967..404722ea0fdd 100644
@@ -620,6 +620,14 @@ int file_check_and_advance_wb_err(struct file *file)
 		trace_file_check_and_advance_wb_err(file, old);
 		spin_unlock(&file->f_lock);
 	}
+