Am 21.01.2013 13:09, schrieb Paolo Bonzini:
> Il 21/01/2013 12:39, Kevin Wolf ha scritto:
>> Am 16.01.2013 18:31, schrieb Paolo Bonzini:
>>> There is really no change in the behavior of the job here, since
>>> there is still a maximum of one in-flight I/O operation between
>>> the source and the target. However, this patch already introduces
>>> the AIO callbacks (which are unmodified in the next patch)
>>> and some of the logic to count in-flight operations and only
>>> complete the job when there is none.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
>>> ---
>>> block/mirror.c | 155
>>> ++++++++++++++++++++++++++++++++++++++++++--------------
>>> trace-events | 2 +
>>> 2 files changed, 119 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index ab41340..75c550a 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -33,8 +33,19 @@ typedef struct MirrorBlockJob {
>>> unsigned long *cow_bitmap;
>>> HBitmapIter hbi;
>>> uint8_t *buf;
>>> +
>>> + int in_flight;
>>> + int ret;
>>> } MirrorBlockJob;
>>>
>>> +typedef struct MirrorOp {
>>> + MirrorBlockJob *s;
>>> + QEMUIOVector qiov;
>>> + struct iovec iov;
>>> + int64_t sector_num;
>>> + int nb_sectors;
>>> +} MirrorOp;
>>> +
>>> static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
>>> int error)
>>> {
>>> @@ -48,15 +59,60 @@ static BlockErrorAction
>>> mirror_error_action(MirrorBlockJob *s, bool read,
>>> }
>>> }
>>>
>>> -static int coroutine_fn mirror_iteration(MirrorBlockJob *s,
>>> - BlockErrorAction *p_action)
>>> +static void mirror_iteration_done(MirrorOp *op)
>>> +{
>>> + MirrorBlockJob *s = op->s;
>>> +
>>> + s->in_flight--;
>>> + trace_mirror_iteration_done(s, op->sector_num, op->nb_sectors);
>>> + g_slice_free(MirrorOp, op);
>>> + qemu_coroutine_enter(s->common.co, NULL);
>>
>> This doesn't check if the job coroutine is actually in a state where
>> it's valid to reenter.
>>
>> Technically it might even be okay because reentering during a sleep is
>> allowed and as good as reentering during the new yield, and bdrv_flush()
>> is only called if s->in_flight == 0. Most other calls _should_ be okay
>> as well, but I'm not so sure about bdrv_drain_all(), especially once
>> .bdrv_drain exists.
>
> bdrv_drain_all is also called only if s->in_flight == 0 too, but I see
> your point. It is indeed quite subtle, but it's okay.
Ah, yes, that's the part I missed. Looks correct indeed.
>> As you can see, this is becoming very subtle, so I would prefer adding
>> some explicit bool s->may_reenter or something like that.
>
> The right boolean to test is already there, it's job->busy. I can add a
> new API block_job_yield/block_job_enter (where block_job_yield
> resets/sets busy across the yield, and block_job_enter only enters if
> !job->busy), but that would be a separate series IMO.
Please put it on your todo list then. I think I can accept the current
state if I know that it will be improved soon, even though I'm not very
comfortable with it.
Kevin
