From 61ab05cac74830f2658cd16138c5876b4b31b4fa Mon Sep 17 00:00:00 2001 From: Rob N Date: Sat, 1 Jul 2023 02:01:58 +1000 Subject: [PATCH] ddt_addref: remove unnecessary phys fill when refcount is 0 The previous comment wondered if this case could happen; it turns out that it really can't. This block can only be entered if dde_type and dde_class are "real"; that only happens when a ddt entry has been previously synced to a ddt store, that is, it was created on a previous txg. Since its gone through that sync, its dde_refcount must be >0. ddt_addref() is called from brt_pending_apply(), which is called at the beginning of spa_sync(), before pending DMU writes/frees are issued. Freeing a dedup block is the only thing that can decrement dde_refcount, so there's no way for it to drop to zero before applying the clone bumps it. Further, even if it _could_ go to zero, it wouldn't be necessary to fill the entry from the block. The phys content is not cleared until the free is issued, which happens when the refcount goes to zero, when the last real free comes through. The cloned block should be identical to what's in the phys already, so the fill should be a no-op anyway. I've replaced this with an assertion because this is all very dependent on the ordering in which BRT and DDT changes are applied, and that might change in the future. Reviewed-by: Brian Behlendorf Signed-off-by: Rob Norris Sponsored-By: Klara, Inc. Closes #15004 --- module/zfs/ddt.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/module/zfs/ddt.c b/module/zfs/ddt.c index 33fea0ba3..1fb198219 100644 --- a/module/zfs/ddt.c +++ b/module/zfs/ddt.c @@ -1209,10 +1209,19 @@ ddt_addref(spa_t *spa, const blkptr_t *bp) ASSERT3S(dde->dde_class, <, DDT_CLASSES); ddp = &dde->dde_phys[BP_GET_NDVAS(bp)]; - if (ddp->ddp_refcnt == 0) { - /* This should never happen? */ - ddt_phys_fill(ddp, bp); - } + + /* + * This entry already existed (dde_type is real), so it must + * have refcnt >0 at the start of this txg. We are called from + * brt_pending_apply(), before frees are issued, so the refcnt + * can't be lowered yet. Therefore, it must be >0. We assert + * this because if the order of BRT and DDT interactions were + * ever to change and the refcnt was ever zero here, then + * likely further action is required to fill out the DDT entry, + * and this is a place that is likely to be missed in testing. + */ + ASSERT3U(ddp->ddp_refcnt, >, 0); + ddt_phys_addref(ddp); result = B_TRUE; } else { -- 2.39.2