[lustre-devel] [PATCH 03/33] lustre: llite: EIO is possible on a race with page reclaim

[James Simmons]

From: Patrick Farrell <pfarrell at whamcloud.com>

We must clear the 'uptodate' page flag when we delete a
page from Lustre, or stale reads can occur.  However,
generic_file_buffered_read requires any pages returned from
readpage() be uptodate.

So, we must retry reading if page truncation happens in
parallel with the read.

This implements the same fix as:
https://review.whamcloud.com/49647
commit e02cfe39f908 ("lustre: llite: SIGBUS is possible on a race with page reclaim")

did for the mmap path.

WC-bug-id: https://jira.whamcloud.com/browse/LU-16649
Lustre-commit: 1d98e5c32b41e19bb ("LU-16649 llite: EIO is possible on a race with page reclaim")
Signed-off-by: Patrick Farrell <pfarrell at whamcloud.com>
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/50344
Reviewed-by: Andrew Perepechko <andrew.perepechko at hpe.com>
Reviewed-by: Qian Yingjin <qian at ddn.com>
Reviewed-by: Alexander Zarochentsev <alexander.zarochentsev at hpe.com>
Reviewed-by: Oleg Drokin <green at whamcloud.com>
Signed-off-by: James Simmons <jsimmons at infradead.org>
---
 fs/lustre/include/obd_support.h |  1 +
 fs/lustre/llite/file.c          | 11 +++++++----
 fs/lustre/llite/rw.c            |  8 ++++++++
 fs/lustre/llite/vvp_io.c        | 25 ++++++++++++++++++++++---
 4 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/fs/lustre/include/obd_support.h b/fs/lustre/include/obd_support.h
index cee7e3164d66..0a63af11db35 100644
--- a/fs/lustre/include/obd_support.h
+++ b/fs/lustre/include/obd_support.h
@@ -492,6 +492,7 @@ extern char obd_jobid_var[];
 #define OBD_FAIL_LLITE_PAGE_INVALIDATE_PAUSE		0x1421
 #define OBD_FAIL_LLITE_READPAGE_PAUSE			0x1422
 #define OBD_FAIL_LLITE_PANIC_ON_ESTALE			0x1423
+#define OBD_FAIL_LLITE_READPAGE_PAUSE2			0x1424
 
 #define OBD_FAIL_FID_INDIR				0x1501
 #define OBD_FAIL_FID_INLMA				0x1502
diff --git a/fs/lustre/llite/file.c b/fs/lustre/llite/file.c
index c99e9c01bc65..b2751b571ea9 100644
--- a/fs/lustre/llite/file.c
+++ b/fs/lustre/llite/file.c
@@ -1990,12 +1990,15 @@ ll_do_fast_read(struct kiocb *iocb, struct iov_iter *iter)
 
 	result = generic_file_read_iter(iocb, iter);
 
-	/*
-	 * If the first page is not in cache, generic_file_aio_read() will be
-	 * returned with -ENODATA.
+	/* If the first page is not in cache, generic_file_aio_read() will be
+	 * returned with -ENODATA.  Fall back to full read path.
 	 * See corresponding code in ll_readpage().
+	 *
+	 * if we raced with page deletion, we might get EIO.  Rather than add
+	 * locking to the fast path for this rare case, fall back to the full
+	 * read path.  (See vvp_io_read_start() for rest of handling.
 	 */
-	if (result == -ENODATA)
+	if (result == -ENODATA || result == -EIO)
 		result = 0;
 
 	if (result > 0) {
diff --git a/fs/lustre/llite/rw.c b/fs/lustre/llite/rw.c
index 0c73258428e6..92a9c252247e 100644
--- a/fs/lustre/llite/rw.c
+++ b/fs/lustre/llite/rw.c
@@ -2046,5 +2046,13 @@ int ll_readpage(struct file *file, struct page *vmpage)
 	if (ra.cra_release)
 		cl_read_ahead_release(env, &ra);
 
+	/* this delay gives time for the actual read of the page to finish and
+	 * unlock the page in vvp_page_completion_read before we return to our
+	 * caller and the caller tries to use the page, allowing us to test
+	 * races with the page being unlocked after readpage() but before it's
+	 * used by the caller
+	 */
+	OBD_FAIL_TIMEOUT(OBD_FAIL_LLITE_READPAGE_PAUSE2, cfs_fail_val);
+
 	return result;
 }
diff --git a/fs/lustre/llite/vvp_io.c b/fs/lustre/llite/vvp_io.c
index 26dfaaa76bd9..86dab3b1a39f 100644
--- a/fs/lustre/llite/vvp_io.c
+++ b/fs/lustre/llite/vvp_io.c
@@ -811,8 +811,10 @@ static int vvp_io_read_start(const struct lu_env *env,
 	size_t cnt = io->u.ci_rd.rd.crw_count;
 	size_t tot = vio->vui_tot_count;
 	struct ll_cl_context *lcc;
+	unsigned int seq;
 	int exceed = 0;
 	int result;
+	int total_bytes_read = 0;
 	struct iov_iter iter;
 	pgoff_t page_offset;
 
@@ -878,12 +880,29 @@ static int vvp_io_read_start(const struct lu_env *env,
 	lcc->lcc_end_index = DIV_ROUND_UP(pos + iter.count, PAGE_SIZE);
 	CDEBUG(D_VFSTRACE, "count:%ld iocb pos:%lld\n", iter.count, pos);
 
-	result = generic_file_read_iter(vio->vui_iocb, &iter);
+	/* this seqlock lets us notice if a page has been deleted on this inode
+	 * during the fault process, allowing us to catch an erroneous short
+	 * read or EIO
+	 * See LU-16160
+	 */
+	do {
+		seq = read_seqbegin(&ll_i2info(inode)->lli_page_inv_lock);
+		result = generic_file_read_iter(vio->vui_iocb, &iter);
+		if (result >= 0) {
+			io->ci_nob += result;
+			total_bytes_read += result;
+		}
+	/* if we got a short read or -EIO and we raced with page invalidation,
+	 * retry
+	 */
+	} while (read_seqretry(&ll_i2info(inode)->lli_page_inv_lock, seq) &&
+		 ((result >= 0 && iov_iter_count(&iter) > 0) ||
+		  result == -EIO));
+
 out:
 	if (result >= 0) {
-		if (result < cnt)
+		if (total_bytes_read < cnt)
 			io->ci_continue = 0;
-		io->ci_nob += result;
 		result = 0;
 	} else if (result == -EIOCBQUEUED) {
 		io->ci_nob += vio->u.readwrite.vui_read;
-- 
2.39.3