zstream: add unit tests - #18829
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an in-process unit-test harness to the zstream command via an (intentionally) undocumented zstream selftest subcommand, introduces a comprehensive queue selftest module, integrates it into ZTS, and makes small zstream_queue fixes uncovered by testing (spindown locking and queue-slot alignment).
Changes:
- Add
zstream selftestharness (-l/-s/-t) and aqueueselftest module with multiple workload-based tests. - Integrate
zstream selftest queueinto the ZFS Test Suite runfiles and scripts. - Add thread registration/tracking helpers and register key
zstreamworker threads; fixzstream_queuespindown locking and slot alignment.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/zfs-tests/tests/Makefile.am | Installs the new ZTS test script for zstream selftest queue. |
| tests/zfs-tests/tests/functional/zstream/zstream_selftest_queue_001_pos.ksh | New ZTS test that runs zstream selftest queue with default and single-thread pools. |
| tests/runfiles/common.run | Adds the new ZTS test case to the common runfile. |
| cmd/zstream/zstream.h | Exposes zstream_do_selftest() declaration. |
| cmd/zstream/zstream.c | Routes new selftest subcommand to zstream_do_selftest() (kept undocumented). |
| cmd/zstream/zstream_util.h | Adds thread tracking API (pthread_register_self) and exports num_pthreads. |
| cmd/zstream/zstream_util.c | Implements pthread lifecycle tracking via TLS destructor and atomics. |
| cmd/zstream/zstream_selftest.h | New header defining the selftest harness interface and small deterministic PRNG helpers. |
| cmd/zstream/zstream_selftest.c | New selftest subcommand harness (arg parsing, listing, watchdog, seeding, execution). |
| cmd/zstream/zstream_selftest_queue.c | New queue module selftests with multiple workload patterns and thread-pool cycling tests. |
| cmd/zstream/zstream_queue.c | Bug fixes: spindown mutex handling, 8-byte slot alignment; plus worker thread registration. |
| cmd/zstream/zstream_chain.c | Registers chain worker threads for thread tracking. |
| cmd/zstream/Makefile.am | Adds new selftest sources to the zstream build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
af2eaf2 to
8c246fc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
cmd/zstream/zstream_util.c:214
- pthread_setspecific() returns 0 on success / an error number on failure (it does not return a pointer). Comparing the return value to NULL is incorrect and can fail to compile or silently mis-handle errors. If the intent is also to avoid double-registration skewing num_pthreads, check pthread_getspecific() before incrementing the counter.
static int sentinel;
static pthread_once_t init_pthread_tracking = PTHREAD_ONCE_INIT;
(void) pthread_once(&init_pthread_tracking, initialize_pthread_key);
if (pthread_setspecific(thread_count_key, (void *)&sentinel) != NULL)
err(1, "pthread_setspecific failed");
atomic_inc_32(&num_pthreads);
8c246fc to
636ea4b
Compare
636ea4b to
627efb4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cmd/zstream/zstream_selftest_queue.c:524
- expect_spindown() correctly uses atomic_add_32_nv(&num_pthreads, 0) for polling, but the failure path reads num_pthreads directly in the errx() call. If threads are still exiting/updating the counter, this reintroduces a data race in the error reporting path; use the same atomic read there too.
errx(1, "thread pool failed to spin down (%u threads, expected %u)",
num_pthreads, baseline);
627efb4 to
d8c214a
Compare
d8c214a to
f5ce208
Compare
f5ce208 to
f37a559
Compare
behlendorf
left a comment
There was a problem hiding this comment.
Building a selftest sub-command in to the utility is a new one for us, but in this instance I think it's pretty reasonable. This way all of the zstream code is self contained. I suppose we could wrap the selftest functionality with #ifdef ZFS_DEBUG guards so it's only enabled in debug builds, but since it's undocumented that seems sufficient.
The CI did seem to trip the watchdog in the last set of runs. It's not clear to me though if that's a real bug, or just the CI being sluggish.
https://github.com/openzfs/zfs/actions/runs/29962575516/job/89067001181?pr=18829
zstream/zstream_selftest_queue_001_pos (54 KiB)
[2026-07-23T02:18:16.073423] Test: /usr/share/zfs/zfs-tests/tests/functional/zstream/zstream_selftest_queue_001_pos (run as root) [02:01] [FAIL]
02:16:14.80 ASSERTION: zstream self-tests for zstream_queue all pass
02:16:14.81 Using seed 0x00415d5997f00adc (replay with -s 0x415d5997f00adc)
02:16:14.82 Running queue_basic ... OK
02:16:14.92 Running queue_edge_cases ... OK
02:16:14.93 Running queue_zero_cost ... OK
02:16:15.39 Running queue_torture ... OK
02:16:15.76 Running queue_multi_producer ... OK
02:16:15.90 Running queue_multi_queue ... OK
02:18:15.91
02:18:15.91 selftest: watchdog timeout in queue_cycles
02:18:15.91 Running queue_cycles ... ERROR: zstream selftest queue exited 1
|
That's not very informative, is it. Will the VM test harness save core dumps? I can always have the watchdog send a SIGABRT to itself. Meanwhile I'll run overnight with ThreadSanitizer on. |
|
The CI does collect logs, but I suspect it wouldn't have scooped up the core dump. You can download what it did gather from the summary page, https://github.com/openzfs/zfs/actions/runs/29962575516?pr=18829 . Also take a look at |
|
@GarthSnyder any news on reproducing the selftest failure locally? |
|
I've run some long test sessions, but no luck reproducing anything yet. This would have been my own watchdog timing out after 120 seconds, not the ZTS watchdog, and it happened on a relatively intensive test. But I still suspect an actual issue and I think it's worth adding backtraces. Unfortunately, the existing libspl backtrace code is thread-specific, so I would need to add some infrastructure to signal individual threads in sequnce. Let me switch this to draft status while I work on that. |
f37a559 to
d588a6d
Compare
|
By progressively reducing the watchdog timer length and pushing updates, I've found that it has to be really short (a few seconds) to have a chance of triggering because of system load or random variation. I think it's more likely than not that the original test failure was the result of a real deadlock. But despite ~10 CI runs and fairly extensive testing on my own systems and VMs, I haven't been able to reproduce any problems. I have set up all-thread backtraces for the test log in the event of a watchdog timeout, and they seem to be working fine. I'd like to prophylactically simplify a couple of things in zstream_queue. Some of the locking complexity comes from the fact that the thread pool terminates threads once there are no active queues. That's not currently serving any useful purpose, and given the one-shot context, I don't think a need for it is likely to show up in the future. Another potential source of risk is lock-free queue scoring. Well, locks are held, just not the locks on individual queues. Because enqueues are blocked while scoring, there shouldn't be any way for scoring to underestimate demand. But I've also found through profiling that this feature isn't actually increasing performance, so the scoring thread may as well lock the queues. There will still be time skew among queues, but no possibility of misreading any individual queue. Does this sound reasonable? I think I'm just going to leave this PR as a draft for now and work on getting those simplifications into shape. |
46bc8a1 to
6446ff7
Compare
This PR makes several changes to `zstream_queue.c` aimed at bulletproofing and simplification. ### Remove thread pool spindown This PR removes code that decomissioned worker threads once the last remaining queue had completed. The interlock between this operation and the creation of new queues complicated the locking system significantly and is known to have introduced at least two subtle locking bugs. With this change, the thread pool will be created once and retained until the process exits. ### Remove lock-free queue scoring The code is designed to work correctly even without scoring threads holding the queue mutexes of the queues they're examining. However, I've confirmed through performance testing that lock-free operation buys essentially nothing in terms of performance. The code doesn't really change, but it now locks around the scoring, ensuring that there is no possibility of skew among the observed queue indexes. Score skew is still possible (and expected) among queues. ### No condition signals without locks `assign_queue_and_get_work()` now retains the enqueue mutex until after signaling the "enqueued" condition if a scoring run determines that there is likely work available for more than one worker. ### Optimize advancement of the "claim" index for no-work items `advance_completion_index()` has been renamed `advance_indexes()` and sweeps both the "claim" and "completed" indexes. This does not affect correctness, but it reduces the number of empty worker loops. Formerly, advancement of the "claim" index only occured while a thread was actively collecting jobs. ### Call `advance_indexes()` even on `zstream_queue_fini()` Enqueues of zero-cost items now consistently trigger a call to `advance_indexes()`. ### Round up requested item sizes to a pessimistic boundary Queues formerly allocated their item workspaces as a single block indexed by slot number. That caused alignment problems when API clients requested peculiarly-shaped buffers. The requested size is now rounded up to a pessimistic boundary (the _Alignof of a worst-case union). ### Two-block memory allocation for new queues Previously, `zstream_queue` allocated memory for queue slots and the item workspaces to which they point as a single block. The code is more readable when these are separate allocations. Signed-off-by: Garth Snyder <garth@garthsnyder.com>
This PR adds a unit-testing harness to the `zstream` command along with additional tests for `zstream_queue.` Tests are run with the (undocumented) `zstream selftest` subcommand: ``` usage: zstream selftest [-l] [-s seed] [-t nthreads] module [test ...] -l list available tests -s seed seed for pseudo-random workloads (for replays) -t num size of the shared worker thread pool Available modules: queue ``` There are 8 tests within the `queue` group, each of which prepares a workload specification that's then sent to `run_queue_workloads()` for execution. The individual tests exercise a variety of load conditions and edge cases. I added a test has been added to the `zstream` group to run `zstream selftest queue` and `zstream selftest -t 1 queue`. I see that there's a provision for unit tests in ZTS, but this seemed like a cleaner approach given the dependencies among parts of `zstream`. If a true unit test is preferable, it probably wouldn't be too hard to port it over. The selftest harness includes a watchdog timer set at 120 seconds. If a test times out, the backtrace code in zstream_backtrace.[ch] runs to dump a stack trace of every thread. That backtrace seems to work fine on all the test VMs. Signed-off-by: Garth Snyder <garth@garthsnyder.com>
6446ff7 to
c8ac1ed
Compare
This PR adds a unit-testing harness to the
zstreamcommand along with additional tests forzstream_queue.Tests are run with the (undocumented)
zstream selftestsubcommand:There are 8 tests within the
queuegroup, each of which prepares a workload specification that's then sent torun_queue_workloads()for execution. The individual tests exercise a variety of load conditions and edge cases.There are a few odds and ends included with this PR:
zstream_util.[ch]gains apthread_register_self()function that pthreads workers can call to maintain a global tally of running threads.pthread_key_create()is used to register a callback that runs whenever a registered thread exits, regardless of its cause of death. The thread tally is used to exercise the thread pool spinup and spindown functions inzstream_queue.c. The motivation for tracking threads this way is that it avoids having to add platform-specific code to enumerate threads. (Pthreads has no native "give me a list of all threads" function.)zstream_chain.candzstream_queue.chave been modified to register their threads.A test has been added to the
zstreamgroup to runzstream selftest queueandzstream selftest -t 1 queue. I see that there's a provision for unit tests in ZTS, but this seemed like a cleaner approach given the dependencies among parts ofzstream. If a true unit test is preferable, it probably wouldn't be too hard to port it over.There are two minor changes to
zstream_queue.cto fix bugs revealed by testing.thread_pool_spindown()no longer attempts to reacquire the pool mutex once it has harvested threads. Andzstream_queue_create()now explicitly aligns queue slots to 8-byte boundaries.Types of Changes
Checklist
Signed-off-by.