vdev_initialize: bound and snapshot chunk size - #19096
matthiasgoergens wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The remaining findings are minor documentation/spelling issues and do not affect the correctness of the chunk-size snapshotting and clamping behavior.
Pull request overview
This PR makes zfs_initialize_chunk_size consistent and safe to use during vdev initialization by sampling and normalizing it once per device initialization thread, preventing mismatches between the ABD allocation size and subsequent write sizes if the tunable changes mid-run.
Changes:
- Snapshot
zfs_initialize_chunk_sizeonce pervdev_initialize_thread()and reuse it for both ABD allocation and write splitting. - Clamp the effective chunk size to
SPA_MINBLOCKSIZE..SPA_MAXBLOCKSIZEand round it down to a 512-byte multiple. - Document the “sample-on-thread-start” behavior (including resume) in
zfs(4).
File summaries
| File | Description |
|---|---|
| module/zfs/vdev_initialize.c | Snapshots, clamps, and reuses a per-thread chunk size for buffer allocation and write sizing. |
| man/man4/zfs.4 | Documents clamping/rounding and when tunable changes take effect. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
19c8a91 to
312589b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Add regression coverage for bounds, rounding, and sampling across suspend/resume.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| chunk_size = MIN(MAX(chunk_size, SPA_MINBLOCKSIZE), | ||
| SPA_MAXBLOCKSIZE); | ||
| chunk_size = P2ALIGN_TYPED(chunk_size, SPA_MINBLOCKSIZE, uint64_t); |
There was a problem hiding this comment.
Added zpool_initialize_chunk_size.ksh. It exercises 0, 1, 513, and 16 MiB + 1, checks the reported initialisation state and error count, changes the tunable while initialisation is active and suspended, and completes the resumed run. The unpatched zero-sized case reproduces the allocator panic.
312589b to
a25b811
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The functional test has a moderate polling issue that can falsely fail when initialization completes before progress is observed.
Review details
Suppressed comments (1)
tests/zfs-tests/tests/functional/cli_root/zpool_initialize/zpool_initialize_chunk_size.ksh:52
wait_for_initializetreats a percentage line as the only indication that initialization started. The test deliberately includes the 16 MiB case, which can finish the 1 GiB file before a poll observes it; a completed vdev no longer has aninitialize_progresspercentage, so this reports “Initializing did not start” for a successful run. Pollinit_stateand accept bothACTIVEandCOMPLETEinstead.
[[ -n "$(initialize_progress $TESTPOOL $VDEV)" ]] && return
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
amotin
left a comment
amotin
left a comment
There was a problem hiding this comment.
General motivation is good. But I think instead of explicitly passing the chunk size we could just use the allocated and filled ABD size, so that there could never be a discrepancy between allocation and access. Also I have feeling that somewhere there should be the vdev's ashift as a limit, not SPA_MINBLOCKSIZE.
a25b811 to
243d498
Compare
|
Thanks. Updated along both lines: the allocation is clamped and rounded against |
243d498 to
0fd6b95
Compare
The initialisation buffer allocation and write loop read zfs_initialize_chunk_size independently. A concurrent tunable change can make the write size inconsistent with the allocated buffer. Snapshot and normalise the value once per device initialisation thread. Clamp it to the configured vdev sector size and maximum block size. Derive every write split from the allocated ABD size. This makes the allocation the single source of truth. Document the effective bounds and when tunable changes take effect. Add regression coverage for bounds, unaligned values, tunable changes, and suspend/resume. Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
0fd6b95 to
e769833
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The regression tests need stronger coverage for round-down alignment and tunable snapshot timing.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
tests/zfs-tests/tests/functional/cli_root/zpool_initialize/zpool_initialize_chunk_size.ksh:75
- This helper only observes the state flag, but
vdev_initialize()sets that flag before creating the worker thread (module/zfs/vdev_initialize.c:663-665). Either caller can therefore change the tunable before the new thread allocates its ABD or issues a delayed write, allowing the snapshot behavior to go untested; wait for an initialization I/O to complete (for example, for the reported initialized-byte count to increase) before changing the tunable.
while (( retries-- > 0 )); do
state=$(initialize_stat init_state 2>/dev/null) || state=
[[ "$state" == "ACTIVE" ]] && return
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
|
|
||
| log_must truncate -s $((MINVDEVSIZE * 4)) "$VDEV" | ||
|
|
||
| for chunk_size in 0 1 $((sector_sz - 1)) $((max_chunk_sz + 1)); do |
zfs_initialize_chunk_sizeis read separately when allocating the initialisation buffer and when splitting writes. Changing it during initialisation can therefore make the write size inconsistent with the buffer. The value also needs to satisfy the maximum block-size limit and the target vdev's physical I/O alignment.Sample and normalise the value once while allocating and filling the per-device ABD. Clamp it to the range from the configured vdev sector size (
2^ashiftbytes) to 16 MiB, then derive every write split fromabd_get_size(data). This makes the allocated ABD the single source of truth even if the tunable changes while the thread is running.Changes take effect when a device initialisation thread next starts, including after initialisation is resumed. The regression test covers lower and upper bounds, sector-unaligned values, active tunable changes, and suspend/resume; controlled I/O delays ensure that the active state is observed before suspension.