Re: [Qemu-devel] [PATCH v3 6/7] scsi: Introduce scsi_req_cancel_async

[Paolo Bonzini <pbonzini@redhat.com>]

Il 25/09/2014 04:20, Fam Zheng ha scritto:
> Devices will call this function to start an asynchronous cancellation. The
> bus->info->cancel will be called after the request is canceled.
> 
> Devices will probably need to track a separate TMF request that triggers this
> cancellation, and wait until the cancellation is done before completing it. So
> we store a notifier list in SCSIRequest and in scsi_req_cancel_complete we
> notify them.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/scsi-bus.c     | 23 +++++++++++++++++++++++
>  include/hw/scsi/scsi.h |  3 +++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index c91db63..df7585a 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -566,6 +566,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>      req->ops = reqops;
>      object_ref(OBJECT(d));
>      object_ref(OBJECT(qbus->parent));
> +    notifier_list_init(&req->cancel_notifiers);
>      trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
>      return req;
>  }
> @@ -1725,9 +1726,31 @@ void scsi_req_cancel_complete(SCSIRequest *req)
>      if (req->bus->info->cancel) {
>          req->bus->info->cancel(req);
>      }
> +    notifier_list_notify(&req->cancel_notifiers, req);

I think you also have to call notifier_list_notify from
scsi_req_complete, because a cancelled request might end up being
completed instead of cancelled.

In fact, the next obvious step (enabled by your bdrv_aio_cancel cleanup)
would be to _not_ call scsi_req_cancel_complete if we can report
completion or if there was an I/O error.  This can happen for
scsi-generic, scsi_dma_complete_noio, etc.  Basically, moving the
io_canceled check from the beginning of the completion routine to just
before bdrv_aio_* or dma_aio_* are called.

Paolo

>      scsi_req_unref(req);
>  }
>  
> +/* Cancel @req asynchronously. @notifier is added to @req's cancellation
> + * notifier list, the bus will be notified the requests cancellation is
> + * completed.
> + * */
> +void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
> +{
> +    trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> +    if (notifier) {
> +        notifier_list_add(&req->cancel_notifiers, notifier);
> +    }
> +    if (req->io_canceled) {
> +        return;
> +    }
> +    scsi_req_ref(req);
> +    scsi_req_dequeue(req);
> +    req->io_canceled = true;
> +    if (req->aiocb) {
> +        bdrv_aio_cancel_async(req->aiocb);
> +    }
> +}
> +
>  void scsi_req_cancel(SCSIRequest *req)
>  {
>      trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index a75a7c8..c47dc53 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -5,6 +5,7 @@
>  #include "block/block.h"
>  #include "hw/block/block.h"
>  #include "sysemu/sysemu.h"
> +#include "qemu/notify.h"
>  
>  #define MAX_SCSI_DEVS	255
>  
> @@ -53,6 +54,7 @@ struct SCSIRequest {
>      void              *hba_private;
>      size_t            resid;
>      SCSICommand       cmd;
> +    NotifierList      cancel_notifiers;
>  
>      /* Note:
>       * - fields before sense are initialized by scsi_req_alloc;
> @@ -267,6 +269,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
>  void scsi_req_abort(SCSIRequest *req, int status);
>  void scsi_req_cancel_complete(SCSIRequest *req);
>  void scsi_req_cancel(SCSIRequest *req);
> +void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier);
>  void scsi_req_retry(SCSIRequest *req);
>  void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
>  void scsi_device_set_ua(SCSIDevice *sdev, SCSISense sense);
>