From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7Eny-0007nG-P1 for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:14:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7Enp-0005rP-6m for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:14:18 -0400 Received: from mail6.webfaction.com ([74.55.86.74]:33459 helo=smtp.webfaction.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7Eno-0005nH-VZ for qemu-devel@nongnu.org; Wed, 07 Aug 2013 21:14:09 -0400 Message-ID: <5202F0DC.3070404@ctshepherd.com> Date: Thu, 08 Aug 2013 02:14:04 +0100 From: Charlie Shepherd MIME-Version: 1.0 References: <1375728247-1306-1-git-send-email-charlie@ctshepherd.com> <1375728247-1306-4-git-send-email-charlie@ctshepherd.com> <20130806092413.GF3117@dhcp-200-207.str.redhat.com> In-Reply-To: <20130806092413.GF3117@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, gabriel@kerneis.info, qemu-devel@nongnu.org, stefanha@gmail.com On 06/08/2013 10:24, Kevin Wolf wrote: > Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben: >> This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of struct BlockDriver >> to be explicitly annotated as coroutine_fn, rather than yielding dynamically depending on whether >> they are executed in a coroutine context or not. > I wonder whether this patch should be split into one for each converted > BlockDriver function. It would probably make review easier. > > For those function where you actually just correct the coroutine_fn > annotation, but whose behaviour doesn't change, a single patch for all > might be enough. > >> block.c | 16 +++++++-------- >> block/blkdebug.c | 10 ++++----- >> block/blkverify.c | 4 ++-- >> block/bochs.c | 8 ++++---- >> block/cloop.c | 6 +++--- >> block/cow.c | 12 +++++------ >> block/curl.c | 12 +++++------ >> block/dmg.c | 6 +++--- >> block/nbd.c | 28 ++++++++++++------------- >> block/parallels.c | 6 +++--- >> block/qcow.c | 8 ++++---- >> block/qcow2-cluster.c | 8 ++++---- >> block/qcow2.c | 48 +++++++++++++++++++++++++++++++++++++------ >> block/qcow2.h | 4 ++-- >> block/qed.c | 8 ++++---- >> block/raw-posix.c | 34 +++++++++++++++--------------- >> block/raw.c | 8 ++++---- >> block/sheepdog.c | 24 +++++++++++----------- >> block/snapshot.c | 32 ++++++++++++++++++++++++++++- >> block/ssh.c | 14 ++++++------- >> block/vdi.c | 12 +++++------ >> block/vhdx.c | 4 ++-- >> block/vmdk.c | 12 +++++------ >> block/vpc.c | 12 +++++------ >> block/vvfat.c | 12 +++++------ >> include/block/block_int.h | 10 ++++----- >> include/block/coroutine.h | 4 ++-- >> include/block/coroutine_int.h | 2 +- >> qemu-coroutine-lock.c | 4 ++-- >> 30 files changed, 218 insertions(+), 152 deletions(-) >> >> diff --git a/block.c b/block.c >> index 6c493ad..aaa122c 100644 >> --- a/block.c >> +++ b/block.c >> @@ -374,7 +374,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque) >> CreateCo *cco = opaque; >> assert(cco->drv); >> >> - cco->ret = cco->drv->bdrv_create(cco->filename, cco->options); >> + cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options); >> } >> >> int bdrv_create(BlockDriver *drv, const char* filename, >> @@ -390,7 +390,7 @@ int bdrv_create(BlockDriver *drv, const char* filename, >> .ret = NOT_DONE, >> }; >> >> - if (!drv->bdrv_create) { >> + if (!drv->bdrv_co_create) { >> ret = -ENOTSUP; >> goto out; >> } >> @@ -697,7 +697,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, >> /* bdrv_open() with directly using a protocol as drv. This layer is already >> * opened, so assign it to bs (while file becomes a closed BlockDriverState) >> * and return immediately. */ >> - if (file != NULL && drv->bdrv_file_open) { >> + if (file != NULL && drv->bdrv_co_file_open) { >> bdrv_swap(file, bs); >> return 0; >> } >> @@ -728,10 +728,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, >> bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB); >> >> /* Open the image, either directly or using a protocol */ >> - if (drv->bdrv_file_open) { >> + if (drv->bdrv_co_file_open) { >> assert(file == NULL); >> assert(drv->bdrv_parse_filename || filename != NULL); >> - ret = drv->bdrv_file_open(bs, options, open_flags); >> + ret = drv->bdrv_co_file_open(bs, options, open_flags); > bdrv_open_common() isn't coroutine_fn, though? > > Ah well, I see that you change it in a later patch. Please make sure > that the code is in a consistent state after each single commit. > > For this series, I think this suggests that you indeed split by converted > function, but put everything related to this function in one patch. For > example, the bdrv_open_common() conversion would be in the same patch as > this hunk. Yeah, it's in this kind of scenario that I struggled to split up the patch series properly. I'll try and implement your approach in the next version. >> } else { >> if (file == NULL) { >> qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a " >> @@ -742,7 +742,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, >> } >> assert(file != NULL); >> bs->file = file; >> - ret = drv->bdrv_open(bs, options, open_flags); >> + ret = drv->bdrv_co_open(bs, options, open_flags); >> } >> >> if (ret < 0) { >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 0eceefe..2ed0bb6 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -58,6 +58,10 @@ typedef struct { >> #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA >> #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857 >> >> + >> +#define NOT_DONE 0x7fffffff >> + >> + >> static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename) >> { >> const QCowHeader *cow_header = (const void *)buf; >> @@ -315,7 +319,7 @@ static QemuOptsList qcow2_runtime_opts = { >> }, >> }; >> >> -static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) >> +static int coroutine_fn qcow2_co_open(BlockDriverState *bs, QDict *options, int flags) >> { >> BDRVQcowState *s = bs->opaque; >> int len, i, ret = 0; >> @@ -590,6 +594,38 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) >> return ret; >> } >> >> +struct QOpenCo { >> + BlockDriverState *bs; >> + QDict *options; >> + int flags; >> + int ret; >> +}; >> + >> +static void coroutine_fn qcow2_co_open_entry(void *opaque) >> +{ >> + struct QOpenCo *qo = opaque; >> + >> + qo->ret = qcow2_co_open(qo->bs, qo->options, qo->flags); >> +} >> + >> +static int qcow2_open(BlockDriverState *bs, QDict *options, int flags) >> +{ >> + Coroutine *co; >> + struct QOpenCo qo = { >> + .bs = bs, >> + .options = options, >> + .flags = flags, >> + .ret = NOT_DONE, >> + }; >> + >> + co = qemu_coroutine_create(qcow2_co_open_entry); >> + qemu_coroutine_enter(co, &qo); >> + while (qo.ret == NOT_DONE) { >> + qemu_aio_wait(); >> + } >> + return qo.ret; >> +} > I think it would be better to convert qcow2_invalidate_cache() to > coroutine_fn (which you apparently do in patch 4) so that it can > directly call qcow2_co_open, and to keep this coroutine wrapper in > the block.c function. Ok I'll make this change. Charlie