Fix ddtprune causing space leak

[tuxoko]

In zio_ddt_free, if a pruned dde is still in ddt, it would do nothing and cause space leak.

Fixes #17982

Motivation and Context

Description

How Has This Been Tested?

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)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• 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.

[robn]

Good catch, very subtle. Took me a while to remember it and understand what was happening, so I've suggested a bit more commentary to explain it. Thank you!

[amotin]

This might be a fix, but I think some deeper look would be good. With B_TRUE passed to ddt_lookup() as verify argument, it should call ddt_entry_lookup_is_valid() for the returned blocks. Which should verify the block pointers. It does not verify the birth time (which might be not good), but in this scenario it should be irrelevant, since there are no new writes after the prune.

[tuxoko]

This might be a fix, but I think some deeper look would be good. With B_TRUE passed to ddt_lookup() as verify argument, it should call ddt_entry_lookup_is_valid() for the returned blocks. Which should verify the block pointers. It does not verify the birth time (which might be not good), but in this scenario it should be irrelevant, since there are no new writes after the prune.

I just tried checking phys birth in ddt_entry_lookup_is_valid, but without the original fix in zio_ddt_free, it doesn't fix the leak. Not sure if I did the right thing here.

diff --git a/module/zfs/ddt.c b/module/zfs/ddt.c
index 0dc9adc7f..b7b332212 100644
--- a/module/zfs/ddt.c
+++ b/module/zfs/ddt.c
@@ -1143,6 +1143,9 @@ ddt_entry_lookup_is_valid(ddt_t *ddt, const blkptr_t *bp, ddt_entry_t *dde)
 	const dva_t *dvas = (ddt->ddt_flags & DDT_FLAG_FLAT) ?
 	    dde->dde_phys->ddp_flat.ddp_dva :
 	    dde->dde_phys->ddp_trad[ndvas].ddp_dva;
+	const uint64_t phys_birth = (ddt->ddt_flags & DDT_FLAG_FLAT) ?
+	    dde->dde_phys->ddp_flat.ddp_phys_birth :
+	    dde->dde_phys->ddp_trad[ndvas].ddp_phys_birth;
 
 	/*
 	 * Compare entry DVAs with the BP. They should all be there, but
@@ -1154,6 +1157,8 @@ ddt_entry_lookup_is_valid(ddt_t *ddt, const blkptr_t *bp, ddt_entry_t *dde)
 		if (!DVA_EQUAL(&dvas[d], &bp->blk_dva[d]))
 			return (B_FALSE);
 	ASSERT3U(d, ==, ndvas);
+	if (BP_GET_PHYSICAL_BIRTH(bp) != phys_birth)
+		return (B_FALSE);
 
 	return (B_TRUE);
 }