From 930ad25733d925021fbce468568acacde219d67c Mon Sep 17 00:00:00 2001 From: Shaun Tancheff Date: Fri, 13 Sep 2024 09:31:08 +0700 Subject: [PATCH] LU-15808 ptlrpc: ptlrpc_set_wait() use wait_woken ptlrpc_set_wait() using a potentially long running condition ptlrpc_check_set() that can also block. If it does block during ptl_send_rpc() it could potentially trigger a warning: do not call blocking ops when !TASK_RUNNING NeilBrown suggested to use wait_woken() instead. Convert ptlrpc_set_wait to use wait_woken() similar to the wait_woken() method used in ptlrpcd. Signed-off-by: Shaun Tancheff Change-Id: I544550db58fa2e89ce18a8a43a64fdea7ed57206 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/56317 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Oleg Drokin Reviewed-by: Neil Brown Reviewed-by: Petros Koutoupis Reviewed-by: Alexander Zarochentsev Reviewed-by: James Simmons --- libcfs/include/libcfs/linux/linux-wait.h | 11 ---- lustre/ptlrpc/client.c | 96 ++++++++++++-------------------- 2 files changed, 36 insertions(+), 71 deletions(-) diff --git a/libcfs/include/libcfs/linux/linux-wait.h b/libcfs/include/libcfs/linux/linux-wait.h index b54370f..cd62fab 100644 --- a/libcfs/include/libcfs/linux/linux-wait.h +++ b/libcfs/include/libcfs/linux/linux-wait.h @@ -565,17 +565,6 @@ do { \ __ret; \ }) -#define l_wait_event_abortable_timeout(wq, condition, timeout) \ -({ \ - sigset_t __new_blocked, __old_blocked; \ - int __ret = 0; \ - siginitsetinv(&__new_blocked, LUSTRE_FATAL_SIGS); \ - sigprocmask(SIG_BLOCK, &__new_blocked, &__old_blocked); \ - __ret = wait_event_interruptible_timeout(wq, condition, timeout);\ - sigprocmask(SIG_SETMASK, &__old_blocked, NULL); \ - __ret; \ -}) - #define l_wait_event_abortable_exclusive(wq, condition) \ ({ \ sigset_t __new_blocked, __old_blocked; \ diff --git a/lustre/ptlrpc/client.c b/lustre/ptlrpc/client.c index 8cd8b63..84b2b8e 100644 --- a/lustre/ptlrpc/client.c +++ b/lustre/ptlrpc/client.c @@ -2014,7 +2014,7 @@ int ptlrpc_check_set(const struct lu_env *env, struct ptlrpc_request_set *set) } /* - * ptlrpc_set_wait uses l_wait_event_abortable_timeout() + * ptlrpc_set_wait uses wait_woken() * so it sets rq_intr regardless of individual rpc * timeouts. The synchronous IO waiting path sets * rq_intr irrespective of whether ptlrpcd @@ -2537,6 +2537,7 @@ time64_t ptlrpc_set_next_timeout(struct ptlrpc_request_set *set) int ptlrpc_set_wait(const struct lu_env *env, struct ptlrpc_request_set *set) { struct ptlrpc_request *req; + sigset_t oldset, newset; time64_t timeout; int rc; @@ -2553,7 +2554,14 @@ int ptlrpc_set_wait(const struct lu_env *env, struct ptlrpc_request_set *set) RETURN(0); do { + DEFINE_WAIT_FUNC(wait, woken_wake_function); + long remaining; + unsigned long allow = 0; + int state = TASK_IDLE; + + rc = 0; timeout = ptlrpc_set_next_timeout(set); + remaining = cfs_time_seconds(timeout ? timeout : 1); /* * wait until all complete, interrupted, or an in-flight @@ -2562,71 +2570,39 @@ int ptlrpc_set_wait(const struct lu_env *env, struct ptlrpc_request_set *set) CDEBUG(D_RPCTRACE, "set %p going to sleep for %lld seconds\n", set, timeout); + add_wait_queue(&set->set_waitq, &wait); if ((timeout == 0 && !signal_pending(current)) || set->set_allow_intr) { - /* - * No requests are in-flight (ether timed out - * or delayed), so we can allow interrupts. - * We still want to block for a limited time, - * so we allow interrupts during the timeout. - */ - rc = l_wait_event_abortable_timeout( - set->set_waitq, - ptlrpc_check_set(NULL, set), - cfs_time_seconds(timeout ? timeout : 1)); - if (rc == 0) { - rc = -ETIMEDOUT; - ptlrpc_expired_set(set); - } else if (rc < 0) { - rc = -EINTR; - ptlrpc_interrupted_set(set); - } else { - rc = 0; - } - } else { - /* - * At least one request is in flight, so no - * interrupts are allowed. Wait until all - * complete, or an in-flight req times out. - */ - rc = wait_event_idle_timeout( - set->set_waitq, - ptlrpc_check_set(NULL, set), - cfs_time_seconds(timeout ? timeout : 1)); - if (rc == 0) { - ptlrpc_expired_set(set); - rc = -ETIMEDOUT; - } else { - rc = 0; + state = TASK_INTERRUPTIBLE; + allow = LUSTRE_FATAL_SIGS; + } + /* block until ready or timeout occurs */ + do { + if (ptlrpc_check_set(NULL, set)) + break; + if (allow) { + siginitsetinv(&newset, allow); + sigprocmask(SIG_BLOCK, &newset, &oldset); } - - /* - * LU-769 - if we ignored the signal because - * it was already pending when we started, we - * need to handle it now or we risk it being - * ignored forever - */ - if (rc == -ETIMEDOUT && - signal_pending(current)) { - sigset_t old, new; - - siginitset(&new, LUSTRE_FATAL_SIGS); - sigprocmask(SIG_BLOCK, &new, &old); - /* - * In fact we only interrupt for the - * "fatal" signals like SIGINT or - * SIGKILL. We still ignore less - * important signals since ptlrpc set - * is not easily reentrant from - * userspace again - */ + remaining = wait_woken(&wait, state, remaining); + if (allow) { if (signal_pending(current)) - ptlrpc_interrupted_set(set); - sigprocmask(SIG_SETMASK, &old, NULL); + remaining = -EINTR; + sigprocmask(SIG_SETMASK, &oldset, NULL); } + } while (remaining > 0); + /* + * wait_woken* returns the result from schedule_timeout() which + * is always a positive number, or 0 on timeout. + */ + if (remaining == 0) { + rc = -ETIMEDOUT; + ptlrpc_expired_set(set); + } else if (remaining < 0) { + rc = -EINTR; + ptlrpc_interrupted_set(set); } - - LASSERT(rc == 0 || rc == -EINTR || rc == -ETIMEDOUT); + remove_wait_queue(&set->set_waitq, &wait); /* * -EINTR => all requests have been flagged rq_intr so next -- 1.8.3.1