From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41110) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKRLE-0000lM-EZ for qemu-devel@nongnu.org; Thu, 21 Aug 2014 08:19:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKRL2-0005pE-9t for qemu-devel@nongnu.org; Thu, 21 Aug 2014 08:19:44 -0400 Received: from mail-wi0-x229.google.com ([2a00:1450:400c:c05::229]:51845) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKRL1-0005p1-UU for qemu-devel@nongnu.org; Thu, 21 Aug 2014 08:19:32 -0400 Received: by mail-wi0-f169.google.com with SMTP id n3so8004326wiv.2 for ; Thu, 21 Aug 2014 05:19:31 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <53F5E3CF.9020001@redhat.com> Date: Thu, 21 Aug 2014 14:19:27 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1408622216-9578-1-git-send-email-famz@redhat.com> <1408622216-9578-10-git-send-email-famz@redhat.com> In-Reply-To: <1408622216-9578-10-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 9/9] scsi: Cancel request asynchronously List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi 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 > --- > 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); > } > } > >