Local storage [4/4]: Use raw zvols #9593
Conversation
Request the new *raw* zvol type when creating disks backed by local storage. Using raw zvols requires waiting for them to be initialized, which the instance start saga is now responsible for polling. In the instance start saga, ensure each raw zvol in parallel because this can take some time. This PR also explicitly removes the inherited encryption for these zvols. This is accomplished by changing `EncryptionDetails` to be an enum where the call site now has to explicitly choose between inheriting the parent dataset's encryption, using `aes-256-gcm`, and explicitly setting encryption off. Also chopped out is setting `volblocksize`: this was a mistake to set, and it should instead be left to the default value. Still to do is to revisit the overhead reserved for zvols: the previous emperical value was not for the new raw zvol type. This is tracked by oxidecomputer#9591. Fixes oxidecomputer#9519.
illumos-utils/src/zfs.rs
Outdated
| match execute_async(cmd).await { | ||
| Ok(_) => Ok(()), | ||
| Ok(_) => { | ||
| // no-op |
There was a problem hiding this comment.
Does this just mean "Command succeeded, nothing else to do"?
There was a problem hiding this comment.
I was a little confused that "no-op" meant "yeah I did it and it worked"
illumos-utils/src/zfs.rs
Outdated
| // Open the raw zvol and read from it. If EINPROGRESS is seen from | ||
| // the read, then it's not done initializing yet. We can't delete it | ||
| // until it's fully done, so return the `NotReady` variant to signal | ||
| // upstack that it should poll. |
There was a problem hiding this comment.
that's a bummer; @grwilson is this something we could change in the future?
There was a problem hiding this comment.
There will be an ioctl interface that will allow for the appstack to stop the initialization so that the process can be canceled or aborted.
There was a problem hiding this comment.
8c4376b changes to use DKIOCRAWVOLSTOP to stop the init
illumos-utils/src/zfs.rs
Outdated
| // Open the raw zvol and read from it. If EINPROGRESS is seen from | ||
| // the read, then it's not done initializing yet. We can't delete it | ||
| // until it's fully done, so return the `NotReady` variant to signal | ||
| // upstack that it should poll. |
There was a problem hiding this comment.
would it be worth creating a fn that delete and create both call rather than having the duplicated code?
There was a problem hiding this comment.
I changed to use DKIOCRAWVOLSTOP so this code looks less duplicated now
illumos-utils/src/zfs.rs
Outdated
| // The numeric type of these constants is not specified due to being | ||
| // different for illumos and linux. They are also `let dkioc` and | ||
| // not `const DKIOC` because `const` requires a type. | ||
| // | ||
| // XXX is there a way to pull this constant in from | ||
| // /usr/include/sys/dkio.h | ||
| let dkioc = 0x04 << 8; | ||
| let dkiocrawvolstop = dkioc | 31; |
There was a problem hiding this comment.
| // The numeric type of these constants is not specified due to being | |
| // different for illumos and linux. They are also `let dkioc` and | |
| // not `const DKIOC` because `const` requires a type. | |
| // | |
| // XXX is there a way to pull this constant in from | |
| // /usr/include/sys/dkio.h | |
| let dkioc = 0x04 << 8; | |
| let dkiocrawvolstop = dkioc | 31; | |
| #[cfg(target_os = "illumos")] | |
| const DKIOCRAWVOLSTOP: libc::c_int = (0x04 << 8) | 0x1f; | |
| #[cfg(not(target_os = "illumos"))] | |
| const DKIOCRAWVOLSTOP: libc::c_ulong = (0x04 << 8) | 0x1f; |
eh?
You could use bindgen--I don't think it's worth it.
There was a problem hiding this comment.
Ah, this works. I was thinking to do the cfg specialization like this but forgot about not and was thinking that I'd have to add a target for each OS. Done in ac3d056
|
Do not merge this yet, it depends on a fix for #9607 |
4 participants
Request the new raw zvol type when creating disks backed by local storage. Using raw zvols requires waiting for them to be initialized, which the instance start saga is now responsible for polling.
In the instance start saga, ensure each raw zvol in parallel because this can take some time.
This PR also explicitly removes the inherited encryption for these zvols. This is accomplished by changing
EncryptionDetailsto be an enum where the call site now has to explicitly choose between inheriting the parent dataset's encryption, usingaes-256-gcm, and explicitly setting encryption off.Also chopped out is setting
volblocksize: this was a mistake to set, and it should instead be left to the default value.Still to do is to revisit the overhead reserved for zvols: the previous emperical value was not for the new raw zvol type. This is tracked by #9591.
Fixes #9519.