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