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