On 2018-06-14 17:22, Kevin Wolf wrote:
> Am 13.06.2018 um 20:18 hat Max Reitz geschrieben:
>> In order to talk to the source BDS (and maybe in the future to the
>> target BDS as well) directly, we need to convert our existing AIO
>> requests into coroutine I/O requests.
>>
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> Reviewed-by: Fam Zheng <f...@redhat.com>
>> ---
>> block/mirror.c | 152 +++++++++++++++++++++++++++++--------------------
>> 1 file changed, 90 insertions(+), 62 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index f5e240611b..47122482ff 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -78,6 +78,10 @@ typedef struct MirrorOp {
>> QEMUIOVector qiov;
>> int64_t offset;
>> uint64_t bytes;
>> +
>> + /* The pointee is set by mirror_co_read(), mirror_co_zero(), and
>> + * mirror_co_discard() before yielding for the first time */
>> + int64_t *bytes_handled;
>
> Should we at least document that this becomes invalid after the
> MirrorOp coroutine has yielded for the first time because it points to a
> stack variable of the caller which returns then?
Can do.
>> } MirrorOp;
>>
>> typedef enum MirrorMethod {
>> @@ -99,7 +103,7 @@ static BlockErrorAction
>> mirror_error_action(MirrorBlockJob *s, bool read,
>> }
>> }
>>
>> -static void mirror_iteration_done(MirrorOp *op, int ret)
>> +static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
>> {
>> MirrorBlockJob *s = op->s;
>> struct iovec *iov;
>> @@ -136,9 +140,8 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
>> }
>> }
>>
>> -static void mirror_write_complete(void *opaque, int ret)
>> +static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
>> {
>> - MirrorOp *op = opaque;
>> MirrorBlockJob *s = op->s;
>>
>> aio_context_acquire(blk_get_aio_context(s->common.blk));
>> @@ -155,9 +158,8 @@ static void mirror_write_complete(void *opaque, int ret)
>> aio_context_release(blk_get_aio_context(s->common.blk));
>> }
>
> One of the effects of this conversion is that we hold the AioContext
> lock for s->common.blk recursively multiple times. I don't think
> anything calls drain in such sections (which would hang), so it's
> probably okay, but it could turn out to be a trap in the long run.
Sounds like something I'll be cleaning up in the planned followup.
https://git.xanclic.moe/XanClic/qemu/commit/0d2f888b9f96634b9665
>> -static void mirror_read_complete(void *opaque, int ret)
>> +static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
>> {
>> - MirrorOp *op = opaque;
>> MirrorBlockJob *s = op->s;
>>
>> aio_context_acquire(blk_get_aio_context(s->common.blk));
>> @@ -172,8 +174,9 @@ static void mirror_read_complete(void *opaque, int ret)
>>
>> mirror_iteration_done(op, ret);
>> } else {
>> - blk_aio_pwritev(s->target, op->offset, &op->qiov,
>> - 0, mirror_write_complete, op);
>> + ret = blk_co_pwritev(s->target, op->offset,
>> + op->qiov.size, &op->qiov, 0);
>> + mirror_write_complete(op, ret);
>> }
>> aio_context_release(blk_get_aio_context(s->common.blk));
>> }
>> @@ -230,60 +233,57 @@ static inline void mirror_wait_for_io(MirrorBlockJob
>> *s)
>> s->waiting_for_io = false;
>> }
>>
>> -/* Submit async read while handling COW.
>> - * Returns: The number of bytes copied after and including offset,
>> - * excluding any bytes copied prior to offset due to alignment.
>> - * This will be @bytes if no alignment is necessary, or
>> - * (new_end - offset) if tail is rounded up or down due to
>> - * alignment or buffer limit.
>> +/* Perform a mirror copy operation.
>> + *
>> + * *op->bytes_handled is set to the number of bytes copied after and
>> + * including offset, excluding any bytes copied prior to offset due
>> + * to alignment. This will be op->bytes if no alignment is necessary,
>> + * or (new_end - op->offset) if the tail is rounded up or down due to
>> + * alignment or buffer limit.
>> */
>> -static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,
>> - uint64_t bytes)
>> +static void coroutine_fn mirror_co_read(void *opaque)
>> {
>> + MirrorOp *op = opaque;
>> + MirrorBlockJob *s = op->s;
>> BlockBackend *source = s->common.blk;
>> int nb_chunks;
>> uint64_t ret;
>> - MirrorOp *op;
>> uint64_t max_bytes;
>>
>> max_bytes = s->granularity * s->max_iov;
>>
>> /* We can only handle as much as buf_size at a time. */
>> - bytes = MIN(s->buf_size, MIN(max_bytes, bytes));
>> - assert(bytes);
>> - assert(bytes < BDRV_REQUEST_MAX_BYTES);
>> - ret = bytes;
>> + op->bytes = MIN(s->buf_size, MIN(max_bytes, op->bytes));
>> + assert(op->bytes);
>> + assert(op->bytes < BDRV_REQUEST_MAX_BYTES);
>> + *op->bytes_handled = op->bytes;
>>
>> if (s->cow_bitmap) {
>> - ret += mirror_cow_align(s, &offset, &bytes);
>> + *op->bytes_handled += mirror_cow_align(s, &op->offset, &op->bytes);
>> }
>
> Actually, if we factor out this block, call it from mirror_perform() and
> only assert the conditions here, we could get completely rid of that
> peculiar op->bytes_handled.
That is definitely something I'll clean up in the followup:
https://git.xanclic.moe/XanClic/qemu/commit/6cbe927e08dd1ec26a4c
I'd rather leave it in this series and worry about it later. I
gradually phased out more and more nice-to-have things from this series
so it could get in at some point, and that apparently was one of those
things.
Max
signature.asc
Description: OpenPGP digital signature
