[PATCH 0/3] scsi: fix issue with Linux guest and unit attention

[PATCH 0/3] scsi: fix issue with Linux guest and unit attention

[Stefano Garzarella]

This series tries to fix a problem highlighted by the commit 8cc5583abe
("virtio-scsi: Send "REPORTED LUNS CHANGED" sense data upon disk hotplug
events").

We initially thought about reverting that commit, but now we have found
a pre-existing issue introduced by commit 1880ad4f4e ("virtio-scsi:
Batched prepare for cmd reqs"). More details in the discussion of the
reverting tentative [1].

Thanks to Thomas for identifying the commit that highlighted the problem
and providing an easy way to reproduce the issue, Stefan for his useful
comments, Mike for the scsi logs, and a big thanks to Paolo for his help
in preparing this series!

Stefano

[1] https://lore.kernel.org/qemu-devel/i3od362o6unuimlqna3aaedliaabauj6g545esg7txidd4s44e@bkx5des6zytx/

Stefano Garzarella (3):
  scsi: fetch unit attention when creating the request
  scsi: cleanup scsi_clear_unit_attention()
  scsi: clear unit attention only for REPORT LUNS commands

 include/hw/scsi/scsi.h |  1 +
 hw/scsi/scsi-bus.c     | 78 ++++++++++++++++++++++--------------------
 2 files changed, 41 insertions(+), 38 deletions(-)

-- 
2.41.0

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

[Stefano Garzarella]

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;
-- 
2.41.0

[PATCH 2/3] scsi: cleanup scsi_clear_unit_attention()

[Stefano Garzarella]

The previous commit moved the unit attention clearing when we create
the request. So now we can clean scsi_clear_unit_attention() to handle
only the case of the REPORT LUNS command.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/scsi/scsi-bus.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f083373021..f9c95dfb50 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -828,26 +828,12 @@ static void scsi_clear_unit_attention(SCSIRequest *req)
         return;
     }
 
-    if (req->dev->unit_attention.key != UNIT_ATTENTION &&
-        req->bus->unit_attention.key != UNIT_ATTENTION) {
-        return;
-    }
-
-    /*
-     * If an INQUIRY command enters the enabled command state,
-     * the device server shall [not] clear any unit attention condition;
-     * See also MMC-6, paragraphs 6.5 and 6.6.2.
-     */
-    if (req->cmd.buf[0] == INQUIRY ||
-        req->cmd.buf[0] == GET_CONFIGURATION ||
-        req->cmd.buf[0] == GET_EVENT_STATUS_NOTIFICATION) {
-        return;
-    }
-
     if (req->dev->unit_attention.key == UNIT_ATTENTION) {
         ua = &req->dev->unit_attention;
-    } else {
+    } else if (req->bus->unit_attention.key == UNIT_ATTENTION) {
         ua = &req->bus->unit_attention;
+    } else {
+        return;
     }
 
     /*
@@ -856,12 +842,10 @@ static void scsi_clear_unit_attention(SCSIRequest *req)
      * with an additional sense code of REPORTED LUNS DATA HAS CHANGED.
      */
     if (req->cmd.buf[0] == REPORT_LUNS &&
-        !(ua->asc == SENSE_CODE(REPORTED_LUNS_CHANGED).asc &&
-          ua->ascq == SENSE_CODE(REPORTED_LUNS_CHANGED).ascq)) {
-        return;
+        ua->asc == SENSE_CODE(REPORTED_LUNS_CHANGED).asc &&
+        ua->ascq == SENSE_CODE(REPORTED_LUNS_CHANGED).ascq) {
+        *ua = SENSE_CODE(NO_SENSE);
     }
-
-    *ua = SENSE_CODE(NO_SENSE);
 }
 
 int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len)
-- 
2.41.0

[PATCH 3/3] scsi: clear unit attention only for REPORT LUNS commands

[Stefano Garzarella]

scsi_clear_unit_attention() now only handles REPORTED LUNS DATA HAS
CHANGED.

This only happens when we handle REPORT LUNS commands, so let's rename
the function in scsi_clear_reported_luns_changed() and call it only in
scsi_target_emulate_report_luns().

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/scsi/scsi-bus.c | 34 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f9c95dfb50..fc4b77fdb0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -22,6 +22,7 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev);
 static void scsi_req_dequeue(SCSIRequest *req);
 static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
 static void scsi_target_free_buf(SCSIRequest *req);
+static void scsi_clear_reported_luns_changed(SCSIRequest *req);
 
 static int next_scsi_bus;
 
@@ -518,6 +519,14 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
 
     /* store the LUN list length */
     stl_be_p(&r->buf[0], len - 8);
+
+    /*
+     * If a REPORT LUNS command enters the enabled command state, [...]
+     * the device server shall clear any pending unit attention condition
+     * with an additional sense code of REPORTED LUNS DATA HAS CHANGED.
+     */
+    scsi_clear_reported_luns_changed(&r->req);
+
     return true;
 }
 
@@ -816,18 +825,10 @@ uint8_t *scsi_req_get_buf(SCSIRequest *req)
     return req->ops->get_buf(req);
 }
 
-static void scsi_clear_unit_attention(SCSIRequest *req)
+static void scsi_clear_reported_luns_changed(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) {
         ua = &req->dev->unit_attention;
     } else if (req->bus->unit_attention.key == UNIT_ATTENTION) {
@@ -836,13 +837,7 @@ static void scsi_clear_unit_attention(SCSIRequest *req)
         return;
     }
 
-    /*
-     * If a REPORT LUNS command enters the enabled command state, [...]
-     * the device server shall clear any pending unit attention condition
-     * with an additional sense code of REPORTED LUNS DATA HAS CHANGED.
-     */
-    if (req->cmd.buf[0] == REPORT_LUNS &&
-        ua->asc == SENSE_CODE(REPORTED_LUNS_CHANGED).asc &&
+    if (ua->asc == SENSE_CODE(REPORTED_LUNS_CHANGED).asc &&
         ua->ascq == SENSE_CODE(REPORTED_LUNS_CHANGED).ascq) {
         *ua = SENSE_CODE(NO_SENSE);
     }
@@ -1528,13 +1523,6 @@ void scsi_req_complete(SCSIRequest *req, int status)
         req->dev->sense_is_ua = false;
     }
 
-    /*
-     * Unit attention state is now stored in the device's sense buffer
-     * if the HBA didn't do autosense.  Clear the pending unit attention
-     * flags.
-     */
-    scsi_clear_unit_attention(req);
-
     scsi_req_ref(req);
     scsi_req_dequeue(req);
     req->bus->info->complete(req, req->residual);
-- 
2.41.0

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

[Paolo Bonzini]

On 7/12/23 15: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

More precisely, it was pretty hard to trigger it; it might be possible 
using a CD-ROM, as it can report a MEDIUM_CHANGED unit attention.  I 
will change "This had no drawback" to "No ill effect was reported"

> "REPORTED LUNS CHANGED" sense data upon disk hotplug events") added a
> bus unit attention.

... that was fairly easy to trigger via SCSI device hotplug/hot-unplug.

Queued the series, thanks for the tests and for applying the cleanups on 
top.

> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>

Heh, I basically only wrote the "if (req->init_req)" statement so that's 
pretty generous, but I'll keep it anyway. :)

Paolo

> 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.

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

[Stefano Garzarella]

On Wed, Jul 12, 2023 at 06:38:14PM +0200, Paolo Bonzini wrote:
>On 7/12/23 15: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
>
>More precisely, it was pretty hard to trigger it; it might be possible 
>using a CD-ROM, as it can report a MEDIUM_CHANGED unit attention.  I 
>will change "This had no drawback" to "No ill effect was reported"

Yep, agree!

>
>>"REPORTED LUNS CHANGED" sense data upon disk hotplug events") added a
>>bus unit attention.
>
>... that was fairly easy to trigger via SCSI device hotplug/hot-unplug.
>
>Queued the series, thanks for the tests and for applying the cleanups 
>on top.

Thanks for queueing!

>
>>Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
>
>Heh, I basically only wrote the "if (req->init_req)" statement so 
>that's pretty generous, but I'll keep it anyway. :)

Your help was invaluable ;-)

Thanks,
Stefano

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

[Michael Galaxy]

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;

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;
>

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

[Michael Galaxy]

Thanks for your help.

- Michael

On 10/9/24 11:28, Paolo Bonzini wrote:
> !-------------------------------------------------------------------|
>    This Message Is From an External Sender
>    This message came from outside your organization.
> |-------------------------------------------------------------------!
>
> 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://urldefense.com/v3/__https://bugzilla.redhat.com/show_bug.cgi?id=2176702__;!!GjvTz_vk!RKyuBiMbJ5CywoidrGSu50njz30GGlEg54dEmfsw_y2Ab8NxP2zkHyYy-Iajg9-KC0IO_9h3L-iac6O3$
>>> 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;

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

[Michael Tokarev]

On 09.10.2024 21:00, Michael Galaxy wrote:
> Thanks for your help.
> 
> - Michael
> 
> On 10/9/24 11:28, Paolo Bonzini wrote:
>> 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.

I picked this one up for 7.2.next.  The change applies cleanly
to 7.2, but I yet to try to even build-test it (I'm traveling
currently).

https://gitlab.com/mjt0k/qemu/-/commits/staging-7.2/

Thanks,

/mjt

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

[Michael Galaxy]

On 10/11/24 14:44, Michael Tokarev wrote:
> !-------------------------------------------------------------------|
>  This Message Is From an External Sender
>  This message came from outside your organization.
> |-------------------------------------------------------------------!
>
> On 09.10.2024 21:00, Michael Galaxy wrote:
>> Thanks for your help.
>>
>> - Michael
>>
>> On 10/9/24 11:28, Paolo Bonzini wrote:
>>> 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.
>
> I picked this one up for 7.2.next.  The change applies cleanly
> to 7.2, but I yet to try to even build-test it (I'm traveling
> currently).

Acknowledged. Thank you!

- Michael