[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] virtio-scsi: Send "REPORTED LUNS CHANGED" sense data upon
From: |
Venu Busireddy |
Subject: |
Re: [PATCH v3] virtio-scsi: Send "REPORTED LUNS CHANGED" sense data upon disk hotplug events. |
Date: |
Thu, 29 Sep 2022 17:31:04 -0500 |
On 2022-09-29 12:49:51 +0200, Paolo Bonzini wrote:
> On Wed, Sep 28, 2022 at 8:06 PM Venu Busireddy
> <venu.busireddy@oracle.com> wrote:
> >
> > Section 5.6.6.3 of VirtIO specification states, "Events will also
> > be reported via sense codes..." However, no sense data is sent when
> > VIRTIO_SCSI_EVT_RESET_RESCAN or VIRTIO_SCSI_EVT_RESET_REMOVED events
> > are reported (when disk hotplug/hotunplug events occur). SCSI layer
> > on Solaris depends on this sense data, and hence does not handle disk
> > hotplug/hotunplug events.
> >
> > When disk inventory changes, return a CHECK_CONDITION status with sense
> > data of 0x06/0x3F/0x0E (sense code REPORTED_LUNS_CHANGED), as per the
> > specifications in Section 5.14 (h) of SAM-4.
> >
> > Signed-off-by: Venu Busireddy <venu.busireddy@oracle.com>
> >
> > v2 -> v3:
> > - Implement the suggestion from Paolo Bonzini <pbonzini@redhat.com>.
> >
> > v1 -> v2:
> > - Send the sense data for VIRTIO_SCSI_EVT_RESET_REMOVED event too.
> > ---
> > hw/scsi/scsi-bus.c | 1 +
> > hw/scsi/virtio-scsi.c | 16 ++++++++++++++++
> > include/hw/scsi/scsi.h | 6 ++++++
> > include/hw/virtio/virtio-scsi.h | 1 +
> > 4 files changed, 24 insertions(+)
> >
> > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> > index 4403717c4aaf..b7cb249f2eab 100644
> > --- a/hw/scsi/scsi-bus.c
> > +++ b/hw/scsi/scsi-bus.c
> > @@ -730,6 +730,7 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag,
> > uint32_t lun,
> > */
> > !(buf[0] == REQUEST_SENSE && d->sense_is_ua))) {
> > ops = &reqops_unit_attention;
> > + d->clear_reported_luns_changed = true;
>
> Any reason to have this flag, and not just clear
> s->reported_luns_changed after scsi_req_new? Is it to handle the
> invalid opcode case?
Immediately after a hotunplug event, qemu (without any action from
the guest) processes a REPORT_LUNS command on the lun 0 of the device
(haven't figured out what causes this). If we unconditionally clear
the s->reported_luns_changed flag right after calling scsi_req_new(),
the action taken in scsi_device_set_ua() is undone by the eventual call
to scsi_clear_unit_attention(). Here is the sequence of the events:
(Note: SCSIDevice = 0x7ff180005010 is lun 1, and SCSIDevice = 0x557fed88fd40 is
lun 0)
virtio_scsi_hotunplug(): Entered, reported_luns_changed = 0, VirtIOSCSI =
0x557feda9f750, SCSIDevice = 0x7ff180005010, bus = 0x557feda9f9c0
virtio_scsi_hotunplug(): Exiting, reported_luns_changed = 1, VirtIOSCSI =
0x557feda9f750, SCSIDevice = 0x7ff180005010, bus = 0x557feda9f9c0
virtio_scsi_handle_cmd_req_prepare(): Entered, reported_luns_changed = 1,
VirtIOSCSI = 0x557feda9f750, SCSIDevice = 0x557fed88fd40, cdb[0] = 0xa0
scsi_device_set_ua(): Entered, SCSIDevice = 0x557fed88fd40
scsi_device_set_ua(): prec1 = 0x7fffffff, sdev->key = 0, sdev->asc = 0x00,
sdev->ascq = 0x00
scsi_device_set_ua(): prec2 = 0x00003f0e, sense->key = 6, sense->asc = 0x3f,
sense->ascq = 0x0e
scsi_req_new(): SCSIDevice = 0x557fed88fd40, bus = 0x557feda9f9c0, buf[0] = a0
scsi_req_new(): sdev.key = 6, sdev.asc = 0x3f, sdev.ascq = 0x0e
virtio_scsi_handle_cmd_req_prepare(): Exiting, reported_luns_changed = 0,
VirtIOSCSI = 0x557feda9f750, SCSIDevice = 0x557fed88fd40, cdb[0] = 0xa0
scsi_clear_unit_attention(): Entered, buf[0] = 0xa0, SCSIDevice =
0x557fed88fd40, key = 6, asc = 0x3f, ascq = 0x0e
scsi_clear_unit_attention(): Exiting, buf[0] = 0xa0, SCSIDevice =
0x557fed88fd40, key = 0, asc = 0x00, ascq = 0x00
As can be seen, before the guest does anything, we cleared the
reported_luns_changed flag as well as the unit attention condition.
Therefore, when the guest eventually sends a TEST_UNIT_READY command,
we don't report anything back, as can be seen by the traces below:
virtio_scsi_handle_cmd_req_prepare(): Entered, reported_luns_changed = 0,
VirtIOSCSI = 0x557feda9f750, SCSIDevice = 0x557fed88fd40, cdb[0] = 0x00
scsi_req_new(): SCSIDevice = 0x557fed88fd40, bus = 0x557feda9f9c0, buf[0] = 00
scsi_req_new(): sdev.key = 0, sdev.asc = 0x00, sdev.ascq = 0x00
virtio_scsi_handle_cmd_req_prepare(): Exiting, reported_luns_changed = 0,
VirtIOSCSI = 0x557feda9f750, SCSIDevice = 0x557fed88fd40, cdb[0] = 0x00
scsi_clear_unit_attention(): Entered, buf[0] = 0x00, SCSIDevice =
0x557fed88fd40, key = 0, asc = 0x00, ascq = 0x00
That is why we need the d->clear_reported_luns_changed flag, to know
when we genuinely processed a command from the guest and only then clear
the reported_luns_changed flag.
>
> I just reread the code and noticed that there is also a *bus* unit
> attention mechanism, which is unused but seems perfect for this
> usecase. The first device on the bus to execute a command successfully
> will consume it.
>
> You need something like
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 4403717c4a..78274e8477 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1616,6 +1616,24 @@ static int scsi_ua_precedence(SCSISense sense)
> return (sense.asc << 8) | sense.ascq;
> }
>
> +void scsi_bus_set_ua(SCSIBus *bus, SCSISense sense)
> +{
> + int prec1, prec2;
> + if (sense.key != UNIT_ATTENTION) {
> + return;
> + }
> +
> + /*
> + * Override a pre-existing unit attention condition, except for a more
> + * important reset condition.
> + */
> + prec1 = scsi_ua_precedence(bus->unit_attention);
> + prec2 = scsi_ua_precedence(sense);
> + if (prec2 < prec1) {
> + bus->unit_attention = sense;
> + }
> +}
> +
> void scsi_device_set_ua(SCSIDevice *sdev, SCSISense sense)
> {
> int prec1, prec2;
I tried the above suggestion. Even with the new suggestion, we suffer
the same fate as with v3, except in this case, we end up clearing the
bus->unit_attention instead of device->unit_attention! Traces below:
(Note: SCSIDevice = 0x7f2c54027790 is lun 1, and SCSIDevice = 0x5599135fbd40 is
lun 0)
virtio_scsi_hotunplug(): Entered, VirtIOSCSI = 0x55991380b750, SCSIDevice =
0x7f2c54027790, bus = 0x55991380b9c0
scsi_bus_set_ua(): Entered, bus = 0x55991380b9c0
scsi_bus_set_ua(): prec1 = 0x7fffffff, bus->key = 0, bus->asc = 0x00,
bus->ascq = 0x00
scsi_bus_set_ua(): prec2 = 0x00003f0e, sense->key = 6, sense->asc = 0x3f,
sense->ascq = 0x0e
virtio_scsi_hotunplug(): Exiting
virtio_scsi_handle_cmd_req_prepare(): Entered, VirtIOSCSI = 0x55991380b750,
SCSIDevice = 0x5599135fbd40, cdb[0] = 0xa0
scsi_req_new(): SCSIDevice = 0x5599135fbd40, bus = 0x55991380b9c0, buf[0] = a0
scsi_req_new(): bus.key = 6, bus.asc = 0x3f, bus.ascq = 0x0e
scsi_clear_unit_attention(): Entered, buf[0] = 0xa0, bus = 0x55991380b9c0, key
= 6, asc = 0x3f, ascq = 0x0e
scsi_clear_unit_attention(): Exiting, buf[0] = 0xa0, bus = 0x55991380b9c0, key
= 0, asc = 0x00, ascq = 0x00
At the end of the above sequence, bus->unit_attention is cleared! After
that, when a TEST_UNIT_READY command arrives from the guest, we do not
report anything back, because no unit_attention is pending, as seen below:
virtio_scsi_handle_cmd_req_prepare(): Entered, VirtIOSCSI = 0x55991380b750,
SCSIDevice = 0x5599135fbd40, cdb[0] = 0x00
scsi_req_new(): SCSIDevice = 0x5599135fbd40, bus = 0x0x55991380b9c0, buf[0] = 00
scsi_req_new(): bus.key = 0, bus.asc = 0x00, bus.ascq = 0x00
scsi_clear_unit_attention(): Entered, buf[0] = 0x00, bus = 0x55991380b9c0, key
= 0, asc = 0x00, ascq = 0x00
As a result, the guest does not see any REPORTED_LUNS_CHANGED sense data.
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 88e1a48343..0c86d0359f 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -186,6 +186,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(
> BlockdevOnError rerror,
> BlockdevOnError werror,
> const char *serial, Error **errp);
> +void scsi_bus_set_ua(SCSIBus *bus, SCSISense sense);
> void scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
> void scsi_legacy_handle_cmdline(void);
>
>
> and if you call the new function in the plug/unplug callbacks it
> should just work.
What shall we do? Keep the d->clear_reported_luns_changed? Or, is there
a better way to define/handle that flag?
Regards,
Venu
>
> Paolo
>