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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 24/09/2014 10:27, Fam Zheng ha scritto:
> For VIRTIO_SCSI_T_TMF_ABORT_TASK and VIRTIO_SCSI_T_TMF_ABORT_TASK_SET,
> use scsi_req_cancel_async to start the cancellation.
> 
> Because each tmf command may cancel multiple requests, we need to use a
> counter to track the number of remaining requests we still need to wait
> for.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 77 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index fa36e23..9bd7d8a 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -208,12 +208,40 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>      return req;
>  }
>  
> -static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
> +typedef struct {
> +    VirtIOSCSIReq  *tmf_req;
> +    int            remaining;
> +} VirtIOSCSICancelTracker;

What about putting "remaining" directly in VirtIOSCSIReq?

> +typedef struct {
> +    Notifier                 notifier;
> +    VirtIOSCSICancelTracker  *tracker;
> +} VirtIOSCSICancelNotifier;
> +
> +static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
> +{
> +    VirtIOSCSICancelNotifier *n = container_of(notifier,
> +                                               VirtIOSCSICancelNotifier,
> +                                               notifier);
> +
> +    if (--n->tracker->remaining == 0) {
> +        virtio_scsi_complete_req(n->tracker->tmf_req);
> +        g_free(n->tracker);
> +    }
> +    g_free(n);
> +}
> +
> +/* Return true if the request is ready to be completed and return to guest;
> + * false if the request will be completed (by some other events) later, for
> + * example in the case of async cancellation. */
> +static bool virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)

Perhaps return 0/-EINPROGRESS so that it's easier to remember the
calling convention?

>  {
>      SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
>      SCSIRequest *r, *next;
>      BusChild *kid;
>      int target;
> +    bool ret = true;
> +    int cancel_count;
>  
>      if (s->dataplane_started && bdrv_get_aio_context(d->conf.bs) != s->ctx) {
>          aio_context_acquire(s->ctx);
> @@ -251,7 +279,18 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
>                   */
>                  req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
>              } else {
> -                scsi_req_cancel(r);
> +                VirtIOSCSICancelNotifier *notifier;
> +                VirtIOSCSICancelTracker *tracker;
> +
> +                notifier           = g_new(VirtIOSCSICancelNotifier, 1);

Slice allocator?

> +                notifier->notifier.notify
> +                                   = virtio_scsi_cancel_notify;
> +                tracker            = g_new(VirtIOSCSICancelTracker, 1);

Same here if you keep VirtIOSCSICancelTracker.

> +                tracker->tmf_req   = req;
> +                tracker->remaining = 1;
> +                notifier->tracker  = tracker;
> +                scsi_req_cancel_async(r, &notifier->notifier);
> +                ret = false;
>              }
>          }
>          break;
> @@ -277,6 +316,7 @@ static void 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;
>          QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
>              if (r->hba_private) {
>                  if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
> @@ -286,10 +326,36 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
>                      req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
>                      break;
>                  } else {
> -                    scsi_req_cancel(r);
> +                    /* 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_new(VirtIOSCSICancelTracker, 1);

Same as above.

> +            tracker->tmf_req   = req;
> +            tracker->remaining = cancel_count;
> +
> +            QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
> +                if (r->hba_private) {
> +                    notifier           = g_new(VirtIOSCSICancelNotifier, 1);

Same as above.

> +                    notifier->notifier.notify
> +                                       = virtio_scsi_cancel_notify;
> +                    notifier->tracker  = tracker;
> +                    scsi_req_cancel_async(r, &notifier->notifier);
> +                }
> +            }
> +            ret = false;
> +        }
> +
>          break;
>  
>      case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
> @@ -310,20 +376,22 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
>          break;
>      }
>  
> -    return;
> +    return ret;
>  
>  incorrect_lun:
>      req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
> -    return;
> +    return ret;
>  
>  fail:
>      req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
> +    return ret;
>  }
>  
>  void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
>  {
>      VirtIODevice *vdev = (VirtIODevice *)s;
>      int type;
> +    bool should_complete = true;
>  
>      if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
>                  &type, sizeof(type)) < sizeof(type)) {
> @@ -337,7 +405,7 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
>                      sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
>              virtio_scsi_bad_req();
>          } else {
> -            virtio_scsi_do_tmf(s, req);
> +            should_complete = virtio_scsi_do_tmf(s, req);
>          }
>  
>      } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
> @@ -350,7 +418,9 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
>              req->resp.an.response = VIRTIO_SCSI_S_OK;
>          }
>      }
> -    virtio_scsi_complete_req(req);
> +    if (should_complete) {
> +        virtio_scsi_complete_req(req);
> +    }
>  }
>  
>  static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> 

Very nice apart from these comments.

Paolo