Skip to content

Commit

Permalink
Don't hold mutex until release cv in cv_wait
Browse files Browse the repository at this point in the history 
If a thread is holding mutex when doing cv_destroy, it might end up waiting a
thread in cv_wait. The waiter would wake up trying to aquire the same mutex
and cause deadlock.

We solve this by move the mutex_enter to the bottom of cv_wait, so that
the waiter will release the cv first, allowing cv_destroy to succeed and have
a chance to free the mutex.

This would create race condition on the cv_mutex. We use xchg to set and check
it to ensure we won't be harmed by the race. This would result in the cv_mutex
debugging becomes best-effort.

Also, the change reveals a race, which was unlikely before, where we call
mutex_destroy while test threads are still holding the mutex. We use
kthread_stop to make sure the threads are exit before mutex_destroy.

Signed-off-by: Chunwei Chen <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Signed-off-by: Tim Chase <[email protected]>
Issue openzfs/zfs#4166
Issue openzfs/zfs#4106
  • Loading branch information
@tuxoko @behlendorf
tuxoko authored and behlendorf committed Jan 12, 2016
1 parent d297a5a commit e843553
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 15 deletions.
55 changes: 40 additions & 15 deletions module/spl/spl-condvar.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,19 @@ static void
cv_wait_common(kcondvar_t *cvp, kmutex_t *mp, int state, int io)
{
DEFINE_WAIT(wait);
kmutex_t *m;

ASSERT(cvp);
ASSERT(mp);
ASSERT(cvp->cv_magic == CV_MAGIC);
ASSERT(mutex_owned(mp));
atomic_inc(&cvp->cv_refs);

if (cvp->cv_mutex == NULL)
cvp->cv_mutex = mp;

m = ACCESS_ONCE(cvp->cv_mutex);
if (!m)
m = xchg(&cvp->cv_mutex, mp);
/* Ensure the same mutex is used by all callers */
ASSERT(cvp->cv_mutex == mp);
ASSERT(m == NULL || m == mp);

prepare_to_wait_exclusive(&cvp->cv_event, &wait, state);
atomic_inc(&cvp->cv_waiters);
Expand All @@ -106,16 +107,25 @@ cv_wait_common(kcondvar_t *cvp, kmutex_t *mp, int state, int io)
io_schedule();
else
schedule();
mutex_enter(mp);

/* No more waiters a different mutex could be used */
if (atomic_dec_and_test(&cvp->cv_waiters)) {
/*
* This is set without any lock, so it's racy. But this is
* just for debug anyway, so make it best-effort
*/
cvp->cv_mutex = NULL;
wake_up(&cvp->cv_destroy);
}

finish_wait(&cvp->cv_event, &wait);
atomic_dec(&cvp->cv_refs);

/*
* Hold mutex after we release the cvp, otherwise we could dead lock
* with a thread holding the mutex and call cv_destroy.
*/
mutex_enter(mp);
}

void
Expand Down Expand Up @@ -148,6 +158,7 @@ __cv_timedwait_common(kcondvar_t *cvp, kmutex_t *mp, clock_t expire_time,
int state)
{
DEFINE_WAIT(wait);
kmutex_t *m;
clock_t time_left;

ASSERT(cvp);
Expand All @@ -156,15 +167,16 @@ __cv_timedwait_common(kcondvar_t *cvp, kmutex_t *mp, clock_t expire_time,
ASSERT(mutex_owned(mp));
atomic_inc(&cvp->cv_refs);

if (cvp->cv_mutex == NULL)
cvp->cv_mutex = mp;

m = ACCESS_ONCE(cvp->cv_mutex);
if (!m)
m = xchg(&cvp->cv_mutex, mp);
/* Ensure the same mutex is used by all callers */
ASSERT(cvp->cv_mutex == mp);
ASSERT(m == NULL || m == mp);

/* XXX - Does not handle jiffie wrap properly */
time_left = expire_time - jiffies;
if (time_left <= 0) {
/* XXX - doesn't reset cv_mutex */
atomic_dec(&cvp->cv_refs);
return (-1);
}
Expand All @@ -179,17 +191,25 @@ __cv_timedwait_common(kcondvar_t *cvp, kmutex_t *mp, clock_t expire_time,
*/
mutex_exit(mp);
time_left = schedule_timeout(time_left);
mutex_enter(mp);

/* No more waiters a different mutex could be used */
if (atomic_dec_and_test(&cvp->cv_waiters)) {
/*
* This is set without any lock, so it's racy. But this is
* just for debug anyway, so make it best-effort
*/
cvp->cv_mutex = NULL;
wake_up(&cvp->cv_destroy);
}

finish_wait(&cvp->cv_event, &wait);
atomic_dec(&cvp->cv_refs);

/*
* Hold mutex after we release the cvp, otherwise we could dead lock
* with a thread holding the mutex and call cv_destroy.
*/
mutex_enter(mp);
return (time_left > 0 ? time_left : -1);
}

Expand All @@ -216,6 +236,7 @@ __cv_timedwait_hires(kcondvar_t *cvp, kmutex_t *mp, hrtime_t expire_time,
int state)
{
DEFINE_WAIT(wait);
kmutex_t *m;
hrtime_t time_left, now;
unsigned long time_left_us;

Expand All @@ -225,11 +246,11 @@ __cv_timedwait_hires(kcondvar_t *cvp, kmutex_t *mp, hrtime_t expire_time,
ASSERT(mutex_owned(mp));
atomic_inc(&cvp->cv_refs);

if (cvp->cv_mutex == NULL)
cvp->cv_mutex = mp;

m = ACCESS_ONCE(cvp->cv_mutex);
if (!m)
m = xchg(&cvp->cv_mutex, mp);
/* Ensure the same mutex is used by all callers */
ASSERT(cvp->cv_mutex == mp);
ASSERT(m == NULL || m == mp);

now = gethrtime();
time_left = expire_time - now;
Expand All @@ -253,17 +274,21 @@ __cv_timedwait_hires(kcondvar_t *cvp, kmutex_t *mp, hrtime_t expire_time,
* interrupts
*/
usleep_range(time_left_us, time_left_us + 100);
mutex_enter(mp);

/* No more waiters a different mutex could be used */
if (atomic_dec_and_test(&cvp->cv_waiters)) {
/*
* This is set without any lock, so it's racy. But this is
* just for debug anyway, so make it best-effort
*/
cvp->cv_mutex = NULL;
wake_up(&cvp->cv_destroy);
}

finish_wait(&cvp->cv_event, &wait);
atomic_dec(&cvp->cv_refs);

mutex_enter(mp);
time_left = expire_time - gethrtime();
return (time_left > 0 ? time_left : -1);
}
Expand Down
30 changes: 30 additions & 0 deletions module/splat/splat-condvar.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ splat_condvar_test12_thread(void *arg)
ct->ct_thread->comm, atomic_read(&cv->cv_condvar.cv_waiters));
mutex_exit(&cv->cv_mtx);

/* wait for main thread reap us */
while (!kthread_should_stop())
schedule();
return 0;
}

Expand Down Expand Up @@ -151,6 +154,12 @@ splat_condvar_test1(struct file *file, void *arg)
/* Wake everything for the failure case */
cv_broadcast(&cv.cv_condvar);
cv_destroy(&cv.cv_condvar);

/* wait for threads to exit */
for (i = 0; i < SPLAT_CONDVAR_TEST_COUNT; i++) {
if (!IS_ERR(ct[i].ct_thread))
kthread_stop(ct[i].ct_thread);
}
mutex_destroy(&cv.cv_mtx);

return rc;
Expand Down Expand Up @@ -199,6 +208,12 @@ splat_condvar_test2(struct file *file, void *arg)

/* Wake everything for the failure case */
cv_destroy(&cv.cv_condvar);

/* wait for threads to exit */
for (i = 0; i < SPLAT_CONDVAR_TEST_COUNT; i++) {
if (!IS_ERR(ct[i].ct_thread))
kthread_stop(ct[i].ct_thread);
}
mutex_destroy(&cv.cv_mtx);

return rc;
Expand Down Expand Up @@ -234,6 +249,9 @@ splat_condvar_test34_thread(void *arg)

mutex_exit(&cv->cv_mtx);

/* wait for main thread reap us */
while (!kthread_should_stop())
schedule();
return 0;
}

Expand Down Expand Up @@ -302,6 +320,12 @@ splat_condvar_test3(struct file *file, void *arg)
/* Wake everything for the failure case */
cv_broadcast(&cv.cv_condvar);
cv_destroy(&cv.cv_condvar);

/* wait for threads to exit */
for (i = 0; i < SPLAT_CONDVAR_TEST_COUNT; i++) {
if (!IS_ERR(ct[i].ct_thread))
kthread_stop(ct[i].ct_thread);
}
mutex_destroy(&cv.cv_mtx);

return rc;
Expand Down Expand Up @@ -372,6 +396,12 @@ splat_condvar_test4(struct file *file, void *arg)
/* Wake everything for the failure case */
cv_broadcast(&cv.cv_condvar);
cv_destroy(&cv.cv_condvar);

/* wait for threads to exit */
for (i = 0; i < SPLAT_CONDVAR_TEST_COUNT; i++) {
if (!IS_ERR(ct[i].ct_thread))
kthread_stop(ct[i].ct_thread);
}
mutex_destroy(&cv.cv_mtx);

return rc;
Expand Down

0 comments on commit e843553

Please sign in to comment.