[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 05/16] block/mirror: Convert to coroutines
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v2 05/16] block/mirror: Convert to coroutines |
Date: |
Wed, 28 Feb 2018 18:07:03 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 2018-02-28 15:13, Max Reitz wrote:
> 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...
Maybe I don't. Doing this breaks iotest 041 because the
assert(data.done) in bdrv_co_yield_to_drain() fails.
Not sure why that is, but under the circumstance I guess it's best to
just pretend this never happened, continue to use qemu_coroutine_enter()
and just replace it in the next patch.
As for in_flight: What is the issue there? We mostly need that to know
how many I/O requests are actually running, that is, how much buffer
space is used, how many I/O is done concurrently, etc. (and later we
need the in-flight information so that we don't access the target in
overlapping areas concurrently). But it doesn't seem to be about how
many coroutines there are.
So as long as the s->in_flight decrement is done in the same critical
section as the op is deleted, we should be good...?
Max
signature.asc
Description: OpenPGP digital signature