qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Fam Zheng
Subject: Re: [Qemu-devel] [PATCH v4 7/7] virtio-scsi: Handle TMF request cancellation asynchronously
Date: Tue, 30 Sep 2014 10:27:45 +0800
User-agent: Mutt/1.5.23 (2014-03-12)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]