Skip to content
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

Overhaul block device interfaces #549

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

pfmooney
Copy link
Collaborator

This reshapes how block devices (frontends) interact with their attached backends. Rather than associating a backend with a device when it is created, backend and device are attached after the fact, when each has had an opportunity to be initialized.

Through its attachment handle, the device is able to notify the backend of new requests, but also query sizing parameters (previously specified during device construction), pause and unpause request retrieval, and (in the future) perform other tasks such as cache mode alteration.

The backend has a corresponding attachment handle through which it retrieves pending requests from the device -- behavior which is largely unchanged from the original structure.

Rather than store device-specific information required to issue request completions to the guest, the device emulation is expecting to use a Tracking structure which will store the completion data to be retrieved using an ID injected into the Request, which is passed back with its result when processed by the backend. This tracking structure also implements several generic USDT probes for tracing block events, rather than requiring the use of per-device probes.

@pfmooney
Copy link
Collaborator Author

I'm still working on additional testing for this, but I wanted to get it posted so folks could look through it at their leisure.

Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

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

A couple small nits but overall definitely an improvement. Thanks Patrick!

Comment on lines +38 to +39
#[usdt::provider(provider = "propolis")]
mod probes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the new device generic probes!

Also ++ on the complete probes providing timing themselves. Definitely beats the more error prone manual approach in a dtrace script. (Good bye results tainted by "dynamic variable drops"; you will not be missed 😭)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's still the hassle of requiring both begin/complete probes if you want sizing or offset information about the operations, but I think that's an acceptable compromise, rather than having an excessive argument list for the completion probes.

An open question following the integration of this work: Should some or all of the per-device probes (nvme, virtio, etc) be stripped out, since the block-generic ones cover most of those needs?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think per-device probes can be useful for correlating against in-guest operations with queue ids, command ids, etc

lib/propolis/src/block/mod.rs Show resolved Hide resolved
lib/propolis/src/block/mod.rs Outdated Show resolved Hide resolved
lib/propolis/src/block/mod.rs Outdated Show resolved Hide resolved
lib/propolis/src/block/crucible.rs Show resolved Hide resolved
lib/propolis/src/block/in_memory.rs Outdated Show resolved Hide resolved
lib/propolis/src/hw/nvme/mod.rs Show resolved Hide resolved
lib/propolis/src/block/mod.rs Outdated Show resolved Hide resolved
This reshapes how block devices (frontends) interact with their attached
backends.  Rather than associating a backend with a device when it is
created, backend and device are attached after the fact, when each has
had an opportunity to be initialized.

Through its attachment handle, the device is able to notify the backend
of new requests, but also query sizing parameters (previously specified
during device construction), pause and unpause request retrieval, and
(in the future) perform other tasks such as cache mode alteration.

The backend has a corresponding attachment handle through which it
retrieves pending requests from the device -- behavior which is largely
unchanged from the original structure.

Rather than store device-specific information required to issue request
completions to the guest, the device emulation is expecting to use a
`Tracking` structure which will store the completion data to be
retrieved using an ID injected into the Request, which is passed back
with its result when processed by the backend.  This tracking structure
also implements several generic USDT probes for tracing block events,
rather than requiring the use of per-device probes.
@pfmooney pfmooney merged commit 6a64a51 into oxidecomputer:master Nov 1, 2023
10 checks passed
@pfmooney pfmooney deleted the block-rework branch November 1, 2023 19:37
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.

2 participants