virtio-scsi: handle device capacity change event
ClosedPublic
Actions

Authored by ray on Tue, May 26, 2:57 PM.

Details

Reviewers

imp
asomers
andrew
kib
markj
glebius

Commits

rGf056e84e5b46: virtio-scsi: handle device capacity change event

Summary

This feature is utilized when updating storage capacity in capable hypervisors such as QEMU.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 73482
Build 70365: arc lint + arc unit

Event Timeline

ray requested review of this revision.Tue, May 26, 2:57 PM

ray created this revision.

Harbormaster completed remote builds in B73445: Diff 178608.Tue, May 26, 2:57 PM

Not a domain expert, so leaving this 100% to Warner. Code & style wise everything LGTM.

P.S. Oleksandr, if you got review from any other domain expert than Warner, consider this approved by mentor (me).

In D57247#1311835, @glebius wrote:

P.S. Oleksandr, if you got review from any other domain expert than Warner, consider this approved by mentor (me).

Sure.
Many thanks, Gleb!

Invite asomers, andrew, markj, and kib to help with review.
Thanks, guys!

ray added a reviewer: markj.Wed, May 27, 2:57 PM

Not really a domain expert, either, am I. Can you give me a clue about how to test this? I can use virtio-scsi with bhyve. But how does one tell bhyve to tell the guest that the storage capacity has changed?

In D57247#1312477, @asomers wrote:

Not really a domain expert, either, am I. Can you give me a clue about how to test this? I can use virtio-scsi with bhyve. But how does one tell bhyve to tell the guest that the storage capacity has changed?

Hey, Alan!
Thanks for joining!
bhyve is unable. At least yet :)
I did it with qemu:

-device virtio-scsi-pci,id=scsi0 \
-drive file=command.drive,id=slon,format=raw,if=none,discard=unmap,cache=none \
-device scsi-hd,drive=slon,bus=scsi0.0 \

then through the qemu's monitor:

block_resize slon 2G

In D57247#1312477, @asomers wrote:

Not really a domain expert, either, am I. Can you give me a clue about how to test this? I can use virtio-scsi with bhyve. But how does one tell bhyve to tell the guest that the storage capacity has changed?

bhyve doesn't advertise VIRTIO_SCSI_F_CHANGE in any case, I guess you'd have to use qemu or something else.

sys/dev/virtio/scsi/virtio_scsi.c
1787	Missing a newline here.
1801	Shouldn't we print the sense code in the default case, similar to what vtscsi_handle_event() does? Otherwise we're just silently ignoring events.

Fix new-line.
Report unhandled case.

Harbormaster completed remote builds in B73482: Diff 178742.Wed, May 27, 4:17 PM

sys/dev/virtio/scsi/virtio_scsi.c
1801	Shouldn't we print the sense code in the default case, similar to what vtscsi_handle_event() does? Otherwise we're just silently ignoring events. Fixed. Thanks for the review, Mark!

two minor nits. Use your judgement on resolving them. The the sbuf thing is too hard, then what's there is fine for the error case.

sys/dev/virtio/scsi/virtio_scsi.c
1806	We have a pretty printer for this stuff in scsi_sense_desc_sbuf, but it requires a sense buffer. I've not checked to see if event->reason is/has that or not.
sys/dev/virtio/scsi/virtio_scsi.h
168	https://www.t10.org/lists/asc-num.txt is better source since it's the canonical source, though this link is interesting. Will have to read in detail later. 2Ah/09h DZ CAPACITY DATA HAS CHANGED

This revision is now accepted and ready to land.Mon, Jun 8, 4:15 PM

Closed by commit rGf056e84e5b46: virtio-scsi: handle device capacity change event (authored by ray). · Explain WhyThu, Jun 11, 1:07 PM

This revision was automatically updated to reflect the committed changes.

ray marked an inline comment as done.

ray added a commit: rGf056e84e5b46: virtio-scsi: handle device capacity change event.

ray marked 2 inline comments as done.Thu, Jun 11, 1:07 PM

ray added inline comments.

sys/dev/virtio/scsi/virtio_scsi.c
1806	I've inspected it before and repeated it after your comment. Way too much to include it here :)
sys/dev/virtio/scsi/virtio_scsi.h
168	Replaced with https://www.t10.org/lists/asc-num.txt Thanks!