Skip to content

Expose dataset encryption status via fast stat path #17368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ixhamza
Copy link
Member

@ixhamza ixhamza commented May 22, 2025

Motivation and Context

In truenas_pylibzfs, we query list of encrypted datasets several times, which is expensive. This commit exposes a public API zfs_is_encrypted() to get encryption status from fast stat path without having to refresh the properties.

Description

This change adds a new field, dds_is_encrypted, to the dmu_objset_stats structure. The field is inserted into the existing padding after dds_origin and does not increase the structure size or alter the position of existing members. This ensures backward compatibility between kernel and user-space consumers of dmu_objset_stats, such as zfs_cmd_t used by IOCTLs.

Before this commit:

hamza@~/zfs-debug (master) $ pahole -C dmu_objset_stats module/zfs.ko
struct dmu_objset_stats {
	uint64_t                   dds_num_clones;       /*     0     8 */
	uint64_t                   dds_creation_txg;     /*     8     8 */
	uint64_t                   dds_guid;             /*    16     8 */
	dmu_objset_type_t          dds_type;             /*    24     4 */
	uint8_t                    dds_is_snapshot;      /*    28     1 */
	uint8_t                    dds_inconsistent;     /*    29     1 */
	uint8_t                    dds_redacted;         /*    30     1 */
	char                       dds_origin[256];      /*    31   256 */

	/* size: 288, cachelines: 5, members: 8 */
	/* padding: 1 */
	/* last cacheline: 32 bytes */
};

After this commit:

hamza@~/zfs-debug (NAS-135951) $ pahole -C dmu_objset_stats module/zfs.ko
struct dmu_objset_stats {
	uint64_t                   dds_num_clones;       /*     0     8 */
	uint64_t                   dds_creation_txg;     /*     8     8 */
	uint64_t                   dds_guid;             /*    16     8 */
	dmu_objset_type_t          dds_type;             /*    24     4 */
	uint8_t                    dds_is_snapshot;      /*    28     1 */
	uint8_t                    dds_inconsistent;     /*    29     1 */
	uint8_t                    dds_redacted;         /*    30     1 */
	char                       dds_origin[256];      /*    31   256 */
	/* --- cacheline 4 boundary (256 bytes) was 31 bytes ago --- */
	uint8_t                    dds_is_encrypted;     /*   287     1 */

	/* size: 288, cachelines: 5, members: 9 */
	/* last cacheline: 32 bytes */
};

How Has This Been Tested?

Manually invoked zfs_is_encrypted() in zfs_main.c to validate that it returns the correct encryption status.

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:

@ixhamza ixhamza requested a review from amotin May 22, 2025 23:40
@amotin amotin added the Status: Code Review Needed Ready for review and testing label May 23, 2025
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for posting the pahole output to confirm there was, just enough, padding available. My only concern is that when running the new library with old kmods zfs_is_encrypt() will return 0 even for encrypted datasets. We should consider unconditionally setting one of the unused bits in dds_is_encrypted to indicate the returned boolean is valid.

@ixhamza
Copy link
Member Author

ixhamza commented May 23, 2025

@behlendorf - That’s a great suggestion. I’ve incorporated it in the latest push!

In truenas_pylibzfs, we query list of encrypted datasets several times,
which is expensive. This commit exposes a public API zfs_is_encrypted()
to get encryption status from fast stat path without having to refresh
the properties.

Signed-off-by: Ameer Hamza <[email protected]>
Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically setting up an extension mechanism for this object, and so I think I would prefer this to be (say) dds_flags, with an enum:

typedef enum {
	DDS_FLAG_ENCRYPTED = (1<<0),
	DDS_FLAG_HAS_FLAGS = (1<<7),
} dmu_objset_flag_t;

I won't insist right now, but at the very least, please add comments to explain the magic constant 0x80.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants