Add scratch object creation at the beginning of the reflow process

[fuporovvStack]

Motivation and Context

Description

How Has This Been Tested?

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)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.

[ahrens]

need some locking on this (or use atomic ops), since this can be called concurrently when each child completes its write.

[fuporovvStack]

TODO: FIX IT

[ahrens]

Are we sure that the boot region can't be used with RAIDZ?

Can we add some checks that the boot region is big enough that we won't have overlapping writes? I think it just has to be at least nchildren <<ashift.

[fuporovvStack]

When I considered boot sector as scratch object, the next calculations were used:
VDEV_BOOT_SIZE eq 3.5M (7ULL << 19) per child. Max number of childs is 255.
255*(7ULL << 19) = 935854080 bytes => max scratch object size

nchildren <<ashift in case of 4k ashift:
255 << 12 = 1044480 bytes

So, if I am correct it should be enough, of course excluding bootloader if it present, but I prefere do not accaunt it for now.
Later it will be needed to add logic which will detect the boot loader and check it size.
The simplest way will be to zero boot sector on pool creatrion phase and check non-zeroed data size to accaunt it.

Also, you mentioned "Copy first new_ncols^2 sectors to scratch object".
Could you please explain why new_ncols^2 and from other side "nchildren <<ashift". I cannot get it clear.

[ahrens]

Sure, I think the design doc talks about "Copy first new_ncols^2 sectors to scratch object". This is the same as each device having "nchildren << ashift" bytes copied to it. Since each device has nchildren sectors, and there are nchildren devices, we have nchildren * nchildren = nchildren^2 sectors copied total (across all devices).

The reason we need to copy this much data is so that after the scratch data, there is at least one whole stripe (aka one whole row, which is at most old_nchildren << ashift bytes) of not-yet-overwritten old data before the next old block to copy. We need this because we read this old data (from just before the "next old block to copy" when there's a stripe that crosses the "next block to copy" boundary, vre_offset_phys. This is implemented by the code in vdev_raidz_map_alloc_expanded() following this comment:

		 * If we are in the middle of a reflow, and any part of this
		 * row has not been copied, then use the old location of
		 * this row.

[ahrens]

in this case the roffset has to be < VDEV_BOOT_SIZE, and this goes negative, right?

[fuporovvStack]

Because of next expression in the zio_vdev_child_io():
if (vd->vdev_ops->vdev_op_leaf) {
ASSERT0(vd->vdev_children);
offset += VDEV_LABEL_START_SIZE;
}

[ahrens]

I think we can assert that progress has been made past the scratch size?

The code flow here of when we use the scratch object is a little hard for me to follow and verify that it's correct. I wonder if we could be more explicit about invalidating the scratch object once we make progress past its size.

[fuporovvStack]

I placed the logs to raidz_reflow_impl() and logged cases when device indexes and offsets are equal for reading and writing.
Had found that it does not work for now, because we can get sectors overwriting after the first vre_offset_phys update.
It is possible do not invalidate scrach object at all, at least until next expansion process, but it is needed to enable first metaslab.

[ahrens]

The problem that the scratch space is addressing is:

• we just started an expansion
• a single stripe has overwritten itself, such that it now has 2 sectors on the same disk
• that disk dies
• we now need to reconstruct this stripe, but we don't have enough of its sectors

To address this we need to ensure that when moving blocks, a stripe never overwrites itself, so that the stripe is always available intact at its old location.

So the place where we need to use the scratch copy is when doing a normal (or scrub/resilver) read. We don't need to use the scratch copy when doing the reflow read (although it shouldn't hurt).

I don't think we can test this case currently, because we don't handle disk failure during reflow. That's another thing on the to-do list. We need to pause the reflow and wait for the disk to be replaced and resilvered.

However, I think we can test this by reading blocks that are in the beginning of the disk, while we are reflowing the beginning of the disk. Today this will simply read from the old location, see this code in vdev_raidz_map_alloc_expanded():

		/*
		 * If we are in the middle of a reflow, and any part of this
		 * row has not been copied, then use the old location of
		 * this row.
		 */
		int row_phys_cols = physical_cols;
		if (b + (logical_cols - nparity) > reflow_offset >> ashift)
			row_phys_cols--;

I think that if the expansion has overwritten a stripe with itself, the read will not get the right data (because the old location has been partially overwritten). We could test this by pausing the reflow in the very beginning, and then reading the block containing the partially-overwritten stripe. I think that @stuartmaybee is working on some code for pausing the reflow for testing purposes.

Note that in this test case, we actually still have all the data and we could write code to read this "split stripe" partially from the old location and partially from the new location. But that code doesn't exist, because it won't be needed once we solve the problem of handling resilvering while in the middle of reflow (by using the scratch copy).

[ahrens]

Oh, the reflow-pausing code is already there, zfs_raidz_expand_max_offset_pause

[ahrens]

Can we assert that the scratch size is entirely contained in the first metaslab?

[fuporovvStack]

Yep, it needed to be checked.

[fuporovvStack]

Ok, thanks a lot for your comments, seems like, I need some more time to understand it.
Also, plus and cons of storing scratch object in MOS as alternative of current implementation.

[ahrens]

@fuporovvStack I think that using the boot space can work, and is probably simpler than storing the scratch data in the MOS. I think that there's enough space in the boot region and it isn't typically used for boot blocks with RAIDZ. We just need to be able to verify those assumptions.

[fuporovvStack]

Close because is not actual.
See: openzfs@9310a69