Page MenuHomeFreeBSD

VirtIO SCSI online disk resize
Needs ReviewPublic

Authored by pjd on Sep 26 2019, 7:23 PM.

Details

Summary

The patch implements support for online disks resize fr virtio-scsi. The patch was tested in Google Cloud Platform.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pjd created this revision.Sep 26 2019, 7:23 PM
mav added a comment.Sep 26 2019, 7:42 PM

This virtio API looks ugly. They conform themselves that device should also report normal UNIT ATTENTION with the same status, which is already handled by CAM DA driver. If we assume that device does generate UNIT ATTENTION, then we should be able to make cam send TUR request to see it by using AC_SCSI_AEN async. Alternatively, it is possible to simulate UNIT ATTENTION receive by using AC_UNIT_ATTENTION with fake CCB, including ASC/ASCQ received from virtio API. Your method should also work, but what I don\t very like it too strict parsing ASC/ASCQ and sending not exactly correct async.

pjd added a comment.Sep 27 2019, 5:39 AM
In D21807#476017, @mav wrote:

This virtio API looks ugly. They conform themselves that device should also report normal UNIT ATTENTION with the same status, which is already handled by CAM DA driver. If we assume that device does generate UNIT ATTENTION, then we should be able to make cam send TUR request to see it by using AC_SCSI_AEN async. Alternatively, it is possible to simulate UNIT ATTENTION receive by using AC_UNIT_ATTENTION with fake CCB, including ASC/ASCQ received from virtio API. Your method should also work, but what I don\t very like it too strict parsing ASC/ASCQ and sending not exactly correct async.

The same method (AC_INQ_CHANGED) is used by camcontrol reprobe too.
The HyperV doesn't send unit attention.
What do you mean by 'too strict parsing of ASC/ASCQ'?

Thanks.

imp added inline comments.Sep 27 2019, 1:38 PM
sys/dev/virtio/scsi/virtio_scsi.c
1773

These are oddly specific values....

mav added a comment.Sep 27 2019, 1:38 PM
In D21807#476160, @pjd wrote:

What do you mean by 'too strict parsing of ASC/ASCQ'?

I mean that considering the codes are passed and they are called ASC/ASCQ in specification, there potentially may be other ones too, for which CAM may have own more specific handling ideas, and I believe it is not a SIM driver duty to handle details of SCSI protocol. I don't object your implementation, just thinking out loud whether it could it be done better/otherwise.

mav added inline comments.Sep 27 2019, 1:40 PM
sys/dev/virtio/scsi/virtio_scsi.c
1773

It is SCSI sense "Capacity data has changed".

pjd added a comment.Sep 27 2019, 6:27 PM
In D21807#476253, @mav wrote:
In D21807#476160, @pjd wrote:

What do you mean by 'too strict parsing of ASC/ASCQ'?

I mean that considering the codes are passed and they are called ASC/ASCQ in specification, there potentially may be other ones too, for which CAM may have own more specific handling ideas, and I believe it is not a SIM driver duty to handle details of SCSI protocol. I don't object your implementation, just thinking out loud whether it could it be done better/otherwise.

I see what you mean. Others implement this the same way:
Linux: https://github.com/torvalds/linux/blob/master/drivers/scsi/virtio_scsi.c#L304
Windows: https://github.com/virtio-win/kvm-guest-drivers-windows/blob/master/vioscsi/vioscsi.c#L1621

I'll experiment with a fake UNIT ATTENTION and just pass ASC/ASCQ along.

mav added a comment.Sep 28 2019, 3:31 PM
In D21807#476430, @pjd wrote:

I see. Though they both handle 3 statuses, not just one.

sef added a subscriber: sef.Sep 28 2019, 10:17 PM
sef added inline comments.
sys/dev/virtio/scsi/virtio_scsi.c
1773

Yeah but why aren't they #defined?

pjd added a comment.Oct 7 2019, 6:01 PM
In D21807#476635, @mav wrote:
In D21807#476430, @pjd wrote:

I see. Though they both handle 3 statuses, not just one.

I can add the other two, I just was able to test just this one.

sys/dev/virtio/scsi/virtio_scsi.c
1773

For some reason none of the code has define for them. Take a look for example at sys/cam/scsi/scsi_da.c and search for ascq.

pjd updated this revision to Diff 63176.Oct 12 2019, 7:16 AM

After discussion with mav@, add ASCQs for PARAMETERS CHANGED and MODE PARAMETERS CHANGED, similar to Linux and Windows. Also add a comment describing those values.