-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
e052efe
to
04e8333
Compare
@behlendorf bump! |
04e8333
to
c3e1275
Compare
c3e1275
to
eacf831
Compare
There was a problem hiding this 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.
eacf831
to
30c2ed3
Compare
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]>
30c2ed3
to
1b905b7
Compare
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 theBEGIN
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 indrr_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
Checklist:
Signed-off-by
.