zhack: add "mmp reclaim" to recover a pool stranded by MMP

[mkhllr]

Follow-up to #18855, which @behlendorf asked for when he merged it.

The problem

#18855 made the MMP uberblock claim require a good write to every mirror leg
the pool config still expects to be present. That is what stops a host
claiming a pool under partial visibility, but it leaves one case with no way
out. When a host fails together with the mirror legs attached to it, the
surviving labels still describe those legs as healthy, so every later import
demands writes to legs that nobody can make. The claim can never be satisfied
and the pool cannot be imported again by any host.

@arturpzol hit this on a real two-node HA cluster while testing #18855. Each
mirror there is a local disk plus an iSCSI disk from the peer, so a dead node
takes its legs with it and the config still lists them present. His logs show
req_writes=6 good_writes=3, refused. That is the ordinary failover in that
topology, and the fix alone declines it.

What this adds

zhack mmp reclaim <pool> imports the pool once with the claim's required
write count relaxed for the mirror legs this host cannot open, marks those
leaves offline so that the ordinary imports which follow succeed, and exports.

The pool comes back DEGRADED with those legs offline, and zpool online
returns each one when the hardware does.

Design notes

Offline instead of removed. Offline persists unconditionally, is already
excluded from the claim by #18855, has zpool online as a first class
inverse, and does not overload hotplug semantics. vdev_not_present, which is
what a failed open actually sets, is not usable here: it is recomputed on
every import, so the pool would need zhack on every failover rather than once.

No existing flag covers this. On the stranded case (2-way mirror, one leg
gone with the peer) -f, -f -o multihost=off, -f -o multihost=off -m and
-fFX -o multihost=off all refuse with req_writes=2 good_writes=1. Setting
multihost=off at import does not help, because
spa_activity_check_required() decides from the on-disk uberblock, which
still has MMP active, before the property is applied.

Why not ZFS_IMPORT_SKIP_MMP. That disables the whole activity check, as
zdb uses it. Here the activity check has to be kept and only the write count
relaxed, so that a competing importer is still caught.

The relaxation cannot exist in the kernel. mmp_claim_relaxed is declared
under #ifndef _KERNEL, and module/Kbuild.in builds the module with
-D_KERNEL, so the symbol is compiled out of every kernel build. libzpool
does not define _KERNEL, so zhack gets it. This follows the
zfeature_checks_disable pattern zhack already uses around the same import,
and is stronger, since that flag does exist in the kernel.

Only mirror legs are forgiven, and exactly those are offlined. A raidz or
draid vdev is required as parity+1 in aggregate instead of one write per
member, so an absent member does not raise the requirement and is left alone.
The claim holds there while parity+1 members stay writeable; a narrower raidz
that has lost more than that is stranded and out of scope for this tool.

What this does not do

The relaxed claim still catches a competing importer that shares any
visibility with us, which is the common operator error. It cannot catch a live
peer whose legs are all invisible from here, because the claim write and the
re-read only ever touch reachable legs. That is inherent to any write and read
scheme under disjoint visibility, and no change to this code can close it.

The fencing is therefore load-bearing. This is a manual recovery that assumes
the peer has been confirmed down, and the man page says so.

Testing

New ZTS test mmp/mmp_zhack_reclaim, six scenarios:

1. a stranded pool is recovered and the claim then accepts it
2. a live host sharing one leg with us is still refused
3. both top level vdevs are counted after a recovery
4. a pool without multihost is left alone
5. a log vdev leg is not touched
6. a raidz2 member is not touched

Every recovery assertion re-imports as a third hostid on purpose. zhack
exports cleanly under its own hostid, so importing again as the same host
takes the exported-and-matching-hostid path in spa_activity_check_required()
and skips the activity check entirely, which would leave the claim
unexercised. The assertions read req_writes and good_writes from the
claim's own dbgmsg line, which 2017622 added. That is the only observable of
the arithmetic, at the cost of coupling the test to a debug message this
change does not control.

The test commit also fixes mmp_pool_destroy(), whose bare pgrep zhack
matched the new test's own comm (mmp_zhack_recla) and killed it.

Verified on a VM harness with the loaded module's srcversion asserted equal to
the tree's, so the results cannot come from a stale zfs.ko. Scenario 6 was
checked against a build with the mirror-only restriction removed, where it
fails as intended. The full mmp group passes, including mmp_active_import
and mmp_concurrent_import, which share the helper that changed.

One path is not exercised. When an absent leaf holds the only copy of some
data, vdev_offline() returns EBUSY, and the tool reports the leaf, leaves it
online, warns that a later import from another host will still be refused, and
exits non-zero. I could not stage that state reliably from userspace, so I am
flagging it as untested. The messages and the exit status are static, and the
man page documents the outcome.

Documentation

@arturpzol asked in #18855 for the recovery flow to be written down.
man/man1/zhack.1 documents the subcommand, the residual risk, the fencing
requirement, and the degraded aftermath.

Worth calling out separately for release notes: a pool that imported on 2.3.5
after this class of failure will refuse on 2.3.8, and the error text
(cannot import 'tank': pool is imported on host '<unknown>' (hostid=0).)
does not point anywhere useful when the truth is that the claim wanted more
writes than it got.

[mkhllr]

Both done, thanks.

The comment was mine to fix. 8cdd9b2 replaced the fixed two writes per
mirror with one write per leg the config expects present, and I left the
comment above the check describing the old rule.

The breadcrumbs are implemented as you laid out: keep the EIO from
mmp_claim_uberblock() distinct in spa_activity_check_claim(), give it its
own case in spa_ld_activity_result(), return the expected EREMOTEIO the way
the ENXIO case does. Worth recording why it mattered, since it is worse than
a missing message. The old console line for this case was

WARNING: pool 'tank' activity detected, aborted import during claim

so an operator whose peer died together with its legs was told another host
holds the pool, and went looking for a host that was not there. Now:

WARNING: pool 'tank' could not claim every device the config expects present, aborted import during claim; if a device is permanently gone see 'zhack mmp reclaim'

One judgement call I would rather you made than me. I read "before it
returns the expected EREMOTEIO" as userspace staying exactly as it is, so the
load info still carries MMP_STATE_ACTIVE and EREMOTEIO and only the console
message is new. zpool import -f still prints

cannot import 'tank': pool is imported on host '<unknown>' (hostid=0).
Export the pool on the other system, then run 'zpool import'.

which is arguably the more misleading of the two, and it is the line
@arturpzol's HA testing actually hit. Changing it means changing what
userspace sees, which is a bigger step than you asked for, so say the word
and I will carry the distinction through.

Two things I would rather flag than have you find. EIO here means the claim
did not get enough good writes, which is usually an absent device but is also
what a transient write failure on a present leg produces, so the message is
worded for the shortfall and only conditions the zhack advice on the device
being gone. And this is the first test in the suite to read dmesg; it is in
commands.cfg, but I used it because cmn_err() is printk on Linux and never
reaches dbgmsg, so the dbgmsg idiom the rest of the test uses for the claim
counters cannot see this message at all.

The ZTS case asserts both directions, since a console message that quietly
stops being emitted is a test that quietly stops testing: the shortfall must
be reported, and it must not be reported as activity detected. Differential on
the VM harness with the module srcversion asserted against the tree on each
arm: with the change the mmp group is 19/19, with only spa.c reverted the
case fails in 24 seconds on the first assertion.

Kept as its own commit so it can be reviewed or dropped without touching the
subcommand.

[behlendorf]

Changing it means changing what userspace sees, which is a bigger step than you asked for, so say the word
and I will carry the distinction through.

Thanks for making me look at this again. Plumbing the error through to userspace actually doesn't look bad at all, and the more useful an error we can report the better. We just need to be careful to handle the user/kernel compatibly change when running mismatched versions. Since zpool doesn't even look at ZPOOL_CONFIG_MMP_RESULT today that does make it a little easier.

EIO here means the claim did not get enough good writes, which is usually an absent device but is also what a transient write failure on a present leg produces

Yeah, that should be a rare case but I don't love lumping them together. Since we want to pass this information along to userspace let's split these cases in to distinct errnos and error messages. Now is the time to do it avoid future compatibility headaches.

this is the first test in the suite to read dmesg; it is in commands.cfg, but I used it because cmn_err() is printk on Linux and never reaches dbgmsg, so the dbgmsg idiom the rest of the test uses for the claim counters cannot see this message at all.

I'd rather not scrape the dmesg output for the expected error. Happily, since zpool import will be reporting a better error message we can check that instead.

[mkhllr]

Agreed on splitting the errnos now. Here is the shape I plan to build, with
two things I would rather you decided than have me guess at.

Splitting the two causes. The distinction is available where the claim is
made, because a write is only ever issued to a leaf that passes
vdev_writeable(). An absent or unopenable leg is in VDEV_STATE_CANT_OPEN,
which vdev_is_dead() covers, so it is never written at all, while a
transient failure on a present device is issued and comes back with an error
in the done callback. Counting issued writes alongside good ones separates
them:

• a shortfall in issued writes means a device the config expects present was
  never writable. That is the persistent case, and the one zhack mmp reclaim recovers. ENODEV.
• enough issued but a shortfall in good ones means the writes went out to
  present devices and failed, which a retry may clear. EIO.

ENODEV is 19 on both Linux and FreeBSD and is unused on this path. ENXIO was
the other candidate and is not available, since it already means "system
hostid not set" here.

First thing I would rather you called. req_writes is accumulated across
every top level together, and a raidz contributes only parity+1 while its
writes go to all of its writeable leaves. So a wide raidz's surplus can cover
a mirror's missing leg in the same pool, and under the rule above that pushes
a genuinely absent device into the EIO bucket. That surplus already lets the
claim itself pass today, so this is not new, but the errno split is the point
where it starts having a visible name. Per-top-level accounting would fix
both; it is a bigger change than you asked for and it changes when the claim
passes, so I have not assumed it. Which would you prefer?

Getting it to userspace. The cause travels in ZPOOL_CONFIG_MMP_RESULT
and nothing else changes: the load info still carries MMP_STATE_ACTIVE, the
ioctl still returns EREMOTEIO, and libzfs reads the result to choose the
message. do_import() needs no change, since the claim never runs during
tryimport. Today that path prints

cannot import 'tank': pool is imported on host '<unknown>' (hostid=0).
Export the pool on the other system, then run 'zpool import'.

and for the ENODEV case it would instead print

cannot import 'tank': the MMP claim could not be written to a device
the pool configuration expects to be present.
If the device is permanently gone, recover with 'zhack mmp reclaim'.

and for EIO

cannot import 'tank': I/O errors occurred while writing the MMP claim.
Clear the device errors, then retry 'zpool import'.

Anything that is neither keeps today's text, so a kernel that does not report
a cause reads exactly as it does now.

On mismatched versions. An old zpool against a new kernel sees exactly
what it sees today, because the errno and MMP_STATE are unchanged and
nothing in lib or cmd reads MMP_RESULT. The one reader anywhere is
mmp.kshlib, which greps the ZFS_LOAD_INFO_DEBUG dump for values this change
does not touch. A new zpool against an old kernel falls back to the current
message, which is the honest answer, since an old kernel does not know which
case it was.

I looked at giving this its own mmp_state_t and decided against it. An old
zpool seeing an unknown state would reach zfs_error_aux() with an aux
buffer that is only filled in for the two states it knows, so the message
would be whatever was on the stack. Keeping MMP_STATE_ACTIVE for a case
where nothing is actually active is a deliberate fiction for the benefit of
old binaries, so I will say so in the commit message rather than leave it to
be found.

Second thing for you. There is an existing asymmetry in the counting:
req_writes is accumulated for every leg the config expects, while good
writes are only counted for top levels that have a metaslab array. A freshly
added and never synced top level therefore cannot satisfy its own
requirement. Whether that surfaces as ENODEV or EIO depends on whether I gate
the issued count the same way, and neither label is really right for it. I
would rather not quietly pick one. Same behaviour as today either way.

Tests. The dmesg scraping goes, replaced by an assertion on what zpool import prints, keeping both directions: the shortfall has to be reported, and
it must not be reported as another host holding the pool.

The EIO branch will ship without ZTS coverage. I could not provoke it through
the injection framework: registering a handler needs an imported pool, and
holding one takes spa_inject_ref, so the export the test then needs comes
back EBUSY. Device fault injection would not help even
if that were solved, since it skips the label offsets deliberately and so
never sees the claim's writes at all.

It is doable outside that framework, with a write-erroring device under one
leg, dm-flakey or similar, armed while the pool is exported. I have not built
it because there is no dmsetup anywhere in the suite today and it would be
Linux only, so it is new infrastructure rather than a test. Say if you would
rather have it than the gap.

[behlendorf]

So a wide raidz's surplus can cover a mirror's missing leg in the same pool, and under the rule above that pushes a genuinely absent device into the EIO bucket.

That's true, although in this case I would argue the EIO is actually preferred. With the addition of a raidz vdev, and its surplus writes, it's safe to relax the all-known-mirror-legs-are-present check. All primary storage top-level vdevs must be present to import the pool and since a raidz doesn't suffer from the mirror split issue it's sufficient to prevent concurrent imports. A shortfall of writes to the mirror isn't inherently a problem in this configuration. So I don't think there's a need for per-top-level accounting.

A freshly added and never synced top level therefore cannot satisfy its own requirement.

How do you mean? If a top-level vdev was added, and the node crashed before the pool config was sync'd, then the pool will use the MOS config from the last intact txg. I don't see the issue here.

Given how tricky the setup for EIO case I think its reasonable to skip the test coverage there. The test suite doesn't use dm-flakey but it does use the scsi-debug kmod in some test to simulate similar failures. That might be workable, but it would be Linux specific and we can live with a gap here.

Everything else sounds good.

[mkhllr]

Thanks, both of those settle it.

The never synced top level. You are right and I was reading too much into
it. What I saw is that req_writes is accumulated for every top level that is
not a log, spare, cache, hole or indirect vdev, with no test on the metaslab
array, while a good write is only counted when
zio->io_vd->vdev_top->vdev_ms_array != 0. On paper that lets a top level be
required without being able to satisfy its own requirement. But the claim runs
from spa_ld_trusted_config(), after the MOS config has replaced the label
config, so the tree it walks came from a synced txg exactly as you say, and I
see no way to reach the gap from there. I have left the check alone and I am
not proposing a change there.

Per top level accounting. Agreed, dropped. The point that all primary top
levels must be present to import, and that a raidz cannot split the way a
mirror can, is the part I was missing.

What the split looks like. mmp_claim_uberblock() now counts the writes it
issues alongside the ones that succeed. A leaf which cannot be written is never
issued one, so too few issued means a device the config expects is absent, and
that returns ENODEV. Enough issued but too few good means the writes reached
present devices and failed, and that stays EIO. The issued count is gated the
same way the good count is, so the two describe the same set of leaves. Both
still return EREMOTEIO to userspace and both keep MMP_STATE_ACTIVE, with the
cause in ZPOOL_CONFIG_MMP_RESULT, so an older zpool prints what it prints
today.

For the ENODEV case zpool import now says:

cannot import 'tank': the multihost claim could not be written to a device
the pool configuration expects to be present.
If the device is permanently gone, recover with 'zhack mmp reclaim'.

One thing worth your attention. This changes a message for behaviour that
is already merged, as well as for the new path. Cases 3 and 5 of
mmp_degraded_import.ksh, from #18855, each exercise a mirror leg this host
cannot open, which is exactly the ENODEV case, and both asserted on the old
"pool is imported on host" text. I have updated them in the same commit, and
the man page example along with it. So an operator on a released version who
hits the case that motivated all of this will see different words after this
lands. That is the intended effect, but it is a user visible change to shipped
behaviour and I would rather you saw it stated than found it in the diff.

The EIO branch ships without ZTS coverage, as you suggested. Thank you for
the scsi-debug pointer, that is a better starting place than the dm-flakey
idea I had, and I will look at it separately rather than hold this up.

The dmesg scraping is gone. The test asserts on what zpool import prints
instead, in both directions: the shortfall has to be reported, and it must not
be reported as another host holding the pool.

Tested on Linux 6.8 with --enable-debug. The mmp group is 19 of 19,
including mmp_degraded_import and mmp_zhack_reclaim, with the loaded module
checked against the tree before and after the run. The branch merges cleanly on
master as of 45eb50a.

[behlendorf]

Thanks this looks great. I'm glad to see this corner case get so nicely resolved.