From 0747373a19bab67d5e01d96073ccc3287fcd3d65 Mon Sep 17 00:00:00 2001 From: Kamil Monicz Date: Sun, 19 Jul 2026 12:40:15 +0200 Subject: [PATCH 1/3] zio: make chained DDT extension retry transactional When a fast-dedup (FDT) entry is written with more copies than it currently holds, zio_ddt_write() issues a child write to allocate the shortfall DVAs and, at READY, extends the entry via ddt_phys_extend(). A later write that wants still more copies adopts the in-flight lead as its own child, forming a chain of leads that complete oldest-first. Three latent defects in that path make it unsafe to add a second reason to reject an extension at READY (see the following commits): 1. The chain shares a single rolling rollback point, dde_rollback_phys. On a successful child it was advanced with ddt_phys_copy() from the live dde_phys, which by then may already include a newer, not-yet-committed lead's DVAs; and on failure it was cleared unconditionally even while a later lead still needed it. A failed lead could then retain an uncommitted DVA or drop a committed one. Advance the rollback point with the completing child's own private BP (ddt_phys_extend(orig, v, zio->io_bp)); clear it only when this was the last lead (dde_lead_zio[p] == NULL). 2. zio_ddt_child_write_ready() unwound the per-metaslab-group allocation queue depth unconditionally on the gang-reject path, but that depth is only incremented for throttled async allocations. A synchronous or unthrottled child underflowed the counter. Gate the decrement on ZIO_FLAG_ALLOC_THROTTLED, which is set iff the increment happened. 3. An intentional EAGAIN (the dedup-to-plain-write fallback) reached spa_log_error()/zfs_ereport_post() in zio_done() before it was classified as a retry, logging a persistent on-disk data error and an ereport for a hole BP. Consume it after transforms, before error reporting. A deliberate extension-refusal EAGAIN has two producers -- zio_write_gang_block()'s allocation-stage refusal to gang a DDT extension child (a mixed gang/non-gang BP would be illegal) and the READY-stage refusal for an already-ganged live phys (joined by the RAIDZ geometry check in a following commit). Disable dedup, clear io_error, and reexecute as a plain write; the retry terminates because zp_dedup transitions monotonically to false. A backend EAGAIN can also take this path and gets one non-dedup retry before ordinary error handling. These are pre-existing on the gang-extension path. No functional change for the common single-lead extension. Signed-off-by: Kamil Monicz --- include/sys/ddt.h | 9 +++++---- module/zfs/ddt.c | 4 ++-- module/zfs/zio.c | 49 +++++++++++++++++++++++++++++------------------ 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/include/sys/ddt.h b/include/sys/ddt.h index d326ee79856f..18960aaa88b6 100644 --- a/include/sys/ddt.h +++ b/include/sys/ddt.h @@ -220,14 +220,14 @@ typedef enum { * because its relatively rarely used. */ typedef struct { - /* protects dde_phys, dde_orig_phys and dde_lead_zio during I/O */ + /* protects dde_phys, dde_rollback_phys and dde_lead_zio during I/O */ kmutex_t dde_io_lock; /* copy of data after a repair read, to be rewritten */ abd_t *dde_repair_abd; - /* original phys contents before update, for error handling */ - ddt_univ_phys_t dde_orig_phys; + /* rollback point for the outstanding extension chain */ + ddt_univ_phys_t dde_rollback_phys; /* in-flight update IOs */ zio_t *dde_lead_zio[DDT_PHYS_MAX]; @@ -363,7 +363,8 @@ extern void ddt_bp_create(enum zio_checksum checksum, const ddt_key_t *ddk, extern void ddt_phys_extend(ddt_univ_phys_t *ddp, ddt_phys_variant_t v, const blkptr_t *bp); -extern void ddt_phys_unextend(ddt_univ_phys_t *cur, ddt_univ_phys_t *orig, +extern void ddt_phys_unextend(ddt_univ_phys_t *cur, + const ddt_univ_phys_t *orig, ddt_phys_variant_t v); extern void ddt_phys_copy(ddt_univ_phys_t *dst, const ddt_univ_phys_t *src, ddt_phys_variant_t v); diff --git a/module/zfs/ddt.c b/module/zfs/ddt.c index 945d79d056f7..dc3fa9d91432 100644 --- a/module/zfs/ddt.c +++ b/module/zfs/ddt.c @@ -809,13 +809,13 @@ ddt_phys_extend(ddt_univ_phys_t *ddp, ddt_phys_variant_t v, const blkptr_t *bp) } void -ddt_phys_unextend(ddt_univ_phys_t *cur, ddt_univ_phys_t *orig, +ddt_phys_unextend(ddt_univ_phys_t *cur, const ddt_univ_phys_t *orig, ddt_phys_variant_t v) { ASSERT3U(v, <, DDT_PHYS_NONE); dva_t *cur_dvas = (v == DDT_PHYS_FLAT) ? cur->ddp_flat.ddp_dva : cur->ddp_trad[v].ddp_dva; - dva_t *orig_dvas = (v == DDT_PHYS_FLAT) ? + const dva_t *orig_dvas = (v == DDT_PHYS_FLAT) ? orig->ddp_flat.ddp_dva : orig->ddp_trad[v].ddp_dva; for (int d = 0; d < SPA_DVAS_PER_BP; d++) diff --git a/module/zfs/zio.c b/module/zfs/zio.c index 92a4665597d0..8474c10422ec 100644 --- a/module/zfs/zio.c +++ b/module/zfs/zio.c @@ -3696,7 +3696,7 @@ zio_ddt_child_write_done(zio_t *zio) if (dde->dde_io->dde_lead_zio[p] == zio) dde->dde_io->dde_lead_zio[p] = NULL; - ddt_univ_phys_t *orig = &dde->dde_io->dde_orig_phys; + ddt_univ_phys_t *orig = &dde->dde_io->dde_rollback_phys; if (zio->io_error != 0) { /* @@ -3705,7 +3705,8 @@ zio_ddt_child_write_done(zio_t *zio) * the last time it was successfully extended. */ ddt_phys_unextend(ddp, orig, v); - ddt_phys_clear(orig, v); + if (dde->dde_io->dde_lead_zio[p] == NULL) + ddt_phys_clear(orig, v); mutex_exit(&dde->dde_io->dde_io_lock); @@ -3733,7 +3734,7 @@ zio_ddt_child_write_done(zio_t *zio) if (dde->dde_io->dde_lead_zio[p] == NULL) ddt_phys_clear(orig, v); else - ddt_phys_copy(orig, ddp, v); + ddt_phys_extend(orig, v, zio->io_bp); mutex_exit(&dde->dde_io->dde_io_lock); } @@ -3751,11 +3752,13 @@ zio_ddt_child_write_ready(zio_t *zio) ddt_phys_variant_t v = DDT_PHYS_VARIANT(ddt, p); if (ddt_phys_is_gang(dde->dde_phys, v)) { - for (int i = 0; i < BP_GET_NDVAS(zio->io_bp); i++) { - dva_t *d = &zio->io_bp->blk_dva[i]; - metaslab_group_alloc_decrement(zio->io_spa, - DVA_GET_VDEV(d), zio->io_allocator, - METASLAB_ASYNC_ALLOC, zio->io_size, zio); + if (zio->io_flags & ZIO_FLAG_ALLOC_THROTTLED) { + for (int i = 0; i < BP_GET_NDVAS(zio->io_bp); i++) { + dva_t *d = &zio->io_bp->blk_dva[i]; + metaslab_group_alloc_decrement(zio->io_spa, + DVA_GET_VDEV(d), zio->io_allocator, + METASLAB_ASYNC_ALLOC, zio->io_size, zio); + } } zio->io_error = EAGAIN; } @@ -4117,7 +4120,7 @@ zio_ddt_write(zio_t *zio) * First time out, take a copy of the stable entry to revert * to if there's an error (see zio_ddt_child_write_done()) */ - ddt_phys_copy(&dde_io->dde_orig_phys, dde->dde_phys, v); + ddt_phys_copy(&dde_io->dde_rollback_phys, dde->dde_phys, v); dde_io->dde_lead_zio[p] = cio; } else { if (dde_io->dde_lead_zio[p] == NULL) { @@ -4126,7 +4129,7 @@ zio_ddt_write(zio_t *zio) * to revert to if there's an error (see * zio_ddt_child_write_done()) */ - ddt_phys_copy(&dde_io->dde_orig_phys, dde->dde_phys, + ddt_phys_copy(&dde_io->dde_rollback_phys, dde->dde_phys, v); } else { /* @@ -5638,6 +5641,23 @@ zio_done(zio_t *zio) zio_pop_transforms(zio); /* note: may set zio->io_error */ + /* + * The DDT extension path deliberately produces EAGAIN at two + * sites: at allocation, when extending would require ganging + * (zio_write_gang_block()), and at READY, when the live phys is + * already ganged. Disable dedup and reexecute as a plain write; + * zp_dedup transitions monotonically to false, so the retry + * terminates. A backend EAGAIN can also take this path; it gets + * one non-dedup retry before ordinary error handling. + */ + if (zio->io_error == EAGAIN && + zio == zio->io_logical && IO_IS_ALLOCATING(zio) && + zio->io_prop.zp_dedup) { + zio->io_prop.zp_dedup = B_FALSE; + zio->io_post |= ZIO_POST_REEXECUTE; + zio->io_error = 0; + } + /* * During thorough scrub, if the dataset key is not loaded, decryption * or MAC verification fails with EACCES (spa_do_crypt_abd() and the @@ -5723,15 +5743,6 @@ zio_done(zio_t *zio) if (zio->io_error && zio == zio->io_logical) { - /* - * A DDT child tried to create a mixed gang/non-gang BP. We're - * going to have to just retry as a non-dedup IO. - */ - if (zio->io_error == EAGAIN && IO_IS_ALLOCATING(zio) && - zio->io_prop.zp_dedup) { - zio->io_post |= ZIO_POST_REEXECUTE; - zio->io_prop.zp_dedup = B_FALSE; - } /* * Determine whether zio should be reexecuted. This will * propagate all the way to the root via zio_notify_parent(). From 874600d42d0c92759cf329781bb9aa68c3528993 Mon Sep 17 00:00:00 2001 From: Kamil Monicz Date: Sun, 19 Jul 2026 11:39:37 +0200 Subject: [PATCH 2/3] vdev_raidz: contain repair maps that exceed their DVA allocation A RAIDZ vdev selects each block's logical stripe width from the block pointer's single physical birth (vdev_raidz_get_logical_width()). After a raidz expansion, an older block reads at its birth-time width and its recorded ASIZE always covers that (wider, more-parity) footprint, so implied == owned. If a block pointer instead names a DVA that was allocated at the new (narrower) width but is stamped with an older physical birth -- which fast-dedup extension across an expansion boundary can produce, and which older FDT code could also write -- then the birth-selected map covers more space than the DVA actually owns. A self-heal or resilver of that copy would map that oversized footprint and write past the DVA's allocation, into an adjacent block -- in any build: the nearby DEBUG-only VERIFY (asize_new <= asize) only checks that width geometry is monotonic in psize terms; nothing compares the implied footprint against the DVA's owned allocation. Detect the condition (vdev_raidz_io_exceeds_dva(): the birth-width footprint exceeds the matched DVA's owned ASIZE, gang headers compared against their constant header extent) and reject it with EIO in vdev_raidz_io_start() -- for reads as well as writes -- before taking the reflow rangelock or constructing a map. A zfs_dbgmsg breadcrumb identifies the rejected RAIDZ vdev and geometry. vdev_raidz_io_done() returns for the resulting NULL map, so no column reads, no vdev_raidz_read_all() re-issue, and no repair writes from a replacing/spare mirror below the RAIDZ are ever emitted. The bad copy fails with a checksum/IO error (degraded redundancy) instead of corrupting a neighbor; the other copies are unaffected and read normally. The classifier only fires for the exact malformed DVA: a legitimate block's implied footprint never exceeds what its DVA owns (a gang child may own more than the final BP implies), so it is never rejected. When the historical and physical widths differ, a direct top-level dispatch whose BP matches no DVA of the addressed vdev is impossible; treat it as the same malformed condition and reject (fail closed) rather than assuming safety. The rejection diagnostic reports the required (birth-width) ASIZE against the DVA's owned ASIZE. Signed-off-by: Kamil Monicz --- module/zfs/vdev_raidz.c | 112 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 104 insertions(+), 8 deletions(-) diff --git a/module/zfs/vdev_raidz.c b/module/zfs/vdev_raidz.c index 5fb568536691..db5f3afe7e00 100644 --- a/module/zfs/vdev_raidz.c +++ b/module/zfs/vdev_raidz.c @@ -2348,31 +2348,90 @@ vdev_raidz_asize_to_psize(vdev_t *vd, uint64_t asize, uint64_t txg) * allocate P+1 sectors regardless of width ("cols", which is at least P+1). */ static uint64_t -vdev_raidz_psize_to_asize(vdev_t *vd, uint64_t psize, uint64_t txg) +vdev_raidz_psize_to_asize_width(vdev_t *vd, uint64_t psize, uint64_t cols) { vdev_raidz_t *vdrz = vd->vdev_tsd; uint64_t asize; uint64_t ashift = vd->vdev_top->vdev_ashift; uint64_t nparity = vdrz->vd_nparity; - uint64_t cols = vdev_raidz_get_logical_width(vdrz, txg); + ASSERT3U(cols, >, nparity); asize = ((psize - 1) >> ashift) + 1; asize += nparity * ((asize + cols - nparity - 1) / (cols - nparity)); - asize = roundup(asize, nparity + 1) << ashift; + return (roundup(asize, nparity + 1) << ashift); +} + +static uint64_t +vdev_raidz_psize_to_asize(vdev_t *vd, uint64_t psize, uint64_t txg) +{ + vdev_raidz_t *vdrz = vd->vdev_tsd; + uint64_t cols = vdev_raidz_get_logical_width(vdrz, txg); + uint64_t asize = + vdev_raidz_psize_to_asize_width(vd, psize, cols); #ifdef ZFS_DEBUG - uint64_t asize_new = ((psize - 1) >> ashift) + 1; - uint64_t ncols_new = vdrz->vd_physical_width; - asize_new += nparity * ((asize_new + ncols_new - nparity - 1) / - (ncols_new - nparity)); - asize_new = roundup(asize_new, nparity + 1) << ashift; + uint64_t asize_new = vdev_raidz_psize_to_asize_width(vd, psize, + vdrz->vd_physical_width); VERIFY3U(asize_new, <=, asize); #endif return (asize); } +typedef enum vdev_raidz_dva_extent { + VDEV_RAIDZ_DVA_EXTENT_SAFE, + VDEV_RAIDZ_DVA_EXTENT_EXCEEDED, + VDEV_RAIDZ_DVA_EXTENT_UNMATCHED, +} vdev_raidz_dva_extent_t; + +static vdev_raidz_dva_extent_t +vdev_raidz_io_exceeds_dva(zio_t *zio, uint64_t logical_width, + uint64_t *required_asizep, uint64_t *owned_asizep) +{ + vdev_t *vd = zio->io_vd; + vdev_t *tvd = vd->vdev_top; + const blkptr_t *bp = zio->io_bp; + uint64_t owned_asize = UINT64_MAX; + + ASSERT3P(bp, !=, NULL); + + /* + * Production RAIDZ expansion applies to a top-level RAIDZ; nested + * ztest layouts are bypassed here. Indirect vdevs cannot coexist + * with a RAIDZ top (device removal and vdev-add both refuse), so + * remapped I/O never reaches this check. + */ + if (vd != tvd) + return (VDEV_RAIDZ_DVA_EXTENT_SAFE); + + for (int d = 0; d < BP_GET_NDVAS(bp); d++) { + const dva_t *dva = &bp->blk_dva[d]; + + if (DVA_GET_VDEV(dva) != tvd->vdev_id || + DVA_GET_OFFSET(dva) != zio->io_offset) + continue; + + uint64_t asize = DVA_GET_GANG(dva) ? + vdev_gang_header_asize(tvd) : DVA_GET_ASIZE(dva); + /* + * A second DVA at the same top vdev and offset can only + * occur in an already-malformed BP; keep the smallest owned + * extent so the containment check stays conservative. + */ + owned_asize = MIN(owned_asize, asize); + } + + if (owned_asize == UINT64_MAX) + return (VDEV_RAIDZ_DVA_EXTENT_UNMATCHED); + + *required_asizep = vdev_raidz_psize_to_asize_width(vd, zio->io_size, + logical_width); + *owned_asizep = owned_asize; + return (*required_asizep > owned_asize ? + VDEV_RAIDZ_DVA_EXTENT_EXCEEDED : VDEV_RAIDZ_DVA_EXTENT_SAFE); +} + /* * The allocatable space for a raidz vdev is N * sizeof(smallest child) * so each child must provide at least 1/Nth of its asize. @@ -2712,6 +2771,37 @@ vdev_raidz_io_start(zio_t *zio) uint64_t logical_width = vdev_raidz_get_logical_width(vdrz, BP_GET_PHYSICAL_BIRTH(zio->io_bp)); if (logical_width != vdrz->vd_physical_width) { + uint64_t required_asize; + uint64_t owned_asize; + vdev_raidz_dva_extent_t extent = vdev_raidz_io_exceeds_dva(zio, + logical_width, &required_asize, &owned_asize); + + if (extent != VDEV_RAIDZ_DVA_EXTENT_SAFE) { + if (extent == VDEV_RAIDZ_DVA_EXTENT_UNMATCHED) { + zfs_dbgmsg("%s: rejecting direct raidz " + "vdev %llu at offset %llx size %llu: " + "no BP DVA owns it", + spa_name(zio->io_spa), + (u_longlong_t)vd->vdev_guid, + (u_longlong_t)zio->io_offset, + (u_longlong_t)zio->io_size); + } else { + zfs_dbgmsg("%s: rejecting raidz vdev %llu " + "at offset %llx size %llu: " + "required birth-width ASIZE %llu " + "exceeds DVA owned ASIZE %llu", + spa_name(zio->io_spa), + (u_longlong_t)vd->vdev_guid, + (u_longlong_t)zio->io_offset, + (u_longlong_t)zio->io_size, + (u_longlong_t)required_asize, + (u_longlong_t)owned_asize); + } + zio->io_error = SET_ERROR(EIO); + zio_execute(zio); + return; + } + zfs_locked_range_t *lr = NULL; uint64_t synced_offset = UINT64_MAX; uint64_t next_offset = UINT64_MAX; @@ -2763,6 +2853,7 @@ vdev_raidz_io_start(zio_t *zio) zio->io_vsd = rm; zio->io_vsd_ops = &vdev_raidz_vsd_ops; + if (zio->io_type == ZIO_TYPE_WRITE) { for (int i = 0; i < rm->rm_nrows; i++) { vdev_raidz_io_start_write(zio, rm->rm_row[i]); @@ -3890,6 +3981,11 @@ vdev_raidz_io_done(zio_t *zio) { raidz_map_t *rm = zio->io_vsd; + if (rm == NULL) { + ASSERT(zio->io_error != 0); + return; + } + ASSERT(zio->io_bp != NULL); if (zio->io_type == ZIO_TYPE_WRITE) { for (int i = 0; i < rm->rm_nrows; i++) { From 842f273a66343ec87d85047279d870599c6d8ddf Mon Sep 17 00:00:00 2001 From: Kamil Monicz Date: Tue, 21 Jul 2026 19:10:08 +0200 Subject: [PATCH 3/3] zio: prevent DDT extension across a RAIDZ expansion boundary A block pointer carries one physical birth for all of its DVAs, and RAIDZ uses that birth to choose the stripe width to read every DVA at. Ordinary allocation upholds the implied invariant -- every DVA in a BP shares one geometry epoch -- because all of a BP's DVAs are allocated in one txg. Fast-dedup extension breaks it. ddt_phys_extend() appends newly allocated DVAs to an existing flat phys while keeping the entry's original ddp_phys_birth, and every referencing BP is filled with that old birth. If the entry was born before a raidz expansion and the added DVA is allocated after it, the two DVAs live in different width epochs but share one birth. The BP cannot represent the newer DVA's geometry epoch, so RAIDZ reads that copy at the wrong layout (a checksum error in the common case), and -- absent the containment added separately -- a self-heal maps an oversized footprint that overruns the DVA's allocation. Reject the extension at READY when a newly allocated DVA would land on a raidz top vdev whose logical width at the entry's birth differs from its width at the allocation txg (vdev_raidz_same_logical_width()). The write falls back to an ordinary non-dedup write -- a fresh, coherent BP whose DVAs share one epoch -- via the existing EAGAIN reexecute path, exactly as the gang-mismatch reject already does. Same-epoch extensions, fresh entries (birth 0), traditional tables, and non-raidz tops are unaffected. No on-disk format change. The intentional EAGAIN is consumed in zio_done() into ZIO_POST_REEXECUTE (dedup disabled, io_error cleared) rather than propagated as an error. Evaluating both reject reasons under dde_io_lock also corrects an existing quirk: a DDT child that fails allocation (e.g. ENOSPC) on a ganged entry now propagates its real error instead of being overwritten with EAGAIN, matching the non-gang path. Add raidz_expand_008_pos: write a copies=1 dedup block, expand 3->4, then request copies=2 of the same content; assert the entry is not extended across the width boundary (it stays a single old-width DVA and the second file gets a fresh non-dedup BP), while a same-epoch control still extends normally. The test pins each DDT dump to the pool GUID and drains the FDT log (a bounded sync loop until both log headers are empty) before asserting persistent DDT entry counts. Signed-off-by: Kamil Monicz --- include/sys/vdev_raidz.h | 1 + module/zfs/vdev_raidz.c | 39 ++- module/zfs/zio.c | 78 ++++- tests/runfiles/common.run | 2 +- tests/zfs-tests/tests/Makefile.am | 1 + .../functional/raidz/raidz_expand_008_pos.ksh | 285 ++++++++++++++++++ 6 files changed, 390 insertions(+), 16 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/raidz/raidz_expand_008_pos.ksh diff --git a/include/sys/vdev_raidz.h b/include/sys/vdev_raidz.h index df8c2aed4045..c976bbae88f2 100644 --- a/include/sys/vdev_raidz.h +++ b/include/sys/vdev_raidz.h @@ -64,6 +64,7 @@ void raidz_dtl_reassessed(vdev_t *); boolean_t vdev_sit_out_reads(vdev_t *, zio_flag_t); void vdev_raidz_sit_child(vdev_t *, uint64_t); void vdev_raidz_unsit_child(vdev_t *); +boolean_t vdev_raidz_same_logical_width(vdev_t *, uint64_t, uint64_t); extern const zio_vsd_ops_t vdev_raidz_vsd_ops; diff --git a/module/zfs/vdev_raidz.c b/module/zfs/vdev_raidz.c index db5f3afe7e00..ad23d5a0b3fb 100644 --- a/module/zfs/vdev_raidz.c +++ b/module/zfs/vdev_raidz.c @@ -2282,15 +2282,16 @@ vdev_raidz_close(vdev_t *vd) * happened. */ static uint64_t -vdev_raidz_get_logical_width(vdev_raidz_t *vdrz, uint64_t txg) +vdev_raidz_get_logical_width_locked(vdev_raidz_t *vdrz, uint64_t txg) { + ASSERT(MUTEX_HELD(&vdrz->vd_expand_lock)); + reflow_node_t lookup = { .re_txg = txg, }; avl_index_t where; uint64_t width; - mutex_enter(&vdrz->vd_expand_lock); reflow_node_t *re = avl_find(&vdrz->vd_expand_txgs, &lookup, &where); if (re != NULL) { width = re->re_logical_width; @@ -2301,9 +2302,43 @@ vdev_raidz_get_logical_width(vdev_raidz_t *vdrz, uint64_t txg) else width = vdrz->vd_original_width; } + return (width); +} + +static uint64_t +vdev_raidz_get_logical_width(vdev_raidz_t *vdrz, uint64_t txg) +{ + mutex_enter(&vdrz->vd_expand_lock); + uint64_t width = vdev_raidz_get_logical_width_locked(vdrz, txg); mutex_exit(&vdrz->vd_expand_lock); + return (width); } + +/* + * Return whether allocations born in these txgs use the same logical + * RAIDZ column width. + */ +boolean_t +vdev_raidz_same_logical_width(vdev_t *vd, uint64_t txg1, uint64_t txg2) +{ + ASSERT3P(vd->vdev_ops, ==, &vdev_raidz_ops); + ASSERT3U(txg1, !=, 0); + ASSERT3U(txg2, !=, 0); + + if (txg1 == txg2) + return (B_TRUE); + + vdev_raidz_t *vdrz = vd->vdev_tsd; + mutex_enter(&vdrz->vd_expand_lock); + boolean_t same = avl_is_empty(&vdrz->vd_expand_txgs) || + vdev_raidz_get_logical_width_locked(vdrz, txg1) == + vdev_raidz_get_logical_width_locked(vdrz, txg2); + mutex_exit(&vdrz->vd_expand_lock); + + return (same); +} + /* * This code converts an asize into the largest psize that can safely be written * to an allocation of that size for this vdev. diff --git a/module/zfs/zio.c b/module/zfs/zio.c index 8474c10422ec..b9d519e6e36d 100644 --- a/module/zfs/zio.c +++ b/module/zfs/zio.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -3739,6 +3740,57 @@ zio_ddt_child_write_done(zio_t *zio) mutex_exit(&dde->dde_io->dde_io_lock); } +static boolean_t +zio_ddt_child_write_layout_matches(zio_t *zio, uint64_t phys_birth) +{ + blkptr_t *bp = zio->io_bp; + + ASSERT3U(phys_birth, !=, 0); + ASSERT3U(BP_GET_PHYSICAL_BIRTH(bp), ==, zio->io_txg); + + /* + * DDT writes run only in syncing context, under spa_sync()'s + * SCL_CONFIG hold, so the vdev configuration is stable and + * vdev_lookup_top() is safe without taking the lock here. + */ + for (int d = 0; d < BP_GET_NDVAS(bp); d++) { + vdev_t *vd = vdev_lookup_top(zio->io_spa, + DVA_GET_VDEV(&bp->blk_dva[d])); + ASSERT3P(vd, !=, NULL); + + if (vd->vdev_ops == &vdev_raidz_ops && + !vdev_raidz_same_logical_width(vd, phys_birth, + zio->io_txg)) + return (B_FALSE); + } + + return (B_TRUE); +} + +static void +zio_ddt_child_write_reject(zio_t *zio) +{ + /* + * READY truncates the pipeline before top-vdev children are issued. + * Release queue-depth holds that those children's DONE callbacks would + * normally release. + * + * At this DDT-child callsite, ALLOC_THROTTLED precisely identifies an + * async allocation into a throttle-enabled class. Synchronous and + * unthrottled allocations did not add these holds. + */ + if (zio->io_flags & ZIO_FLAG_ALLOC_THROTTLED) { + for (int d = 0; d < BP_GET_NDVAS(zio->io_bp); d++) { + dva_t *dva = &zio->io_bp->blk_dva[d]; + metaslab_group_alloc_decrement(zio->io_spa, + DVA_GET_VDEV(dva), zio->io_allocator, + METASLAB_ASYNC_ALLOC, zio->io_size, zio); + } + } + + zio->io_error = EAGAIN; +} + static void zio_ddt_child_write_ready(zio_t *zio) { @@ -3751,23 +3803,22 @@ zio_ddt_child_write_ready(zio_t *zio) int p = DDT_PHYS_FOR_COPIES(ddt, zio->io_prop.zp_copies); ddt_phys_variant_t v = DDT_PHYS_VARIANT(ddt, p); - if (ddt_phys_is_gang(dde->dde_phys, v)) { - if (zio->io_flags & ZIO_FLAG_ALLOC_THROTTLED) { - for (int i = 0; i < BP_GET_NDVAS(zio->io_bp); i++) { - dva_t *d = &zio->io_bp->blk_dva[i]; - metaslab_group_alloc_decrement(zio->io_spa, - DVA_GET_VDEV(d), zio->io_allocator, - METASLAB_ASYNC_ALLOC, zio->io_size, zio); - } - } - zio->io_error = EAGAIN; - } - if (zio->io_error != 0) return; mutex_enter(&dde->dde_io->dde_io_lock); + uint64_t phys_birth = ddt_phys_birth(dde->dde_phys, v); + boolean_t reject = ddt_phys_is_gang(dde->dde_phys, v) || + (phys_birth != 0 && + !zio_ddt_child_write_layout_matches(zio, phys_birth)); + + if (reject) { + mutex_exit(&dde->dde_io->dde_io_lock); + zio_ddt_child_write_reject(zio); + return; + } + ddt_phys_extend(dde->dde_phys, v, zio->io_bp); zio_t *pio; @@ -5645,7 +5696,8 @@ zio_done(zio_t *zio) * The DDT extension path deliberately produces EAGAIN at two * sites: at allocation, when extending would require ganging * (zio_write_gang_block()), and at READY, when the live phys is - * already ganged. Disable dedup and reexecute as a plain write; + * already ganged or RAIDZ geometry differs across an expansion + * boundary. Disable dedup and reexecute as a plain write; * zp_dedup transitions monotonically to false, so the retry * terminates. A backend EAGAIN can also take this path; it gets * one non-dedup retry before ordinary error handling. diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run index 5f9713d42399..f0671cfd9d85 100644 --- a/tests/runfiles/common.run +++ b/tests/runfiles/common.run @@ -919,7 +919,7 @@ tags = ['functional', 'redacted_send'] tests = ['raidz_001_neg', 'raidz_002_pos', 'raidz_expand_001_pos', 'raidz_expand_002_pos', 'raidz_expand_003_neg', 'raidz_expand_003_pos', 'raidz_expand_004_pos', 'raidz_expand_005_pos', 'raidz_expand_006_neg', - 'raidz_expand_007_neg', 'raidz_zinject'] + 'raidz_expand_007_neg', 'raidz_expand_008_pos', 'raidz_zinject'] tags = ['functional', 'raidz'] timeout = 1200 diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am index f99bfd9588da..a32617488afe 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/tests/zfs-tests/tests/Makefile.am @@ -1954,6 +1954,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \ functional/raidz/raidz_expand_005_pos.ksh \ functional/raidz/raidz_expand_006_neg.ksh \ functional/raidz/raidz_expand_007_neg.ksh \ + functional/raidz/raidz_expand_008_pos.ksh \ functional/raidz/raidz_zinject.ksh \ functional/raidz/setup.ksh \ functional/redacted_send/cleanup.ksh \ diff --git a/tests/zfs-tests/tests/functional/raidz/raidz_expand_008_pos.ksh b/tests/zfs-tests/tests/functional/raidz/raidz_expand_008_pos.ksh new file mode 100755 index 000000000000..7fad26e08fa6 --- /dev/null +++ b/tests/zfs-tests/tests/functional/raidz/raidz_expand_008_pos.ksh @@ -0,0 +1,285 @@ +#!/bin/ksh -p +# SPDX-License-Identifier: CDDL-1.0 +# shellcheck disable=SC2154 +# +# CDDL HEADER START +# +# The contents of this file are subject to the terms of the +# Common Development and Distribution License (the "License"). +# You may not use this file except in compliance with the License. +# +# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE +# or http://www.opensolaris.org/os/licensing. +# See the License for the specific language governing permissions +# and limitations under the License. +# +# When distributing Covered Code, include this CDDL HEADER in each +# file and include the License file at usr/src/OPENSOLARIS.LICENSE. +# If applicable, add the following below this CDDL HEADER, with the +# fields enclosed by brackets "[]" replaced with your own identifying +# information: Portions Copyright [yyyy] [name of copyright owner] +# +# CDDL HEADER END +# + +# +# DESCRIPTION: +# Fast-dedup (FDT) extension must not append a DVA allocated at a +# post-raidz-expansion width onto a DDT entry whose single physical +# birth predates the expansion. A BP cannot represent per-DVA geometry +# epochs, permitting wrong-layout reads. Verify the extension is refused +# (the write falls back to a coherent non-dedup write) while same-epoch +# extension still works. +# +# STRATEGY: +# 1. Create a fast-dedup RAIDZ1 pool (ashift=12) with dedup=on. +# 2. Control: on the un-expanded pool, copies=1 -> copies=2 of the +# same content must extend the DDT entry (two equal-ASIZE DVAs). +# 3. Write a copies=1 block, expand raidz1 3->4, cross the width +# marker, then request copies=2 of the same content. +# 4. Assert the entry is NOT extended across the width boundary: it +# stays one old-width DVA and the new file gets a fresh non-dedup +# BP; then scrub clean. +# + +. "$STF_SUITE"/include/libtest.shlib + +verify_runnable "global" + +log_assert "FDT extension does not mix RAIDZ expansion geometry epochs" + +typeset -r devs=4 +typeset -r dev_size_mb=256 +typeset -r block_size=128k + +typeset -a disks +typeset mnt="$TEST_BASE_DIR/raidz-fdt-mnt" +typeset ddt_dump="$TEST_BASE_DIR/raidz-fdt-ddt.$$" +typeset bp_dump="$TEST_BASE_DIR/raidz-fdt-bp.$$" +typeset pool_guid + +typeset -r same_epoch_re='^index [[:xdigit:]]+ refcnt 2 phys 0 DVA\[0\]=<0:[[:xdigit:]]+:30000> DVA\[1\]=<0:[[:xdigit:]]+:30000> ' +typeset -r mixed_epoch_re='^index [[:xdigit:]]+ refcnt 2 phys 0 DVA\[0\]=<0:[[:xdigit:]]+:30000> DVA\[1\]=<0:[[:xdigit:]]+:2c000> ' +typeset -r fixed_entry_re='^index [[:xdigit:]]+ refcnt 1 phys 0 DVA\[0\]=<0:[[:xdigit:]]+:30000> ' +typeset -r fresh_bp_re='L0 DVA\[0\]=<0:[[:xdigit:]]+:2c000> DVA\[1\]=<0:[[:xdigit:]]+:2c000> \[L0 .* unique double .*size=20000L/20000P birth=' + +log_must save_tunable DEDUP_LOG_TXG_MAX +log_must save_tunable SCRUB_AFTER_EXPAND +log_must save_tunable RAIDZ_EXPAND_MAX_REFLOW_BYTES + +function cleanup +{ + if poolexists "$TESTPOOL"; then + destroy_pool "$TESTPOOL" + fi + + for ((i = 0; i < devs; i++)); do + [[ -n "${disks[$i]}" ]] && + log_must rm -f "${disks[$i]}" + done + + log_must rm -f "$ddt_dump" "$bp_dump" + log_must rm -rf "$mnt" + + log_must restore_tunable DEDUP_LOG_TXG_MAX + log_must restore_tunable SCRUB_AFTER_EXPAND + log_must restore_tunable RAIDZ_EXPAND_MAX_REFLOW_BYTES +} + +function dump_ddt +{ + log_must eval "zdb -CDDDDD '$TESTPOOL' > '$ddt_dump'" + log_must grep -Eq "^[[:space:]]*pool_guid: ${pool_guid}\$" "$ddt_dump" + log_note "$(grep '^index ' "$ddt_dump")" +} + +function ddt_logs_are_drained +{ + typeset headers + + headers=$(grep -Ec '^DDT-log-.* entries=[0-9]+$' "$ddt_dump") + if ((headers == 0)); then + return 0 + fi + + ((headers == 2)) || return 1 + awk ' + /^DDT-log-.* entries=/ && $NF != "entries=0" { bad = 1 } + END { exit bad } + ' "$ddt_dump" +} + +function drain_ddt_logs +{ + typeset -i i + + for ((i = 0; i < 10; i++)); do + sync_pool "$TESTPOOL" true + dump_ddt + if ddt_logs_are_drained; then + return + fi + done + + log_fail "DDT logs did not drain after 10 forced syncs" +} + +function assert_one_ddt_entry +{ + typeset count + + ddt_logs_are_drained || + log_fail "cannot count persistent DDT entries before logs drain" + count=$(awk '/^index / { n++ } END { print n + 0 }' "$ddt_dump") + ((count == 1)) || + log_fail "expected one DDT entry, found $count" +} + +# +# No pool_guid pinning here: zdb never prints the pool config for a +# dataset target, and every phase pins the GUID via dump_ddt anyway. +# +function dump_bp +{ + log_must eval \ + "zdb -ddddddbbbbbb '$TESTPOOL/' '$obj' > '$bp_dump'" +} + +log_onexit cleanup + +log_must set_tunable32 DEDUP_LOG_TXG_MAX 1 +log_must set_tunable32 SCRUB_AFTER_EXPAND 0 +log_must set_tunable64 RAIDZ_EXPAND_MAX_REFLOW_BYTES 0 + +log_must mkdir -p "$mnt" + +for ((i = 0; i < devs; i++)); do + disks[i]="$TEST_BASE_DIR/raidz-fdt-dev-$i" + log_must truncate -s "${dev_size_mb}M" "${disks[$i]}" +done + +log_must zpool create -f \ + -o ashift=12 \ + -o feature@fast_dedup=enabled \ + -o feature@raidz_expansion=enabled \ + -o feature@block_cloning=disabled \ + -O mountpoint="$mnt" \ + -O dedup=sha256 \ + -O compression=off \ + -O recordsize="$block_size" \ + -O copies=1 \ + -O primarycache=metadata \ + -O xattr=sa \ + "$TESTPOOL" raidz1 \ + "${disks[0]}" "${disks[1]}" "${disks[2]}" + +pool_guid=$(zpool get -H -o value guid "$TESTPOOL") +[[ -n "$pool_guid" ]] || + log_fail "could not get GUID for $TESTPOOL" + +# +# Control: same-geometry copies=1 -> copies=2 extension must still work. +# +log_must dd if=/dev/urandom of="$mnt/control-1" \ + bs="$block_size" count="1" +sync_pool "$TESTPOOL" + +log_must zfs set copies=2 "$TESTPOOL" +log_must dd if="$mnt/control-1" of="$mnt/control-2" \ + bs="$block_size" count="1" +sync_pool "$TESTPOOL" + +drain_ddt_logs +assert_one_ddt_entry +log_must grep -Eq "$same_epoch_re" "$ddt_dump" + +# Remove the control so the target phase has exactly one DDT entry. +log_must rm "$mnt/control-1" "$mnt/control-2" +sync_pool "$TESTPOOL" +drain_ddt_logs +log_must grep -q 'All DDTs are empty' "$ddt_dump" + +# +# Establish one old-width physical copy. +# +log_must zfs set copies=1 "$TESTPOOL" +log_must dd if=/dev/urandom of="$mnt/target-1" \ + bs="$block_size" count="1" +sync_pool "$TESTPOOL" + +# +# Expand 3 -> 4 and cross the installed logical-width marker. +# +log_must zpool attach "$TESTPOOL" raidz1-0 "${disks[3]}" +log_must zpool wait -t raidz_expand "$TESTPOOL" +# re_txg = C + TXG_CONCURRENT_STATES in raidz_reflow_complete_sync(). +sync_pool "$TESTPOOL" +sync_pool "$TESTPOOL" +sync_pool "$TESTPOOL" + +# +# Request one additional physical copy of the existing FDT entry. +# +log_must zfs set copies=2 "$TESTPOOL" +log_must dd if="$mnt/target-1" of="$mnt/target-2" \ + bs="$block_size" count="1" +sync_pool "$TESTPOOL" + +drain_ddt_logs +assert_one_ddt_entry + +# +# Exact RED diagnostic. Stop before scrub because repair of this shape can +# exceed DVA[1]'s allocation. +# +if grep -Eq "$mixed_epoch_re" "$ddt_dump"; then + log_fail "FDT extended across RAIDZ width epochs: " \ + "$(grep '^index ' "$ddt_dump")" +fi + +# +# GREEN: the original DDT phys remains refcnt=1 and one old-width DVA. +# +log_must grep -Eq "$fixed_entry_re" "$ddt_dump" +if grep '^index ' "$ddt_dump" | grep -q 'DVA\[1\]'; then + log_fail "fixed DDT entry unexpectedly has DVA[1]" +fi + +# +# target-2 must be a fresh ordinary two-copy BP at the new width. +# +typeset obj +obj=$(get_objnum "$mnt/target-2") +dump_bp + +typeset bp_line +bp_line=$(grep -m 1 'L0 DVA' "$bp_dump") +[[ -n "$bp_line" ]] || + log_fail "target-2 L0 BP not found" +log_note "$bp_line" + +log_must grep -Eq "$fresh_bp_re" "$bp_dump" + +typeset births logical_birth physical_birth +births=$(print -r -- "$bp_line" | sed -n \ + 's/.*birth=\([0-9][0-9]*\)L\/\([0-9][0-9]*\)P.*/\1 \2/p') +[[ -n "$births" ]] || + log_fail "target-2 BP birth not found" + +logical_birth=${births%% *} +physical_birth=${births##* } +((logical_birth == physical_birth)) || + log_fail "fresh target-2 BP has split birth " \ + "$logical_birth/$physical_birth" + +log_must cmp "$mnt/target-1" "$mnt/target-2" + +log_must zpool scrub -w "$TESTPOOL" +log_must check_pool_status "$TESTPOOL" "scan" "with 0 errors" +log_must check_pool_status "$TESTPOOL" "scan" "repaired 0B" +log_must check_pool_status "$TESTPOOL" "errors" \ + "No known data errors" + +log_must cmp "$mnt/target-1" "$mnt/target-2" + +log_pass "FDT did not extend across RAIDZ expansion geometry epochs"