scsi rebase #301

Ablu · 2023-03-07T07:26:25Z

Summary of the PR

This is a rebase of #4.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR are signed (with git commit -s), and the commit
message has max 60 characters for the summary and max 75 characters for each
description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

Ablu · 2023-03-07T07:27:12Z

All commits in this PR are signed (with git commit -s), and the commit
message has max 60 characters for the summary and max 75 characters for each
description line.

Is there documentation of what signing means? I assume DCO, but I find it a bit weird to sign things without that having specified somewhere :)

Ablu

left some comments for myself

crates/scsi/rustfmt.toml

crates/scsi/src/main.rs

crates/scsi/src/scsi/emulation/block_device.rs

crates/scsi/src/virtio.rs

Ablu · 2023-03-08T16:14:40Z

Rough changelog:

squashed things, splitted into (hopefully sensible) chunks
moved tests under emulation (thats what they were testing)
implemented idx
fixed most previous comments

TODOs:

fix author ship... Somewhere during rebase I lost the authorship
review pubs. How do we want this crate to be consumed? @stsquad: Do we want this to just be a daemon or shall the interfaces be exported as lib (assumed the latter for now)?
throughout self-review

Ablu · 2023-03-08T16:19:52Z

@Gaelan: Did I get the authorship right? Do we need to credit other people/projects from which things were adopted?

crates/scsi/src/scsi/emulation/block_device.rs

Gaelan · 2023-03-08T16:30:30Z

Many thanks for picking this back up!

Did I get the authorship right? Do we need to credit other people/projects from which things were adopted?

All the code here was written by me (as best I recall; I'm generally pretty good about noting if it isn't). The work was done as part of GSoC, but I don't believe any copyright assignment takes place there.

EDIT: I do, at various points, quote the SCSI spec, which is an ISO standard that isn't (officially) freely available. I'm assuming short excerpts fall under fair use and it should be fine?

Incidentally, I had a scsi-write branch that may be of interest; there was sufficient write support to boot and run Debian, though I can't vouch for its correctness in the face of writeback failures - IIRC there's a tricky mismatch here between the level of detail SCSI expects and what the POSIX file APIs give us. I think I might just fsync everything, which is probably slow-but-good-enough?

Gaelan · 2023-03-08T16:53:16Z

Please let me know if you have any questions, by the way! This was a couple years ago now, so no promises, but happy to try and remember what I was thinking.

Ablu · 2023-03-09T08:16:08Z

All the code here was written by me (as best I recall; I'm generally pretty good about noting if it isn't). The work was done as part of GSoC, but I don't believe any copyright assignment takes place there.

Yeah, you had sign-offs on the relevant commits, mostly tried to make sure that the way I split up stuff is ok with you (since I put your name as main author). That said, we might be missing SPDX-License-Identifiers (at least current code has it, do not find anything in https://github.com/rust-vmm/community/blob/main/CONTRIBUTING.md). I would add: Apache-2.0 or BSD-3-Clause to all files, ok?

Incidentally, I had a scsi-write branch that may be of interest; there was sufficient write support to boot and run Debian, though I can't vouch for its correctness in the face of writeback failures - IIRC there's a tricky mismatch here between the level of detail SCSI expects and what the POSIX file APIs give us. I think I might just fsync everything, which is probably slow-but-good-enough?

I guess for now, I would aim at getting the readonly state merged. So I have not spent any thoughts on writes. Will check it out when I do so!

Please let me know if you have any questions, by the way! This was a couple years ago now, so no promises, but happy to try and remember what I was thinking.

Sure. Right now I focused on getting stuff work properly. Will let you know when I stumble over something. Feel free to provide review on the stuff that I changed!

crates/scsi/src/scsi/emulation/mod.rs

Ablu · 2023-03-13T08:32:09Z

Changelog (in order of significance):

Split the emulation interface into handling LUN-independent and LUN-specific messages
- This allows the LUN-specific part to be oblivious to the target
- Allowed for some de-duplication too
Create own trait to decouple from io::File
De-boilerplated the daemon by moving parsing to virtio.rs
- Still requires some boilerplate to copy between virtio and scsi datastructures
- Could be further simplified if we are fine with scsi-virtio-specific stuff returning datastructures of src/scsi/ (currently they were kept separated on purpose)
Removed no longer needed suppressing of linting (either due to simplified code or newer linter version)
Random cleanups here and there

Ablu · 2023-03-13T08:35:12Z

CI seems to be broken by dirty permissions from left-overs?

Ablu · 2023-03-13T08:40:39Z

Open clarification items:

SPDX identifiers?
abstraction-levels: What should be public? Which parts do we want to allow other people to use?
- "Just the daemon?" vs "daemon, virtio-scsi abstraction, target interface, inner-emulation interfaces (LogicalUnit)"?
How to test the daemon?
Meaning of signoff is not documented.
No CHANGELOG available.

crates/scsi/src/scsi/emulation/tests/generic.rs

Gaelan · 2023-03-13T10:27:12Z

crates/scsi/src/scsi/emulation/command.rs

+#[derive(Debug)]
+pub enum Command {
+    LunIndependentCommand(LunIndependentCommand),
+    LunSpecificCommand(LunSpecificCommand),


It's worth noting that several of the "LUN-specific" commands here are used for multiple LUN types, not just block devices. (TEST UNIT READY, REQUEST SENSE, REPORT SUPPORTED OPERATION CODES, and INQUIRY are required for all LUN types; commands like READ CAPACITY and READ are used by several storage-like devices.)

So if we ever wanted to do more than just block devices, we'd need to do some more thinking about how we want to handle that. Unfortunately we can't just parse a command without knowing the intended device type, as sometimes operation codes are reused.

Anyway, cross that bridge when we get to it - just potentially useful background.

Ablu · 2023-03-13T11:34:36Z

abstraction-levels: What should be public? Which parts do we want to allow other people to use?

"Just the daemon?" vs "daemon, virtio-scsi abstraction, target interface, inner-emulation interfaces (LogicalUnit)"?

Currently, we have multiple layers of abstraction in here:

the daemon that allows connection to the socket, parses things using the virtio module and forwards things to the EmulatedTarget after registering a BlockDevice for each image.
virtio that abstracts some of the virtio-scsi related parsing and provides some convenience features
scsi::Target that abstracts over the actual backend (only consists of execute_command)
scsi::emulation::target::EmulationTarget that implements scsi::Target by maintaining a list of LogicalUnits, handling REPORT_LUNS and forwarding other commands to the LogicalUnits.
scsi::emulation::block_device::BlockDevice that emulates an actual drive against a BlockDeviceBackend
scsi::emulation::block_device::FileBackend that implements BlockDeviceBackend

Comparing that against https://github.com/rust-vmm/vm-virtio/tree/main/crates/devices/virtio-blk/src, my feeling is that there the virtio and high-level device-specific abstractions are sharing the same data structures.

With that, there are a series of choices that could be made:

About virtio parsing + scsi::Target:
- Move it to https://github.com/rust-vmm/vm-virtio/tree/main/crates/devices.
  - Will probably require some extensive review and additions for features that we do not cover.
  - Unifying the two interface, would make the main.rs more simple, since no conversion is necessary.
- Keep it here and public.
  - People could depend on this without consuming the rest, but it would not be as clean as pulling it from a separate crate...
  - We would probably at least still need to document this a bit better...
- Keep it here, but mark it private.
  - Only our daemon would profit from it then.
  - we could mature it over time and eventually move it to https://github.com/rust-vmm/vm-virtio/tree/main/crates/devices.
- No real benefit from defining separate scsi (virtio-less) and scsi-virtio interfaces though...
About the emulation-specific abstraction
- Exposing these traits probably only makes sense if we also provide enough other support for the virtio layers in order to allow people to write their own daemon.
- If we do not do that, we should probably just mark that API as crate-private.

I got no strong feelings about any of these options and are open to opinions.

Ablu

Thanks @vireshk.

Fixed/reacted to all comments.

While rebasing I noticed missing tests. While adding them I went into a rabbit hole of trying to fix some API weirdness around the <W, R> generics. I ended up dropping them in favor of dynamic dispatch. That simplifies the code quite a bit. And I think spending too much time thinking through the monomorphization without any benchmarks in place is not worth scarifying code clarity.

Changelog

Dropped scsi-specific .lock file
Match the gpiod Cargo.toml fields
Fixed invocation in docs
Merged backend.rs into main.rs again
Switched from generics over Read/Write to dynamic dispatch
Dropped data_in/data_out from Request and turned them into ordinary function arguments
Minor API cleanup (some pub/pub(crate), &mut changes)
Made newlines more consistent
Dropped (for now) unused unmap field
Removed some left-over comment
Added tests for Write(10), WriteSame(16)
Dropped test_all* tests that would require annoying updates every time a new command is added
Reordered usings (cargo fmt -- --config group_imports=StdExternalCrate)

Ablu · 2023-05-12T04:49:55Z

crates/scsi/test/.gitignore

@@ -0,0 +1,2 @@
+results/


The test results will be downloaded to results/, but we do not want to have those in SCM. The .containerignore prevents sending the (potentially large) results/ and test-data/ folders when building the container (though I am not sure whether this is actually still as relevant with root-less containers, probably no copy happens under the hood anymore...) Especially test-data/ might become "large-ish" with the Fedora image and test blockdevice.

crates/scsi/Cargo.lock

crates/scsi/src/main.rs

crates/scsi/src/backend.rs

crates/scsi/README.md

crates/scsi/Cargo.toml

crates/scsi/README.md

epilys · 2023-05-16T10:46:05Z

crates/scsi/src/main.rs

+            info!("vhost-user connection closed with partial message. If the VM is shutting down, this is expected behavior; otherwise, it might be a bug.");
+        }
+        Err(e) => {
+            warn!("Error running daemon: {:?}", e);


Is this a warning or should it be log::error! instead?

This is copied from the other daemons, that also log warn!() here. I agree that error!() is probably better. But I think this may be better fixed once we get rid of the boilerplate code (https://linaro.atlassian.net/browse/ORKO-32).

crates/scsi/src/scsi/emulation/block_device.rs

epilys · 2023-05-16T10:53:59Z

crates/scsi/src/scsi/emulation/block_device.rs

+        // TODO: Ideally, this would be a read_vectored directly into guest
+        // address space. Instead, we have an allocation and several copies.
+
+        let mut ret = vec![0; (blocks * u64::from(self.backend.block_size())) as usize];


(Early optimization) this buffer (and the one in write_blocks()) looks like it's "one use" only. It could be an internal field of BlockDevice that gets cleared every time it's reused (which is constant time) to avoid reallocation.

This code certainly is not very optimal. However, when moving to a single buffer shared across read, we will keep the memory of a single large read allocated until we tear down the device. I think I would rather spend the time on implementing a proper vectored read (that avoids doing the extra copies altogether). Before investing more time around the Read/Write DescriptorChains implementations I would try to think about a way to combine that with rust-vmm/vm-virtio#33.

crates/scsi/src/scsi/emulation/block_device.rs

crates/scsi/src/scsi/emulation/command.rs

Ablu

@epilys: Thanks!

Changelog

Introduced new-types for ByteOffset, BlockOffset, BlockSize

Ablu · 2023-05-16T12:34:04Z

crates/scsi/src/scsi/emulation/block_device.rs

+        // TODO: Ideally, this would be a read_vectored directly into guest
+        // address space. Instead, we have an allocation and several copies.
+
+        let mut ret = vec![0; (blocks * u64::from(self.backend.block_size())) as usize];


This code certainly is not very optimal. However, when moving to a single buffer shared across read, we will keep the memory of a single large read allocated until we tear down the device. I think I would rather spend the time on implementing a proper vectored read (that avoids doing the extra copies altogether). Before investing more time around the Read/Write DescriptorChains implementations I would try to think about a way to combine that with rust-vmm/vm-virtio#33.

Ablu · 2023-05-16T12:36:42Z

crates/scsi/src/main.rs

+            info!("vhost-user connection closed with partial message. If the VM is shutting down, this is expected behavior; otherwise, it might be a bug.");
+        }
+        Err(e) => {
+            warn!("Error running daemon: {:?}", e);


This is copied from the other daemons, that also log warn!() here. I agree that error!() is probably better. But I think this may be better fixed once we get rid of the boilerplate code (https://linaro.atlassian.net/browse/ORKO-32).

crates/scsi/src/scsi/emulation/block_device.rs

Ablu · 2023-05-17T07:14:47Z

CI reports dropped coverage. But that seems to happen because the CI reports slightly different percentages compared to what my local toolchain reports. I will adjust the number to make CI happy once this is reviewed. Overall the coverage improved, the failure is only happening because I bumped the percentage to what the script tells me locally.

Ablu · 2023-06-02T07:52:52Z

Changelog

Rebased on top of main
Bumped dependencies to match other crates
Dropped unneeded dependency on queue test-utils

Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

This defines the basic interface that any scsi device will have to implement (along with some sensing constants that may be useful to share). The vast majority of this work was done by Gaelan Steele as part of a GSoC project [1][2]. [1] rust-vmm#4 [2] https://gist.github.com/Gaelan/febec4e4606e1320026a0924c3bf74d0 Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

This implements the previously defined interface by emulating the commands against a file-backed block device. The vast majority of this work was done by Gaelan Steele as part of a GSoC project [1][2]. [1] rust-vmm#4 [2] https://gist.github.com/Gaelan/febec4e4606e1320026a0924c3bf74d0 Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

The vast majority of this work was done by Gaelan Steele as part of a GSoC project [1][2]. [1] rust-vmm#4 [2] https://gist.github.com/Gaelan/febec4e4606e1320026a0924c3bf74d0 Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

This adds the virtio-specific parts that use the previously formed interfaces and scsi emulation in order to build a daemon that offers files from the host system as drives to the guest. The vast majority of this work was done by Gaelan Steele as part of a GSoC project [1][2]. [1] rust-vmm#4 [2] https://gist.github.com/Gaelan/febec4e4606e1320026a0924c3bf74d0 Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

Signed-off-by: Erik Schilling <[email protected]>

Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

The config that we send is based on the current QEMU defaults (as of 60ca584b8af0de525656f959991a440f8c191f12). This allows testing using Alex Bennee's vhost-user-generic series and will be required for hypervisors that do not come with the stubs that QEMU has (eg: Xen). Signed-off-by: Erik Schilling <[email protected]> Link: https://lore.kernel.org/all/[email protected]/

This adds write support. Being very similar to READ(10) in structure, much of the code is very similar. Signed-off-by: Erik Schilling <[email protected]>

WRITE SAME allows writing a block for a repeated number of times. Mostly, it can also be used to deallocate parts of the block device (the fstrim functionality uses this). We do not support that aspect yet. Instead, we will just stupidly repeat the pattern as many times as we are told. A future, smarter implementation could just punch a hole into the backend instead of filling it with zeros. Signed-off-by: Erik Schilling <[email protected]>

While the command also allows syncing individual regions of a LUN, we do not support that here and simply sync the entire file. Signed-off-by: Erik Schilling <[email protected]>

This provides some tooling for running blktests. The README.md contains documentation about the architecture. I am seeing some race-conditions that sometimes lead to boot freezes [1], so this is not really ready for automatic evaluation during a CI pipeline. [1] https://linaro.atlassian.net/browse/ORKO-37 Signed-off-by: Erik Schilling <[email protected]> Link: https://linaro.atlassian.net/browse/ORKO-17

Polishing this up for inclusion is currently not high on the priority list, but we can at least link it. Signed-off-by: Erik Schilling <[email protected]>

Ablu · 2023-06-05T08:13:15Z

Changelog

Hopefully fixed coverage

vireshk · 2023-06-05T09:14:02Z

This is just too much (and complex) code to review. I have looked at it from the perspective of vhost-device stuff and that it is implemented in a way similar to other crates in the workspace.

Approving it now.

@stsquad @mathieupoirier @stefano-garzarella

stsquad · 2023-06-05T09:29:56Z

This is just too much (and complex) code to review. I have looked at it from the perspective of vhost-device stuff and that it is implemented in a way similar to other crates in the workspace.

Approving it now.

@stsquad @mathieupoirier @stefano-garzarella

I think its had some good cycles of review but there is a trade off to a growing stack of commits vs incremental updates. I'm happy with merging this so follow on enhancements aren't forever blocked.

Ablu requested review from mathieupoirier, stsquad, vireshk and stefano-garzarella as code owners March 7, 2023 07:26

Ablu mentioned this pull request Mar 7, 2023

Initial vhost-user-scsi implementation #4

Closed

Ablu force-pushed the scsi-rebase branch from 412f253 to e6e99e1 Compare March 7, 2023 08:22

Ablu commented Mar 7, 2023

View reviewed changes

Ablu mentioned this pull request Mar 7, 2023

virtio-bindings: add scsi bindings rust-vmm/vm-virtio#231

Merged

4 tasks

Ablu marked this pull request as draft March 8, 2023 14:53

Ablu force-pushed the scsi-rebase branch from e6e99e1 to 4c0b8b7 Compare March 8, 2023 16:10

Ablu force-pushed the scsi-rebase branch from 4c0b8b7 to 8824852 Compare March 8, 2023 16:18

Ablu commented Mar 8, 2023

View reviewed changes

crates/scsi/src/scsi/emulation/block_device.rs Outdated Show resolved Hide resolved

Ablu commented Mar 9, 2023

View reviewed changes

crates/scsi/src/scsi/emulation/mod.rs Outdated Show resolved Hide resolved

Ablu force-pushed the scsi-rebase branch 2 times, most recently from 4097016 to f0eab05 Compare March 13, 2023 08:21

Ablu marked this pull request as ready for review March 13, 2023 08:26

Ablu force-pushed the scsi-rebase branch 3 times, most recently from c8577be to 7c45dd6 Compare March 13, 2023 09:50

Gaelan reviewed Mar 13, 2023

View reviewed changes

Ablu force-pushed the scsi-rebase branch from bcdc1ef to a001e66 Compare May 16, 2023 09:00

Ablu commented May 16, 2023

View reviewed changes

epilys reviewed May 16, 2023

View reviewed changes

Ablu force-pushed the scsi-rebase branch from a001e66 to 451064f Compare May 16, 2023 19:26

Ablu commented May 16, 2023

View reviewed changes

Ablu force-pushed the scsi-rebase branch 2 times, most recently from a862ab8 to e7ac0c2 Compare June 2, 2023 07:52

Gaelan and others added 13 commits June 5, 2023 08:56

scsi: Initial boilerplate.

eb26a8c

Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

scsi: Add tests for daemon and virtio code

5947a41

Signed-off-by: Erik Schilling <[email protected]>

scsi: Add documentation

d0e26b1

Co-developed-by: Erik Schilling <[email protected]> Signed-off-by: Erik Schilling <[email protected]> Signed-off-by: Gaelan Steele <[email protected]>

scsi: add support for WRITE(10)

7aea10c

This adds write support. Being very similar to READ(10) in structure, much of the code is very similar. Signed-off-by: Erik Schilling <[email protected]>

scsi: add support for SYNCHRONIZE CACHE(10)

8bad5fa

While the command also allows syncing individual regions of a LUN, we do not support that here and simply sync the entire file. Signed-off-by: Erik Schilling <[email protected]>

scsi: mention out-of-tree fuzzing infrastructure

81fd2f6

Polishing this up for inclusion is currently not high on the priority list, but we can at least link it. Signed-off-by: Erik Schilling <[email protected]>

Ablu force-pushed the scsi-rebase branch from e7ac0c2 to 81fd2f6 Compare June 5, 2023 08:12

vireshk enabled auto-merge (rebase) June 5, 2023 09:14

vireshk approved these changes Jun 5, 2023

View reviewed changes

stsquad approved these changes Jun 5, 2023

View reviewed changes

vireshk merged commit f50e714 into rust-vmm:main Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scsi rebase #301

scsi rebase #301

Ablu commented Mar 7, 2023 •

edited

Loading

Ablu commented Mar 7, 2023

Ablu left a comment

Ablu commented Mar 8, 2023 •

edited

Loading

Ablu commented Mar 8, 2023

Gaelan commented Mar 8, 2023 •

edited

Loading

Gaelan commented Mar 8, 2023

Ablu commented Mar 9, 2023

Ablu commented Mar 13, 2023

Ablu commented Mar 13, 2023 •

edited

Loading

Ablu commented Mar 13, 2023 •

edited

Loading

Gaelan Mar 13, 2023

Ablu commented Mar 13, 2023

Ablu left a comment

Ablu May 12, 2023

epilys May 16, 2023

Ablu May 16, 2023

epilys May 16, 2023

Ablu May 16, 2023

Ablu left a comment

Ablu May 16, 2023

Ablu May 16, 2023

Ablu commented May 17, 2023 •

edited

Loading

Ablu commented Jun 2, 2023

Ablu commented Jun 5, 2023

vireshk commented Jun 5, 2023

stsquad commented Jun 5, 2023

scsi rebase #301

scsi rebase #301

Conversation

Ablu commented Mar 7, 2023 • edited Loading

Summary of the PR

Requirements

Ablu commented Mar 7, 2023

Ablu left a comment

Choose a reason for hiding this comment

Ablu commented Mar 8, 2023 • edited Loading

Ablu commented Mar 8, 2023

Gaelan commented Mar 8, 2023 • edited Loading

Gaelan commented Mar 8, 2023

Ablu commented Mar 9, 2023

Ablu commented Mar 13, 2023

Ablu commented Mar 13, 2023 • edited Loading

Ablu commented Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Ablu commented Mar 13, 2023

Ablu left a comment

Choose a reason for hiding this comment

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ablu left a comment

Choose a reason for hiding this comment

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ablu commented May 17, 2023 • edited Loading

Ablu commented Jun 2, 2023

Changelog

Ablu commented Jun 5, 2023

Changelog

vireshk commented Jun 5, 2023

stsquad commented Jun 5, 2023

Ablu commented Mar 7, 2023 •

edited

Loading

Ablu commented Mar 8, 2023 •

edited

Loading

Gaelan commented Mar 8, 2023 •

edited

Loading

Ablu commented Mar 13, 2023 •

edited

Loading

Ablu commented Mar 13, 2023 •

edited

Loading

Ablu commented May 17, 2023 •

edited

Loading