Re: [Qemu-devel] [PATCH v4 7/7] virtio-scsi: Handle TMF request cancellation asynchronously

[Fam Zheng]

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, &notifier->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, &notifier->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;
>