Re: [Qemu-devel] [PATCH v2 05/16] block/mirror: Convert to coroutines

[Max Reitz]

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