zfsonlinux/spl — pull request #198 by stevenj (2012-12-05 05:01:49+00:00)
Title: splat taskq:front race condition fix

The taskq:front test has a race condition where task 4 and 8 race to complete, due to an incorrectly calculated set of delay "factors" (T). If task 4 wins and actually finishes first, the verification of the order of completion will fail.

The delays calculated to order task completion do not take into account the terminal line in the table, and so are all off by a factor of 1. This causes all the tasks in all queues to finish sooner than expected and the accumulated error is the root cause of tasks 4 and 8 racing to complete first. Before the change the "actual" table looks like I commented in https://github.com/zfsonlinux/spl/issues/130

I changed:
* the table in the comment to correctly reflect the test and the factor timings needed.
* the individual task delay factors of T so that ONLY 1 task will every 2T. (on average)
* 1T was reduced from 100ms to 50ms. This halves the duration of the test and makes any remaining raciness more likely to cause failures, but it did not cause the test to fail.
* simplified the delay factor logic by using a table look-up instead of a switch.
* Added a "task started" message so that with -v it is possible to see the order tasks are started.
* Moved the "task completed" message inside the spinlock so that with -v the message truly reflects the absolute order of completion as guaranteed by the spinlock.

This corrects issue https://github.com/zfsonlinux/spl/issues/130 however this statement from that issue does not seem correct.
> this test basically assumes that the loop dispatching these tests takes zero walltime to run

The loop dispatching the tasks only needs to dispatch tasks 1 to 3 in less than 2T time, not instantly. If the first 3 tasks can not be reliably dispatched in less than 2T time, that could possibly be fixed by using a wait queue to synchronize the running start of tasks 1 to 3 to the 100ms delay in the dispatcher but doing that did not correct or improve the occurrence of the fault in my testing.

It may be prudent to revert the size of a 1T from 50ms back to 100ms as a 2T = 100ms may be too quick to dispatch on a slow machine. However I don't think this likely, as even on a slow CPU a 2T of 100ms is a subjective eternity. My test machine cpu is an AMD E-450 APU, which is hardly a fast CPU and it completed the trials without error with this reduced 1T.

It may be prudent (or more correct?) to change the delay in the dispatcher after task 1 to 3 are queued to match the size of a 1T, but being a fixed 100ms did not cause me any problems or spurious failures, so I did not change it.

With the fault corrected by this patch I was unable to trigger any other fault in the test case due to raciness and so I would be loath to add this proposed synchronization or change the duration of 1T unless further issues due to raciness of the test are shown and verifiable.

I ran the test case for more than 7200 trials with 0 failures. Prior to this patch I would get about 500+ failures in 10,000 trials, more or less distributed evenly through the trial set.