Local storage [4/4]: Use raw zvols #9593

jmpesp · 2026-01-05T20:38:48Z

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 #9591.

Fixes #9519.

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.

ahl · 2026-01-05T21:17:48Z

illumos-utils/src/zfs.rs

        match execute_async(cmd).await {
-            Ok(_) => Ok(()),
+            Ok(_) => {
+                // no-op


Does this just mean "Command succeeded, nothing else to do"?

I was a little confused that "no-op" meant "yeah I did it and it worked"

ahl · 2026-01-05T21:21:43Z

illumos-utils/src/zfs.rs

+            // 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.


that's a bummer; @grwilson is this something we could change in the future?

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.

8c4376b changes to use DKIOCRAWVOLSTOP to stop the init

and 529ab04 fixes that commit...

ahl · 2026-01-05T21:22:27Z

illumos-utils/src/zfs.rs

+            // 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.


would it be worth creating a fn that delete and create both call rather than having the duplicated code?

I changed to use DKIOCRAWVOLSTOP so this code looks less duplicated now

nexus/src/app/sagas/instance_start.rs

ahl · 2026-01-06T22:43:09Z

illumos-utils/src/zfs.rs

+            // 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;


Suggested change

// 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.

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

jmpesp · 2026-01-07T21:52:41Z

Do not merge this yet, it depends on a fix for #9607

jmpesp · 2026-01-12T17:12:12Z

Note to self: must change this PR to set volblocksize = 128k again.

sled-agent/src/sled_agent.rs

illumos-utils/src/zfs.rs

leftwo · 2026-01-17T20:06:55Z

illumos-utils/src/zfs.rs

+
+    pub async fn delete_dataset_volume(
+        params: DatasetVolumeDeleteArgs<'_>,
+    ) -> Result<(), DeleteDatasetVolumeError> {


Random comment here to remind us to figure out why the delete failed on berlin over the weekend.
Sorry for this message not actually having any useful information regarding that failure.

94345a3 fixes one of the bugs you saw, but doesn't address why the parent dataset could not be deleted originally

I'll try again deleting and see what shakes out.

If the /dev/zvol/rdsk/... path is gone, then skip trying to delete what is probably already deleted. Check the DKIOCRAWVOLSTATUS output to see if the init thread even needs stopping, and return early accordingly. In the face of a transient EINTR stopping the raw zvol init thread, retry the ioctl once. In the face of a transient "dataset is busy" when deleting the parent dataset, retry once. Other parts of sled-agent (like inventory collection) maybe caused this, we never nailed it down. Refactor waiting logic into separate `wait_for_raw_volume_ok_to_delete` function to ensure that the fd is dropped and this part of sled agent isn't holding an open file descriptor to the raw zvol while trying to delete it.

jmpesp · 2026-01-28T22:26:12Z

I have no idea how that got closed...?

leftwo · 2026-01-29T05:35:32Z

I have no idea how that got closed...?

leftwo

Mostly questions :)

illumos-utils/src/zfs.rs

leftwo · 2026-01-30T18:32:33Z

illumos-utils/src/zfs.rs

+            }
+
+            _ => {
+                // The zero init thread encountered an error, we can proceed


Is this where we might change the current code to handle an init that runs forever retrying the init? Or, in this case are we okay and the init will have failed with an error and we will just go ahead and delete it?

If there's an error running the zero init thread (which I'm assuming also does the allocation), I'm assuming we fail and are not retrying, yeah. We'll have to delete the raw vol and try again.

leftwo · 2026-01-30T18:35:55Z

illumos-utils/src/zfs.rs

-    pub async fn delete_dataset_volume(
-        params: DatasetVolumeDeleteArgs<'_>,
+    /// Returns Ok when a raw volume is ok to delete, and an error otherwise
+    pub async fn wait_for_raw_volume_ok_to_delete(


Are we really waiting here? We do a few retries but I don't see a loop that will continue checking until the conditions are satisfied. If I'm correct, then maybe name this check_for_raw_volume_ok_to_delete()?

I believe it will wait in the STOP ioctl for the thread to stop, so I wanted that to be reflected in the function name :)

Could we throw a comment in then, so next time I read this I won't think this is will wait for the deletion before returning? :)

leftwo · 2026-01-30T18:38:00Z

illumos-utils/src/zfs.rs

        }
+
+        // Execution should never reach here?
+        Ok(())


Should we panic if we do get here? How could this happen

b3a2168 changes this, have a look :)

grwilson

The raw volume status check logic looks good to me (including the EFRAGS). I also verified that the current code will report back EFRAGS if we encounter gang blocks with a blocksize of 128k.

AlejandroME added the local storage relating to the local storage feature label Jan 5, 2026

AlejandroME assigned jmpesp Jan 5, 2026

AlejandroME added this to the 18 milestone Jan 5, 2026

ahl approved these changes Jan 5, 2026

View reviewed changes

jmpesp added 2 commits January 6, 2026 21:21

use DKIOCRAWVOLSTOP to stop initialization

8c4376b

correctly use libc::ioctl!

529ab04

ahl approved these changes Jan 6, 2026

View reviewed changes

back to const

ac3d056

leftwo reviewed Jan 14, 2026

View reviewed changes

sled-agent/src/sled_agent.rs Outdated Show resolved Hide resolved

leftwo reviewed Jan 15, 2026

View reviewed changes

illumos-utils/src/zfs.rs Outdated Show resolved Hide resolved

leftwo reviewed Jan 17, 2026

View reviewed changes

jmpesp added 5 commits January 20, 2026 21:45

undo change to EncryptionDetails

d43364b

set volblocksize to 128k

2590137

read 4k instead of 64b from raw zvol

eed1e7f

check for zvol rdsk path existing before issuing ioctl to it

94345a3

jmpesp closed this Jan 28, 2026

jmpesp reopened this Jan 28, 2026

jmpesp added 7 commits January 29, 2026 15:00

Merge branch 'main' into local_storage_4_use_raw_zvols

5ae05e2

cargo hakari generate

00eadc2

revert Cargo.lock

c0962b7

cargo hakari generate

e406cab

e.is_not_ready not used for DeleteDatasetVolumeError

fe82030

dataset id typo

aded831

idempotently call Zfs::destroy_dataset

39fa3e5

leftwo reviewed Jan 30, 2026

View reviewed changes

jmpesp added 2 commits January 30, 2026 19:28

have to check status during ensure for raw vols as well

672324d

use another pattern for retrying in a loop

b3a2168

grwilson approved these changes Jan 31, 2026

View reviewed changes

Local storage [4/4]: Use raw zvols #9593

Are you sure you want to change the base?

Local storage [4/4]: Use raw zvols #9593

Conversation

jmpesp commented Jan 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmpesp commented Jan 7, 2026

Uh oh!

jmpesp commented Jan 12, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmpesp commented Jan 28, 2026

Uh oh!

leftwo commented Jan 29, 2026

Uh oh!

leftwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants