Re: [Qemu-devel] fixing qemu-0.1X endless loop in qcow2_alloc_cluster_offset

[Kevin Wolf <kwolf@redhat.com>]

Am 12.06.2012 15:33, schrieb Andreas Färber:
> Am 14.05.2012 14:20, schrieb Kevin Wolf:
>> Am 13.05.2012 10:03, schrieb Zhouyi Zhou:
>>> hi all
>>>   
>>>   sometimes, qemu/kvm-0.1x will hang in endless loop in qcow2_alloc_cluster_offset.
>>>   after some investigation, I found that:
>>>   in function posix_aio_process_queue(void *opaque)
>>> 440             ret = qemu_paio_error(acb);
>>> 441             if (ret == ECANCELED) {
>>> 442                 /* remove the request */
>>> 443                 *pacb = acb->next;
>>> 444                 qemu_aio_release(acb);
>>> 445                 result = 1;
>>> 446             } else if (ret != EINPROGRESS) {
>>>   in line 444 acb got released but acb->common.opaque does not.
>>> which will be released via guest OS via ide_dma_cancel which 
>>> will in term call qcow_aio_cancel which does not check its argument
>>> is in flight list or not.
>>>   The fix is as follows: (debian 6's qemu-kvm-0.12.5)
>>> #######################################
>>> --- block/qcow2.h~      2010-07-27 08:43:53.000000000 +0800
>>> +++ block/qcow2.h       2012-05-13 15:51:39.000000000 +0800
>>> @@ -143,6 +143,7 @@
>>>      QLIST_HEAD(QCowAioDependencies, QCowAIOCB) dependent_requests;
>>>  
>>>      QLIST_ENTRY(QCowL2Meta) next_in_flight;
>>> +    int inflight;       
>>>  } QCowL2Meta;
>>> --- block/qcow2.c~  2012-05-13 15:57:09.000000000 +0800
>>> +++ block/qcow2.c       2012-05-13 15:57:24.000000000 +0800
>>> @@ -349,6 +349,10 @@
>>>      QCowAIOCB *acb = (QCowAIOCB *)blockacb;
>>>      if (acb->hd_aiocb)
>>>          bdrv_aio_cancel(acb->hd_aiocb);
>>> +    if (acb->l2meta.inflight) {
>>> +        QLIST_REMOVE(&acb->l2meta, next_in_flight);
>>> +       acb->l2meta.inflight = 0;
>>> +    }
>>>      qemu_aio_release(acb);
>>>  }
>>>  
>>> @@ -506,6 +510,7 @@
>>>      acb->n = 0;
>>>      acb->cluster_offset = 0;
>>>      acb->l2meta.nb_clusters = 0;
>>> +    acb->l2meta.inflight = 0;
>>>      QLIST_INIT(&acb->l2meta.dependent_requests);
>>>      return acb;
>>>  }
>>> @@ -534,6 +539,7 @@
>>>      /* Take the request off the list of running requests */
>>>      if (m->nb_clusters != 0) {
>>>          QLIST_REMOVE(m, next_in_flight);
>>> +       m->inflight = 0;
>>>      }
>>>  
>>>      /*
>>> @@ -632,6 +638,7 @@
>>>  fail:
>>>      if (acb->l2meta.nb_clusters != 0) {
>>>          QLIST_REMOVE(&acb->l2meta, next_in_flight);
>>> +       acb->l2meta.inflight  = 0;
>>>      }
>>>  done:
>>>      if (acb->qiov->niov > 1)
>>> --- block/qcow2-cluster.c~      2010-07-27 08:43:53.000000000 +0800
>>> +++ block/qcow2-cluster.c       2012-05-13 15:53:53.000000000 +0800
>>> @@ -827,6 +827,7 @@
>>>      m->offset = offset;
>>>      m->n_start = n_start;
>>>      m->nb_clusters = nb_clusters;
>>> +    m->inflight = 1;
>>>  
>>>  out:
>>>      m->nb_available = MIN(nb_clusters << (s->cluster_bits - 9), n_end);
>>>
>>>  Thanks for investigation
>>> Zhouyi
>>
>> The patch looks reasonable to me. Note however that while it fixes the
>> hang, it still causes cluster leaks. I'm not sure if someone is
>> interested in picking these up for old stable releases. Andreas, I think
>> you were going to take 0.15? The first version that doesn't have the
>> problem is 1.0.
> 
> Kevin, the policy as I understood it is to cherry-pick patches from
> qemu.git into qemu-stable-x.y.git. So I don't think me applying this
> patch to stable-0.15 would be right. I don't spot a particular qcow2 fix
> among our 0.15 backports that I have now pushed. Do you have a pointer
> which one(s) would fix this issue so that I can recheck?

It's "fixed" as a side effect of the block layer conversion to
coroutines. Not exactly the kind of patches you'd want to cherry-pick
for stable-0.15.

The better fix for 0.15 could be to backport the new behaviour of
coroutine based requests with bdrv_aio_cancel:

static void bdrv_aio_co_cancel_em(BlockDriverAIOCB *blockacb)
{
    qemu_aio_flush();
}

Using that as the implementation for qcow2_aio_cancel should be safe and
fix this problem.

Kevin