From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57553) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwqtq-0003v3-0i for qemu-devel@nongnu.org; Wed, 10 Jul 2013 05:41:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uwqtl-0004LD-3k for qemu-devel@nongnu.org; Wed, 10 Jul 2013 05:41:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31290) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwqtk-0004L7-SZ for qemu-devel@nongnu.org; Wed, 10 Jul 2013 05:41:21 -0400 Date: Wed, 10 Jul 2013 11:41:13 +0200 From: Kevin Wolf Message-ID: <20130710094113.GE3898@dhcp-200-207.str.redhat.com> References: <1372338695-411-1-git-send-email-pl@kamp.de> <1372338695-411-4-git-send-email-pl@kamp.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1372338695-411-4-git-send-email-pl@kamp.de> Subject: Re: [Qemu-devel] [PATCHv2 03/11] iscsi: add bdrv_co_is_allocated List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: pbonzini@redhat.com, ronniesahlberg@gmail.com, qemu-devel@nongnu.org, stefanha@redhat.com Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: > this patch adds a coroutine for .bdrv_co_is_allocated as well as > a generic framework that can be used to build coroutines in block/iscsi. > > Signed-off-by: Peter Lieven > --- > block/iscsi.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 123 insertions(+) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 2e2455d..d31ee95 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -58,6 +58,15 @@ typedef struct IscsiLun { > uint32_t max_unmap_bdc; > } IscsiLun; > > +typedef struct IscsiTask { > + int status; > + int complete; > + int retries; > + int do_retry; > + struct scsi_task *task; > + Coroutine *co; > +} IscsiTask; > + > typedef struct IscsiAIOCB { > BlockDriverAIOCB common; > QEMUIOVector *qiov; > @@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb) > qemu_bh_schedule(acb->bh); > } > > +static void > +iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, > + void *command_data, void *opaque) > +{ > + struct IscsiTask *iTask = opaque; > + struct scsi_task *task = command_data; > + > + iTask->complete = 1; > + iTask->status = status; > + iTask->do_retry = 0; > + iTask->task = task; > + > + if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION > + && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) { > + iTask->do_retry = 1; > + goto out; > + } > + > + if (status != SCSI_STATUS_GOOD) { > + error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi)); > + goto out; > + } > + > +out: > + if (iTask->status != SCSI_STATUS_GOOD) { > + scsi_free_scsi_task(iTask->task); > + iTask->task = NULL; > + } > + if (iTask->co) { > + qemu_coroutine_enter(iTask->co, NULL); > + } > +} In which context does this callback run? If this is from a separate thread, you may not do all of this (without even holding the QBL). You should probably use a BH to switch to the I/O thread. > + > +static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask *iTask) > +{ > + memset(iTask, 0x00, sizeof(struct IscsiTask)); > + iTask->co = qemu_coroutine_self(); > + iTask->retries = ISCSI_CMD_RETRIES; > +} Matter of taste, but in my own code I prefer not to have a memset: *iTask = (struct IscsiTask) { .co = qemu_coroutine_self(), .retries = ISCSI_CMD_RETRIES, }; > static void > iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data, > @@ -807,6 +855,79 @@ iscsi_getlength(BlockDriverState *bs) > return len; > } > > +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, > + int64_t sector_num, > + int nb_sectors, int *pnum) > +{ > + IscsiLun *iscsilun = bs->opaque; > + struct scsi_get_lba_status *lbas = NULL; > + struct scsi_lba_status_descriptor *lbasd = NULL; > + struct IscsiTask iTask; > + int ret; > + > + /* LUN does not support logical block provisioning */ > + if (iscsilun->lbpme == 0) { > + *pnum = nb_sectors; > + return 1; > + } > + > + iscsi_co_init_iscsitask(iscsilun, &iTask); > + > +retry: > + if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun, > + sector_qemu2lun(sector_num, iscsilun), > + 8 + 16, iscsi_co_generic_cb, > + &iTask) == NULL) { > + goto fail; > + } > + > + while (!iTask.complete) { > + iscsi_set_events(iscsilun); > + qemu_coroutine_yield(); > + } > + > + if (iTask.do_retry) { > + goto retry; > + } > + > + if (iTask.status != SCSI_STATUS_GOOD) { > + /* in case the get_lba_status_callout fails (i.e. > + * because the device is busy or the cmd is not > + * supported) we pretend all blocks are allocated > + * for backwards compatiblity */ > + *pnum = nb_sectors; > + return 1; > + } > + > + lbas = scsi_datain_unmarshall(iTask.task); > + if (lbas == NULL) { > + goto fail; > + } > + > + lbasd = &lbas->descriptors[0]; > + > + if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) { > + goto fail; > + } > + > + *pnum = lbasd->num_blocks * (iscsilun->block_size / BDRV_SECTOR_SIZE); > + if (*pnum > nb_sectors) { > + *pnum = nb_sectors; > + } > + > + ret = lbasd->provisioning == SCSI_PROVISIONING_TYPE_MAPPED ? 1 : 0; Wouldn't it be safer to default to allocated, in case new values are added in a later protocol version? I agree with returning 0 for both unmapped and anchored, though. > + scsi_free_scsi_task(iTask.task); > + return ret; > + > +fail: > + if (iTask.task != NULL) { > + scsi_free_scsi_task(iTask.task); > + } > + *pnum = 0; > + return 0; > +} Kevin