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

cmd/zpool: deny creation and upgrade of zpools with block_cloning on FreeBSD #14935

Closed

Conversation

Mattia-Pascal
Copy link

@Mattia-Pascal Mattia-Pascal commented Jun 4, 2023

The block_cloning feature is still under active stabilization. To avoid accidentally enabling during create or during upgrade add a deny enable flag. The user can still explicitly enable after creation.

Signed-off-by: Mattia Pascal mattia.pascal42@hotmail.com
Requested by: Mateusz Guzik mjg@FreeBSD.org
Sponsored by: Rubicon Communications, LLC ("Netgate")

Motivation and Context

Description

How Has This Been Tested?

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:

@Mattia-Pascal Mattia-Pascal force-pushed the block_cloning_disable branch 3 times, most recently from 3ac2b2e to 02e9748 Compare June 4, 2023 17:16
…FreeBSD

The block_cloning feature is still under active stabilization. To avoid
accidentally enabling during create or during upgrade add a deny enable
flag.

The user can still explicitly enable after creation.

Signed-off-by: Mattia Pascal <mattia.pascal42@hotmail.com>

Sponsored by: Rubicon Communications, LLC ("Netgate")
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 6, 2023
@behlendorf
Copy link
Contributor

@pjd @allanjude thoughts on this.

@Mattia-Pascal the zloop failures here are suspicious. There's backtraces in the logs are incomplete so I can't say for certain, but normally zloop passes and in this case both runs failed.

@pjd
Copy link
Contributor

pjd commented Jun 14, 2023

@pjd @allanjude thoughts on this.

@Mattia-Pascal the zloop failures here are suspicious. There's backtraces in the logs are incomplete so I can't say for certain, but normally zloop passes and in this case both runs failed.

I've added sysctl vfs.zfs.brt.enabled to FreeBSD (disabled by default) to control block cloning. I personally don't see the reason to complicate ZFS further with change like that. I haven't received any reports since the last round of fixes, but I can consider renaming sysctl to something like vfs.zfs.brt.experimental_enabled.
Something like this should be added to Linux too for the time being.
I want to keep block cloning disabled by default at least for 14.0 time frame.

@grahamperrin
Copy link
Contributor

Thank you,

vfs.zfs.brt.enabled

vfs.zfs.bclone_enabled, I think; ⚙ D39613 zfs: Add vfs.zfs.bclone_enabled sysctl..

(If I'm missing a more recent commit: sorry.)

@behlendorf
Copy link
Contributor

Something like this should be added to Linux too for the time being.

For the moment at least I don't think we need this on Linux since it hasn't been wired up to the VFS.

@rincebrain
Copy link
Contributor

I would agree that this shouldn't be FreeBSD-specific. If it's going to be disabled by default, it should be disabled everywhere by default.

I'd also agree it shouldn't be marked enabled on a pool by default if we're going to have a tunable mitigating that anyway. (Really, I suppose the same argument kind of applies to hole_birth....)

(I haven't done any testing of the feature recently, and this is not a vote on its stability, I just don't think we should have this special-cased to FreeBSD-only...)

@behlendorf
Copy link
Contributor

Closing, I'd prefer not to add this additional complexity. In addition, this feature has now been wired up on Linux and seen further stabilization.

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.

None yet

5 participants