Allow zhack label repair to restore detached devices. #14773
Conversation
buzzingwires
force-pushed
the
master
branch
3 times, most recently
from
2501c63
to
fcd5693
Compare
|
As of the pull request first being opened, this code has seen a couple style revisions, the tests have been rewritten to use loopback devices instead of files, and fstat64_blk is now used to portably determine the size of a device or file. The tests specific to this code seem to pass without issue on the buildbot, though a some unrelated tests seem to fail. Please let me know if there is anything else I need to do. This is my first ever pull request I've made anywhere, so a lot of the finer details are still unclear to me. For future reference, does the 🚀 emoji have any special significance as a reaction? I've become aware that emojis on GitHub often do in this context, but am unaware of what norms the ZFS community may follow. And, should successive changes after opening the pull request be in the form of extra commits, or should they just be squashed and force-pushed back to origin, as I have now? |
There was a problem hiding this comment.
Thanks for picking up this work and opening this PR.
Instead of files, and fstat64_blk is now used to portably determine the size of a device or file. The tests specific to this code seem to pass without issue on the buildbot, though a some unrelated tests seem to fail.
We do have a couple unrelated failures in the CI you can safely ignore those.
Please let me know if there is anything else I need to do. This is my first ever pull request I've made anywhere, so a lot of the finer details are still unclear to me.
The next steps here are for reviewers to go over the PR and provide some feedback for discussion. I've gone ahead and made a first round of comments to get the ball rolling.
For future reference, does the rocket emoji have any special significance as a reaction?
Not really, we don't use the emoji's as part of the official workflow. However, folks do use them as a way to express interest in a change.
And, should successive changes after opening the pull request be in the form of extra commits, or should they just be squashed and force-pushed back to origin, as I have now?
Please go ahead and squash the commits and force update. This will need to happen as a final step before any change is integrated anyway so it's helpful to do frequently.
This commit expands on the zhack label repair command in d04b5c9 by adding the -u option to undetach a device by regenerating uberblocks, in addition to the existing functionality of fixing checksums, now represented by -c. Previous behavior is retained in the case of no options. The changes are heavily inspired by Jeff Bonwick's labelfix utility, as archived at: https://gist.github.com/jjwhitney/baaa63144da89726e482 Additionally, it is now capable of properly determining the size of block devices and other media, as well as handling sizes which are not divisible by 2^18. This should make it viable for use on physical devices and partitions, in addition to files. These changes should make it possible to import zpools that have had their uberblocks erased, such as in the case of pools rendered inaccessible by erroneous detach commands. Signed-off-by: buzzingwires <buzzingwires@outlook.com>
behlendorf
added
Status: Accepted
I'm glad! Thank you so much for all your guidance over the last couple weeks. Please let me know if there's anything else you need. |
This commit expands on the zhack label repair command in d04b5c9 by adding the -u option to undetach a device by regenerating uberblocks, in addition to the existing functionality of fixing checksums, now represented by -c. Previous behavior is retained in the case of no options. The changes are heavily inspired by Jeff Bonwick's labelfix utility, as archived at: https://gist.github.com/jjwhitney/baaa63144da89726e482 Additionally, it is now capable of properly determining the size of block devices and other media, as well as handling sizes which are not divisible by 2^18. This should make it viable for use on physical devices and partitions, in addition to files. These changes should make it possible to import zpools that have had their uberblocks erased, such as in the case of pools rendered inaccessible by erroneous detach commands. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: buzzingwires <buzzingwires@outlook.com> Closes openzfs#14773
Motivation and Context
This change is intended to address #4187 and expand upon the
zhack label repaircommand implemented in d04b5c9 with the ultimate goal of allowing the zpool of a detached device to be rendered reimportable. Additionally, it allows zhack label repair to handle block devices and other media, as well as sizes which are not divisible by the size of a label.Description
The concepts in Jeff Bonwick's original code at https://gist.github.com/jjwhitney/baaa63144da89726e482 are given a modernized reimplementation. Using the
-uoption, uberblocks are regenerated using the birth TXG of the root block pointer, and the nvlist is also set to reference this TXG. The previous checksum repairing functionality is retained with the-coption, or as a default behavior if no options are specified. Both options may be run together in the same command, for both functionalities. fstat64_blk is also used to determine the size of block devices and other media, so the code will work on them, in addition to normal files.How Has This Been Tested?
The existing test suite for the
zhack label repaircommand has been rewritten to have four test types.-cand-uoptions work when used in the same command, rather than separate ones.Additionally, each test ensures that
-cwill not undetach a device, and that-uwill not proceed with corrupted checksums.Each test runs on a device of randomly determined size. Approximately half the time, the standard minimum device size will be used, and the other half, there will be a possible variation of 0 to 2^15-1 bytes more.
Lint and checkstyle have been run, as have the aforementioned tests.
Types of changes
Checklist:
Signed-off-by.