From 08892fdf71e5325b3397c62f7947ac03ae19010f Mon Sep 17 00:00:00 2001 From: Paul Lawrence Date: Wed, 19 Jun 2024 12:44:38 -0700 Subject: [PATCH] ANDROID: Incremental fs: Retry page faults on non-fatal errors In order to not freeze on corrupt data, we need to turn off FAULT_FLAG_ALLOW_RETRY. However, this means we no longer retry on EINTR, so an interrupted read will lead to page faults. The fault handler does not seem to allow dynamic decisions as to whether to turn on or off this flag. To resolve both issues, add a flag to indicate if there are corrupt pages in a file, and only if there are turn off this flag. Also fsanitize changed the behavior of mlock - mlock should fail if the page reads fail, but with fsanitize it returns 0 then page faults on access. This broke this test, and fsanitize offers little value on test code, so disable it. Test: incfs_test passes Bug: 343532239 Change-Id: Id2ced4be3310109206d65dcc92dea05c05131182 Signed-off-by: Paul Lawrence --- fs/incfs/format.h | 5 ++++ fs/incfs/sysfs.c | 2 ++ fs/incfs/vfs.c | 30 ++++++++++++++++++- .../selftests/filesystems/incfs/Makefile | 4 +-- 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/fs/incfs/format.h b/fs/incfs/format.h index 14e475b79a16..5e534416866f 100644 --- a/fs/incfs/format.h +++ b/fs/incfs/format.h @@ -314,6 +314,11 @@ struct backing_file_context { * there is no need to get/put the creds */ const struct cred *bc_cred; + + /* + * The file has a bad block, i.e. one that has failed checksumming. + */ + bool bc_has_bad_block; }; struct metadata_handler { diff --git a/fs/incfs/sysfs.c b/fs/incfs/sysfs.c index ba91c07d2887..96f6b02d7a66 100644 --- a/fs/incfs/sysfs.c +++ b/fs/incfs/sysfs.c @@ -35,6 +35,7 @@ DECLARE_FEATURE_FLAG(zstd); DECLARE_FEATURE_FLAG(v2); DECLARE_FEATURE_FLAG(bugfix_throttling); DECLARE_FEATURE_FLAG(bugfix_inode_eviction); +DECLARE_FEATURE_FLAG(bugfix_retry_page_fault); static struct attribute *attributes[] = { &corefs_attr.attr, @@ -42,6 +43,7 @@ static struct attribute *attributes[] = { &v2_attr.attr, &bugfix_throttling_attr.attr, &bugfix_inode_eviction_attr.attr, + &bugfix_retry_page_fault_attr.attr, NULL, }; diff --git a/fs/incfs/vfs.c b/fs/incfs/vfs.c index f393f17435d3..1dd6a0251fa1 100644 --- a/fs/incfs/vfs.c +++ b/fs/incfs/vfs.c @@ -115,7 +115,28 @@ static const struct address_space_operations incfs_address_space_ops = { static vm_fault_t incfs_fault(struct vm_fault *vmf) { - vmf->flags &= ~FAULT_FLAG_ALLOW_RETRY; + struct file *file = vmf->vma->vm_file; + struct data_file *df = get_incfs_data_file(file); + struct backing_file_context *bfc = df ? df->df_backing_file_context : NULL; + + /* + * This is something of a kludge + * We want to retry if the read from the underlying file is interrupted, + * but not if the read fails because the stored data is corrupt since the + * latter causes an infinite loop. + * + * However, whether we wish to retry must be set before we call + * filemap_fault, *and* there is no way of getting the read error code out + * of filemap_fault. + * + * So unless there is a robust solution to both the above problems, we can + * solve the actual issues we have encoutered by retrying unless there is + * known corruption in the backing file. This does mean that we won't retry + * with a corrupt backing file if a (good) read is interrupted, but we + * don't really handle corruption well anyway at this time. + */ + if (bfc && bfc->bc_has_bad_block) + vmf->flags &= ~FAULT_FLAG_ALLOW_RETRY; return filemap_fault(vmf); } @@ -581,6 +602,13 @@ static int read_single_page(struct file *f, struct page *page) else if (read_result < PAGE_SIZE) zero_user(page, read_result, PAGE_SIZE - read_result); + if (result == -EBADMSG) { + struct backing_file_context *bfc = df ? df->df_backing_file_context : NULL; + + if (bfc) + bfc->bc_has_bad_block = 1; + } + if (result == 0) SetPageUptodate(page); else diff --git a/tools/testing/selftests/filesystems/incfs/Makefile b/tools/testing/selftests/filesystems/incfs/Makefile index 5a2f6301c4b2..a2033488d423 100644 --- a/tools/testing/selftests/filesystems/incfs/Makefile +++ b/tools/testing/selftests/filesystems/incfs/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 -CFLAGS += -D_FILE_OFFSET_BITS=64 -Wall -Werror -I../.. -I../../../../.. -fno-omit-frame-pointer -fsanitize=address -g -LDLIBS := -llz4 -lzstd -lcrypto -lpthread -fsanitize=address +CFLAGS += -D_FILE_OFFSET_BITS=64 -Wall -Wno-deprecated-declarations -Werror -I../.. -I../../../../.. -fno-omit-frame-pointer -g +LDLIBS := -llz4 -lzstd -lcrypto -lpthread TEST_GEN_PROGS := incfs_test incfs_stress incfs_perf include ../../lib.mk