Netflix doesn't use efi_system_reset() in our version of FreeBSD. Changing the signature is likely fine.

Put 'local' variables on the same line; just forgot to do this last revision.

I'm not familiar with this tool or the test anymore, and not interested in studying it again now. I'm sure your fix is fine, but I am not a good reviewer.

• comment the full behavior of 'local -' rather than the subset used in this function, per dteske
• Use the slightly more explicit 'set -o noglob', in preference to 'set -f', per jilles
• De-indent cases, to match existing style (per dteske and jilles)
• Drop case body statements to independent lines -- unlike in C, there is a lot going on in the 'case' itself

I didn't know we built a rescue savecore!

• set -f
• s/LINE/line/

Generally LGTM. Thanks for doing this.

LGTM. I still find the 'mirror' name confusing, but maybe that's a personal problem ;-).

Mostly LGTM, thanks.

Test code:

• Remove extraneous sleeps
• Check that generation ids compare in expectated order
• Add additional tests that mirror components are evicted during STARTING, prior to 'gmirror status', via geom reference counts.
• Use gmirror label -h and insert -h instead of gnop -o 512

Fix brain-o in cleanup leading to tty-read stall / kyua failure.

Add a basic test case showing we evict stale mirror components (on the basis of
genid, anyway).

In D18062#392769, @pho wrote:

I have started the tests.

• gmirrors -> mirrors in sysctl description
• drop g_mirror_ctl.c change for now

Move metadata kickout loop above quorum check.

In D14409#392398, @mmel wrote:

if you run 'head' on

**kernel**  compiled  without 'option CAPSICUM' then it fail with 'function not implemented'.

This isn't the leak, and radeon crashed on me too many times for non-memleak reasons. Now happily using the nvidia binary driver with the 610 GT I had lying around :-).

Some initial comments. I'm not necessarily a good reviewer to wait on for this, sorry. I'm unfamiliar with TPM and don't have time to study it now.

The new changes LGTM. The scope of this review is getting broader than I'd prefer, though. It'd be easier on me as a reviewer if any future scope increases went into a new review. (I understand that's more work for you as long as this portion is in flux, but maybe that's motivation for getting this batch committed first? :-))

In D18403#391655, @imp wrote:

Thanks for the feedback. I'll see how much I can include in my next round as some of them are asking for more structural changes beyond the scope of this set of changes (the suggestions are good, btw, just am keen to avoid too much scope creep).

Mostly looks good to me. A few suggestions but nothing show-stopping other than the "Unknown command" thing. Thanks!

In D18380#391393, @eugen_grosbein.net wrote:

There is nothing GEOM-specific in this ioctl and 'G' in the name seems to be kind of mistake in the retrospection (it was not 11 years ago).

In D18380#391248, @eugen_grosbein.net wrote:

First, DIOCGDELETE is not GEOM-specific ioctl()

• Restore stale component kick-out to g_mirror_device_update, but relocate it *prior* to determining whether or not we have quorum to RUN the mirrorset.
• Perform sanity check assertions re: accumulated sc_genid and sc_syncid at the same time.
• For now, continue to kill mismatched md_all components *of the quorum genid*. Newer genid md_all components will override mirrorset configuration at taste time.
• Skip stale mirrors during g_mirror_add_disk prior to attempting to refresh from them.
• Remove stale component kick-out from g_mirror_add_disk. As Markj@ points out, g_mirror_add_disk synchronously invokes g_mirror_disk_update and g_mirror_device_update via g_mirror_event_send() in the middle of g_mirror_add_disk. So we might as well centralize back in g_mirror_device_update.
• Simplify early exit logic in g_mirror_refresh_device(), per markj@.

• Fix indentation
• Common subroutine for softc field initialization from md

No problem!

Mark might be on to something. I went back and looked at my syslog from before the system totally hung and guess what? After a bunch of swap_pager_getswapspace spam (seriously, 62567/69256 log lines between Nov 19 and 24 are this message), and buried a little (it's not in the bottom 100 lines or so prior to the shutdown due to more swap_pager lines), we see:

In D18327#389053, @jmd wrote:

Thanks for figuring this out!

We'd prefer to have the TTM patch directly in our github to forward carry it easily and also apply it to all existing branches and ports - could you make a PR there for the TTM change? If not, I'm happy to do that.

In D18327#389052, @markj wrote:

it's a last resort mechanism, as opposed to the regular PID-controller pagedaemons

It should run regularly (with a 10s period) whenever the pagedaemon is attempting to reclaim pages.

In D18062#389219, @markj wrote:

Ideally this would come with a regression test.

Works for me, thanks. Could we use a similar trick for userspace asserts?

LGTM!

LGTM modulo nits and assuming that test case will be committed shortly.

I like the change. All of my comments describe existing issues in the code, nothing you introduced in this change. Maybe they can be addressed separately.

Looks great to me, thanks!

I'm not a fan of the general concept. As far as execution goes, I don't believe stdout from rc scripts is logged? So someone would have to be watching boot to notice this? If we must do this, put it in a log file.

The code and documentation look good; I'm just not sure if we want a new API for this, or if changing taskqueue_drain_all()'s behavior to match this would be unobjectionable.