Skip to content
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

send/recv: open up additional stream feature flags #15454

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

robn
Copy link
Member

@robn robn commented Oct 25, 2023

Motivation and Context

We have only one DMU feature flag bit left. In #14997 I proposed using it to signal the presence of a features nvlist in the BEGIN record where more features may be listed, following the pattern used by pool features. @pcd1193182 suggested that maybe we could just sidestep the problem by taking the extra "reserved" bits in drr_versioninfo. This PR does just that.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

Description

The docs for drr_versioninfo have marked the top 32 bits as "reserved" since its introduction (illumos/illumos-gate@9e69d7d). There's no indication of why they're reserved, so it seems uncontroversial to make a lot more flags available.

I'm keeping the top eight reserved, and explicitly calling them out as such, so we can extend the header further in the future if we run out of flags or want to do some kind of change that isn't about feature flags.

This is actually only a documentation update. The "reserved" bits were never actually expressed in code anywhere.

I've also moved some comments around a little to put them near the thing they're describing. This file seems a little confused, but now maybe slightly less so.

Discussion

I still think #14997 or something like it is the better solution. Its true that there is not a huge demand for stream features, so making more flags available means we're unlikely to need to consider it for a good long while, but also supply can increase demand, especially if we start taking on more "new algorithm" features (#15215 was my original reason for looking at this). It also still suffers from the problem of private use of conflicting flags: bits 3-15, 18 and 28 are apparently unusable but there's no record of what they are or whether they're still needed (that I could discover, at least).

If nothing else, the commentary is wrong: bit #29 is not actually the last available bit by the old scheme, but actually bit #31. I assume this was a misreading of the BF64_GET/BF64_SET macros: the second arg is number of bits, not top bit.

How Has This Been Tested?

Compile checked only.

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:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 30, 2023
@robn robn force-pushed the dmu-stream-more-features branch from e052efe to 04e8333 Compare July 3, 2024 07:40
@robn
Copy link
Member Author

robn commented Jul 3, 2024

@behlendorf bump!

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Solution to both problems of number of bits and possible conflicts would be using features representation similar to pool features, including textual names and may be compatibility restrictions. But I don't insist on it now. For now just one comment below.

include/sys/zfs_ioctl.h Show resolved Hide resolved
@robn
Copy link
Member Author

robn commented Sep 6, 2024

Solution to both problems of number of bits and possible conflicts would be using features representation similar to pool features, including textual names and may be compatibility restrictions.

That's sort of what #14997 is looking towards, though I haven't thought about it for a long while.

The docs for drr_versioninfo have marked the top 32 bits as "reserved"
since its introduction (illumos/illumos-gate@9e69d7d). There's no
indication of why they're reserved, so it seems uncontroversial to make
a lot more flags available.

I'm keeping the top eight reserved, and explicitly calling them out as
such, so we can extend the header further in the future if we run out of
flags or want to do some kind of change that isn't about feature flags.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 1, 2024
@behlendorf behlendorf merged commit 7e957fd into openzfs:master Oct 1, 2024
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants