Re: [Qemu-devel] [PATCH 1/3] scsi-bus: Unify request unref in scsi_req_cancel

[Paolo Bonzini]

Il 18/09/2014 04:36, Fam Zheng ha scritto:
> Before, scsi_req_cancel will take ownership of the canceled request and
> unref it. This is because we didn't know if AIO CB will be called or not
> during the cancelling, so we set the io_canceled flag before calling it,
> and skip to unref in the potentially called callbacks, which is not
> very nice.
> 
> Now, bdrv_aio_cancel has a stricter contract that the completion
> callback is always called, so we can remove the special case of
> req->io_canceled.
> 
> It will also make implementing asynchronous cancellation easier.
> 
> Also, as the existing SCSIReqOps.cancel_io implementations are exactly
> the same, we can move the code to bus for now as we are touching them in
> this patch.
> 
> All AIO callbacks in devices, those will be called because of
> bdrv_aio_cancel, should call scsi_req_canceled if req->io_canceled is
> set. That's the uniform place we release the reference we grabbed in
> scsi_req_cancel and notify buses.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  hw/scsi/scsi-bus.c     | 30 ++++++++++++-------------
>  hw/scsi/scsi-disk.c    | 59 
> ++++++++++++++------------------------------------
>  hw/scsi/scsi-generic.c | 28 +++++-------------------
>  include/hw/scsi/scsi.h |  2 +-
>  4 files changed, 37 insertions(+), 82 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 954c607..74172cc 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1613,7 +1613,6 @@ void scsi_req_continue(SCSIRequest *req)
>  {
>      if (req->io_canceled) {
>          trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag);
> -        return;

I think this is incorrect; same in scsi_req_data.

As discussed on IRC, scsi-generic is the case where this happens.  We
can change it to use the same io_canceled handling as everyone else, so
that the subsequent changes are more mechanical.

>      }
>      trace_scsi_req_continue(req->dev->id, req->lun, req->tag);
>      if (req->cmd.mode == SCSI_XFER_TO_DEV) {
> @@ -1631,7 +1630,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);
> @@ -1716,35 +1714,37 @@ void scsi_req_complete(SCSIRequest *req, int status)
>      scsi_req_unref(req);
>  }
>  
> +/* Called by the devices when the request is canceled. */
> +void scsi_req_canceled(SCSIRequest *req)
> +{
> +    assert(req->io_canceled);
> +    if (req->bus->info->cancel) {
> +        req->bus->info->cancel(req);
> +    }
> +    scsi_req_unref(req);
> +}

I think this patch does a bit too much.  I would do it like this:

- first, as mentioned above, make scsi-generic follow the usual pattern with

    if (req->io_canceled) {
        goto done;
    }
    ...
done:
    if (!req->io_canceled)
        scsi_req_unref(&r->req);
    }

- second, remove the scsi_req_unref from the scsi_cancel_io
implementations, and remove the "if" statement around

done:
    if (!r->req.io_canceled) {
        scsi_req_unref(&r->req);
    }

We can do this now that the callback is always invoked, and it
simplifies the reference counting quite a bit.

- third, uninline scsi_cancel_io and introduce scsi_req_canceled

>  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);
>      scsi_req_dequeue(req);
>      req->io_canceled = true;
> -    if (req->ops->cancel_io) {
> -        req->ops->cancel_io(req);
> +    if (req->aiocb) {
> +        bdrv_aio_cancel(req->aiocb);
>      }
> -    if (req->bus->info->cancel) {
> -        req->bus->info->cancel(req);
> -    }
> -    scsi_req_unref(req);
>  }
>  
>  void scsi_req_abort(SCSIRequest *req, int status)
>  {
> -    if (!req->enqueued) {
> +    if (req->io_canceled) {
>          return;
>      }
>      scsi_req_ref(req);
> -    scsi_req_dequeue(req);
> -    req->io_canceled = true;
> -    if (req->ops->cancel_io) {
> -        req->ops->cancel_io(req);
> -    }
> +    scsi_req_cancel(req);

This is a problem, because we would complete the request twice: once
from scsi_req_canceled, once in scsi_req_complete below.

Let's just drop scsi_req_abort.  The only user can be removed like this:

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 048cfc7..ec5dc0d 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -77,8 +77,9 @@ typedef struct vscsi_req {
     SCSIRequest             *sreq;
     uint32_t                qtag; /* qemu tag != srp tag */
     bool                    active;
-    uint32_t                data_len;
     bool                    writing;
+    bool                    dma_error;
+    uint32_t                data_len;
     uint32_t                senselen;
     uint8_t                 sense[SCSI_SENSE_BUF_SIZE];

@@ -536,8 +537,8 @@ static void vscsi_transfer_data(SCSIRequest *sreq,
uint32_t len)
     }
     if (rc < 0) {
         fprintf(stderr, "VSCSI: RDMA error rc=%d!\n", rc);
-        vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
-        scsi_req_abort(req->sreq, CHECK_CONDITION);
+        req->dma_error = true;
+        scsi_req_cancel(req->sreq);
         return;
     }

@@ -591,6 +592,10 @@ static void vscsi_request_cancelled(SCSIRequest *sreq)
 {
     vscsi_req *req = sreq->hba_private;

+    if (req->dma_error) {
+        vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
+        vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
+    }
     vscsi_put_req(req);
 }


(Yet another separate patch...)

I like this patch.  It is very mechanical, which makes it easier to
review than the size would suggest.  However, splitting it would make
the review even easier. :)

Paolo

>      scsi_req_complete(req, status);
>      scsi_req_unref(req);
>  }
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e34a544..56d7e6d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -105,23 +105,6 @@ static void scsi_check_condition(SCSIDiskReq *r, 
> SCSISense sense)
>      scsi_req_complete(&r->req, CHECK_CONDITION);
>  }
>  
> -/* Cancel a pending data transfer.  */
> -static void scsi_cancel_io(SCSIRequest *req)
> -{
> -    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, 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 the moment scsi_req_cancel is called, independent of whether
> -         * bdrv_aio_cancel completes the request or not.  */
> -        scsi_req_unref(&r->req);
> -    }
> -    r->req.aiocb = NULL;
> -}
> -
>  static uint32_t scsi_init_iovec(SCSIDiskReq *r, size_t size)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> @@ -185,6 +168,7 @@ static void scsi_aio_complete(void *opaque, int ret)
>      r->req.aiocb = NULL;
>      bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -197,9 +181,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)
> @@ -233,6 +215,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>  
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -245,9 +228,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)
> @@ -260,6 +241,7 @@ static void scsi_dma_complete_noio(void *opaque, int ret)
>          bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -279,9 +261,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)
> @@ -302,6 +282,7 @@ static void scsi_read_complete(void * opaque, int ret)
>      r->req.aiocb = NULL;
>      bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -319,9 +300,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.  */
> @@ -336,6 +315,7 @@ static void scsi_do_read(void *opaque, int ret)
>          bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -361,9 +341,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.  */
> @@ -456,6 +434,7 @@ static void scsi_write_complete(void * opaque, int ret)
>          bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      }
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -478,9 +457,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)
> @@ -1548,6 +1525,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
>  
>      r->req.aiocb = NULL;
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -1577,9 +1555,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);
>  }
>  
> @@ -1649,6 +1625,7 @@ static void scsi_write_same_complete(void *opaque, int 
> ret)
>      r->req.aiocb = NULL;
>      bdrv_acct_done(s->qdev.conf.bs, &r->acct);
>      if (r->req.io_canceled) {
> +        scsi_req_canceled(&r->req);
>          goto done;
>      }
>  
> @@ -1672,9 +1649,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);
>  }
> @@ -2337,7 +2312,6 @@ static const SCSIReqOps scsi_disk_emulate_reqops = {
>      .send_command = scsi_disk_emulate_command,
>      .read_data    = scsi_disk_emulate_read_data,
>      .write_data   = scsi_disk_emulate_write_data,
> -    .cancel_io    = scsi_cancel_io,
>      .get_buf      = scsi_get_buf,
>  };
>  
> @@ -2347,7 +2321,6 @@ static const SCSIReqOps scsi_disk_dma_reqops = {
>      .send_command = scsi_disk_dma_command,
>      .read_data    = scsi_read_data,
>      .write_data   = scsi_write_data,
> -    .cancel_io    = scsi_cancel_io,
>      .get_buf      = scsi_get_buf,
>      .load_request = scsi_disk_load_request,
>      .save_request = scsi_disk_save_request,
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 20587b4..5bde909 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -132,27 +132,12 @@ 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);
> -    }
> -}
> -
> -/* 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);
> +        scsi_req_complete(&r->req, status);
> +    } else {
> +        scsi_req_canceled(&r->req);
>      }
> -    r->req.aiocb = NULL;
> +    scsi_req_unref(&r->req);
>  }
>  
>  static int execute_command(BlockDriverState *bdrv,
> @@ -211,9 +196,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);
>      }
>  }
>  
> @@ -465,7 +448,6 @@ const SCSIReqOps scsi_generic_req_ops = {
>      .send_command = scsi_send_command,
>      .read_data    = scsi_read_data,
>      .write_data   = scsi_write_data,
> -    .cancel_io    = scsi_cancel_io,
>      .get_buf      = scsi_get_buf,
>      .load_request = scsi_generic_load_request,
>      .save_request = scsi_generic_save_request,
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 2e3a8f9..25a5617 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -123,7 +123,6 @@ struct SCSIReqOps {
>      int32_t (*send_command)(SCSIRequest *req, uint8_t *buf);
>      void (*read_data)(SCSIRequest *req);
>      void (*write_data)(SCSIRequest *req);
> -    void (*cancel_io)(SCSIRequest *req);
>      uint8_t *(*get_buf)(SCSIRequest *req);
>  
>      void (*save_request)(QEMUFile *f, SCSIRequest *req);
> @@ -259,6 +258,7 @@ void scsi_req_complete(SCSIRequest *req, int status);
>  uint8_t *scsi_req_get_buf(SCSIRequest *req);
>  int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len);
>  void scsi_req_abort(SCSIRequest *req, int status);
> +void scsi_req_canceled(SCSIRequest *req);
>  void scsi_req_cancel(SCSIRequest *req);
>  void scsi_req_retry(SCSIRequest *req);
>  void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense);
>