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

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.

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.

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

These are oddly specific values....

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.

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

It is SCSI sense "Capacity data has changed".

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.

sef added inline comments.
sys/dev/virtio/scsi/virtio_scsi.c
1772

Yeah but why aren't they #defined?

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
1772

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.

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.