From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48517) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uq7WF-0003s3-PV for qemu-devel@nongnu.org; Fri, 21 Jun 2013 16:01:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uq7WD-0004vU-CO for qemu-devel@nongnu.org; Fri, 21 Jun 2013 16:01:15 -0400 Received: from mail-ea0-x22a.google.com ([2a00:1450:4013:c01::22a]:56730) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uq7WD-0004vF-4Q for qemu-devel@nongnu.org; Fri, 21 Jun 2013 16:01:13 -0400 Received: by mail-ea0-f170.google.com with SMTP id h10so4841914eaj.15 for ; Fri, 21 Jun 2013 13:01:12 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <51C4B104.6060608@redhat.com> Date: Fri, 21 Jun 2013 22:01:08 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1371752409-16313-1-git-send-email-pl@kamp.de> <1371752409-16313-2-git-send-email-pl@kamp.de> <20130621091842.GB2986@dhcp-200-207.str.redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: ronnie sahlberg Cc: Kevin Wolf , Peter Lieven , qemu-devel , Stefan Hajnoczi Il 21/06/2013 18:06, ronnie sahlberg ha scritto: > Should we really mix co-routines and AIO in the same backend? > > Would it not be better to instead add a new bdrb_aio_is_allocaed and > use non-blocking async calls to libiscsi ? Certainly, but is_allocated's code is not the tidiest. I'm going to replace the is_allocated callback with a more generic one (Peter knows, and Kevin might have suspected it too), and I will do the cleanups at the time I do that. Paolo > > On Fri, Jun 21, 2013 at 2:18 AM, Kevin Wolf wrote: >> Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben: >>> Signed-off-by: Peter Lieven >>> --- >>> block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 57 insertions(+) >>> >>> diff --git a/block/iscsi.c b/block/iscsi.c >>> index 0bbf0b1..e6b966d 100644 >>> --- a/block/iscsi.c >>> +++ b/block/iscsi.c >>> @@ -49,6 +49,7 @@ typedef struct IscsiLun { >>> uint64_t num_blocks; >>> int events; >>> QEMUTimer *nop_timer; >>> + uint8_t lbpme; >>> } IscsiLun; >>> >>> typedef struct IscsiAIOCB { >>> @@ -800,6 +801,60 @@ 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_task *task = NULL; >>> + struct scsi_get_lba_status *lbas = NULL; >>> + struct scsi_lba_status_descriptor *lbasd = NULL; >>> + int ret; >>> + >>> + *pnum = nb_sectors; >>> + >>> + if (iscsilun->lbpme == 0) { >>> + return 1; >>> + } >>> + >>> + /* in-flight requests could invalidate the lba status result */ >>> + while (iscsi_process_flush(iscsilun)) { >>> + qemu_aio_wait(); >>> + } >> >> Note that you're blocking here. The preferred way would be something >> involving a yield from the coroutine and a reenter as soon as all >> requests are done. Maybe a CoRwLock does what you need? >> >>> + >>> + task = iscsi_get_lba_status_sync(iscsilun->iscsi, iscsilun->lun, >>> + sector_qemu2lun(sector_num, iscsilun), >>> + 8+16); >> >> Spacing around operators (8 + 16) >> >>> + >>> + if (task == NULL || task->status != SCSI_STATUS_GOOD) { >>> + scsi_free_scsi_task(task); >>> + return 1; >> >> Error cases should set *pnum = 0 and return 0. >> >> Kevin > >