Il 29/09/2014 12:11, Paolo Bonzini ha scritto:
> Il 28/09/2014 03:48, Fam Zheng ha scritto:
>> + virtio_scsi_complete_req(req);
>> + } else {
>> + assert(r = -EINPROGRESS);
>> + }
>> }
>
> = instead of == here.
>
> Apart from this, the patch looks good. Thanks!
Hmm, there's actually another problem. I think you cannot be sure
that the two visits of d->requests see the same elements. In
particular, one request could have non-NULL r->hba_private in the
first loop, but it could become NULL by the time you reach the
second loop.
So we either use one loop only, or we have to save the list of
requests in a separate list (for example a GSList).
We can use a single loop by adding 1 to the "remaining" count
while virtio_scsi_do_tmf is running, and dropping it at the end.
However, you then add unnecessary complication to check for
QUERY_TASK_SET around the allocation of the VirtIOSCSICancelTracker,
and to free the VirtIOSCSICancelTracker if the TMF actually had
nothing to do.
If we move the "remaining" field inside VirtIOSCSIReq, things
become much simpler. Can you review this incremental patch
on top of yours?
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 7a6b71a..9e56528 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -209,13 +209,8 @@ static void *virtio_scsi_load_request(QEMUFile *f,
SCSIRequest *sreq)
}
typedef struct {
+ Notifier notifier;
VirtIOSCSIReq *tmf_req;
- int remaining;
-} VirtIOSCSICancelTracker;
-
-typedef struct {
- Notifier notifier;
- VirtIOSCSICancelTracker *tracker;
} VirtIOSCSICancelNotifier;
static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
@@ -224,9 +219,8 @@ static void virtio_scsi_cancel_notify(Notifier *notifier,
void *data)
VirtIOSCSICancelNotifier,
notifier);
- if (--n->tracker->remaining == 0) {
- virtio_scsi_complete_req(n->tracker->tmf_req);
- g_slice_free(VirtIOSCSICancelTracker, n->tracker);
+ if (--n->tmf_req->remaining == 0) {
+ virtio_scsi_complete_req(n->tmf_req);
}
g_slice_free(VirtIOSCSICancelNotifier, n);
}
@@ -241,7 +235,6 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq
*req)
BusChild *kid;
int target;
int ret = 0;
- int cancel_count;
if (s->dataplane_started && bdrv_get_aio_context(d->conf.bs) != s->ctx) {
aio_context_acquire(s->ctx);
@@ -280,15 +273,10 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s,
VirtIOSCSIReq *req)
req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
} else {
VirtIOSCSICancelNotifier *notifier;
- VirtIOSCSICancelTracker *tracker;
-
- notifier = g_slice_new(VirtIOSCSICancelNotifier);
- notifier->notifier.notify
- = virtio_scsi_cancel_notify;
- tracker = g_slice_new(VirtIOSCSICancelTracker);
- tracker->tmf_req = req;
- tracker->remaining = 1;
- notifier->tracker = tracker;
+
+ req->remaining = 1;
+ notifier = g_slice_new(VirtIOSCSICancelNotifier);
+ notifier->notifier.notify = virtio_scsi_cancel_notify;
scsi_req_cancel_async(r, ¬ifier->notifier);
ret = -EINPROGRESS;
}
@@ -316,7 +304,13 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq
*req)
if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
goto incorrect_lun;
}
- cancel_count = 0;
+
+ /* Add 1 to "remaining" until virtio_scsi_do_tmf returns.
+ * This way, if the bus starts calling back to the notifiers
+ * even before we finish the loop, virtio_scsi_cancel_notify
+ * will not complete the TMF too early.
+ */
+ req->remaining = 1;
QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
if (r->hba_private) {
if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
@@ -324,35 +318,19 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s,
VirtIOSCSIReq *req)
req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
break;
} else {
- /* Before we actually cancel any requests in the next for
- * loop, let's count them. This way, if the bus starts
- * calling back to the notifier even before we finish the
- * loop, the counter, which value is already seen in
- * virtio_scsi_cancel_notify, will prevent us from
- * completing the tmf too quickly. */
- cancel_count++;
- }
- }
- }
- if (cancel_count) {
- VirtIOSCSICancelNotifier *notifier;
- VirtIOSCSICancelTracker *tracker;
-
- tracker = g_slice_new(VirtIOSCSICancelTracker);
- tracker->tmf_req = req;
- tracker->remaining = cancel_count;
+ VirtIOSCSICancelNotifier *notifier;
- QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
- if (r->hba_private) {
+ req->remaining++;
notifier = g_slice_new(VirtIOSCSICancelNotifier);
notifier->notifier.notify = virtio_scsi_cancel_notify;
- notifier->tracker = tracker;
+ notifier->tmf_req = req;
scsi_req_cancel_async(r, ¬ifier->notifier);
}
}
+ }
+ if (--req->remaining > 0) {
ret = -EINPROGRESS;
}
-
break;
case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 60dbfc9..d6e5e79 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -214,8 +214,13 @@ typedef struct VirtIOSCSIReq {
/* Set by dataplane code. */
VirtIOSCSIVring *vring;
- /* Used for two-stage request submission */
- QTAILQ_ENTRY(VirtIOSCSIReq) next;
+ union {
+ /* Used for two-stage request submission */
+ QTAILQ_ENTRY(VirtIOSCSIReq) next;
+
+ /* Used for cancellation of request during TMFs */
+ int remaining;
+ };
SCSIRequest *sreq;
size_t resp_size;
