On Wed, 2019-05-08 at 07:22 +1000, Dave Chinner wrote: > On Tue, May 07, 2019 at 01:05:28PM -0400, Rik van Riel wrote: > > The code in xlog_wait uses the spinlock to make adding the task to > > the wait queue, and setting the task state to UNINTERRUPTIBLE > > atomic > > with respect to the waker. > > > > Doing the wakeup after releasing the spinlock opens up the > > following > > race condition: > > > > - add task to wait queue > > > > - wake up task > > > > - set task state to UNINTERRUPTIBLE > > > > Simply moving the spin_unlock to after the wake_up_all results > > in the waker not being able to see a task on the waitqueue before > > it has set its state to UNINTERRUPTIBLE. > > Yup, seems like an issue. Good find, Rik. > > So, what problem is this actually fixing? Was it noticed by > inspection, or is it actually manifesting on production machines? > If it is manifesting IRL, what are the symptoms (e.g. hang running > out of log space?) and do you have a test case or any way to > exercise it easily? Chris spotted a hung kworker task, in UNINTERRUPTIBLE state, but with an empty wait queue. This does not seem like something that is easily reproducible. > And, FWIW, did you check all the other xlog_wait() users for the > same problem? I did not, but am looking now. The xlog_wait code itself is fine, but it seems there are a few other wakers that are doing the wakeup after releasing the lock. It looks like xfs_log_force_umount() and the other wakeup in xlog_state_do_callback() suffer from the same issue. > > The lock ordering of taking the waitqueue lock inside the > > l_icloglock > > is already used inside xlog_wait; it is unclear why the waker was > > doing > > things differently. > > Historical, most likely, and the wakeup code has changed in years > gone by and a race condition that rarely manifests is unlikely to be > noticed. > > .... > > Yeah, the conversion from counting semaphore outside the iclog lock > to use wait queues in 2008 introduced this bug. The wait queue > addition was moved inside the lock, the wakeup left outside. So: It looks like that conversion may have introduced the same bug in multiple places. That first wakeup in xlog_state_do_callback() looks pretty straightforward. That spin_unlock could be moved down as well, or a lock & unlock pair could be placed around the wake_up_all. I am not sure about xfs_log_force_umount(). Could the unlock be moved to after the wake_up_all, or does that create lock ordering issues with the xlog_grant_head_wake_all calls? Could a simple lock + unlock of log->l_icloglock around the wake_up_all do the trick, or is there some other state that also needs to stay locked? -- All Rights Reversed.
Attachment:
signature.asc
Description: This is a digitally signed message part
