[RFC,9/9] scsi: Cancel request asynchronously

We are blocking the whole VM, which means that an irresponsive storage
backend will hang the whole guest. Let's switch to bdrv_aio_cancel_async
to improve this.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-bus.c     |  6 ++----
 hw/scsi/scsi-disk.c    | 41 ++++++++++-------------------------------
 hw/scsi/scsi-generic.c | 21 ++++++---------------
 3 files changed, 18 insertions(+), 50 deletions(-)

Il 21/08/2014 13:56, Fam Zheng ha scritto:
> We are blocking the whole VM, which means that an irresponsive storage
> backend will hang the whole guest. Let's switch to bdrv_aio_cancel_async
> to improve this.

Unforuntately, the TMF must only return after the request has been
canceled.  I think you need to add a scsi_cancel_io_async function, and
keep all the remaining machinery (also, you need a better commit message
that explains what you are removing and the new invariants).

Then in virtio-scsi you need to add a list of "dependent" (controlq)
VirtIOSCSIReq to the "main" (requestq) VirtIOSCSIReq, and complete them
all after signaling the completion of the main request.

Paolo

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/scsi-bus.c     |  6 ++----
>  hw/scsi/scsi-disk.c    | 41 ++++++++++-------------------------------
>  hw/scsi/scsi-generic.c | 21 ++++++---------------
>  3 files changed, 18 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 6f4462b..59ec9f9 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1615,7 +1615,6 @@ void scsi_req_continue(SCSIRequest *req)
>  {
>      if (req->io_canceled) {
>          trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag);
> -        return;
>      }
>      trace_scsi_req_continue(req->dev->id, req->lun, req->tag);
>      if (req->cmd.mode == SCSI_XFER_TO_DEV) {
> @@ -1633,7 +1632,6 @@ void scsi_req_data(SCSIRequest *req, int len)
>      uint8_t *buf;
>      if (req->io_canceled) {
>          trace_scsi_req_data_canceled(req->dev->id, req->lun, req->tag, len);
> -        return;
>      }
>      trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
>      assert(req->cmd.mode != SCSI_XFER_NONE);
> @@ -1721,7 +1719,7 @@ void scsi_req_complete(SCSIRequest *req, int status)
>  void scsi_req_cancel(SCSIRequest *req)
>  {
>      trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
> -    if (!req->enqueued) {
> +    if (req->io_canceled) {
>          return;
>      }
>      scsi_req_ref(req);
> @@ -1738,7 +1736,7 @@ void scsi_req_cancel(SCSIRequest *req)
>  
>  void scsi_req_abort(SCSIRequest *req, int status)
>  {
> -    if (!req->enqueued) {
> +    if (req->io_canceled) {
>          return;
>      }
>      scsi_req_ref(req);
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index d55521d..46b1e53 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -112,14 +112,9 @@ static void scsi_cancel_io(SCSIRequest *req)
>  
>      DPRINTF("Cancel tag=0x%x\n", req->tag);
>      if (r->req.aiocb) {
> -        bdrv_aio_cancel(r->req.aiocb);
> -
> -        /* This reference was left in by scsi_*_data.  We take ownership of
> -         * it the moment scsi_req_cancel is called, independent of whether
> -         * bdrv_aio_cancel completes the request or not.  */
> -        scsi_req_unref(&r->req);
> +        assert(req->io_canceled);
> +        bdrv_aio_cancel_async(r->req.aiocb);
>      }
> -    r->req.aiocb = NULL;
>  }
>  
>  static uint32_t scsi_init_iovec(SCSIDiskReq *r, size_t size)
> @@ -197,9 +192,7 @@ static void scsi_aio_complete(void *opaque, int ret)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static bool scsi_is_cmd_fua(SCSICommand *cmd)
> @@ -245,9 +238,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static void scsi_dma_complete_noio(void *opaque, int ret)
> @@ -279,9 +270,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
>      }
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static void scsi_dma_complete(void *opaque, int ret)
> @@ -319,9 +308,7 @@ static void scsi_read_complete(void * opaque, int ret)
>      scsi_req_data(&r->req, r->qiov.size);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  /* Actually issue a read to the block device.  */
> @@ -361,9 +348,7 @@ static void scsi_do_read(void *opaque, int ret)
>      }
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  /* Read more data from scsi device into buffer.  */
> @@ -478,9 +463,7 @@ static void scsi_write_complete(void * opaque, int ret)
>      }
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>  }
>  
>  static void scsi_write_data(SCSIRequest *req)
> @@ -1577,9 +1560,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>      g_free(data);
>  }
>  
> @@ -1672,9 +1653,7 @@ static void scsi_write_same_complete(void *opaque, int ret)
>      scsi_req_complete(&r->req, GOOD);
>  
>  done:
> -    if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> -    }
> +    scsi_req_unref(&r->req);
>      qemu_vfree(data->iov.iov_base);
>      g_free(data);
>  }
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 0b2ff90..1c29ecb 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -132,27 +132,20 @@ static void scsi_command_complete(void *opaque, int ret)
>      DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
>              r, r->req.tag, status);
>  
> -    scsi_req_complete(&r->req, status);
>      if (!r->req.io_canceled) {
> -        scsi_req_unref(&r->req);
> +        scsi_req_complete(&r->req, status);
>      }
> +    scsi_req_unref(&r->req);
>  }
>  
>  /* Cancel a pending data transfer.  */
>  static void scsi_cancel_io(SCSIRequest *req)
>  {
> -    SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
> -
>      DPRINTF("Cancel tag=0x%x\n", req->tag);
> -    if (r->req.aiocb) {
> -        bdrv_aio_cancel(r->req.aiocb);
> -
> -        /* This reference was left in by scsi_*_data.  We take ownership of
> -         * it independent of whether bdrv_aio_cancel completes the request
> -         * or not.  */
> -        scsi_req_unref(&r->req);
> +    if (req->aiocb) {
> +        req->io_canceled = true;
> +        bdrv_aio_cancel_async(req->aiocb);
>      }
> -    r->req.aiocb = NULL;
>  }
>  
>  static int execute_command(BlockDriverState *bdrv,
> @@ -211,9 +204,7 @@ static void scsi_read_complete(void * opaque, int ret)
>          bdrv_set_guest_block_size(s->conf.bs, s->blocksize);
>  
>          scsi_req_data(&r->req, len);
> -        if (!r->req.io_canceled) {
> -            scsi_req_unref(&r->req);
> -        }
> +        scsi_req_unref(&r->req);
>      }
>  }
>  
>

On Thu, 08/21 14:19, Paolo Bonzini wrote:
> Il 21/08/2014 13:56, Fam Zheng ha scritto:
> > We are blocking the whole VM, which means that an irresponsive storage
> > backend will hang the whole guest. Let's switch to bdrv_aio_cancel_async
> > to improve this.
> 
> Unforuntately, the TMF must only return after the request has been
> canceled.  I think you need to add a scsi_cancel_io_async function, and
> keep all the remaining machinery (also, you need a better commit message
> that explains what you are removing and the new invariants).
> 
> Then in virtio-scsi you need to add a list of "dependent" (controlq)
> VirtIOSCSIReq to the "main" (requestq) VirtIOSCSIReq, and complete them
> all after signaling the completion of the main request.

OK, I didn't know that. I'll try again :)

Thanks,
Fam