geom(4) mirror: Do not panic on gmirror(8) insert, resize
ClosedPublic
Actions

Authored by cem on May 9 2020, 3:27 PM.

Details

Reviewers

bdrewery
markj
vangyzen

Commits

rS360941: geom(4) mirror: Do not panic on gmirror(8) insert, resize

Summary

Geom_mirror initialization occurs in spurts and the present of a
non-destroyed g_mirror softc does not always indicate that the geom has
launched (i.e., has an sc_provider).

Some gmirror(8) commands (via g_mirror_ctl) depend on a g_mirror's
sc_provider (insert and resize). For those commands, g_mirror_ctl is
modified to sleep-poll in an interruptible way until the target geom is
either launched or destroyed.

Test Plan

gmirror test suite with kern.geom.mirror.launch_mirror_before_timeout=0 seems to be enough to
repro this in-house. I will try that in a FreeBSD head VM as well.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cem requested review of this revision.May 9 2020, 3:27 PM

cem created this revision.

Harbormaster completed remote builds in B31001: Diff 71587.May 9 2020, 3:27 PM

cem added inline comments.May 9 2020, 3:30 PM

sys/geom/mirror/g_mirror_ctl.c
68–71 ↗	(On Diff #71587)	This is sort of orthogonal; it just seemed to be missing before.
654 ↗	(On Diff #71587)	Use of `sc_provider`
879 ↗	(On Diff #71587)	Use of `sc_provider`

cem edited the test plan for this revision. (Show Details)May 9 2020, 3:30 PM

I verified that head panics when running the test suite with kern.geom.mirror.launch_mirror_before_timeout=0, and that it passes with this patch applied.

sys/geom/mirror/g_mirror_ctl.c
111 ↗	(On Diff #71587)	I don't really see why having a G_MIRROR_DEVICE_STATE_LAUNCHED state, indicating that the device is ready to handle requests from upper layers, is so complicated to add.

This revision is now accepted and ready to land.May 11 2020, 4:10 PM

cem added inline comments.May 11 2020, 5:39 PM

sys/geom/mirror/g_mirror_ctl.c
111 ↗	(On Diff #71587)	Every state assertion and condition in g_mirror needs revisiting (~11), and most g_mirror_ctl consumers do not actually require LAUNCHED (and some almost certainly should not require LAUNCHED). Adding the 3rd state doesn't fix the dummy sleep loop hack, as we don't reliably signal anything on errors that would prevent a gmirror from reaching `LAUNCHED`; so consumers cannot reliably sx_sleep(). So it might be slightly cleaner conceptually, but it adds a bit of risk and doesn't seem to help with the main ugliness (the polling). Also, LAUNCHED is a bit tricky to order; it seems like there are conditions where we might not create an sc_provider, or might create it before RUNNING. I did try this approach first before abandoning it. I may be missing something :-).