diff mbox series

[02/18] lustre: llite: Check vmpage in releasepage

Message ID 1654777994-29806-3-git-send-email-jsimmons@infradead.org (mailing list archive)
State Not Applicable
Headers show
Series lustre: sync with OpenSFS tree June 8, 2022 | expand

Commit Message

James Simmons June 9, 2022, 12:32 p.m. UTC 
From: Patrick Farrell <pfarrell@whamcloud.com>

We cannot release a page if the vmpage reference count is
>1, otherwise we will detach a vmpage from Lustre when the
page is still referenced in the VM.

This creates a situation where page discard for lock
cancellation will not find the page, so we can get stale
data reads.

This re-introduces the LU-12587 issue where direct I/O on
a client falls back to buffered I/O if there are pages in
cache, since it cannot flush them.  This is annoying but
not a huge problem.

WC-bug-id: https://jira.whamcloud.com/browse/LU-14541
Lustre-commit: c524079f4f59a39b9 ("LU-14541 llite: Check vmpage in releasepage")
Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com>
Reviewed-on: https://review.whamcloud.com/47262
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: John L. Hammond <jhammond@whamcloud.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 fs/lustre/include/cl_object.h |  9 +++++++++
 fs/lustre/llite/rw26.c        | 19 +++++++++++++------
 fs/lustre/osc/osc_page.c      |  9 +++++++--
 3 files changed, 29 insertions(+), 8 deletions(-)
diff mbox series

Patch

diff --git a/fs/lustre/include/cl_object.h b/fs/lustre/include/cl_object.h
index ab7f0f2..b98109d 100644
--- a/fs/lustre/include/cl_object.h
+++ b/fs/lustre/include/cl_object.h
@@ -91,6 +91,7 @@ 
 #include <linux/uio.h>
 #include <lu_object.h>
 #include <linux/atomic.h>
+#include <linux/mm.h>
 #include <linux/mutex.h>
 #include <linux/radix-tree.h>
 #include <linux/spinlock.h>
@@ -1071,6 +1072,14 @@  static inline bool __page_in_use(const struct cl_page *page, int refc)
  */
 #define cl_page_in_use_noref(pg) __page_in_use(pg, 0)
 
+/* references: cl_page, page cache, optional + refcount for caller reference
+ * (always 0 or 1 currently)
+ */
+static inline int vmpage_in_use(struct page *vmpage, int refcount)
+{
+	return (page_count(vmpage) - page_mapcount(vmpage) > 2 + refcount);
+}
+
 /** @} cl_page */
 
 /** \addtogroup cl_lock cl_lock
diff --git a/fs/lustre/llite/rw26.c b/fs/lustre/llite/rw26.c
index a5cdb01..8b379ca 100644
--- a/fs/lustre/llite/rw26.c
+++ b/fs/lustre/llite/rw26.c
@@ -102,7 +102,7 @@  static int ll_releasepage(struct page *vmpage, gfp_t gfp_mask)
 {
 	struct lu_env *env;
 	struct cl_object *obj;
-	struct cl_page *page;
+	struct cl_page *clpage;
 	struct address_space *mapping;
 	int result = 0;
 
@@ -118,16 +118,23 @@  static int ll_releasepage(struct page *vmpage, gfp_t gfp_mask)
 	if (!obj)
 		return 1;
 
-	page = cl_vmpage_page(vmpage, obj);
-	if (!page)
+	clpage = cl_vmpage_page(vmpage, obj);
+	if (!clpage)
 		return 1;
 
 	env = cl_env_percpu_get();
 	LASSERT(!IS_ERR(env));
 
-	if (!cl_page_in_use(page)) {
+	/* we must not delete the cl_page if the vmpage is in use, otherwise we
+	 * disconnect the vmpage from Lustre while it's still alive(!), which
+	 * means we won't find it to discard on lock cancellation.
+	 *
+	 * References here are: caller + cl_page + page cache.
+	 * Any other references are potentially transient and must be ignored.
+	 */
+	if (!cl_page_in_use(clpage) && !vmpage_in_use(vmpage, 1)) {
 		result = 1;
-		cl_page_delete(env, page);
+		cl_page_delete(env, clpage);
 	}
 
 	/* To use percpu env array, the call path can not be rescheduled;
@@ -144,7 +151,7 @@  static int ll_releasepage(struct page *vmpage, gfp_t gfp_mask)
 	 * that we won't get into object delete path.
 	 */
 	LASSERT(cl_object_refc(obj) > 1);
-	cl_page_put(env, page);
+	cl_page_put(env, clpage);
 
 	cl_env_percpu_put(env);
 	return result;
diff --git a/fs/lustre/osc/osc_page.c b/fs/lustre/osc/osc_page.c
index f46b4e7..b56bc1a 100644
--- a/fs/lustre/osc/osc_page.c
+++ b/fs/lustre/osc/osc_page.c
@@ -539,8 +539,13 @@  static inline bool lru_page_busy(struct client_obd *cli, struct cl_page *page)
 	if (cli->cl_cache->ccc_unstable_check) {
 		struct page *vmpage = cl_page_vmpage(page);
 
-		/* vmpage have two known users: cl_page and VM page cache */
-		if (page_count(vmpage) - page_mapcount(vmpage) > 2)
+		/* this check is racy because the vmpage is not locked, but
+		 * that's OK - the code which does the actual page release
+		 * checks this again before releasing
+		 *
+		 * vmpage have two known users: cl_page and VM page cache
+		 */
+		if (vmpage_in_use(vmpage, 0))
 			return true;
 	}
 	return false;