Fast Dedup: Cleanup and documentation ahead of integrating Fast Dedup #15887

allanjude · 2024-02-14T13:41:16Z

Motivation and Context

This is a collection of cleanups, refactors and documenting of the existing dedup system. There should be no functional changes here at all.

Description

See each commit message.

How Has This Been Tested?

Targeted tests all passing. Basic manual dedup sanity tests work as expected. CI tests will get the rest of the way there.

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)
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.

behlendorf

This cleanup all looks very reasonable to me!

module/zfs/ddt_zap.c

module/zfs/ddt.c

include/sys/ddt_impl.h

module/zfs/ddt.c

include/sys/ddt.h

Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

I think I can say with some confidence that anyone making a new storage type in 2023 is doing their own thing with compression, not this. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Always do them on the heap, and when we know how much we need, only that much. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

We're about to have different kinds of things that we'll compare on key, so generalise this function to support that. (It actually worked fine because of the way the casts work out, but it requires the key to be at the start of the object so the cast through ddt_entry_t works, and even then it reads strangely for anything that's not a ddt_entry_t). Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

We want to add other kinds of dedup-related objects and keep stats for them. This makes those functions easier to use from outside ddt.c. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

It was a weird and confusing name, because it wasn't actually returning the number of DVAs in the entry (as in, in the value/phys part) but the maximum number of possible DVAs in a BP generated from the entry, based on the encrypt bit in the key. This is unlike the similarly named BP_GET_NDVAS, which really does return the number of DVAs. Since its only used in this one place, and for a specific purpose, it seemed more sensible to just write it in-place and remove the name. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Just to make it easier to know which bits to pay attention to. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Mostly for consistency, so the reader is less likely to wonder why these things look different. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Things get confused when there's more than one name for a thing. Note that we don't do this for ddt_object_t, ddt_histogram_t and ddt_stat_t because they're part of the public ZFS interface. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

ddt_get_dedup_histogram() was actually checking it, just in an extremely cursed way. ddt_get_dedup_object_stats() wasn't, but wasn't being called from a dangerous place so no one noticed. These checks are necessary, because spa_ddt[] is not populated until spa_load(), but the spa can exist before that, while being created, and as vdevs and metaslabs are initialised the space accounting functions will be called to update pool space counts. Probably the whole create path doesn't need to go asking for space accounting from metadata subsystems until after the pool is created. This will at least catch misuse. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Store objects store keys and values, so have them take those types and nothing more. This way, they don't need to be concerned about the "kind" of entry being operated on; the dispatch layer can take care of the appropriate conversions. This adds a "contains" op to see if a particular entry exists without loading it, which makes a couple of things easier to do; in particular, it allows us to avoid an allocation in ddt_class_contains(). Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Nothing uses it. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Only a single bit is needed to track entry state, and definitely not two whole bytes. Some light refactoring in ddt_lookup() is needed to support this, but it reads a lot better now. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Most values in zio_checksum can never be used for dedup, partly because the dedup= property only offers a limited list, but also some values (eg ZIO_CHECKSUM_OFF) aren't real and will never be seen. A true flag would be better than a hardcoded list, but thats more cleanup elsewhere than I want to do right now. Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc.

I think I can say with some confidence that anyone making a new storage type in 2023 is doing their own thing with compression, not this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes #15887

Always do them on the heap, and when we know how much we need, only that much. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes #15887

We're about to have different kinds of things that we'll compare on key, so generalise this function to support that. (It actually worked fine because of the way the casts work out, but it requires the key to be at the start of the object so the cast through ddt_entry_t works, and even then it reads strangely for anything that's not a ddt_entry_t). Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes #15887

We want to add other kinds of dedup-related objects and keep stats for them. This makes those functions easier to use from outside ddt.c. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes #15887

It was a weird and confusing name, because it wasn't actually returning the number of DVAs in the entry (as in, in the value/phys part) but the maximum number of possible DVAs in a BP generated from the entry, based on the encrypt bit in the key. This is unlike the similarly named BP_GET_NDVAS, which really does return the number of DVAs. Since its only used in this one place, and for a specific purpose, it seemed more sensible to just write it in-place and remove the name. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes #15887

Just to make it easier to know which bits to pay attention to. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes #15887

Mostly for consistency, so the reader is less likely to wonder why these things look different. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes #15887

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

I think I can say with some confidence that anyone making a new storage type in 2023 is doing their own thing with compression, not this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Always do them on the heap, and when we know how much we need, only that much. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

We're about to have different kinds of things that we'll compare on key, so generalise this function to support that. (It actually worked fine because of the way the casts work out, but it requires the key to be at the start of the object so the cast through ddt_entry_t works, and even then it reads strangely for anything that's not a ddt_entry_t). Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

We want to add other kinds of dedup-related objects and keep stats for them. This makes those functions easier to use from outside ddt.c. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

It was a weird and confusing name, because it wasn't actually returning the number of DVAs in the entry (as in, in the value/phys part) but the maximum number of possible DVAs in a BP generated from the entry, based on the encrypt bit in the key. This is unlike the similarly named BP_GET_NDVAS, which really does return the number of DVAs. Since its only used in this one place, and for a specific purpose, it seemed more sensible to just write it in-place and remove the name. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Just to make it easier to know which bits to pay attention to. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Mostly for consistency, so the reader is less likely to wonder why these things look different. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Things get confused when there's more than one name for a thing. Note that we don't do this for ddt_object_t, ddt_histogram_t and ddt_stat_t because they're part of the public ZFS interface. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

ddt_get_dedup_histogram() was actually checking it, just in an extremely cursed way. ddt_get_dedup_object_stats() wasn't, but wasn't being called from a dangerous place so no one noticed. These checks are necessary, because spa_ddt[] is not populated until spa_load(), but the spa can exist before that, while being created, and as vdevs and metaslabs are initialised the space accounting functions will be called to update pool space counts. Probably the whole create path doesn't need to go asking for space accounting from metadata subsystems until after the pool is created. This will at least catch misuse. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Store objects store keys and values, so have them take those types and nothing more. This way, they don't need to be concerned about the "kind" of entry being operated on; the dispatch layer can take care of the appropriate conversions. This adds a "contains" op to see if a particular entry exists without loading it, which makes a couple of things easier to do; in particular, it allows us to avoid an allocation in ddt_class_contains(). Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Nothing uses it. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Only a single bit is needed to track entry state, and definitely not two whole bytes. Some light refactoring in ddt_lookup() is needed to support this, but it reads a lot better now. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887

Most values in zio_checksum can never be used for dedup, partly because the dedup= property only offers a limited list, but also some values (eg ZIO_CHECKSUM_OFF) aren't real and will never be seen. A true flag would be better than a hardcoded list, but thats more cleanup elsewhere than I want to do right now. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-by: Klara, Inc. Sponsored-by: iXsystems, Inc. Closes openzfs#15887