On 10/17/25 19:54, Stefan Hajnoczi wrote:
On Fri, Oct 17, 2025 at 11:43:30AM +0200, Fiona Ebner wrote:
Changes in v2:
* Different approach, collect requests for cancelling in a list for a
localized solution rather than keeping track of the lock status via
function arguments.
hw/scsi/virtio-scsi.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block
Thanks Stefan; sorry for the delay in reviewing. The fix
of releasing the lock around virtio_scsi_tmf_cancel_req():
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 9b12ee7f1c6..ac17c97f224 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1503,6 +1503,10 @@ SCSIRequest *scsi_req_ref(SCSIRequest *req)
void scsi_req_unref(SCSIRequest *req)
{
+ if (!req) {
+ return;
+ }
+
assert(req->refcount > 0);
if (--req->refcount == 0) {
BusState *qbus = req->dev->qdev.parent_bus;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index d817fc42b4c..481e78e4771 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -364,7 +364,11 @@ static void virtio_scsi_do_tmf_aio_context(void *opaque)
}
WITH_QEMU_LOCK_GUARD(&d->requests_lock) {
+ SCSIRequest *prev = NULL;
QTAILQ_FOREACH(r, &d->requests, next) {
+ scsi_req_unref(prev);
+ prev = NULL;
+
VirtIOSCSIReq *cmd_req = r->hba_private;
assert(cmd_req); /* request has hba_private while enqueued */
@@ -374,8 +378,20 @@ static void virtio_scsi_do_tmf_aio_context(void *opaque)
if (match_tag && cmd_req->req.cmd.tag != tmf->req.tmf.tag) {
continue;
}
+
+ /*
+ * Keep it alive while the lock is released, and also to be
+ * able to read "next".
+ */
+ scsi_req_ref(r);
+ prev = r;
+
+ qemu_mutex_unlock(&d->request_lock);
virtio_scsi_tmf_cancel_req(tmf, r);
+ qemu_mutex_lock(&d->request_lock);
}
+
+ scsi_req_unref(prev);
}
/* Incremented by virtio_scsi_do_tmf() */
would have a bug too, in that the loop is not using
QTAILQ_FOREACH_SAFE and scsi_req_dequeue() removes the
request from the list.
I think scsi_req_ref/unref should also be changed to use atomics.
free_request is only implemented by hw/usb/dev-uas.c and all the
others do not need a lock, so we're fine with that.
And QOM references held by the requests are not necessary, because
anyway the requests won't survive scsi_qdev_unrealize (at which
point the device is certainly alive). I'll test this, add some
comments and send a patch:
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 9b12ee7f1c6..7fcacc178da 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -838,8 +838,6 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops,
SCSIDevice *d,
req->status = -1;
req->host_status = -1;
req->ops = reqops;
- object_ref(OBJECT(d));
- object_ref(OBJECT(qbus->parent));
notifier_list_init(&req->cancel_notifiers);
if (reqops->init_req) {
@@ -1496,15 +1494,15 @@ void scsi_device_report_change(SCSIDevice *dev,
SCSISense sense)
SCSIRequest *scsi_req_ref(SCSIRequest *req)
{
- assert(req->refcount > 0);
- req->refcount++;
+ assert(qatomic_read(&req->refcount) > 0);
+ qatomic_inc(&req->refcount);
return req;
}
void scsi_req_unref(SCSIRequest *req)
{
- assert(req->refcount > 0);
- if (--req->refcount == 0) {
+ assert(qatomic_read(&req->refcount) > 0);
+ if (qatomic_fetch_dec(&req->refcount) == 1) {
BusState *qbus = req->dev->qdev.parent_bus;
SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, qbus);
@@ -1514,8 +1512,6 @@ void scsi_req_unref(SCSIRequest *req)
if (req->ops->free_req) {
req->ops->free_req(req);
}
- object_unref(OBJECT(req->dev));
- object_unref(OBJECT(qbus->parent));
g_free(req);
}
}
Paolo
