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

[Fam Zheng]

On Wed, 02/28 18:07, Max Reitz wrote:
> 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...?

I don't have a specific problem in my mind but is just generally concerned about
the "if (s->in_flight == 0)" checks around mirror_exit.

Fam