This results in lock recursion if any regular or sync BIOs are on
sc_queue when the provider is destroyed, for example when the last
component of a mirror fails. g_mirror_done() and g_mirror_sync_done()
both acquire the queue lock, but we're already holding it in order to
drain the queue. Instead, just free the BIOs.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/geom/mirror/g_mirror.c | ||
|---|---|---|
| 2124 ↗ | (On Diff #17737) | This warrants a comment about what you're doing. Why does that test make sense? |
| sys/geom/mirror/g_mirror.c | ||
|---|---|---|
| 2124 ↗ | (On Diff #17737) | Added, thanks. |
Comment Actions
Perfect. I alway struggle between too much and too little information, and this strikes a good balance.. Thanks.
Comment Actions
I don't remember gmirror good enough to properly review this, sorry. But speaking about lock recursion, it could probably be solved in different way -- grab queue content under the lock and then deliver errors after lock is dropped. I don't very like magic with SYNC flag and free() in proposed change, it looks dirty.
Comment Actions
That would fix the lock recursion but still be undesirable: the bio_done handlers for the mirror BIOs just enqueue the BIO again, so we end up inserting into the queue we're trying to drain. And at this point, the mirror worker does not run the queue again, so these requeued BIOs would be leaked. I could modify the bio_done handlers in question to check for a flag in the mirror softc, but if they are not executed directly, the GEOM up thread will race with a free of the softc.