vdev_mirror: avoid false scrub errors during sequential rebuilds

[Zaczero]

Motivation and Context

A sequential attach or replacement publishes its new child before the rebuild thread cancels an active scrub. Scrub reads can reach that child before its data has been copied and report checksum errors against a healthy device. This affects mirrors and replacing or spare columns beneath dRAID, including successive replacements overlapping the post-rebuild scrub.

Waiting for the scrub to finish before starting another sequential attach or replacement avoids the overlap.

Description

Keep the scrub's normal reads of intact children, but skip rebuilding children while the sequential rebuild is active. Snapshot that state in the mirror map so dispatch and completion agree even if the worker finishes between them. Persistent leaf rebuild markers can outlive the worker while missing-data DTLs remain; they must not keep children excluded from the subsequent scrub.

Preserve last-resort reads from DTL-marked children: the DTL covers a transaction range, and an individual block may already have been copied. A nested column read without a block pointer cannot authorize repairs from those bytes. Leave repairs to the rebuild worker or checksum-owning parent.

A preparatory commit makes scrub, resilver and replacement waits fail on timeout, and makes verify_pool require a completed scrub with zero errors.

How Has This Been Tested?

The tests passed in an isolated Linux debug guest:

• scrub_rebuild covers ordinary mirror attachment and a dRAID replacement beneath an established distributed spare. It checks scrub cancellation, a clean subsequent scrub and data after export/import.
• scrub_rebuild_residual leaves failed writes on an online replacement after worker completion. It requires the first scrub to finish with zero errors, then verifies the data after export/import with another child offline and no intervening healing pass. Three data columns ensure that the changed pattern also changes parity.
• ztest holds a real rebuild and submits scrub reads through the normal I/O pipeline. It varies dRAID parity, data width and sector size. With source read failures exhausting parity, recovery requires a valid but DTL-marked replacement copy. A corrupted candidate must be rejected without overwriting the original; parent-verified repairs remain possible. The scenario runs during initialization, before the concurrent workload, so its scan suspension and fault injection remain isolated.

Negative controls check the need for active-rebuild state, last-resort reads and raw-read repair suppression.

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[behlendorf]

We shouldn't need to add another field for this. The mm->mm_rebuilding field already indicates if there's an active rebuild, and each mirror child stores a similar mc->mc_rebuilding boolean. What we want is for the scrub IO selection logic to skip any children which are known to be rebuilding, while still reading from all online and intact children. This is untested but I believe should do the trick:

diff --git a/module/zfs/vdev_mirror.c b/module/zfs/vdev_mirror.c
index 844889af1e..83cb725e67 100644
--- a/module/zfs/vdev_mirror.c
+++ b/module/zfs/vdev_mirror.c
@@ -634,6 +634,14 @@ vdev_mirror_io_start(zio_t *zio)
                                        continue;
                                }
 
+                               /* Don't issue ZIOs to rebuilding children */
+                               if (mc->mc_rebuilding) {
+                                       mc->mc_error = SET_ERROR(ESTALE);
+                                       mc->mc_skipped = 1;
+                                       mc->mc_speculative = 1;
+                                       continue;
+                               }
+
                                mc->mc_abd = first ? zio->io_abd :
                                    abd_alloc_sametype(zio->io_abd,
                                    zio->io_size);

[Zaczero]

Updated to use your suggested child-skipping approach, keeping scrub reads of intact children.

I retained the active-rebuild snapshot because mm_rebuilding tracks leaf markers that can remain set after the worker finishes. Skipping based on those markers alone also skips the replacement during the subsequent scrub. The residual-data test reproduces this: the scrub repairs nothing, and importing with another child offline then fails.

[behlendorf]

Actually, looking at this again I believe there's a better approach. The core issue is the small overlap between when a rebuild thread completes and the scrub starts. Rather than modify the mirror code to tolerate this; we should do the opposite. That is, 1) update the scrub code to defer the handful of txgs until the rebuild thread exits to prevent the overlap, and 2) ASSERT0P(vd->vdev_top->vdev_rebuilding) in the mirror scrub path.

The point of automatically kicking off the scrub after a sequential rebuild is to verify the data on all reconstructed disks using the block checksums. If we skip reading online children during the scrub, then there's the risk that the scrub completes and fails to detect a block which was for some reason incorrectly reconstructed from parity.

We already skip issuing scrubs for a TXGs for variety of reasons, the code is built to allow this, so a small change like this should be sufficient:

diff --git a/module/zfs/dsl_scan.c b/module/zfs/dsl_scan.c
index 2d00faa78f..b6a3b6d968 100644
--- a/module/zfs/dsl_scan.c
+++ b/module/zfs/dsl_scan.c
@@ -4580,6 +4580,10 @@ dsl_scan_sync(dsl_pool_t *dp, dmu_tx_t *tx)
        if (!scn->scn_async_stalled && !dsl_scan_active(scn))
                return;
 
+       /* If a rebuild thread is still active, wait until it's done */
+       if (vdev_rebuild_active(spa->spa_root_vdev))
+               return;
+
        /* reset scan statistics */
        scn->scn_visited_this_txg = 0;
        scn->scn_async_frees_this_txg = 0;

[Zaczero]

continued in #19432

[behlendorf]

Then this shouldn't be needed since mm->mm_resilvering won't be set.

[Zaczero]

unexpected_errors can still trigger repairs when mm_resilvering is false. If the original read fails and we fall back to the replacement, a nested column read has no block pointer with which to verify those bytes.

Removing this guard makes the recovery test overwrite the original with corrupt replacement data before the parent rejects the checksum. I’ve therefore kept it; parent-verified repairs still work.

[amotin]

Another case of a huge ztest test, which make me shiver about its maintenance later.

[behlendorf]

Agreed, this looks unmaintainable. It might be a bit tricky, but surely we could setup this scenario as a ZTS test.

[Zaczero]

I'm still trying to figure out the testing side of things on ZFS; sorry for that. I will take a deeper look. Because of testing, I still have many of my PRs as drafts (or unpublished) and needing work, so it's not that I'm ignoring the problem!

[Zaczero]

Superseded by #19432