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

RFC: send/recv: support for additional features in backup streams #14997

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

robn
Copy link
Member

@robn robn commented Jun 19, 2023

Motivation and Context

I'm currently working on a compression option for a customer. I'm not yet sure if it will pan out, but if it does, its likely to require a stream feature flag to keep send/recv working. However, we're out of DMU_BACKUP_FEATURE_* bits, so we need some other mechanism for adding more features. This PR adds such a mechanism.

This PR is meant as a starting point for discussion. Please comment!

Description

This takes the last remaining feature flag and names it DMU_BACKUP_FEATURE_EXT_FEATURES. That flag is used to signal that there is a features key in the nvlist in the stream's BEGIN record. In there, is an nvlist that lists all the features required to recieve the stream.

The conception here is very simple. If the stream says it needs a feature, then the receiving end must support that that feature in order to recieve it. What "support" means will depend on the feature, but at least it starts with the checks in recv_begin_check_feature_flags_impl().

The values are ignored. I can imagine them being a good spot for providing parameters for the named feature, but for now simple flags will do.

This PR includes a simple per-dataset feature, com.despairlabs:fancy_butter that does nothing except change the enabled/activated state when a corresponding property is enabled on the dataset. Obviously, I don't expect this to be included; I just need something to show how it hooks up (seriously though: fancy butters are awesome).

I've set up recv_begin_check_feature_flags_impl() to check that the requested features are enabled on the pool, and added something to setup_featureflags() to show how they might be automatically added to the stream, at least for ones with matching SPA features. Quite possibly its better done feature-by-feature; I just wanted to show a possibility.

If its useful, here's the original features proposal, that includes streams: https://marc.info/?l=zfs-discuss&m=130635021411635. I did this before I read it, and its pretty close.

Just to prove it, here's some test output:

# zpool create tank loop0 loop1
# zpool get feature@fancy_butter tank
NAME  PROPERTY              VALUE                 SOURCE
tank  feature@fancy_butter  enabled               local

# zfs create tank/a
# zfs create tank/b
# zfs set fancy_butter=on tank/b

# zfs snapshot tank/a@snap
# zfs snapshot tank/b@snap

# zfs send -ecL tank/a@snap | tee /tmp/a | zstream dump
BEGIN record
	hdrtype = 1
	features = 430004
	magic = 2f5bacbac
	creation_time = 64902469
	type = 2
	flags = 0xc
	toguid = 84f5a84967cdc2af
	fromguid = 0
	toname = tank/a@snap
	payloadlen = 0
END checksum = 198ed0802cf/23ba2122b17bad/ec756920605ff4f6/1b2514005dc98c96
SUMMARY:
	Total DRR_BEGIN records = 1 (0 bytes)
	Total DRR_END records = 1 (0 bytes)
	Total DRR_OBJECT records = 6 (176 bytes)
	Total DRR_FREEOBJECTS records = 2 (0 bytes)
	Total DRR_WRITE records = 4 (34816 bytes)
	Total DRR_WRITE_BYREF records = 0 (0 bytes)
	Total DRR_WRITE_EMBEDDED records = 3 (136 bytes)
	Total DRR_FREE records = 11 (0 bytes)
	Total DRR_SPILL records = 0 (0 bytes)
	Total records = 28
	Total payload size = 35128 (0x8938)
	Total header overhead = 8736 (0x2220)
	Total stream length = 43864 (0xab58)

# zfs send -ecL tank/b@snap | tee /tmp/b | zstream dump
BEGIN record
	hdrtype = 1
	features = 20430004
	magic = 2f5bacbac
	creation_time = 64902469
	type = 2
	flags = 0xc
	toguid = 7ff5783562eb52d5
	fromguid = 0
	toname = tank/b@snap
	payloadlen = 132
nvlist version: 0
	features = (embedded nvlist)
	nvlist version: 0
		com.despairlabs:fancy_butter = 1
	(end features)

END checksum = 19aaaf80293/24726f4bcdb2bd/fc7c1dfeaf7afa1e/1b19a276bc2b7674
SUMMARY:
	Total DRR_BEGIN records = 1 (132 bytes)
	Total DRR_END records = 1 (0 bytes)
	Total DRR_OBJECT records = 6 (176 bytes)
	Total DRR_FREEOBJECTS records = 2 (0 bytes)
	Total DRR_WRITE records = 4 (34816 bytes)
	Total DRR_WRITE_BYREF records = 0 (0 bytes)
	Total DRR_WRITE_EMBEDDED records = 3 (136 bytes)
	Total DRR_FREE records = 11 (0 bytes)
	Total DRR_SPILL records = 0 (0 bytes)
	Total records = 28
	Total payload size = 35260 (0x89bc)
	Total header overhead = 8736 (0x2220)
	Total stream length = 43996 (0xabdc)

# zpool destroy tank

# zpool create -f -o feature@fancy_butter=disabled tank loop0 loop1
# cat /tmp/a | zfs recv tank/a@snap

# zfs recv tank/b@snap
cannot receive new filesystem stream: pool must be upgraded to receive this stream.

# zpool upgrade -a
This system supports ZFS pool feature flags.

Enabled the following features on 'tank':
  fancy_butter

# cat /tmp/b |  zfs recv tank/b@snap

I think that's it? Not much more to it, just a place to stash more strings. Tell me what you think, and if its worth polishing up, or if there should be more in it. Cheers!

How Has This Been Tested?

Only light manual testing. I am not expecting the test suite to pass.

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:

@robn robn marked this pull request as draft June 19, 2023 09:54
@robn robn force-pushed the dmu-backup-spa-features branch from d99ffcc to e0df20a Compare June 19, 2023 23:46
@robn
Copy link
Member Author

robn commented Jun 20, 2023

Someone pointed out "DMU features and SPA features may not always line up, so we'll need to support both kinds". Of course I knew this, and I had gone back and forth a couple of times on how to do it, and then decided I was getting into the weeds for a proof of concept and so went with the dumbest thing I could imagine, which was in fact too dumb. Update pushed, original description edited.

Since we're out of DMU_BACKUP_FEATURE_* flags, we need another way to
signal required features to a reciever.

This takes the last remaining feature flag as
DMU_BACKUP_FEATURE_EXT_FEATURES, and uses it to signal that additional
features are listed in the "features" key in the nvlist in the
stream's BEGIN record.

The conception here is very simple. If the stream says it needs a
feature, then the recieving end must support that that feature in order
to recieve it. What "support" means will depend on the feature, but at
least it starts with the checks in
recv_begin_check_feature_flags_impl().
@pcd1193182
Copy link
Contributor

I'm not opposed to this idea in general, though I do think that we could probably just steal some or all of the reserved bits in the remainder of the drr_versioninfo struct. I don't know of any proposed use for them. I also don't know what the reserved Delphix flag is for; I don't believe we have any deployed changes that make use of it, though I would want to check. That might have been for a precursor to redacted send/recv?

@robn
Copy link
Member Author

robn commented Oct 25, 2023

I do think that we could probably just steal some or all of the reserved bits in the remainder of the drr_versioninfo struct.

Mm, you might be right. Of course, its just kicking the can down the road, but its a lot more road so maybe that's ok!

Seems like they've been marked "reserved" since the beginning (illumos/illumos-gate@9e69d7d) so probably its just fine. I'd like to know the thought behind reserving them in the first place, but I doubt it makes much difference.

I'll make another PR to open these flags up and see how that goes (probably 99% doc patch, but I'll check the code too).

I also don't know what the reserved Delphix flag is for.

I'd love to know (and the Nutanix feature too), but its not critical.

@robn
Copy link
Member Author

robn commented Oct 25, 2023

Oh huh, also:

#define DMU_GET_FEATUREFLAGS(vi)        BF64_GET((vi), 2, 30)
#define DMU_SET_FEATUREFLAGS(vi, x)     BF64_SET((vi), 2, 30, x)

which explains why bit 29 is the "last flag", but I can't find anything about why 30 and 31 aren't used.

I love mysteries :)

@robn
Copy link
Member Author

robn commented Oct 25, 2023

Oh, right:

#define DMU_GET_FEATUREFLAGS(vi)        BF64_GET((vi), 2, 30)
#define DMU_SET_FEATUREFLAGS(vi, x)     BF64_SET((vi), 2, 30, x)

30 is a length, not a max, so the macros are right, but the comments are off - the last flag is 31, not 29.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants