raidz: refuse too small replacements during expansion
While a raidz expansion is in progress, `zpool replace` of a raidz
child accepts a device that is too small. The resilver or the reflow
then writes past the end of the new device, tripping this VERIFY in a
debug build (or writing past the end of the device otherwise):
VERIFY3U(rc->rc_offset + rc->rc_size, <,
cvd->vdev_psize - VDEV_LABEL_END_SIZE) failed
PANIC at vdev_raidz.c:vdev_raidz_io_start_write()
Reproducer:
truncate -s 1G /var/tmp/d{1..4}; truncate -s 800M /var/tmp/small
zpool create tank raidz1 /var/tmp/d1 /var/tmp/d2 /var/tmp/d3
zfs set compression=off tank
dd if=/dev/urandom of=/tank/f bs=1M count=1200; zpool sync tank
zpool replace tank /var/tmp/d1 /var/tmp/small # "device is too small"
echo 1 > /sys/module/zfs/parameters/raidz_expand_max_reflow_bytes
[40 lines not shown]
ZTS: test too small raidz replacement during expansion
Add raidz_expand_012_neg, which pauses a raidz expansion and checks
that 'zpool replace' refuses a device too small for the not yet
reflowed, narrower part of the vdev, including after an export/import
with the expansion still paused. It then checks that a device of the
original size is accepted, and that the pool scrubs clean once the
expansion completes.
This currently fails: the too small device is accepted, and the resilver
or reflow then writes past its end (a VERIFY panic in
vdev_raidz_io_start_write() on debug builds). The fix follows in the
next commit.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Signed-off-by: Will Rouesnel <wrouesnel at wrouesnel.com>
Closes #19279
ZTS: let udev settle before zfs_copies destroys the volume
zfs_copies_006_pos fails now and then in CI right after unmounting
the ext2 file system on its volume:
cannot destroy 'testpool/testvol1': dataset is busy
ERROR: zfs destroy testpool/testvol1 exited 1
new_fs writes the volume and closes it, so udev's watch on zd* makes
it rescan the volume, and blkid holds the device open while it does.
The test waits for udev before new_fs but not after it, and a destroy
issued while that scan still has the volume open gets EBUSY. The
cleanup's destroy_dataset, which retries, succeeds a moment later.
do_vol_test() is shared with zfs_copies_003_pos, whose zpool create
also writes the volume.
Wait for udev with block_device_wait before the destroy and retry it
while it is busy, the way alloc_class_016_pos and destroy_dataset do.
The destroy must still succeed.
[3 lines not shown]
zap: Fix zap_trunc() to account next ptrtbl blocks
In zap_trunc(), a case involving the expansion of ptrtbl was overlooked.
As a result, freeblk could end up pointing out to a block that had been
allocated for next ptrtbl. A next leaf allocation would lead to ptrtbl
corruption.
The fix checks whether ptrtbl is being expanded and accounts the blocks
allocated for it to compute freeblk.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin at TrueNAS.com>
Signed-off-by: Alexander Stetsenko <alex.stetsenko at gmail.com>
Closes #19591
Closes #19693
Linux: unhash zero-link znodes when resuming after rollback
zfs_resume_fs() calls zfs_rezget() for every cached znode after a
rollback or a receive into a mounted filesystem. If zfs_rezget()
fails, the inode is unhashed so that a new object created with the
same number does not collide with it.
zfs_rezget() can also succeed for an object that has zero links in
the new state: a file that was open-unlinked when the snapshot was
taken and is still held open. It then sets z_unlinked and drops the
SA handle, but the inode stays hashed. The unlinked drain that runs
after resume finds no znode attached to the object, so it allocates a
new unhashed one and frees the object. When the object number is
reused, insert_inode_locked() finds the old inode and fails:
VERIFY0(insert_inode_locked(ip)) failed (-16)
PANIC at zfs_znode_os.c:641:zfs_znode_alloc()
The panicking thread holds an assigned transaction, so txg_quiesce
[13 lines not shown]
ZTS: test rollback with held open-unlinked files
A file that is open-unlinked when a snapshot is taken is a zero-link
object in that snapshot's unlinked set. If the file is still held
open when a mounted filesystem is rolled back to the snapshot,
zfs_rezget() succeeds for it, and the unlinked drain that runs after
resume frees the object while the old inode is still in use.
Add zfs_rollback_held_unlinked, which holds open-unlinked files across
a snapshot and a mounted rollback, then creates files until one reuses
a held object number. It also checks that a write through a stale
descriptor fails and does not modify the new file.
This currently fails on Linux: the create that reuses the object
number fails VERIFY0(insert_inode_locked(ip)) in zfs_znode_alloc()
with EBUSY, because the old inode is still hashed. The fix follows
in the next commit.
Co-Authored-By: Claude Opus 5.5 <noreply at anthropic.com>
[3 lines not shown]
ZTS: Race fallocate against a buffered append too
zpl_iter_write() takes no inode lock for a buffered write either, so
a buffered write that extends the file races fallocate the same way an
O_DIRECT one does.
dio_append_fallocate gains -b, which makes the append a buffered write
instead of an O_DIRECT one. fallocate_odirect_append runs each of the
extend, punch and zero modes with both, and names the buffered ones
buffered-MODE when they lose a round. The buffered runs use 1000
rounds instead of 100: their window is shorter, and with 100 rounds
some runs on master lost none.
On a debug build of master the test failed 10 of 10 runs, and the
buffered runs lost 21 to 952 of 1000 rounds in every mode of every
run. With the fallocate change in this PR it passed 10 of 10, and no
round was lost.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
[5 lines not shown]
Linux: do not truncate a concurrent extending write in fallocate
zpl_iter_write() takes no inode lock, so a buffered or O_DIRECT write
that extends a file updates the file size without holding it. A
fallocate that extends the file reads the size into olen first. If
the write grows the file after that, the fallocate truncates it back
to offset + len, the written data is gone, and the application was
told its write succeeded.
The extend path set the new size with zfs_freesp(zp, offset + len, 0,
0, TRUE). With len 0, zfs_freesp() sets the size to off. It grows the
file when off is past z_size and truncates it otherwise. The write had
already moved z_size past offset + len, so this took the truncate
branch.
zfs_freesp() could also lose the write when it read z_size before the
write raised it. zfs_extend() then found the file long enough and did
nothing, but zfs_freesp() still logged TX_TRUNCATE at offset + len and
called truncate_setsize(), so i_size went short and replay would cut
[23 lines not shown]
ZTS: Race fallocate against a concurrent O_DIRECT append
fallocate_odirect_append runs a new helper, dio_append_fallocate.
Each round writes one 128 KiB block, then appends another with
O_DIRECT while a second thread calls fallocate, and checks that the
file still holds the appended block. The test runs 100 rounds each of
an extending fallocate, a keep-size punch at EOF and a keep-size
zero-range at EOF, logs each mode's count, and fails naming every
mode that lost a round. It sets recordsize=128k and direct=standard
so the append takes the Direct I/O path. It runs on Linux only.
On a debug build of master it failed 10 of 10 runs. In the last one,
every mode lost all 100 of its 100 rounds.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Will Rouesnel <wrouesnel at wrouesnel.com>
Reviewed-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Signed-off-by: Patrick Domack <patrickdk at patrickdk.com>
Closes #19344
zap: don't use a stale ptrtbl shift in zap_shrink()
zap_shrink() computed prefix_diff = zt_shift - prefix_len at the top of
each loop iteration, from a zt_shift sampled when the function was
entered. When zap_lock_try_upgrade() fails, zap_lock_upgrade() drops
zap_rwlock and waits for it as WRITER. zt_shift was re-read after that,
but prefix_diff, already computed from the old value, was not, and it is
what the collapse uses:
idx = (slbit ? prefix : sl_prefix) << prefix_diff;
nptrs = 1ULL << prefix_diff;
While the lock is down, a concurrent fzap_add() can grow the pointer
table (zap_expand_leaf() / zap_put_leaf_maybe_grow_ptrtbl() ->
zap_grow_ptrtbl(), zt_shift++). Growing the table doesn't change any
leaf's prefix or prefix_len, so the checks done after relocking (both
leaves still empty, still at prefix_len) still pass. zap_shrink() then
points a range of the wrong size at the wrong index at the surviving
leaf. The removed prefix is always odd, so the stale range never
[30 lines not shown]
zap: test zap_shrink() with ptrtbl growth via a test-only tunable
zap_shrink() can drop zap_rwlock while upgrading it to WRITER, and
another thread may grow the pointer table in that window. That only
happens under a narrow race, so nothing in the test suite exercises it.
Add zap_shrink_test_grow_ptrtbl, a test-only module parameter
(default 0). While it is nonzero, each zap_shrink() that has to upgrade
its lock takes the path where the in-place upgrade failed and grows the
pointer table after reacquiring the lock, as a concurrent
zap_expand_leaf() could have done. Each use decrements it, so a test
can bound how often the table grows. The check is only made when
zap_shrink() is about to collapse two leaves.
Add zap_shrink/zap_shrink_002_pos, which fills a directory, sets the
tunable, removes every file, checks that the pointer table grew, and
then creates and removes the same files again, checking that every
name is found and that the ZAP shrinks back to a single leaf.
[4 lines not shown]
nvlist_exists: use the hashtable to determine key existence
The hashtable is always maintained even for non-unique nvlists (even
though they don't permit a lookup by name). With the hash algorithm
changes, a hashtable lookup starts to beat a linear list walk past about
8 pairs, and is close to 100x faster once we get up around 4K pairs.
Sponsored-by: TrueNAS
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Kamil Monicz <kamil at monicz.dev>
Reviewed-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Signed-off-by: Rob Norris <rob.norris at truenas.com>
Closes #19429
nvpair: cache name hash in the pair, use for comparison
Previously, the hash was recomputed from the name any time it was needed
to find a hashtable bucket (add, lookup, remove), however searches for
existing pairs within the bucket were then always done as a straight
name comparison.
Worse, during resize, all name hashes on all pairs would have to be
recomputed to assign each pair to its new bucket.
Instead, let's compute the hash once when a pair is added, and then
cache it on the pair itself. Hashtable buckets can then be found without
recomputing the hash (remove, resize), and name comparisons (lookup,
remove) can be prefiltered by a hash comparison before doing the full
string compare to handle collisions.
Testing suggests another 1.1-1.3x improvement on nvlists with >= 64
elements, up to ~1.5x on remove-heavy workloads. Together with the
faster hash function we're pushing closer to 2x.
[13 lines not shown]
nvt_hash: switch to FNV-1a with fmix32 finaliser
The previous hash function is the old PJW/ELF hasher, which is both very
weak on short/similar strings, and slow in large part due to the
comparison in the loop, which makes it very reliant on the branch
predictor.
FNV-1a meanwhile is extremely common for small string hashing, much
faster for not needing a compare in the loop, and trivial to implement.
A downside is that the low K bits of the hash only depend on the low K
bits of the input bytes, which we mask off for hashtable bucket
selection. To counter this, we use MurmurHash3's fmix32 finaliser at the
last stage to avalanche the bits.
On the stress tests the hashing alone appears to result in some 20-30%
fewer instructions on x86_64, and 10-20% on aarch64. The majority of
tests are faster by 1.3-1.9x on x86_64, 1.2-1.6x on aarch64. Branch
predictor misses on x86_64 fell by 15-40%.
[6 lines not shown]
fnvlist_num_pairs: return pair count directly
Now that we have a direct pair count, we can use it to speed up
fnvlist_num_pairs(). Since that is a "utility" function, it doesn't
really have access to the nvlist internals, so move it into nvpair.c. We
keep the name to avoid changing the ABI at this time.
Sponsored-by: TrueNAS
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Kamil Monicz <kamil at monicz.dev>
Reviewed-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Signed-off-by: Rob Norris <rob.norris at truenas.com>
Closes #19429
nvpair: keep count of pairs on pair list
As pairs are added to and removed from the pair list, keep a count. This
lets us make nvlist_num_pairs() a getter instead of a list walk (see
next commit), and also gives us a raw number to quickly see where the
list is at.
(This was initially added for an experiment in delaying creation of the
hashtable until the list grew large enough that the hash lookup was
faster than a linear scan. That worked, but in practice the threshold
was around three pairs, which didn't seem worth the additional
complexity. That may change in the future though if the list becomes
more efficient, and regardless, this is still a useful counter to have).
Sponsored-by: TrueNAS
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Kamil Monicz <kamil at monicz.dev>
Reviewed-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Signed-off-by: Rob Norris <rob.norris at truenas.com>
Closes #19429
unit/nvpair_stress: nvpair stress tests
These are stress/profiling tests for nvpair, inspired by
https://jacksonallan.github.io/c_cpp_hash_tables_benchmark/.
Since they aren't really unit tests, but use the unit testing framework,
this commit also updates the unit build/run machinery to have a list of
"extra" tests, that are run through the same system but aren't included
in the default `make unit` run.
Sponsored-by: TrueNAS
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Kamil Monicz <kamil at monicz.dev>
Reviewed-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Signed-off-by: Rob Norris <rob.norris at truenas.com>
Closes #19429
ZTS: make dio_read_verify_decline's first read fail its verify
dio_read_verify_decline expects at most 10 Direct I/O reads before the
file handle declines Direct I/O. The first read only fails its
checksum verify if manipulate_user_buffer's thread happens to change
the buffer between the read completing and the verify, so on a busy
runner 13 to 37 reads went by before the first failure and the test
failed, although the decline itself worked (one dio_verify_rd event,
no direct reads after it).
Delay every read of the pool's vdevs by 25 ms with zinject. The vdev
copies the data into the caller's buffer before the delay, so the
buffer is always modified before the verify and the first direct read
fails. The test now expects exactly one direct read, and reads 100
blocks instead of 500 to keep the run time down.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Signed-off-by: Will Rouesnel <wrouesnel at wrouesnel.com>
Closes #19724
ZTS: failmode_create_continue: use a new pool for each operation
After each operation, zpool clear resumes the pool and starts a
resilver of the writes that failed. Its txgs (scan setup, scan, scan
done, DTL updates) sync back to back, so they can still be running
after the next iteration's zpool sync, while the errors for the next
operation are injected. Their writes then fail and suspend the pool:
- If the pool suspends before the next operation assigns its
transaction, dmu_tx_assign() returns EIO (failmode=continue) and the
operation fails without creating the entry. The test then reports
"entry hidden by a stale dentry" and "entry missing" for mkdir or
symlink, as seen on almalinux8 and fedora44 in CI.
- If it suspends while the resilver's SPA_ASYNC_RESILVER_DONE task
waits for SCL_ALL, zpool clear blocks on the config lock behind it
and the pool never resumes: the test hangs until the job times out,
as seen once on almalinux8.
[8 lines not shown]
ZTS: failmode_create_continue: don't leave an exported pool behind
The test ends by exporting the pool, so its cleanup's destroy_pool
finds no pool and both disks keep labels of an exported testpool.
The next test on the same disks that imports testpool by name then
fails with "more than one matching pool", e.g.
large_dnode_009_pos after the failmode group.
Import the pool again by guid after the export, so the cleanup
destroys it.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Signed-off-by: Will Rouesnel <wrouesnel at wrouesnel.com>
Closes #19717
ZTS: failmode_create_continue: don't rely on mkdir(1)'s exit status
On Ubuntu 26.04, mkdir is uutils coreutils, and uutils mkdir 0.8.0
exits 0 when mkdir(2) fails but the directory exists afterwards, which
is exactly the case this test creates: zfs_mkdir() creates the entry,
then returns EIO from the failed zil_commit(). So the test fails on
ubuntu26 with "mkdir: blocked=0 rc=0" although the kernel returned
EIO. GNU mkdir exits 1, as does uutils ln for the symlink case.
Issue a single mkdir(2) from python3 instead, as the create case does
with a single open(O_CREAT).
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Signed-off-by: Will Rouesnel <wrouesnel at wrouesnel.com>
Closes #19717
Decrypt when holding an unmounted filesystem's objset
zfsvfs_hold() sets up an unmounted filesystem with zfsvfs_create_hold(),
which holds its objset without DS_HOLD_FLAG_DECRYPT, so no key mapping
is created. zfsvfs_init() then fails to read the encrypted master node
with EACCES, even though the key is loaded. As a result, zfs userspace,
groupspace and projectspace, and setting a user, group or project
quota, fail with "permission denied" on an encrypted filesystem that
isn't mounted, unless its blocks happen to be cached.
Hold the objset with decryption, as zfsvfs_create() does when it owns
it. This was introduced by eb0c674c2a.
Reviewed-by: Rob Norris <rob.norris at truenas.com>
Reviewed by: George Wilson <gwilson at zfsmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Signed-off-by: Matthew Ahrens <matt at mahrens.org>
Co-authored-by: Claude Opus 5.5 <noreply at anthropic.com>
Closes #19713
Fix direct reads of blocks pending free
Direct reads only check the on-disk block pointer for holes, so they
can read stale data after a block has been freed but before the free
has synced. Share buffered read hole detection with direct reads to
honor pending frees as well.
Keep cached data ahead of hole detection and preserve the transaction
ordering of block clone and direct write overrides. Retain the block
pointer recheck after dnode_block_freed() for concurrent syncing.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Signed-off-by: Wale Zhang <wale.zhang.ftd at gmail.com>
Closes #19633
ZIL: Sync the txg that a suspended commit waits for
zil_suspend() raises zl_suspend and then calls zil_commit_impl(). With
zl_suspend set, zil_get_commit_list() leaves every itx on the per-txg
lists, including the commit itx that carries the caller's own waiter,
and returns that txg for the caller to wait for instead. But
zil_commit_impl() waits on the waiter first, and the waiter is on no
lwb. Only zil_itxg_clean() can mark it done, once the txg syncs, and
nothing asks for that sync. The wait lasts until the sync thread's
zfs_txg_timeout timer fires. A zil_commit() that loses the race with
zil_suspend() waits the same way.
spa_reset_logs() suspends the ZIL of each dataset in turn while it
holds spa_namespace_lock. zpool remove, zpool split, removing a log
device that holds log blocks and offlining a non-redundant log device
all call it. Each dataset with a ZIL chain adds up to zfs_txg_timeout
to the command, and every other pool command waits for the lock
meanwhile. At the default of 5 seconds, the zpool remove in
removal_nopwrite takes 10 to 11 seconds for its two file systems.
[17 lines not shown]
dmu_recv: keep rrd->abd current in do_corrective_recv()
do_corrective_recv() works on a local copy of rrd->abd, and may replace
it up to three times: with the decompressed stream data, with the
recompressed data, and with the re-encrypted data. Each time the old
buffer is freed, but rrd->abd is only updated just before the rewrite
zio is issued. receive_process_record() unconditionally frees
rrd->abd after do_corrective_recv() returns for a healing receive, so
any error return between the first swap and that assignment frees the
original stream buffer a second time and leaks the current one.
The affected returns are the three error returns in the re-encryption
block (taken for a non-raw receive into an encrypted dataset), when
reached after the data has been decompressed (compressed stream) or
recompressed (compressed on-disk block):
- dsl_dataset_hold_flags(..., DS_HOLD_FLAG_DECRYPT, ...) failing
(returns EACCES),
- spa_keystore_lookup_key() failing (returns EACCES),
[20 lines not shown]
ZTS: create the fdomain pool in zpool_create_draid_005_pos
The random loop of zpool_create_draid_005_pos builds the device list
of each layout a second time with fdomain keywords, but then creates
the pool from the fgroup list again, so the fdomain form of those
layouts is never tested. Use the fdomain list.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Tony Hutter <hutter2 at llnl.gov>
Signed-off-by: Tigerblue77 <37409593+tigerblue77 at users.noreply.github.com>
Closes #19708
zpool: reject empty dRAID failure groups and stray devices
construct_spec() counts the devices after each fgroup or fdomain
keyword and requires each group to hold as many as the previous one.
It misses two cases: devices listed before the first keyword, which
are counted as children but belong to no group, and groups with no
devices, because a previous count of zero is also how it recognises
the first group. Both let through specifications in which the number
of devices is not the number of groups times the size of a group:
- "draid1 d1 d2 d3 d4 fgroup fgroup" is accepted as two groups of two
disks, although no disk was listed in a group;
- "draid1 d1 d2 d3 fdomain fdomain fdomain fdomain" makes
draid_config_by_type() compute zero failure groups and divide by
zero, and zpool is killed by SIGFPE;
- "draid2 fdomain fdomain <16 disks> fdomain <16 disks>" leaves the
last entries of the reordered child array uninitialized, and the
verify() around nvlist_add_nvlist_array() aborts zpool.
[13 lines not shown]
cstyle: clean up bugs in some of the style lints
There were a few little mistakes in the cstyle script (a missing
boundary on else-checking, a missing $ on the `special` variable,
and a flipped regex.
I also cleaned up the few instances of cstyle mistakes that resulted
from fixing these lints.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Reviewed-by: Allan Jude <allan at klarasystems.com>
Signed-off-by: Mark Musante <mmusante at wasabi.com>
Closes #19592
dmu_recv: fix corrective recv of zstd compressed blocks
A corrective ("healing") receive into a dataset using zstd compression
panics on debug builds, and overruns a heap buffer on production builds:
zpool create -o ashift=12 tp /var/tmp/zd
zfs create -o compression=zstd -o recordsize=128k tp/fs
<write ~4MB of compressible data to /tp/fs/f>
zfs snap tp/fs at s; zfs send tp/fs at s > s.zs
<corrupt one L0 block on disk, export/import>
zfs recv -c tp/fs at s < s.zs
VERIFY3U(aiter->iter_pos, <=, aiter->iter_abd->abd_size) failed
(131072 <= 24576)
PANIC at abd_os.c:841:abd_iter_at_end()
do_corrective_recv() recompresses the stream data with the block's
algorithm and rwa->os->os_complevel, into a buffer of the block's
physical size. There are three problems with this:
[50 lines not shown]
ZTS: test corrective receive of zstd compressed blocks
The existing corrective receive tests write data from /dev/urandom,
which is not compressible, so they never exercise the recompression
path. Add zfs_receive_zstd_corrective, which heals compressible data
on zstd, zstd-12, zstd-fast-10 and encrypted zstd-9 datasets from
uncompressed send streams.
This currently fails: recompressing the stream data for the corrupted
block overruns its buffer, which panics debug builds in
abd_iter_at_end() (VERIFY3U(aiter->iter_pos, <=,
aiter->iter_abd->abd_size)) and corrupts kernel memory otherwise.
The fix follows in the next commit.
Reviewed-by: Brian Behlendorf <behlendorf1 at llnl.gov>
Signed-off-by: Will Rouesnel <wrouesnel at wrouesnel.com>
Closes #19281
