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 <f...@redhat.com>
> ---
> 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);
>
