xfs: add writepage map error tag
PATCH 1/1 xfs: add writepage map error tag
Add an error tag to inject errors in the writeback block mapping codepath. This facilitates testing of the error path responsible for discarding delalloc blocks that could not be converted to real blocks.
---
Hi all,
I'm sending this along with a quick xfstest to call attention to an issue I ran into when poking around the writepage error handling path. The test demonstrates the details, but the short of it is that returning an error from ->writepages() interrupts a sync writeback and allows us to unmount a filesystem with dirty+delalloc pages. This behavior can also be observed with something like the following:
# xfs_io -fc "pwrite 0 64k" /mnt/file wrote 65536/65536 bytes at offset 0 64 KiB, 16 ops; 0.0000 sec (102.627 MiB/sec and 26272.5780 ops/sec) # xfs_io -c fsync /mnt/file fsync: Input/output error # xfs_io -c fsync /mnt/file fsync: Input/output error # xfs_io -c fsync /mnt/file fsync: Input/output error
... and so on until we finally process every dirty page.
A quick hack to return 0 from xfs_writepage_map() changes the behavior to process (i.e. toss/discard) all of the dirty pages as if writeback was successful. I don't necessarily think that is the right approach since ->writepage() should probably return an error even if ->writepages() doesn't, but it raises the question as to what the expected behavior should be in this case.
ISTM that we have a bit of an impedence mismatch between what a return code and mapping error mean vs. what XFS does. XFS essentially tosses the page by throwing away delalloc blocks, clearing uptodate status on the page and setting an error on the page/mapping (the latter of which makes it to userspace on fsync, for example). By also returning an error, we're telling mm not to continue processing the writeback, but should we be doing that if we need to process these dirty pages whether writeback succeeds or fails? For background writeback it might not matter, it will loop around again until all pages are processed, but for an integrity sync (or the umount case)?
Further, it also seems we're a bit inconsistent with page states between writepage failures and I/O errors. The former clears the uptodate status and thus immediately loses the page content, whereas the latter looks like it just clears writeback and the content presumably sticks around if/until the page is reclaimed and the old data is read from disk. This is somewhat of a separate issue [1], however..
I need to poke around the writeback code some more, but I was thinking of doing something like filtering out an error return somehow or another if we're in ->writepages() and doing an integrity writeback. Any high level thoughts on that? Other ideas?
Brian
[1] I'm mulling over the idea of deferring all such page processing to a workqueue by setting some kind of an inode flag (a la inode reclaim, eofblocks processing, etc.) such that we can make page error processing 1.) consistent, 2.) properly serialized with buffered writes and 3.) perhaps even configurable via something like the metadata error handling tunables.
fs/xfs/libxfs/xfs_errortag.h | 4 +++- fs/xfs/xfs_aops.c | 6 ++++++ fs/xfs/xfs_error.c | 3 +++ 3 files changed, 12 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_errortag.h +3 -1
@@ -54,7 +54,8 @@#define XFS_ERRTAG_BUF_LRU_REF 31#define XFS_ERRTAG_FORCE_SCRUB_REPAIR 32#define XFS_ERRTAG_FORCE_SUMMARY_RECALC 33 -#define XFS_ERRTAG_MAX 34 +#define XFS_ERRTAG_WRITEPAGE_MAP 34 +#define XFS_ERRTAG_MAX 35/* * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
@@ -93,5 +94,6 @@#define XFS_RANDOM_BUF_LRU_REF 2#define XFS_RANDOM_FORCE_SCRUB_REPAIR 1#define XFS_RANDOM_FORCE_SUMMARY_RECALC 1 +#define XFS_RANDOM_WRITEPAGE_MAP 2#endif /* __XFS_ERRORTAG_H_ */
fs/xfs/xfs_aops.c +6 -0
@@ -21,6 +21,7 @@#include "xfs_bmap_util.h" #include "xfs_bmap_btree.h" #include "xfs_reflink.h" +#include "xfs_errortag.h" #include <linux/writeback.h> /*
@@ -718,6 +719,11 @@ xfs_writepage_map(if (iop && !test_bit (i, iop->uptodate))continue ; +if (XFS_TEST_ERROR (false,XFS_I (inode)->i_mount, +XFS_ERRTAG_WRITEPAGE_MAP )) { + error = -EIO ; +break ; + } error =xfs_map_blocks (wpc, inode, file_offset);if (error)break ;
fs/xfs/xfs_error.c +3 -0
@@ -51,6 +51,7 @@ static unsigned int xfs_errortag_random_default[] = {XFS_RANDOM_BUF_LRU_REF ,XFS_RANDOM_FORCE_SCRUB_REPAIR ,XFS_RANDOM_FORCE_SUMMARY_RECALC , + XFS_RANDOM_WRITEPAGE_MAP, }; struct xfs_errortag_attr {
@@ -159,6 +160,7 @@ XFS_ERRORTAG_ATTR_RW(log_item_pin, XFS_ERRTAG_LOG_ITEM_PIN);XFS_ERRORTAG_ATTR_RW (buf_lru_ref,XFS_ERRTAG_BUF_LRU_REF );XFS_ERRORTAG_ATTR_RW (force_repair,XFS_ERRTAG_FORCE_SCRUB_REPAIR );XFS_ERRORTAG_ATTR_RW (bad_summary,XFS_ERRTAG_FORCE_SUMMARY_RECALC ); +XFS_ERRORTAG_ATTR_RW (writepage_map,XFS_ERRTAG_WRITEPAGE_MAP );static struct attribute *xfs_errortag_attrs[] = {XFS_ERRORTAG_ATTR_LIST (noerror),
@@ -195,6 +197,7 @@ static struct attribute *xfs_errortag_attrs[] = {XFS_ERRORTAG_ATTR_LIST (buf_lru_ref),XFS_ERRORTAG_ATTR_LIST (force_repair),XFS_ERRORTAG_ATTR_LIST (bad_summary), +XFS_ERRORTAG_ATTR_LIST (writepage_map), NULL, };
Signature · 1 line
2.17.2
PATCH 1/1 tests/xfs: writepage map error unmount test
Certain failures in the page writeback codepath can cause a broader writeback (i.e., a sync) sequence to exit prematurely. If this occurs during an unmount, it's likely the filesystem will reclaim inodes without having cleared all delayed allocation blocks. This produces a warning and leaves the filesystem inconsistent.
This test reproduces this scenario using the 'writepage_map' errortag. It performs delayed allocation, injects writeback errors and immediately unmounts the filesystem.
---
Note that this depends on a not yet available error tag.
Brian
tests/xfs/494 | 57 +++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/494.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 60 insertions(+) create mode 100755 tests/xfs/494 create mode 100644 tests/xfs/494.out
tests/xfs/494 new file +57 -0
@@ -0,0 +1,57 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved. +# +# FS QA Test 494 +# +# Simulate a serialization problem between page writeback error handling and +# unmount. If ->writepages() fails and returns an error back to the mm +# subsystem, writeback of subsequent pages is interrupted and the filesystem +# unmounts before processing all dirty pages. This results in unmounting a +# filesystem with dirty+delalloc pages, which in turn causes unmount time +# warnings and filesystem inconsistency. +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/inject + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_supported_os Linux +_require_error_injection +_require_xfs_io_error_injection "writepage_map" +_require_scratch + +_scratch_mkfs >> $seqres.full 2>&1 + +_scratch_mount +_scratch_inject_error "writepage_map" 1 +# use 512k to dirty multiple pages on large page size systems +$XFS_IO_PROG -fc "pwrite 0 512k" $SCRATCH_MNT/file >> $seqres.full 2>&1 +_scratch_unmount + +echo Silence is golden + +# success, all done +status=0 +exit
tests/xfs/494.out new file +2 -0
@@ -0,0 +1,2 @@ +QA output created by 494 +Silence is golden
tests/xfs/group +1 -0
@@ -491,3 +491,4 @@ 491 auto quick fuzz 492 auto quick fuzz 493 auto quick fuzz +494 auto quick
Signature · 1 line
2.17.2
Re: [PATCH] tests/xfs: writepage map error unmount test
Quoted 6 lines · Brian Foster
Certain failures in the page writeback codepath can cause a broader writeback (i.e., a sync) sequence to exit prematurely. If this occurs during an unmount, it's likely the filesystem will reclaim inodes without having cleared all delayed allocation blocks. This produces a warning and leaves the filesystem inconsistent.
Quoted 8 lines
This test reproduces this scenario using the '
writepage_map' errortag. It performs delayed allocation, injects writeback errors and immediately unmounts the filesystem.Signed-off-by Brian Foster <[email protected]>---
Note that this depends on a not yet available error tag.
I'll wait for the "error tag" patch goes in first, or gets some ACKs first. One really minor issue below.
Quoted 45 lines
Brian
tests/xfs/494 | 57 +++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/494.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 60 insertions(+) create mode 100755 tests/xfs/494 create mode 100644 tests/xfs/494.outtests/xfs/494 new file +29
-0@@ -0,0 +1,57 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved. +# +# FS QA Test 494 +# +# Simulate a serialization problem between page writeback error handling and +# unmount. If ->writepages() fails and returns an error back to the mm +# subsystem, writeback of subsequent pages is interrupted and the filesystem +# unmounts before processing all dirty pages. This results in unmounting a +# filesystem with dirty+delalloc pages, which in turn causes unmount time +# warnings and filesystem inconsistency. +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks
tests/xfs/494 line 39
Quoted 10 lines
tests/xfs/494 new file +10
-0+. ./common/rc +. ./common/inject + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic
Supported fs should be 'xfs' here.
Thanks,
Eryu
Quoted 38 lines
tests/xfs/494 new file +18
-0+_supported_os Linux +_require_error_injection +_require_xfs_io_error_injection "writepage_map" +_require_scratch + +_scratch_mkfs >> $seqres.full 2>&1 + +_scratch_mount +_scratch_inject_error "writepage_map" 1 +# use 512k to dirty multiple pages on large page size systems +$XFS_IO_PROG -fc "pwrite 0 512k" $SCRATCH_MNT/file >> $seqres.full 2>&1 +_scratch_unmount + +echo Silence is golden + +# success, all done +status=0 +exittests/xfs/494.out new file +2
-0@@ -0,0 +1,2 @@ +QA output created by 494 +Silence is goldentests/xfs/group +1
-0@@ -491,3 +491,4 @@ 491 auto quick fuzz 492 auto quick fuzz 493 auto quick fuzz +494 auto quickSignature · 1 line
2.17.2
Re: [PATCH] tests/xfs: writepage map error unmount test
Quoted 11 lines · Eryu Guan
Quoted 10 lines · Brian Foster
Certain failures in the page writeback codepath can cause a broader writeback (i.e., a sync) sequence to exit prematurely. If this occurs during an unmount, it's likely the filesystem will reclaim inodes without having cleared all delayed allocation blocks. This produces a warning and leaves the filesystem inconsistent.
This test reproduces this scenario using the '
writepage_map' errortag. It performs delayed allocation, injects writeback errors and immediately unmounts the filesystem.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Brian Foster <[email protected]>---
Note that this depends on a not yet available error tag.
I'll wait for the "error tag" patch goes in first, or gets some ACKs first. One really minor issue below.
Yep. I probably should have been more clear and/or set this RFC. This test is wholly dependent on if/whether/how to deal with the associated error handling issue, which atm is still up in the air. The tag/test is primarily to demonstrate the problem and may not be independently useful until there's a supporting fix. I may float a patch for that soon..
Quoted 49 lines
Quoted 49 lines
Brian
tests/xfs/494 | 57 +++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/494.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 60 insertions(+) create mode 100755 tests/xfs/494 create mode 100644 tests/xfs/494.outtests/xfs/494 new file +33
-0@@ -0,0 +1,57 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved. +# +# FS QA Test 494 +# +# Simulate a serialization problem between page writeback error handling and +# unmount. If ->writepages() fails and returns an error back to the mm +# subsystem, writeback of subsequent pages is interrupted and the filesystem +# unmounts before processing all dirty pages. This results in unmounting a +# filesystem with dirty+delalloc pages, which in turn causes unmount time +# warnings and filesystem inconsistency. +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/inject + +# remove previous $seqres.full before test
Quoted 9 lines
tests/xfs/494 line 39
Quoted 6 lines
tests/xfs/494 new file +6
-0+rm -f $seqres.full + +# real QA test starts here + +# Modify as appropriate. +_supported_fs genericSupported fs should be 'xfs' here.
Will fix, thanks.
Brian
Quoted 41 lines
Thanks,
EryuQuoted 38 lines
tests/xfs/494 new file +18
-0+_supported_os Linux +_require_error_injection +_require_xfs_io_error_injection "writepage_map" +_require_scratch + +_scratch_mkfs >> $seqres.full 2>&1 + +_scratch_mount +_scratch_inject_error "writepage_map" 1 +# use 512k to dirty multiple pages on large page size systems +$XFS_IO_PROG -fc "pwrite 0 512k" $SCRATCH_MNT/file >> $seqres.full 2>&1 +_scratch_unmount + +echo Silence is golden + +# success, all done +status=0 +exittests/xfs/494.out new file +2
-0@@ -0,0 +1,2 @@ +QA output created by 494 +Silence is goldentests/xfs/group +1
-0@@ -491,3 +491,4 @@ 491 auto quick fuzz 492 auto quick fuzz 493 auto quick fuzz +494 auto quickSignature · 1 line
2.17.2