| Message ID | 54293AD9.8020701@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 09/29 12:56, Paolo Bonzini wrote: > 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. Yeah, because of the scsi_req_cancel_async in previous iterations. > > 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? Looks very good. I like it as it simplifies code. It'll be good if you already squashed it in while applying, with the assert = fixed. Please let me know if it's better to respin. (Although I don't know how to add your name while applying your patch because there isn't a s-o-b line here, but there will be one eventually :) Fam > > 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; >
On Mon, 09/29 12:56, Paolo Bonzini wrote: > @@ -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); Missing: + notifier->tmf_req = req; Fam > + notifier->notifier.notify = virtio_scsi_cancel_notify; > scsi_req_cancel_async(r, ¬ifier->notifier); > ret = -EINPROGRESS; > } > 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; >
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;