Skip to content

Do not set volblocksize for raw zvols#10186

Merged
jmpesp merged 3 commits into
oxidecomputer:mainfrom
jmpesp:no_set_volblocksize_for_raw_vol
Mar 31, 2026
Merged

Do not set volblocksize for raw zvols#10186
jmpesp merged 3 commits into
oxidecomputer:mainfrom
jmpesp:no_set_volblocksize_for_raw_vol

Conversation

@jmpesp

@jmpesp jmpesp commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Internal server errors were seen trying to initialize raw zvols after integrating optimized allocation: sled-agent is attempting to set a volblocksize that will be ignored as a better one will be automatically selected, and the current comparison for expected and actual volblocksize fails.

Change sled-agent to not set a volblocksize for raw zvols. In fact, structurally change the DatasetVolumeEnsureArgs so that raw zvols will never have volblocksize set.

Fixes #10184

Internal server errors were seen trying to initialize raw zvols after
integrating optimized allocation: sled-agent is attempting to set a
volblocksize that will be ignored as a better one will be automatically
selected, and the current comparison for expected and actual
volblocksize fails.

Change sled-agent to not set a volblocksize for raw zvols. In fact,
structurally change the `DatasetVolumeEnsureArgs` so that raw zvols will
never have volblocksize set.

Fixes oxidecomputer#10184
@jmpesp jmpesp requested a review from leftwo March 30, 2026 16:43

@jgallagher jgallagher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In fact, structurally change the DatasetVolumeEnsureArgs so that raw zvols will never have volblocksize set.

I love this! Made a small style suggestion about how we consume this; the getter methods that suppress fields other than the one they're getting make perfect sense, but I think I'd try to avoid it in other code.

Comment thread illumos-utils/src/zfs.rs Outdated
args.push("rawvol=on".to_string());
}
match &params {
DatasetVolumeEnsureArgs::Raw { .. } => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Matching on .. is kinda awkward, since adding new fields in the future will silently be accepted by this. I wonder if it would be more straightforward (albeit with a little duplication) to match on all the fields? Totally untested but something like:

let args = match &params {
    DatasetVolumeEnsureArgs::Raw { name, size } => vec![
        "-V".to_string(), size.to_bytes().to_string(),
        "-o".to_string(), "rawvol=on".to_string(),
        name.to_string(),
    ],
    DatasetVolumeEnsureArgs::Regular { name, size, volblocksize } => {
        let mut args = vec![ "-V".to_string(), size.to_bytes().to_string()];
        if let Some(volblocksize) = &volblocksize {
            args.push("-o".to_string());
            args.push(format!("volblocksize={}", volblocksize));
        }
        args.push(name.to_string();
        args
    }
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point - done in c405abd

Comment thread illumos-utils/src/zfs.rs
Comment thread illumos-utils/src/zfs.rs Outdated
args.push("-o".to_string());
args.push("rawvol=on".to_string());

// No need to set volblocksize: either the default record size

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the volblocksize value requested here now becomes the floor (minimum) for the resulting zvol. If we can't find enough of the requested size, then then init will fail.

If someone wanted a specific minimum, then they can send a value here, so we should update this comment to reflect that. Also. #915 is integrated :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a bit to the comment saying that volblocksize is the minimum like this - I still think it should use the default record size for that minimum so I didn't change any code. See: c405abd

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still think it should use the default record size for that minimum so I didn't change any code

Agreed.

jmpesp added 2 commits March 30, 2026 17:52
update comment to mention that volblocksize is minimum
@jmpesp jmpesp merged commit 68e62ef into oxidecomputer:main Mar 31, 2026
16 checks passed
@jmpesp jmpesp deleted the no_set_volblocksize_for_raw_vol branch March 31, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

500 error with local storage create: expected volblocksize to be 131072, but saw 16777216

3 participants