[Zaczero]

Motivation and Context

A sequential rebuild can read a block while the syncing txg rewrites it in place. Disabling metaslab allocations and waiting for pending allocations does not cover these rewrites: they reuse the block's address and allocate nothing.

On dRAID, a read can combine columns from before and after the rewrite, producing contradictory parity and withholding a repair. The same ordering can copy an older block over a newer copy on the rebuild destination.

This affects sequential rebuilds from 2.0.0. Replacing without -s avoids the race. The dRAID repair omission was reproduced; the newer-copy overwrite follows from the ordering but was not observed.

Description

After disabling metaslab allocations, also wait for the txg currently syncing. Later txgs cannot allocate from that metaslab, so they cannot rewrite new blocks there in place. Keep the existing wait for pending allocations.

The added wait costs at most one txg sync per metaslab and returns immediately when none is syncing. It also prevents healthy rows from being rejected by #19121's dRAID error accounting.

How Has This Been Tested?

With only this wait toggled in an instrumented build, redundancy_draid_spare3 withheld a repair for contradictory parity in 5 of 80 runs without the fix and none of 80 with it. The post-rebuild scrub was disabled; every run's final scrub found no checksum errors.

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]

Thank you! We've been seeing the rare checksum errors in the redundancy_draid_spare3 causing failures since the test was add and it was never clear why the ->ms_allocating was insufficient. A rewrite in the syncing txg while there are no outstanding allocations would explain it.

@andriytk spent some time chasing this subtle failure as well and I'm sure he'd be interested in reviewing the fix.

[andriytk]

I thought we've fixed it at #18473 where we are waiting for last rebuilt transactions of a metaslab to be sync-ed before enabling allocations from it. But this additional fix to wait for in-place re-writes before starting the rebuild of a metaslab also makes sense.

I only didn't quite understand from the description how it was tested. And I also didn't get this: "It also prevents healthy rows from being rejected".

[Zaczero]

@andriytk

Yes, this covers the in-place rewrites before rebuilding starts.

For testing, I instrumented the rebuild path to record repairs skipped because of inconsistent parity, then ran redundancy_draid_spare3 80 times with the added wait and 80 times without it. Those skips occurred in 5 runs without the wait and none with it. The automatic post-rebuild scrub was disabled; the final verification scrubs found no checksum errors.

By “healthy rows being rejected,” I meant that the rebuild could read some columns before a rewrite and others after it. That mixed view has inconsistent parity, so the repair is skipped even though the stored data isn’t damaged.

[andriytk]

Thanks, that makes sense now. I only still wonder, why scrubs didn't find any issues in cases when the repairs were skipped...

[Zaczero]

The in-place rewrite also writes to the replacement device, independently of the rebuild. So even if the rebuild skips its repair, the rewrite can leave the replacement with the correct contents, and the later scrub has nothing wrong to find.

[andriytk]

Right, that makes sense. But there's still race possible between in-place re-writes and rebuilds which can result with wrong data, especially when the parity cannot verify the integrity of data (for example, when the amount of missing columns in the parity group equals the amount of parity columns), and that's why we need this fix. Thanks!

See also:

zfs/module/zfs/vdev_raidz.c

Lines 3308 to 3318 in 22013c3

	/*
	* If we have only ndata columns, the data integrity will
	* be checked by the checksums normally, but not in case
	* of rebuild when we don't have checksums. In this case,
	* we add ZIO_FLAG_SPECULATIVE and try to not spread
	* unverified data. For example, when the target vdev happens
	* to be the mirroring spare vdev, we would repair only that
	* child in it which is being rebuilt.
	*/
	if (!parity_verify && zio->io_priority == ZIO_PRIORITY_REBUILD)
	add_flags |= ZIO_FLAG_SPECULATIVE;

and:

zfs/module/zfs/vdev_mirror.c

Lines 672 to 685 in 22013c3

	/*
	* When sequentially resilvering and the integrity of the data
	* is speculative (ZIO_FLAG_SPECULATIVE), issue write repair IOs
	* only to the vdev which is being rebuilt. Existing data on
	* other children must never be overwritten with unconfirmed
	* data to avoid unrecoverable damage to the pool.
	*/
	if ((zio->io_priority == ZIO_PRIORITY_REBUILD) &&
	(zio->io_flags & ZIO_FLAG_IO_REPAIR) &&
	!(zio->io_flags & ZIO_FLAG_SCRUB) &&
	(zio->io_flags & ZIO_FLAG_SPECULATIVE) &&
	mm->mm_rebuilding && !mc->mc_rebuilding) {
	continue;
	}