Re: [PATCH 1/3] scsi: fetch unit attention when creating the request

[Paolo Bonzini]

Yes, it looks like an easy backport. Adding Michael Tokarev and qemu-stable.

Paolo


On Wed, Oct 9, 2024 at 6:03 PM Michael Galaxy <mgalaxy@akamai.com> wrote:
>
> Hi All,
>
> We have stumbled upon this bug in our production systems on QEMU 7.2.x.
> This is a pretty nasty bug because it has the effect of causing the root
> filesystem in the guest to switch into read only mode if our block
> storage products change attachments to running virtual machines.
>
> Could we kindly ask to pull this identical patch for 7.2.15?
>
> Last year, it just went to master and landed in 8.0.50. We're planning
> to upgrade, but it will be quite some time before we get around to that,
> and I suspect others are also running 7.2.x in production.
>
> - Michael Galaxy
>
> On 7/12/23 08:43, Stefano Garzarella wrote:
> > Commit 1880ad4f4e ("virtio-scsi: Batched prepare for cmd reqs") split
> > calls to scsi_req_new() and scsi_req_enqueue() in the virtio-scsi device.
> > This had no drawback, until commit 8cc5583abe ("virtio-scsi: Send
> > "REPORTED LUNS CHANGED" sense data upon disk hotplug events") added a
> > bus unit attention.
> >
> > Having the two calls separated, all requests in the batch were prepared
> > calling scsi_req_new() to report a sense.
> > Then only the first one submitted calling scsi_req_enqueue() reported the
> > right sense and reset it to NO_SENSE.
> > The others reported NO_SENSE, causing SCSI errors in Linux.
> >
> > To solve this issue, let's fetch the unit attention as early as possible
> > when we prepare the request, that way only the first request in the batch
> > will carry the right sense.
> >
> > Fixes: 1880ad4f4e ("virtio-scsi: Batched prepare for cmd reqs")
> > Fixes: 8cc5583abe ("virtio-scsi: Send "REPORTED LUNS CHANGED" sense data 
> > upon disk hotplug events")
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2176702
> > Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >   include/hw/scsi/scsi.h |  1 +
> >   hw/scsi/scsi-bus.c     | 36 +++++++++++++++++++++++++++++++++---
> >   2 files changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> > index e2bb1a2fbf..3692ca82f3 100644
> > --- a/include/hw/scsi/scsi.h
> > +++ b/include/hw/scsi/scsi.h
> > @@ -108,6 +108,7 @@ int cdrom_read_toc_raw(int nb_sectors, uint8_t *buf, 
> > int msf, int session_num);
> >   /* scsi-bus.c */
> >   struct SCSIReqOps {
> >       size_t size;
> > +    void (*init_req)(SCSIRequest *req);
> >       void (*free_req)(SCSIRequest *req);
> >       int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
> >       void (*read_data)(SCSIRequest *req);
> > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> > index f80f4cb4fc..f083373021 100644
> > --- a/hw/scsi/scsi-bus.c
> > +++ b/hw/scsi/scsi-bus.c
> > @@ -412,19 +412,35 @@ static const struct SCSIReqOps reqops_invalid_opcode 
> > = {
> >
> >   /* SCSIReqOps implementation for unit attention conditions.  */
> >
> > -static int32_t scsi_unit_attention(SCSIRequest *req, uint8_t *buf)
> > +static void scsi_fetch_unit_attention_sense(SCSIRequest *req)
> >   {
> > +    SCSISense *ua = NULL;
> > +
> >       if (req->dev->unit_attention.key == UNIT_ATTENTION) {
> > -        scsi_req_build_sense(req, req->dev->unit_attention);
> > +        ua = &req->dev->unit_attention;
> >       } else if (req->bus->unit_attention.key == UNIT_ATTENTION) {
> > -        scsi_req_build_sense(req, req->bus->unit_attention);
> > +        ua = &req->bus->unit_attention;
> >       }
> > +
> > +    /*
> > +     * Fetch the unit attention sense immediately so that another
> > +     * scsi_req_new does not use reqops_unit_attention.
> > +     */
> > +    if (ua) {
> > +        scsi_req_build_sense(req, *ua);
> > +        *ua = SENSE_CODE(NO_SENSE);
> > +    }
> > +}
> > +
> > +static int32_t scsi_unit_attention(SCSIRequest *req, uint8_t *buf)
> > +{
> >       scsi_req_complete(req, CHECK_CONDITION);
> >       return 0;
> >   }
> >
> >   static const struct SCSIReqOps reqops_unit_attention = {
> >       .size         = sizeof(SCSIRequest),
> > +    .init_req     = scsi_fetch_unit_attention_sense,
> >       .send_command = scsi_unit_attention
> >   };
> >
> > @@ -699,6 +715,11 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, 
> > SCSIDevice *d,
> >       object_ref(OBJECT(d));
> >       object_ref(OBJECT(qbus->parent));
> >       notifier_list_init(&req->cancel_notifiers);
> > +
> > +    if (reqops->init_req) {
> > +        reqops->init_req(req);
> > +    }
> > +
> >       trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
> >       return req;
> >   }
> > @@ -798,6 +819,15 @@ uint8_t *scsi_req_get_buf(SCSIRequest *req)
> >   static void scsi_clear_unit_attention(SCSIRequest *req)
> >   {
> >       SCSISense *ua;
> > +
> > +    /*
> > +     * scsi_fetch_unit_attention_sense() already cleaned the unit attention
> > +     * in this case.
> > +     */
> > +    if (req->ops == &reqops_unit_attention) {
> > +        return;
> > +    }
> > +
> >       if (req->dev->unit_attention.key != UNIT_ATTENTION &&
> >           req->bus->unit_attention.key != UNIT_ATTENTION) {
> >           return;
>