[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 05/16] block/mirror: Convert to coroutines
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 05/16] block/mirror: Convert to coroutines |
Date: |
Wed, 28 Feb 2018 15:13:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 2018-02-27 08:44, Fam Zheng wrote:
> On Mon, 01/22 23:07, Max Reitz wrote:
>> @@ -101,7 +105,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;
>
> I think we want s/qemu_coroutine_enter/aio_co_wake/ in
> mirror_iteration_done().
> As an AIO callback before, this didn't matter, but now we are in an
> terminating
> coroutine, so it is pointless to defer the termination, or even risky in that
> we
> are in a aio_context_acquire/release section, but have already decremented
> s->in_flight, which is fishy.
I guess I'll still do the replacement, regardless of whether the next
patch overwrites it again...
>> @@ -138,9 +142,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));
>> @@ -157,9 +160,8 @@ static void mirror_write_complete(void *opaque, int ret)
>> aio_context_release(blk_get_aio_context(s->common.blk));
>> }
>>
>> -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));
>> @@ -174,8 +176,11 @@ 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);
>> + int ret;
>
> s/ret/ret2/ or drop the definition?
> because ret is already the paramter of the function.
Oh, right, yes, will do.
>> +
>> + 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));
>> }
>
> <snip>
>
>> +static void coroutine_fn mirror_co_discard(void *opaque)
>> +{
>> + MirrorOp *op = opaque;
>> + int ret;
>> +
>> + op->s->in_flight++;
>> + op->s->bytes_in_flight += op->bytes;
>> + *op->bytes_handled = op->bytes;
>> +
>> + ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes);
>> + mirror_write_complete(op, ret);
>> }
>>
>> static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
>> unsigned bytes, MirrorMethod mirror_method)
>
> Doesn't mirror_perform need coroutine_fn annotation too?
I don't think it needs one. We could give it one, but as far as I've
understood (which may be wrong), all functions that need to be run from
a coroutine need the tag -- but functions that may be called from either
coroutines or just normal code don't need it.
(And I think this function should be fine either way, so I don't think
it needs a tag.)
Also, thanks for reviewing! :-)
Max
signature.asc
Description: OpenPGP digital signature