Re: [Qemu-devel] [PATCH] qed: fix bdrv_qed_drain

[Paolo Bonzini <pbonzini@redhat.com>]

On 17/02/2016 03:57, Fam Zheng wrote:
> On Tue, 02/16 16:53, Paolo Bonzini wrote:
>> The current implementation of bdrv_qed_drain can cause a double
>> completion of a request.
>>
>> The problem is that bdrv_qed_drain calls qed_plug_allocating_write_reqs
>> unconditionally, but this is not correct if an allocating write
>> is queued.  In this case, qed_unplug_allocating_write_reqs will
>> restart the allocating write and possibly cause it to complete.
>> The aiocb however is still in use for the L2/L1 table writes,
>> and will then be completed again as soon as the table writes
>> are stable.
>>
>> The fix is to only call qed_plug_allocating_write_reqs and
>> bdrv_aio_flush (which is the same as the timer callback---the patch
>> makes this explicit) only if the timer was scheduled in the first
>> place.  This fixes qemu-iotests test 011.
>>
>> Cc: qemu-stable@nongnu.org
>> Cc: qemu-block@nongnu.org
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/qed.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/qed.c b/block/qed.c
>> index 404be1e..ebba220 100644
>> --- a/block/qed.c
>> +++ b/block/qed.c
>> @@ -380,12 +380,13 @@ static void bdrv_qed_drain(BlockDriverState *bs)
>>  {
>>      BDRVQEDState *s = bs->opaque;
>>  
>> -    /* Cancel timer and start doing I/O that were meant to happen as if it
>> -     * fired, that way we get bdrv_drain() taking care of the ongoing requests
>> -     * correctly. */
>> -    qed_cancel_need_check_timer(s);
>> -    qed_plug_allocating_write_reqs(s);
>> -    bdrv_aio_flush(s->bs, qed_clear_need_check, s);
>> +    /* Fire the timer immediately in order to start doing I/O as soon as the
>> +     * header is flushed.
>> +     */
>> +    if (s->need_check_timer && timer_pending(s->need_check_timer)) {
> 
> We can assert(s->need_check_timer);

I've seen it NULL, but didn't check why.  This was also a source of
segmentation faults.

>> +        qed_cancel_need_check_timer(s);
>> +        qed_need_check_timer_cb(s);
>> +    }
> 
> What if an allocating write is queued (the else branch case)? Its completion
> will be in bdrv_drain and it could arm the need_check_timer which is wrong.
> 
> We need to drain the allocating_write_reqs queue before checking the timer.

You're right, but how?  That's what bdrv_drain(bs) does, it's a
chicken-and-egg problem.

In my new series, draining works separately on each BlockDriverState
along the chain, from parent (bs) to children (bs->file->bs).  We could
then have .before_drain and .after_drain; .before_drain disables the
timer, while .after_drain does the same operation as the timer.  But
that couldn't be backported, and 2.5 would remain broken forever.  This
patch at least is a band-aid to make QED functional.

Perhaps the alternative is to remove write support for QED altogether
(and the drain callback with it, since it's the only user).  Not sure
why anyone would use it these days.

Paolo