Sure. I meant this bit to be a discussion point.

If we got here, we have some number of disks, all with the same genid. The one with the latest syncid says sc_ndisks is N. If there are other disk(s) with the same genid and sc_ndisks != N, do we want to kick them out? That was the existing behavior (via g_mirror_check_metadata).

I guess preserving the existing behavior for now makes some sense (smaller bite-size change) even if it isn't the right long-term behavior. I'm happy to drop the extraneous #if and comment(s).

I think the existing behaviour was reasonable. At least, I can't think of a scenario where it'd be harmful.

But now we're being kind of inconsistent - shouldn't we do the same if, e.g. the balance algorithm is different?

Sure, I guess we could reject mismatched balance and slice here as well. (And mediasize?)

I don't see any reason to assume the inconsistent disk has lower syncid, and the bump is pretty harmless (one-time write). If we can assume the inconsistent disk has lower syncid, we might as well KASSERT it.

I guess all of the metadata is encoded in a single disk block and locks are held such that the genid bump should happen simultaneously with metadata updates. Operative word being "should;" it wouldn't surprise me if there are bugs remaining in gmirror :-).

I guess we should move this kick-out loop above auto-start, like the genid one (and add the other metadata fields that can change at runtime). Should we actually kick out disks with inconsistent metadata if they have a lower syncid, or just dirty them?

I don't think it does — the kick-out of stale components, and marking for SYNCID bump were just relocated from this function to g_mirror_add_disk.

Suppose g_launch_mirror_before_timeout is true. The g_mirror_event_send() call in g_mirror_add_disk() will start the mirror once a second mirror is found. The event_send is synchronous, so the mirror's state will be updated to RUNNING before we perform the genid check on the last discovered component. In particular, in a mirror with two components, we'll always end up adding the broken component to the mirror, so the bug being introduced is actually larger than the one from r306743.

Oh, I see. I think the sc->sc_state == G_MIRROR_DEVICE_STATE_RUNNING condition (line 2950~2967 or so in the current incarnation of the diff) needs to be removed from g_mirror_add_disk to prevent us from adding the broken m2 component after initially discovering m1 in the r306743 scenario.

For the ordering where m2 is tasted before m1, we either need to run the kickout loop before event_send, or g_mirror_ndisks could be modified not to count stale components. My concern with the former option would be the mirror having zero components (ephemerally). I'm not sure there's any reason to be concerned about the latter option. I suppose a 3rd option would be to just temporarily bump sc_ndisks while the event_send and kickout loop runs in g_mirror_add_disk, and decrement it afterwards.

FWIW, I should mention that I introduced this bug (deleting the "Remove all disks without the biggest genid" from g_mirror_device_update) in my own modification to Cindy's patch — it's not present in the patch she submitted. (Blame where blame is due 🙂.)

And I should also add that a 4th option would be to just restore the kickout loop to g_mirror_update_device, perhaps via common subroutine.

I'll move the kick-out logic back to g_mirror_device_update, but locate it prior to the auto-start logic.