From c524079f4f59a39b99467d9868ee4aafdcf033e9 Mon Sep 17 00:00:00 2001 From: Patrick Farrell Date: Thu, 12 May 2022 14:53:08 -0400 Subject: [PATCH] LU-14541 llite: Check vmpage in releasepage 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. Fixes: e59f0c9a245f ("LU-12587 llite: don't check vmpage refcount in ll_releasepage()") Signed-off-by: Patrick Farrell Change-Id: I3aa1cd7330f5e7d1ba2ddb0c12779aa22f3d70b7 Reviewed-on: https://review.whamcloud.com/47262 Reviewed-by: Andreas Dilger Tested-by: Andreas Dilger Reviewed-by: John L. Hammond Tested-by: jenkins --- lustre/include/cl_object.h | 8 ++++++++ lustre/llite/rw26.c | 19 +++++++++++++------ lustre/osc/osc_page.c | 9 +++++++-- lustre/tests/sanity.sh | 19 +++++++++++-------- lustre/tests/sanityn.sh | 6 ++---- 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/lustre/include/cl_object.h b/lustre/include/cl_object.h index 2a0d921..e6cc54d 100644 --- a/lustre/include/cl_object.h +++ b/lustre/include/cl_object.h @@ -1069,6 +1069,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/lustre/llite/rw26.c b/lustre/llite/rw26.c index 98bf28a..e2a3049 100644 --- a/lustre/llite/rw26.c +++ b/lustre/llite/rw26.c @@ -114,7 +114,7 @@ static int ll_releasepage(struct page *vmpage, RELEASEPAGE_ARG_TYPE 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; @@ -130,16 +130,23 @@ static int ll_releasepage(struct page *vmpage, RELEASEPAGE_ARG_TYPE gfp_mask) if (obj == NULL) return 1; - page = cl_vmpage_page(vmpage, obj); - if (page == NULL) + clpage = cl_vmpage_page(vmpage, obj); + if (clpage == NULL) 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; @@ -156,7 +163,7 @@ static int ll_releasepage(struct page *vmpage, RELEASEPAGE_ARG_TYPE 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/lustre/osc/osc_page.c b/lustre/osc/osc_page.c index fa5d86e..fd4c1f3 100644 --- a/lustre/osc/osc_page.c +++ b/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; diff --git a/lustre/tests/sanity.sh b/lustre/tests/sanity.sh index 94774c8..c7a8015 100755 --- a/lustre/tests/sanity.sh +++ b/lustre/tests/sanity.sh @@ -41,8 +41,8 @@ init_logging ALWAYS_EXCEPT="$SANITY_EXCEPT " # bug number for skipped test: LU-9693 LU-6493 LU-9693 ALWAYS_EXCEPT+=" 42a 42b 42c " -# bug number: LU-8411 LU-9054 -ALWAYS_EXCEPT+=" 407 312 " +# bug number: LU-8411 LU-9054 LU-14541 +ALWAYS_EXCEPT+=" 407 312 277 " if $SHARED_KEY; then # bug number: LU-14181 LU-14181 @@ -24284,18 +24284,21 @@ test_398a() { # LU-4198 $LFS setstripe -c 1 -i 0 $DIR/$tfile $LCTL set_param ldlm.namespaces.*.lru_size=clear + # Disabled: DIO does not push out buffered I/O pages, see LU-12587 # request a new lock on client - dd if=/dev/zero of=$DIR/$tfile bs=1M count=1 + #dd if=/dev/zero of=$DIR/$tfile bs=1M count=1 - dd if=/dev/zero of=$DIR/$tfile bs=1M count=1 oflag=direct conv=notrunc - local lock_count=$($LCTL get_param -n \ - ldlm.namespaces.$imp_name.lru_size) - [[ $lock_count -eq 0 ]] || error "lock should be cancelled by direct IO" + #dd if=/dev/zero of=$DIR/$tfile bs=1M count=1 oflag=direct conv=notrunc + #local lock_count=$($LCTL get_param -n \ + # ldlm.namespaces.$imp_name.lru_size) + #[[ $lock_count -eq 0 ]] || error "lock should be cancelled by direct IO" $LCTL set_param ldlm.namespaces.*-OST0000-osc-ffff*.lru_size=clear # no lock cached, should use lockless DIO and not enqueue new lock - dd if=/dev/zero of=$DIR/$tfile bs=1M count=1 oflag=direct conv=notrunc + dd if=/dev/zero of=$DIR/$tfile bs=1M count=1 oflag=direct \ + conv=notrunc || + error "dio write failed" lock_count=$($LCTL get_param -n \ ldlm.namespaces.$imp_name.lru_size) [[ $lock_count -eq 0 ]] || error "no lock should be held by direct IO" diff --git a/lustre/tests/sanityn.sh b/lustre/tests/sanityn.sh index a520b27..0972705 100755 --- a/lustre/tests/sanityn.sh +++ b/lustre/tests/sanityn.sh @@ -18,10 +18,8 @@ init_test_env $@ init_logging ALWAYS_EXCEPT="$SANITYN_EXCEPT " -# bug number for skipped test: LU-7105 -ALWAYS_EXCEPT+=" 28 " -# bug number for skipped test: LU-14541 -ALWAYS_EXCEPT+=" 16f" +# bug number for skipped test: LU-7105 LU-14541 +ALWAYS_EXCEPT+=" 28 16f" # UPDATE THE COMMENT ABOVE WITH BUG NUMBERS WHEN CHANGING ALWAYS_EXCEPT! # skip tests for PPC until they are fixed -- 1.8.3.1