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

Add zpool labelclear --unsafe option #12474

Closed
wants to merge 1 commit into from

Conversation

tonyhutter
Copy link
Contributor

@tonyhutter tonyhutter commented Aug 12, 2021

Motivation and Context

Provide a way to delete a vdev label on an active pool. There have been cases where this would have been useful during development. It could also be useful in cases where a disk is mistakenly replaced with a disk from the same pool, but you don't want to export the pool. In that case you could do zpool offline -f <pool> <disk> && zpool labelclear -f --unsafe <disk> && zpool replace <pool> <disk> to get things going again.

Description

Add an --unsafe option to zpool labelclear to preform no safety checks and unconditionally clear a label. Note, --unsafe only modifies -f, so you must pass both flags to get the unsafe behavior (zpool labelclear -f --unsafe <vdev>). This forces you to pass two flags before shooting yourself in the foot.

How Has This Been Tested?

Force faulted a disk with zpool offline -f. Ran zpool labelclear <disk> and zpool labelclear -f <disk> on an active pool and verified they correctly did not complete the operation. Then ran zpool labelclear -f --unsafe and saw it complete. Cleared the force fault with zpool clear and saw the vdev correctly report "invalid label".

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:

Add an --unsafe option to zpool labelclear to preform no safety checks
and unconditionally clear a label.  This allows you to clear a label
on a vdev that is part of an active pool.  This can be useful during
development.  Note, --unsafe only modifies -f, so you must pass both
flags to get the unsafe behavior (zpool labelclear -f --unsafe <vdev>)

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 13, 2021
@tonyhutter
Copy link
Contributor Author

The test failures appear to be unrelated to this PR

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.

I'm OK with adding an --unsafe option since sometimes as an administrator you really do need a way (other than dd) to overwrite the labels that you know aren't in use. But I wonder if we couldn't solve most of this by making the activity check a bit smarter so --unsafe would very rarely be needed.

One possible approach would be to additionally try an open the vdev path with O_EXCL. On Linux at least this will fail if the vdev is open (i.e. imported by the kernel). We could use this as a cross check that the vdev really is active. Though I'm not sure if this would work on FreeBSD.

@asomers since I know you've been looking at the label code recently I was wondering if we could get your thoughts on this.

@@ -1215,11 +1215,13 @@ zpool_do_remove(int argc, char **argv)
}

/*
* zpool labelclear [-f] <vdev>
* zpool labelclear [-f[--unsafe]] <vdev>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* zpool labelclear [-f[--unsafe]] <vdev>
* zpool labelclear [-f [--unsafe]] <vdev>

@behlendorf behlendorf requested a review from ahrens August 24, 2021 17:06
@asomers
Copy link
Contributor

asomers commented Aug 24, 2021

I find OpenZFS's labelclear behavior to be very frustrating. The --unsafe behavior proposed by the submitter ought to be the default. That's how zpool labelclear worked on FreeBSD 12 and earlier. Note that on FreeBSD, GEOM will automatically prevent you from writing to a disk that is actually in use, even if you don't use O_EXCL. However, I think that O_EXCL is probably a good idea regardless.

@tonyhutter
Copy link
Contributor Author

I did a little test and confirmed you can open(disk, O_EXCL) disks that were zpool offline'd (including "force fault" offlined: zpool offline -f). That covers our particular use case.

Unless I hear otherwise, I'll refactor this patch to get rid of --unsafe option and add O_EXCL check with -f. That will let you labelclear offlined disks with -f alone.

@tonyhutter
Copy link
Contributor Author

Closing this PR in favor of #12511

@tonyhutter tonyhutter closed this Aug 26, 2021
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.

3 participants