Skip to content

scsidisk: Support WRITE_ATOMIC(16) SCSI command in NVMe-backed storage #1518

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eric135
Copy link
Contributor

@eric135 eric135 commented Jun 16, 2025

This PR adds support for the WRITE_ATOMIC(16) SCSI command in the SCSI device OpenHCL presents to guests. NVMe devices support atomic writes implicitly, as long as the bounds of the operation are kept within specific bounds specified in the NVMe identify information.

#947

.unwrap_or(0),
"transfer length OR non-zero atomic boundary not multiple of atomic transfer length granularity"
);
return Err(ScsiError::IllegalRequest(AdditionalSenseCode::INVALID_CDB));
Copy link
Contributor

Choose a reason for hiding this comment

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

I recognize that you are still mulling over the validation strategy here, but this type of thing is useful to include as a unit test (basic packet parsing & protocol validations).

let cdb = scsi::Cdb16Atomic::read_from_prefix(&request.cdb[..])
.unwrap()
.0; // TODO: zerocopy: use-rest-of-range (https://github.com/microsoft/openvmm/issues/759)
let len = cdb.transfer_blocks.get() as u64;
Copy link
Member

@jstarks jstarks Jun 18, 2025

Choose a reason for hiding this comment

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

len in Rust usually refers to a byte length. Consider block_count or sector_count something.

if len as u32
% self
.scsi_parameters
.atomic_transfer_length_granularity
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to require the granularity to be a power of 2? If so, avoid %--store a shift and use shift/mask operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be dependent upon the underlying storage. I don't think we can guarantee such a thing.

@jstarks
Copy link
Member

jstarks commented Jun 18, 2025

So this changes the DiskIo contract--writes must be atomic if the DiskIo implementation says it supports atomic writes.

What that means is that this will be impractical to implement for anything other than our NVMe driver, since other interfaces require opt in (e.g., SCSI, Linux block layer). This may be artificially limiting.

I think we should consider adding some kind of write_atomic method that would allow explicit opt in, plus some boolean metadata that says if all appropriately sized+aligned writes are atomic. The SCSI emulator would use write_atomic, but the NVMe emulator would just use the existing write method and only report NVMe atomic write metadata if the "always atomic" bit is set.

@jstarks
Copy link
Member

jstarks commented Jun 18, 2025

There's zero way to test any of this currently. I think you need to implement this in the NVMe emulator and something like RAM disk (which will also require extending this through the layered disk infra... which shouldn't be too bad).

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.

3 participants