Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue

[Zhi Yong Wu]

On Mon, Feb 20, 2012 at 5:39 PM, Kevin Wolf <address@hidden> wrote:
> Am 20.02.2012 10:29, schrieb Zhi Yong Wu:
>> On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf <address@hidden> wrote:
>>> Am 20.02.2012 05:50, schrieb address@hidden:
>>>> From: Zhi Yong Wu <address@hidden>
>>>>
>>>> If one guest has multiple disks with enabling I/O throttling function 
>>>> separately, when draining activities are done, some requests maybe are in 
>>>> the throttled queue; So we need to restart them at first.
>>>>
>>>> Moreover, when only one disk need to be drained such as hotplug out, if 
>>>> another disk still has some requests in its throttled queue, these request 
>>>> should not be effected.
>>>>
>>>> Signed-off-by: Zhi Yong Wu <address@hidden>
>>>> ---
>>>>  block.c     |   29 ++++++++++++++++++++++-------
>>>>  block_int.h |    1 +
>>>>  2 files changed, 23 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index ae297bb..f78df78 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -853,25 +853,40 @@ void bdrv_close_all(void)
>>>>      }
>>>>  }
>>>>
>>>> -/*
>>>> - * Wait for pending requests to complete across all BlockDriverStates
>>>> - *
>>>> - * This function does not flush data to disk, use bdrv_flush_all() for 
>>>> that
>>>> - * after calling this function.
>>>> - */
>>>> -void bdrv_drain_all(void)
>>>> +void bdrv_drain_request(BlockDriverState *throttled_bs)
>>>>  {
>>>>      BlockDriverState *bs;
>>>>
>>>> +    QTAILQ_FOREACH(bs, &bdrv_states, list) {
>>>> +        if (throttled_bs && throttled_bs != bs) {
>>>> +            continue;
>>>> +        }
>>>> +        qemu_co_queue_restart_all(&bs->throttled_reqs);
>>>> +    }
>>>> +
>>>>      qemu_aio_flush();
>>>
>>> Why doesn't qemu_aio_flush() invoke whatever is needed? I think this is
>>> the real bug that should be fixed.
>> Do you mean that why qemu_aio_flush doesn't invoke those lines of
>> codes above it? As what i said in above comments, when one VM has 2
>> multiple disks with enabling I/O throttling, if only one disk need to
>> be flushed, only this disk's throttled request need to be drained, and
>> another disk's throttled requests don't need.
>
> So this is an optimisation rather than a fix? Would the right
It is a fix. i think that current handling is wrong when I/O
throttling is enabled
> optimisation be a qemu_aio_flush_disk() that flushes the requests for a
> single disk?
No. If bdrv_drain_request is invoked with specified bs, it will flush
All the throttled requests in the throttled queue for this specific bs
 +  pending emulated aio for all disks (in aio engine)
But if bdrv_drain_request(NULL) is invoked, it will flush
All the throttled request in the throttled queue for all disks  +
pending emulated aio for all disks (in aio engine)

>
> Kevin



-- 
Regards,

Zhi Yong Wu