Re: [Qemu-devel] [PATCH v2 09/12] mirror: switch mirror_iteration to AIO

[Kevin Wolf <kwolf@redhat.com>]

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 <pbonzini@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