From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49399) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2qtU-0001Nh-1i for qemu-devel@nongnu.org; Sat, 18 Aug 2012 17:49:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T2qtS-0006nm-HO for qemu-devel@nongnu.org; Sat, 18 Aug 2012 17:49:19 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:40872) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T2qtS-0006nV-7v for qemu-devel@nongnu.org; Sat, 18 Aug 2012 17:49:18 -0400 Received: by wibhm2 with SMTP id hm2so2072003wib.10 for ; Sat, 18 Aug 2012 14:49:17 -0700 (PDT) Sender: Paolo Bonzini From: Paolo Bonzini Date: Sat, 18 Aug 2012 23:49:03 +0200 Message-Id: <1345326543-10677-4-git-send-email-pbonzini@redhat.com> In-Reply-To: <1345326543-10677-1-git-send-email-pbonzini@redhat.com> References: <1345326543-10677-1-git-send-email-pbonzini@redhat.com> Subject: [Qemu-devel] [PATCH 3/3] iscsi: fix races between task completion and abort List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, s.priebe@profihost.ag Cc: ronniesahlberg@gmail.com This patch fixes two main issues with block/iscsi.c: 1) iscsi_task_mgmt_abort_task_async calls iscsi_scsi_task_cancel which was also directly called in iscsi_aio_cancel 2) a race between task completion and task abortion could happen cause the scsi_free_scsi_task were done before iscsi_schedule_bh has finished. To fix this, all the freeing of IscsiTasks and releasing of the AIOCBs is centralized in iscsi_bh_cb, independent of whether the SCSI command has completed or was cancelled. 3) iscsi_aio_cancel was not synchronously waiting for the end of the command. Signed-off-by: Paolo Bonzini --- block/iscsi.c | 54 +++++++++++++++++++++++++----------------------------- 1 file modificato, 25 inserzioni(+), 29 rimozioni(-) diff --git a/block/iscsi.c b/block/iscsi.c index fd5ac3b..70d6908 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -83,12 +83,20 @@ iscsi_bh_cb(void *p) acb->common.cb(acb->common.opaque, acb->status); } + if (acb->task != NULL) { + scsi_free_scsi_task(acb->task); + acb->task = NULL; + } + qemu_aio_release(acb); } static void iscsi_schedule_bh(IscsiAIOCB *acb) { + if (acb->bh) { + return; + } acb->bh = qemu_bh_new(iscsi_bh_cb, acb); qemu_bh_schedule(acb->bh); } @@ -98,6 +106,10 @@ static void iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data, void *private_data) { + IscsiAIOCB *acb = private_data; + + acb->status = -ECANCELED; + iscsi_schedule_bh(acb); } static void @@ -106,15 +118,19 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb) IscsiAIOCB *acb = (IscsiAIOCB *)blockacb; IscsiLun *iscsilun = acb->iscsilun; - acb->common.cb(acb->common.opaque, -ECANCELED); + if (acb->status != -EINPROGRESS) { + return; + } + acb->canceled = 1; /* send a task mgmt call to the target to cancel the task on the target */ iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task, - iscsi_abort_task_cb, NULL); + iscsi_abort_task_cb, acb); - /* then also cancel the task locally in libiscsi */ - iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task); + while (acb->status == -EINPROGRESS) { + qemu_aio_wait(); + } } static AIOPool iscsi_aio_pool = { @@ -192,9 +208,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, g_free(acb->buf); if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -206,8 +219,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun) @@ -236,6 +247,7 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num, acb->qiov = qiov; acb->canceled = 0; + acb->status = -EINPROGRESS; /* XXX we should pass the iovec to write16 to avoid the extra copy */ /* this will allow us to get rid of 'buf' completely */ @@ -293,9 +305,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled); if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -307,8 +316,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB * @@ -334,6 +341,7 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num, acb->qiov = qiov; acb->canceled = 0; + acb->status = -EINPROGRESS; acb->read_size = qemu_read_size; acb->buf = NULL; @@ -409,9 +417,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, IscsiAIOCB *acb = opaque; if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -423,8 +428,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB * @@ -439,6 +442,7 @@ iscsi_aio_flush(BlockDriverState *bs, acb->iscsilun = iscsilun; acb->canceled = 0; + acb->status = -EINPROGRESS; acb->task = iscsi_synchronizecache10_task(iscsi, iscsilun->lun, 0, 0, 0, 0, @@ -463,9 +467,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, IscsiAIOCB *acb = opaque; if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -477,8 +478,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB * @@ -495,6 +494,7 @@ iscsi_aio_discard(BlockDriverState *bs, acb->iscsilun = iscsilun; acb->canceled = 0; + acb->status = -EINPROGRESS; list[0].lba = sector_qemu2lun(sector_num, iscsilun); list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size; @@ -523,9 +523,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, IscsiAIOCB *acb = opaque; if (acb->canceled != 0) { - qemu_aio_release(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; return; } @@ -554,8 +551,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, } iscsi_schedule_bh(acb); - scsi_free_scsi_task(acb->task); - acb->task = NULL; } static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs, @@ -573,6 +568,7 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs, acb->iscsilun = iscsilun; acb->canceled = 0; + acb->status = -EINPROGRESS; acb->buf = NULL; acb->ioh = buf; -- 1.7.11.2