Avoid panic with recordsize > 128k, raw sending and no large_blocks

[gamanakis]

Motivation and Context

Closes #12275.

Description

The current codebase does not support raw sending buffers with block
size > 128kB when large_blocks is not active. This can happen in the
codepath dsl_dataset_sync()->dmu_objset_sync()->zio_nowait() which
calls back dmu_objset_write_done()->dsl_dataset_block_born(). If
dsl_dataset_sync() completes its run before dsl_dataset_block_born() is
called, we will end up not activating some of the necessary flags, while
having blocks based on those flags written in the filesystem. A
subsequent send will then panic.

Fix this by directly deciding in dmu_objset_sync() whether these flags
need to be activated later by dsl_dataset_sync(). Instead of panicking
due to a NULL pointer dereference in dmu_dump_write() in case of a send,
print out an error message. Also during scrub verify there are no
contradicting filesystem flags.

How Has This Been Tested?

Running the replicator in #12275 does not panic.

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)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil 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.

[scineram]

with block size > 128kB when large_blocks is not active

At first I thought this was nonsense. Is the issue here sending large blocks raw but without -L flag? Then shouldn't it be implied like the -c flag?

[gamanakis]

with block size > 128kB when large_blocks is not active

At first I thought this was nonsense. Is the issue here sending large blocks raw but without -L flag? Then shouldn't it be implied like the -c flag?

Right, sending raw should always imply large blocks if the recordsize is >128kB. However, issue #12275 describes a situation where raw sending panics because large_blocks is not active on the dataset being sent.

[gamanakis]

I think there may be a better way to report the error.

99c0720: report the error properly
3764406: Teach zpool scrub to check if buffers > 128kB exist. If they do check if large_blocks is active. If it isn't activate it.

[gamanakis]

@pcd1193182 would you mind taking a look at this? Thank you.

[gamanakis]

@amotin thanks for taking a look! In the current codebase I cannot see how the activation code in dsl_dataset_block_count() could be bypassed. I still believe though that a scrub should be checking for this possibility.

[peterjeremy]

I have several comments on the proposed change:

• I agree that the patches to lib/libzfs/libzfs_sendrecv.c and module/zfs/dmu_send.c are worthwhile to protect against the system panicing is this case.
• I think the patch to module/zfs/dsl_scan.c to correct the dataset inconsistency is worthwhile but the need for the fix should be logged - IMHO, inconsistencies should not be silently corrected.
• The proposed patch doesn't actually fix the bug reported in Linux 'kernel NULL pointer dereference' on zfs send of a specific encrypted snapshot #12275 - it just prevents it panicing.

According to the code, dsl_dataset_block_born() will mark SPA_FEATURE_LARGE_BLOCKS as needed if a large block is written and the subsequent dsl_dataset_sync() will then mark the feature as "active" on the dataset. In some cases, this is failing and the actual bug exists somewhere in that code path.

[gamanakis]

I am putting this one on hold, till we have a better understanding of the underlying problem.

[gamanakis]

302e0f3: rebased to master, and implemented a new logic: the current codebase does not support raw sending buffers with block size > 128kB when large_blocks is not active. This can happen in the codepath dsl_dataset_sync()->dmu_objset_sync()->zio_nowait() which calls back dmu_objset_write_done()->dsl_dataset_block_born().

If dsl_dataset_sync() completes its run before dsl_dataset_block_born() is called, we will end up not activating some of the necessary flags, while having blocks based on those flags written in the filesystem. A subsequent send will then panic.

Fix this by directly deciding in dmu_objset_sync() whether these flags need to be activated later by dsl_dataset_sync(). Also during scrub verify there are no contradicting filesystem flags.

[jumbi77]

Thanks for this fix! Just a (dump) question: Does ist make sense to write a test case to test if the necessary flags are implicitly activate when sending a snapshot?

[gamanakis]

Does ist make sense to write a test case to test if the necessary flags are implicitly activate when sending a snapshot?

That's a good question; the corner case this PR resolves is however difficult to trigger in a test.

[gamanakis]

51341c2, 32107e6: rebased to master.

[gamanakis]

3dfb54d: Rebased to master.