[{"id":1769880,"web_url":"http://patchwork.ozlabs.org/comment/1769880/","msgid":"<20170918060222.GE15551@lemon.lan>","list_archive_url":null,"date":"2017-09-18T06:02:22","subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Wed, 09/13 20:18, Max Reitz wrote:\n> In order to talk to the source BDS (and maybe in the future to the\n> target BDS as well) directly, we need to convert our existing AIO\n> requests into coroutine I/O requests.\n> \n> Signed-off-by: Max Reitz \n> ---\n> block/mirror.c | 134 +++++++++++++++++++++++++++++++++------------------------\n> 1 file changed, 78 insertions(+), 56 deletions(-)\n> \n> diff --git a/block/mirror.c b/block/mirror.c\n> index 4664b0516f..2b3297aa61 100644\n> --- a/block/mirror.c\n> +++ b/block/mirror.c\n> @@ -80,6 +80,9 @@ typedef struct MirrorOp {\n> QEMUIOVector qiov;\n> int64_t offset;\n> uint64_t bytes;\n> +\n> + /* Set by mirror_co_read() before yielding for the first time */\n> + uint64_t bytes_copied;\n> } MirrorOp;\n> \n> typedef enum MirrorMethod {\n> @@ -101,7 +104,7 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,\n> }\n> }\n> \n> -static void mirror_iteration_done(MirrorOp *op, int ret)\n> +static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)\n> {\n> MirrorBlockJob *s = op->s;\n> struct iovec *iov;\n> @@ -138,9 +141,8 @@ static void mirror_iteration_done(MirrorOp *op, int ret)\n> }\n> }\n> \n> -static void mirror_write_complete(void *opaque, int ret)\n> +static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)\n> {\n> - MirrorOp *op = opaque;\n> MirrorBlockJob *s = op->s;\n> \n> aio_context_acquire(blk_get_aio_context(s->common.blk));\n> @@ -158,9 +160,8 @@ static void mirror_write_complete(void *opaque, int ret)\n> aio_context_release(blk_get_aio_context(s->common.blk));\n> }\n> \n> -static void mirror_read_complete(void *opaque, int ret)\n> +static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)\n> {\n> - MirrorOp *op = opaque;\n> MirrorBlockJob *s = op->s;\n> \n> aio_context_acquire(blk_get_aio_context(s->common.blk));\n> @@ -176,8 +177,11 @@ static void mirror_read_complete(void *opaque, int ret)\n> \n> mirror_iteration_done(op, ret);\n> } else {\n> - blk_aio_pwritev(s->target, op->offset, &op->qiov,\n> - 0, mirror_write_complete, op);\n> + int ret;\n> +\n> + ret = blk_co_pwritev(s->target, op->offset,\n> + op->qiov.size, &op->qiov, 0);\n> + mirror_write_complete(op, ret);\n> }\n> aio_context_release(blk_get_aio_context(s->common.blk));\n> }\n> @@ -242,53 +246,49 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)\n> * (new_end - offset) if tail is rounded up or down due to\n> * alignment or buffer limit.\n> */\n> -static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,\n> - uint64_t bytes)\n> +static void coroutine_fn mirror_co_read(void *opaque)\n> {\n> + MirrorOp *op = opaque;\n> + MirrorBlockJob *s = op->s;\n> BlockBackend *source = s->common.blk;\n> int nb_chunks;\n> uint64_t ret;\n> - MirrorOp *op;\n> uint64_t max_bytes;\n> \n> max_bytes = s->granularity * s->max_iov;\n> \n> /* We can only handle as much as buf_size at a time. */\n> - bytes = MIN(s->buf_size, MIN(max_bytes, bytes));\n> - assert(bytes);\n> - assert(bytes < BDRV_REQUEST_MAX_BYTES);\n> - ret = bytes;\n> + op->bytes = MIN(s->buf_size, MIN(max_bytes, op->bytes));\n> + assert(op->bytes);\n> + assert(op->bytes < BDRV_REQUEST_MAX_BYTES);\n> + op->bytes_copied = op->bytes;\n> \n> if (s->cow_bitmap) {\n> - ret += mirror_cow_align(s, &offset, &bytes);\n> + op->bytes_copied += mirror_cow_align(s, &op->offset, &op->bytes);\n> }\n> - assert(bytes <= s->buf_size);\n> + /* Cannot exceed BDRV_REQUEST_MAX_BYTES + INT_MAX */\n> + assert(op->bytes_copied <= UINT_MAX);\n> + assert(op->bytes <= s->buf_size);\n> /* The offset is granularity-aligned because:\n> * 1) Caller passes in aligned values;\n> * 2) mirror_cow_align is used only when target cluster is larger. */\n> - assert(QEMU_IS_ALIGNED(offset, s->granularity));\n> + assert(QEMU_IS_ALIGNED(op->offset, s->granularity));\n> /* The range is sector-aligned, since bdrv_getlength() rounds up. */\n> - assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));\n> - nb_chunks = DIV_ROUND_UP(bytes, s->granularity);\n> + assert(QEMU_IS_ALIGNED(op->bytes, BDRV_SECTOR_SIZE));\n> + nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);\n> \n> while (s->buf_free_count < nb_chunks) {\n> - trace_mirror_yield_in_flight(s, offset, s->in_flight);\n> + trace_mirror_yield_in_flight(s, op->offset, s->in_flight);\n> mirror_wait_for_io(s);\n> }\n> \n> - /* Allocate a MirrorOp that is used as an AIO callback. */\n> - op = g_new(MirrorOp, 1);\n> - op->s = s;\n> - op->offset = offset;\n> - op->bytes = bytes;\n> -\n> /* Now make a QEMUIOVector taking enough granularity-sized chunks\n> * from s->buf_free.\n> */\n> qemu_iovec_init(&op->qiov, nb_chunks);\n> while (nb_chunks-- > 0) {\n> MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);\n> - size_t remaining = bytes - op->qiov.size;\n> + size_t remaining = op->bytes - op->qiov.size;\n> \n> QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);\n> s->buf_free_count--;\n> @@ -297,53 +297,75 @@ static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,\n> \n> /* Copy the dirty cluster. */\n> s->in_flight++;\n> - s->bytes_in_flight += bytes;\n> - trace_mirror_one_iteration(s, offset, bytes);\n> + s->bytes_in_flight += op->bytes;\n> + trace_mirror_one_iteration(s, op->offset, op->bytes);\n> \n> - blk_aio_preadv(source, offset, &op->qiov, 0, mirror_read_complete, op);\n> - return ret;\n> + ret = blk_co_preadv(source, op->offset, op->bytes, &op->qiov, 0);\n> + mirror_read_complete(op, ret);\n> }\n> \n> -static void mirror_do_zero_or_discard(MirrorBlockJob *s,\n> - int64_t offset,\n> - uint64_t bytes,\n> - bool is_discard)\n> +static void coroutine_fn mirror_co_zero(void *opaque)\n> {\n> - MirrorOp *op;\n> + MirrorOp *op = opaque;\n> + int ret;\n> \n> - /* Allocate a MirrorOp that is used as an AIO callback. The qiov is zeroed\n> - * so the freeing in mirror_iteration_done is nop. */\n> - op = g_new0(MirrorOp, 1);\n> - op->s = s;\n> - op->offset = offset;\n> - op->bytes = bytes;\n> + op->s->in_flight++;\n> + op->s->bytes_in_flight += op->bytes;\n> \n> - s->in_flight++;\n> - s->bytes_in_flight += bytes;\n> - if (is_discard) {\n> - blk_aio_pdiscard(s->target, offset,\n> - op->bytes, mirror_write_complete, op);\n> - } else {\n> - blk_aio_pwrite_zeroes(s->target, offset,\n> - op->bytes, s->unmap ? BDRV_REQ_MAY_UNMAP : 0,\n> - mirror_write_complete, op);\n> - }\n> + ret = blk_co_pwrite_zeroes(op->s->target, op->offset, op->bytes,\n> + op->s->unmap ? BDRV_REQ_MAY_UNMAP : 0);\n> + mirror_write_complete(op, ret);\n> +}\n> +\n> +static void coroutine_fn mirror_co_discard(void *opaque)\n> +{\n> + MirrorOp *op = opaque;\n> + int ret;\n> +\n> + op->s->in_flight++;\n> + op->s->bytes_in_flight += op->bytes;\n> +\n> + ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes);\n> + mirror_write_complete(op, ret);\n> }\n> \n> static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,\n> unsigned bytes, MirrorMethod mirror_method)\n> {\n> + MirrorOp *op;\n> + Coroutine *co;\n> + unsigned ret = bytes;\n> +\n> + op = g_new(MirrorOp, 1);\n> + *op = (MirrorOp){\n> + .s = s,\n> + .offset = offset,\n> + .bytes = bytes,\n> + };\n> +\n> switch (mirror_method) {\n> case MIRROR_METHOD_COPY:\n> - return mirror_do_read(s, offset, bytes);\n> + co = qemu_coroutine_create(mirror_co_read, op);\n> + break;\n> case MIRROR_METHOD_ZERO:\n> + co = qemu_coroutine_create(mirror_co_zero, op);\n> + break;\n> case MIRROR_METHOD_DISCARD:\n> - mirror_do_zero_or_discard(s, offset, bytes,\n> - mirror_method == MIRROR_METHOD_DISCARD);\n> - return bytes;\n> + co = qemu_coroutine_create(mirror_co_discard, op);\n> + break;\n> default:\n> abort();\n> }\n> +\n> + qemu_coroutine_enter(co);\n> +\n> + if (mirror_method == MIRROR_METHOD_COPY) {\n> + /* Same assertion as in mirror_co_read() */\n> + assert(op->bytes_copied <= UINT_MAX);\n> + ret = op->bytes_copied;\n> + }\n\nThis special casing is a bit ugly. Can you just make mirror_co_zero and\nmirror_co_discard set op->bytes_copied too? (and perhaps rename to\nop->bytes_handled) If so the comment in MirrorOp needs an update too.\n\nAnd is it better to initialize it to -1 before entering coroutine, then assert\nit is != -1 afterwards?\n\n> +\n> + return ret;\n> }\n> \n> static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)\n> -- \n> 2.13.5\n> \n\nFam","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=famz@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xwb5x2S8Fz9s7c\n\tfor ;\n\tMon, 18 Sep 2017 16:03:15 +1000 (AEST)","from localhost ([::1]:34798 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtp9E-0004BV-MM\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 02:03:12 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:42041)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dtp8g-00049T-5V\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 02:02:39 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dtp8e-0006Bo-L4\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 02:02:38 -0400","from mx1.redhat.com ([209.132.183.28]:42240)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dtp8Z-00069X-9P; Mon, 18 Sep 2017 02:02:31 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 23C035AFD9;\n\tMon, 18 Sep 2017 06:02:30 +0000 (UTC)","from localhost (ovpn-12-141.pek2.redhat.com [10.72.12.141])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 6014F1880C;\n\tMon, 18 Sep 2017 06:02:25 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 23C035AFD9","Date":"Mon, 18 Sep 2017 14:02:22 +0800","From":"Fam Zheng ","To":"Max Reitz ","Message-ID":"<20170918060222.GE15551@lemon.lan>","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-6-mreitz@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170913181910.29688-6-mreitz@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tMon, 18 Sep 2017 06:02:30 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Kevin Wolf , John Snow ,\n\tStefan Hajnoczi , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1770304,"web_url":"http://patchwork.ozlabs.org/comment/1770304/","msgid":"","list_archive_url":null,"date":"2017-09-18T16:41:01","subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","submitter":{"id":36836,"url":"http://patchwork.ozlabs.org/api/people/36836/","name":"Max Reitz","email":"mreitz@redhat.com"},"content":"On 2017-09-18 08:02, Fam Zheng wrote:\n> On Wed, 09/13 20:18, Max Reitz wrote:\n>> In order to talk to the source BDS (and maybe in the future to the\n>> target BDS as well) directly, we need to convert our existing AIO\n>> requests into coroutine I/O requests.\n>>\n>> Signed-off-by: Max Reitz \n>> ---\n>> block/mirror.c | 134 +++++++++++++++++++++++++++++++++------------------------\n>> 1 file changed, 78 insertions(+), 56 deletions(-)\n>>\n>> diff --git a/block/mirror.c b/block/mirror.c\n>> index 4664b0516f..2b3297aa61 100644\n>> --- a/block/mirror.c\n>> +++ b/block/mirror.c\n>> @@ -80,6 +80,9 @@ typedef struct MirrorOp {\n>> QEMUIOVector qiov;\n>> int64_t offset;\n>> uint64_t bytes;\n>> +\n>> + /* Set by mirror_co_read() before yielding for the first time */\n>> + uint64_t bytes_copied;\n>> } MirrorOp;\n>> \n>> typedef enum MirrorMethod {\n>> @@ -101,7 +104,7 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,\n>> }\n>> }\n>> \n>> -static void mirror_iteration_done(MirrorOp *op, int ret)\n>> +static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)\n>> {\n>> MirrorBlockJob *s = op->s;\n>> struct iovec *iov;\n>> @@ -138,9 +141,8 @@ static void mirror_iteration_done(MirrorOp *op, int ret)\n>> }\n>> }\n>> \n>> -static void mirror_write_complete(void *opaque, int ret)\n>> +static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)\n>> {\n>> - MirrorOp *op = opaque;\n>> MirrorBlockJob *s = op->s;\n>> \n>> aio_context_acquire(blk_get_aio_context(s->common.blk));\n>> @@ -158,9 +160,8 @@ static void mirror_write_complete(void *opaque, int ret)\n>> aio_context_release(blk_get_aio_context(s->common.blk));\n>> }\n>> \n>> -static void mirror_read_complete(void *opaque, int ret)\n>> +static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)\n>> {\n>> - MirrorOp *op = opaque;\n>> MirrorBlockJob *s = op->s;\n>> \n>> aio_context_acquire(blk_get_aio_context(s->common.blk));\n>> @@ -176,8 +177,11 @@ static void mirror_read_complete(void *opaque, int ret)\n>> \n>> mirror_iteration_done(op, ret);\n>> } else {\n>> - blk_aio_pwritev(s->target, op->offset, &op->qiov,\n>> - 0, mirror_write_complete, op);\n>> + int ret;\n>> +\n>> + ret = blk_co_pwritev(s->target, op->offset,\n>> + op->qiov.size, &op->qiov, 0);\n>> + mirror_write_complete(op, ret);\n>> }\n>> aio_context_release(blk_get_aio_context(s->common.blk));\n>> }\n>> @@ -242,53 +246,49 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)\n>> * (new_end - offset) if tail is rounded up or down due to\n>> * alignment or buffer limit.\n>> */\n>> -static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,\n>> - uint64_t bytes)\n>> +static void coroutine_fn mirror_co_read(void *opaque)\n>> {\n>> + MirrorOp *op = opaque;\n>> + MirrorBlockJob *s = op->s;\n>> BlockBackend *source = s->common.blk;\n>> int nb_chunks;\n>> uint64_t ret;\n>> - MirrorOp *op;\n>> uint64_t max_bytes;\n>> \n>> max_bytes = s->granularity * s->max_iov;\n>> \n>> /* We can only handle as much as buf_size at a time. */\n>> - bytes = MIN(s->buf_size, MIN(max_bytes, bytes));\n>> - assert(bytes);\n>> - assert(bytes < BDRV_REQUEST_MAX_BYTES);\n>> - ret = bytes;\n>> + op->bytes = MIN(s->buf_size, MIN(max_bytes, op->bytes));\n>> + assert(op->bytes);\n>> + assert(op->bytes < BDRV_REQUEST_MAX_BYTES);\n>> + op->bytes_copied = op->bytes;\n>> \n>> if (s->cow_bitmap) {\n>> - ret += mirror_cow_align(s, &offset, &bytes);\n>> + op->bytes_copied += mirror_cow_align(s, &op->offset, &op->bytes);\n>> }\n>> - assert(bytes <= s->buf_size);\n>> + /* Cannot exceed BDRV_REQUEST_MAX_BYTES + INT_MAX */\n>> + assert(op->bytes_copied <= UINT_MAX);\n>> + assert(op->bytes <= s->buf_size);\n>> /* The offset is granularity-aligned because:\n>> * 1) Caller passes in aligned values;\n>> * 2) mirror_cow_align is used only when target cluster is larger. */\n>> - assert(QEMU_IS_ALIGNED(offset, s->granularity));\n>> + assert(QEMU_IS_ALIGNED(op->offset, s->granularity));\n>> /* The range is sector-aligned, since bdrv_getlength() rounds up. */\n>> - assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));\n>> - nb_chunks = DIV_ROUND_UP(bytes, s->granularity);\n>> + assert(QEMU_IS_ALIGNED(op->bytes, BDRV_SECTOR_SIZE));\n>> + nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);\n>> \n>> while (s->buf_free_count < nb_chunks) {\n>> - trace_mirror_yield_in_flight(s, offset, s->in_flight);\n>> + trace_mirror_yield_in_flight(s, op->offset, s->in_flight);\n>> mirror_wait_for_io(s);\n>> }\n>> \n>> - /* Allocate a MirrorOp that is used as an AIO callback. */\n>> - op = g_new(MirrorOp, 1);\n>> - op->s = s;\n>> - op->offset = offset;\n>> - op->bytes = bytes;\n>> -\n>> /* Now make a QEMUIOVector taking enough granularity-sized chunks\n>> * from s->buf_free.\n>> */\n>> qemu_iovec_init(&op->qiov, nb_chunks);\n>> while (nb_chunks-- > 0) {\n>> MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);\n>> - size_t remaining = bytes - op->qiov.size;\n>> + size_t remaining = op->bytes - op->qiov.size;\n>> \n>> QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);\n>> s->buf_free_count--;\n>> @@ -297,53 +297,75 @@ static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,\n>> \n>> /* Copy the dirty cluster. */\n>> s->in_flight++;\n>> - s->bytes_in_flight += bytes;\n>> - trace_mirror_one_iteration(s, offset, bytes);\n>> + s->bytes_in_flight += op->bytes;\n>> + trace_mirror_one_iteration(s, op->offset, op->bytes);\n>> \n>> - blk_aio_preadv(source, offset, &op->qiov, 0, mirror_read_complete, op);\n>> - return ret;\n>> + ret = blk_co_preadv(source, op->offset, op->bytes, &op->qiov, 0);\n>> + mirror_read_complete(op, ret);\n>> }\n>> \n>> -static void mirror_do_zero_or_discard(MirrorBlockJob *s,\n>> - int64_t offset,\n>> - uint64_t bytes,\n>> - bool is_discard)\n>> +static void coroutine_fn mirror_co_zero(void *opaque)\n>> {\n>> - MirrorOp *op;\n>> + MirrorOp *op = opaque;\n>> + int ret;\n>> \n>> - /* Allocate a MirrorOp that is used as an AIO callback. The qiov is zeroed\n>> - * so the freeing in mirror_iteration_done is nop. */\n>> - op = g_new0(MirrorOp, 1);\n>> - op->s = s;\n>> - op->offset = offset;\n>> - op->bytes = bytes;\n>> + op->s->in_flight++;\n>> + op->s->bytes_in_flight += op->bytes;\n>> \n>> - s->in_flight++;\n>> - s->bytes_in_flight += bytes;\n>> - if (is_discard) {\n>> - blk_aio_pdiscard(s->target, offset,\n>> - op->bytes, mirror_write_complete, op);\n>> - } else {\n>> - blk_aio_pwrite_zeroes(s->target, offset,\n>> - op->bytes, s->unmap ? BDRV_REQ_MAY_UNMAP : 0,\n>> - mirror_write_complete, op);\n>> - }\n>> + ret = blk_co_pwrite_zeroes(op->s->target, op->offset, op->bytes,\n>> + op->s->unmap ? BDRV_REQ_MAY_UNMAP : 0);\n>> + mirror_write_complete(op, ret);\n>> +}\n>> +\n>> +static void coroutine_fn mirror_co_discard(void *opaque)\n>> +{\n>> + MirrorOp *op = opaque;\n>> + int ret;\n>> +\n>> + op->s->in_flight++;\n>> + op->s->bytes_in_flight += op->bytes;\n>> +\n>> + ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes);\n>> + mirror_write_complete(op, ret);\n>> }\n>> \n>> static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,\n>> unsigned bytes, MirrorMethod mirror_method)\n>> {\n>> + MirrorOp *op;\n>> + Coroutine *co;\n>> + unsigned ret = bytes;\n>> +\n>> + op = g_new(MirrorOp, 1);\n>> + *op = (MirrorOp){\n>> + .s = s,\n>> + .offset = offset,\n>> + .bytes = bytes,\n>> + };\n>> +\n>> switch (mirror_method) {\n>> case MIRROR_METHOD_COPY:\n>> - return mirror_do_read(s, offset, bytes);\n>> + co = qemu_coroutine_create(mirror_co_read, op);\n>> + break;\n>> case MIRROR_METHOD_ZERO:\n>> + co = qemu_coroutine_create(mirror_co_zero, op);\n>> + break;\n>> case MIRROR_METHOD_DISCARD:\n>> - mirror_do_zero_or_discard(s, offset, bytes,\n>> - mirror_method == MIRROR_METHOD_DISCARD);\n>> - return bytes;\n>> + co = qemu_coroutine_create(mirror_co_discard, op);\n>> + break;\n>> default:\n>> abort();\n>> }\n>> +\n>> + qemu_coroutine_enter(co);\n>> +\n>> + if (mirror_method == MIRROR_METHOD_COPY) {\n>> + /* Same assertion as in mirror_co_read() */\n>> + assert(op->bytes_copied <= UINT_MAX);\n>> + ret = op->bytes_copied;\n>> + }\n> \n> This special casing is a bit ugly. Can you just make mirror_co_zero and\n> mirror_co_discard set op->bytes_copied too? (and perhaps rename to\n> op->bytes_handled) If so the comment in MirrorOp needs an update too.\n\nSure.\n\n> And is it better to initialize it to -1 before entering coroutine, then assert\n> it is != -1 afterwards?\n\nSounds good, will do.\n\nMax","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=mreitz@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xwsGf2G29z9s72\n\tfor ;\n\tTue, 19 Sep 2017 02:41:45 +1000 (AEST)","from localhost ([::1]:37832 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dtz78-0007YM-Qr\n\tfor incoming@patchwork.ozlabs.org; Mon, 18 Sep 2017 12:41:42 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:54413)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dtz6o-0007Wm-D5\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 12:41:23 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dtz6m-0007Bz-Ho\n\tfor qemu-devel@nongnu.org; Mon, 18 Sep 2017 12:41:22 -0400","from mx1.redhat.com ([209.132.183.28]:55982)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dtz6g-00075V-F9; Mon, 18 Sep 2017 12:41:14 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 2F28C85550;\n\tMon, 18 Sep 2017 16:41:13 +0000 (UTC)","from dresden.str.redhat.com (unknown [10.40.205.73])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id D51F360610;\n\tMon, 18 Sep 2017 16:41:03 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 2F28C85550","To":"Fam Zheng ","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-6-mreitz@redhat.com>\n\t<20170918060222.GE15551@lemon.lan>","From":"Max Reitz ","Message-ID":"","Date":"Mon, 18 Sep 2017 18:41:01 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170918060222.GE15551@lemon.lan>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"eC0k6e0JeWRTWWHwG7dBuCTuDIpETvWHe\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.28]);\n\tMon, 18 Sep 2017 16:41:13 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Kevin Wolf , John Snow ,\n\tStefan Hajnoczi , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1783538,"web_url":"http://patchwork.ozlabs.org/comment/1783538/","msgid":"<20171010091420.GD4177@dhcp-200-186.str.redhat.com>","list_archive_url":null,"date":"2017-10-10T09:14:20","subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/","name":"Kevin Wolf","email":"kwolf@redhat.com"},"content":"Am 13.09.2017 um 20:18 hat Max Reitz geschrieben:\n> In order to talk to the source BDS (and maybe in the future to the\n> target BDS as well) directly, we need to convert our existing AIO\n> requests into coroutine I/O requests.\n> \n> Signed-off-by: Max Reitz \n\nPlease follow through with it and add a few patches that turn it into\nnatural coroutine code rather than just any coroutine code. I know I did\nthe same kind of half-assed conversion in qed, but mirror is code that\nis actually used and that people look at for more than just a bad\nexample.\n\nYou'll probably notice more things when you do this, but the obvious\nthings would be changing mirror_co_read() into a mirror_co_copy() with\nthe former callbacks inlined; keeping op on the stack instead of\nmallocing it in mirror_perform() and free it deep inside the nested\nfunctions that used to be callbacks; and probably also cleaning up the\nrandom calls to aio_context_acquire/release() that will now appear in\nthe middle of the function.\n\nAnyway, that's for follow-up patches (though ideally in the same\nseries), so for this one you can have:\n\nReviewed-by: Kevin Wolf ","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx06.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=kwolf@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yBBKL1JYNz9t6Y\n\tfor ;\n\tTue, 10 Oct 2017 20:15:18 +1100 (AEDT)","from localhost ([::1]:33666 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e1qdA-0005sz-93\n\tfor incoming@patchwork.ozlabs.org; Tue, 10 Oct 2017 05:15:16 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35940)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e1qcd-0005pb-7L\n\tfor qemu-devel@nongnu.org; Tue, 10 Oct 2017 05:14:47 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e1qcX-0005rF-JC\n\tfor qemu-devel@nongnu.org; Tue, 10 Oct 2017 05:14:43 -0400","from mx1.redhat.com ([209.132.183.28]:18913)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e1qcS-0005oa-FO; Tue, 10 Oct 2017 05:14:32 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 918A2356EC;\n\tTue, 10 Oct 2017 09:14:31 +0000 (UTC)","from dhcp-200-186.str.redhat.com (dhcp-200-186.str.redhat.com\n\t[10.33.200.186])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id EB3C562460;\n\tTue, 10 Oct 2017 09:14:21 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 918A2356EC","Date":"Tue, 10 Oct 2017 11:14:20 +0200","From":"Kevin Wolf ","To":"Max Reitz ","Message-ID":"<20171010091420.GD4177@dhcp-200-186.str.redhat.com>","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-6-mreitz@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170913181910.29688-6-mreitz@redhat.com>","User-Agent":"Mutt/1.9.1 (2017-09-22)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.30]);\n\tTue, 10 Oct 2017 09:14:31 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Stefan Hajnoczi , John Snow ,\n\tFam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1784541,"web_url":"http://patchwork.ozlabs.org/comment/1784541/","msgid":"<3ecdb2b4-735d-ac96-f1b8-e48ffa63e476@redhat.com>","list_archive_url":null,"date":"2017-10-11T11:43:44","subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","submitter":{"id":36836,"url":"http://patchwork.ozlabs.org/api/people/36836/","name":"Max Reitz","email":"mreitz@redhat.com"},"content":"On 2017-10-10 11:14, Kevin Wolf wrote:\n> Am 13.09.2017 um 20:18 hat Max Reitz geschrieben:\n>> In order to talk to the source BDS (and maybe in the future to the\n>> target BDS as well) directly, we need to convert our existing AIO\n>> requests into coroutine I/O requests.\n>>\n>> Signed-off-by: Max Reitz \n> \n> Please follow through with it and add a few patches that turn it into\n> natural coroutine code rather than just any coroutine code. I know I did\n> the same kind of half-assed conversion in qed, but mirror is code that\n> is actually used and that people look at for more than just a bad\n> example.\n> \n> You'll probably notice more things when you do this, but the obvious\n> things would be changing mirror_co_read() into a mirror_co_copy() with\n> the former callbacks inlined; keeping op on the stack instead of\n> mallocing it in mirror_perform() and free it deep inside the nested\n> functions that used to be callbacks; and probably also cleaning up the\n> random calls to aio_context_acquire/release() that will now appear in\n> the middle of the function.\n> \n> Anyway, that's for follow-up patches (though ideally in the same\n> series), so for this one you can have:\n> \n> Reviewed-by: Kevin Wolf \n\nPhew. :-)\n\nI think I'll write the patches (while working on v2), but I'll send them\nas a follow-up.\n\nMax","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=mreitz@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yBsbZ4K2lz9sNx\n\tfor ;\n\tWed, 11 Oct 2017 22:44:58 +1100 (AEDT)","from localhost ([::1]:40394 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e2FRY-00039I-OG\n\tfor incoming@patchwork.ozlabs.org; Wed, 11 Oct 2017 07:44:56 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56227)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e2FQg-0002pZ-2y\n\tfor qemu-devel@nongnu.org; Wed, 11 Oct 2017 07:44:06 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e2FQf-0005AU-7f\n\tfor qemu-devel@nongnu.org; Wed, 11 Oct 2017 07:44:02 -0400","from mx1.redhat.com ([209.132.183.28]:51940)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e2FQa-00058k-P5; Wed, 11 Oct 2017 07:43:56 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id C2DBF68D3;\n\tWed, 11 Oct 2017 11:43:55 +0000 (UTC)","from dresden.str.redhat.com (unknown [10.40.205.86])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 61A4C5C8AF;\n\tWed, 11 Oct 2017 11:43:46 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com C2DBF68D3","To":"Kevin Wolf ","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-6-mreitz@redhat.com>\n\t<20171010091420.GD4177@dhcp-200-186.str.redhat.com>","From":"Max Reitz ","Message-ID":"<3ecdb2b4-735d-ac96-f1b8-e48ffa63e476@redhat.com>","Date":"Wed, 11 Oct 2017 13:43:44 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20171010091420.GD4177@dhcp-200-186.str.redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"5mO7mj1HjCRRVIVEVe1RETHAufSM3HLpb\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tWed, 11 Oct 2017 11:43:55 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Stefan Hajnoczi , John Snow ,\n\tFam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]