I believe this reintroduces the bug fixed in r306743.

Why? Recomputing it is cheap.

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.

It just makes the code clearer; it's not about the computation cost. It could be replaced with some more complicated expression (LIST_FIRST(&sc->sc_disks)->d_genid) in g_mirror_refresh_device but that wouldn't really improve the code, as far as I can tell.

Will fix

Yeah, me too (I'm not the original author of the patch). I tried to avoid making too stylistic changes (other than minimal style(9) stuff) to the Isilon patch when uploading to avoid adding conflict when we merge this back. I'm happy to make the change if you want, though.

I agree and suggested it in the internal code review ;-). It just didn't happen. I'll make the change.

It can't be changed during the lifetime of a gmirror; the rest of these can.

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.

How can mediasize change?

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.

g_mirror_ctl_resize (r258357). It's why the mediasize check was disabled in g_mirror_check_metadata (although I don't understand why it wasn't just deleted).

Ah fuck, I rebased my patch series wrong. Ignore this.

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.

Extra #if

I mean, you can just loop over the disks each time, which is what we do in the INVARIANTS block anyway. Keeping it in the softc is just more state to keep track of, but I don't feel strongly about it.

Extra newline.

I'd prefer to have the suggested change; in these situations I'd just do the merge back myself while the change is still fresh, rather than waiting for the tip of the merge branch to catch up.

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?

Why is the bump necessary? Shouldn't we be able to assume that the inconsistent disk already has a lower syncid?

What's the point of this part of the change? If g_mirror_ndisks(sc, -1) != sc->sc_ndisks, then we started with a missing disk, in which case we must have bumped the syncid on the remaining disks, so we will not use the lost disk's metadata if it reappears.

I added this bit b/c during testing I saw syncid did not get automatically bumped if a disk failed to add to a running mirror during the early part of a rebalance. If the node got rebooted at this very instant, the valid components and disconnected component would have the same genid and syncid.

The 'forget' ioctl allows us to force gmirror to increment the genid from the userspace. The ioctl anyway updates sc->ndisks when a mirror is degraded, so it makes sense to update genid as well.

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 don't know the exact motivation for this change. Cindy?

g_mirror_ndisks(sc, -1) != sc->sc_ndisks can sometimes occur at runtime, when a disk disappears (STATE_DISCONNECTED) or is manually removed (g_mirror_ctl_remove). But it isn't clear to me if that can race with this function or not. If it is an invariant, maybe we should MPASS(sc->sc_state == G_MIRROR_DEVICE_STATE_STARTING);.

Argh ... my previous reply was misaligned. Here you go:

I added this bit b/c during testing I saw syncid did not get automatically bumped if a disk failed to add to a running mirror during the early part of a rebalance. If the node got rebooted at this very instant, the valid components and disconnected component would have the same genid and syncid. The 'forget' ioctl allows us to force gmirror to increment the genid from the userspace. The ioctl anyway updates sc->ndisks when a mirror is degraded, so it makes sense to update genid as well.

They have contradictory metadata. It shouldn't be possible for them to have the same syncid and genid unless something other than gmirror was messing with the metadata block. At least, if they do, then there is a bug elsewhere in gmirror.

Sorry for being picky, but gmirror has historically been pretty tricky to reason about, so I really want to make sure I understand the intent of each change.

Can you elaborate on what "failed to add" means? In general, any time a mirror is in a degraded state, the remaining components should have a larger genid or syncid than the missing components. If a component is kicked out while the mirror is running, we should bump the genid. If the mirror is started with one or more components missing, we bump the syncid (upon the first write to the mirror).

Unrelated to that, the condition in the if statement is kind of weird. How can we have a running mirror with 0 active disks? Why do we not just always bump the genid?

The 'forget' ioctl allows us to force gmirror to increment the genid from the userspace.

Currently it does that only if the mirror is degraded, so "force" isn't really true.

The ioctl anyway updates sc->ndisks when a mirror is degraded, so it makes sense to update genid as well.

The "insert" verb also updates sc->ndisks but doesn't update genid. I agree that there is no harm in updating the genid here, but I don't see the point of it given that this change also adds code to kick out mirrors with inconsistent md_all fields.

No problem. Thanks for reviewing this patch.

The "failed to add" scenario actually came out of a field issue. On a factory fresh node, during early boot a couple of SEDs dropped out while gmirror tried to replicate/sync the root device from the primary copy onto the secondary copy. It happened twice in a row and finally the 3rd attempt succeeded and the root mirror transitioned to RUNNING.

The data I saw was that one of the disconnected components had the same genid as the that of the valid ones. I think you're correct w.r.t. using the highest syncid as a tie breaker to pick out the correct component.

All we are interested is that genid gets incremented when the mirror is degraded. As our userspace daemon runs the rebalance routine as often as it can, forget() is called to proactively update genid as needed. This is a nice trick for gmirror to outright reject old component on the next boot based on genid comparison.

I think the g_mirror_ndisks(sc, G_MIRROR_DISK_STATE_ACTIVE) > 0 is me being paranoid and trying to avoid hitting the assert in g_mirror_bump_genid().

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?

Should I drop this section, at least for the initial upstream revision?

Sounds good to me.

For reasons I'm not sure of, in the kyua environment this times out; it seems the sub-shell process is stuck in ttyread for some reason. In atf-sh, of course, it runs fine.

What is the purpose of this sleep? Sleeps in tests are very dangerous. You should figure out how to eliminate them.

Everything geom does is asynchronous. The sleep gives it a chance to settle in the background, even if it is logging at a high debug level to a relatively slow console device.

I agree that the sleep represents a hacky way to wait for an asynchronous event, and may timeout before that event actually fires, but I don't really follow how that makes them very dangerous.

(Tiny sleeps are commonplace in existing geom tests and I don't think eliminating the need for them is within thee scope of this revision.)

I'm much more concerned with why cleanup() times out in kyua, but not atf-sh.

Reducing the value of the sleeps only makes it more likely that the test will fail someday, and no one will know why. Better to remove all of the sleeps and replace them with short polling loops.

I think that's out of scope for this change.

How is it out of scope? This is new code we're talking about, not a bug fix to existing code.

The test code is adjacent to a bugfix to existing code.

Several other tests in this component use short sleeps to yield / allow geom to taste/settle. Adding some polling mechanism expands the scope of work required. The point of the test is to demonstrate the bug is fixed, not to be glorious code in its own right.

I can leave the test disabled if that's really your preference... you might also want to disable 6_test.sh and sync_error.sh.