From 6af696f0a4e04eaf08de01b549ffa01e6b9bb8ef Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Wed, 12 Dec 2018 16:09:33 +1100 Subject: [PATCH] LU-9679 osc: simplify osc_extent_wait() Taking a spinlock to check the current value of the state is unnecessary. The wake_up() and wait_event() calls have sufficient barriers to ensure that the value will be seen and the wait will abort properly. In most cases, osc_extent_wait() is followed by osc_object_lock() before any shared data is touched - in those cases there is no need for osc_extent_wait() to wait for the spinlock to be released. The one case where osc_object_lock() does not immediately follow is in osc_cache_truncate_start(). The extra locking was introduced in a patch which fixed a problem with truncation, so it is likely that this is the call that was thought to be relevant. In that case, following osc_extent_wait(), an extent that had been detached from the per-object list (oe_link linkage) is worked on it without any locking. In this case the code is waiting for OES_TRUNC, so any changes that happen after the osc_extent_state_set(ext, OES_TRUNC) and when the lock is dropped, might not be seen by the woken code. The only thing changed is ->oe_trunc_pending, and the woken code doesn't look at that. The only remaining possible need for extra synchronization is if some other value was changed before the wakeup and is needed after the wait. According to memory-barriers.txt, a barrier might be needed to ensure that is visible. Such a barrier is most clearly presented by used smp_store_release() to set the state before wakeup, and smp_load_acquire() to view it after waiting. Also use a simple wake_up() instead of wake_up_all() - the latter is only needed when exclusive waiting is being used. Linux-Commit: 2a7e8a40cb11 ("lustre: osc: simplify osc_extent_wait()") Signed-off-by: Mr NeilBrown Change-Id: I0945902b55d598af263779787b9641a86e9476c3 Reviewed-on: https://review.whamcloud.com/37598 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Andreas Dilger Reviewed-by: Patrick Farrell Reviewed-by: James Simmons Reviewed-by: Oleg Drokin --- lustre/osc/osc_cache.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/lustre/osc/osc_cache.c b/lustre/osc/osc_cache.c index d3e2378..884d14c 100644 --- a/lustre/osc/osc_cache.c +++ b/lustre/osc/osc_cache.c @@ -314,8 +314,8 @@ static void osc_extent_state_set(struct osc_extent *ext, int state) /* LASSERT(sanity_check_nolock(ext) == 0); */ /* TODO: validate the state machine */ - ext->oe_state = state; - wake_up_all(&ext->oe_waitq); + smp_store_release(&ext->oe_state, state); + wake_up(&ext->oe_waitq); } static struct osc_extent *osc_extent_alloc(struct osc_object *obj) @@ -893,17 +893,6 @@ int osc_extent_finish(const struct lu_env *env, struct osc_extent *ext, RETURN(0); } -static int extent_wait_cb(struct osc_extent *ext, enum osc_extent_state state) -{ - int ret; - - osc_object_lock(ext->oe_obj); - ret = ext->oe_state == state; - osc_object_unlock(ext->oe_obj); - - return ret; -} - /** * Wait for the extent's state to become @state. */ @@ -932,14 +921,16 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext, osc_extent_release(env, ext); /* wait for the extent until its state becomes @state */ - rc = wait_event_idle_timeout(ext->oe_waitq, extent_wait_cb(ext, state), + rc = wait_event_idle_timeout(ext->oe_waitq, + smp_load_acquire(&ext->oe_state) == state, cfs_time_seconds(600)); if (rc == 0) { OSC_EXTENT_DUMP(D_ERROR, ext, "%s: wait ext to %u timedout, recovery in progress?\n", cli_name(osc_cli(obj)), state); - wait_event_idle(ext->oe_waitq, extent_wait_cb(ext, state)); + wait_event_idle(ext->oe_waitq, + smp_load_acquire(&ext->oe_state) == state); } if (ext->oe_rc < 0) rc = ext->oe_rc; -- 1.8.3.1