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

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

[zwu.kernel]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

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 <wuzhy@linux.vnet.ibm.com>
---
 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();
 
     /* If requests are still pending there is a bug somewhere */
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         assert(QLIST_EMPTY(&bs->tracked_requests));
+        if (throttled_bs && throttled_bs != bs) {
+            continue;
+        }
         assert(qemu_co_queue_empty(&bs->throttled_reqs));
     }
 }
 
+/*
+ * 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)
+{
+    bdrv_drain_request(NULL);
+}
+
 /* make a BlockDriverState anonymous by removing from bdrv_state list.
    Also, NULL terminate the device_name to prevent double remove */
 void bdrv_make_anon(BlockDriverState *bs)
diff --git a/block_int.h b/block_int.h
index 7946cf6..1311288 100644
--- a/block_int.h
+++ b/block_int.h
@@ -323,6 +323,7 @@ void qemu_aio_release(void *p);
 
 void bdrv_set_io_limits(BlockDriverState *bs,
                         BlockIOLimit *io_limits);
+void bdrv_drain_request(BlockDriverState *bs);
 
 #ifdef _WIN32
 int is_windows_drive(const char *filename);
-- 
1.7.6

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

[Kevin Wolf]

Am 20.02.2012 05:50, schrieb zwu.kernel@gmail.com:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> 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 <wuzhy@linux.vnet.ibm.com>
> ---
>  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.

Kevin

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:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 20.02.2012 05:50, schrieb zwu.kernel@gmail.com:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> 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 <wuzhy@linux.vnet.ibm.com>
>> ---
>>  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.

>
> Kevin



-- 
Regards,

Zhi Yong Wu

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

[Zhi Yong Wu]

For example, one disk of one guest is hot plugout, its other disks
should not be affected if those disks also enable I/O throttling. But
if one whole VM need to be stored, all throttled requests for all
disks need to be drained.

On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 20.02.2012 05:50, schrieb zwu.kernel@gmail.com:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> 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 <wuzhy@linux.vnet.ibm.com>
>> ---
>>  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.
>
> Kevin



-- 
Regards,

Zhi Yong Wu

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

[Kevin Wolf]

Am 20.02.2012 10:29, schrieb Zhi Yong Wu:
> On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 20.02.2012 05:50, schrieb zwu.kernel@gmail.com:
>>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>>
>>> 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 <wuzhy@linux.vnet.ibm.com>
>>> ---
>>>  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
optimisation be a qemu_aio_flush_disk() that flushes the requests for a
single disk?

Kevin

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 <kwolf@redhat.com> wrote:
> Am 20.02.2012 10:29, schrieb Zhi Yong Wu:
>> On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 20.02.2012 05:50, schrieb zwu.kernel@gmail.com:
>>>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>>>
>>>> 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 <wuzhy@linux.vnet.ibm.com>
>>>> ---
>>>>  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

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

[Stefan Hajnoczi]

On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> 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 <wuzhy@linux.vnet.ibm.com>
> ---
>  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();

Since I/O throttling is still enabled, the restarted requests could
enqueue again if they exceed the limit.  We could still hit the assert.

If the semantics of bdrv_drain_request() are that no requests are
pending when it returns then we need a loop here.

BTW bdrv_drain() would be a shorter name for this function.

Stefan

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

[Zhi Yong Wu]

On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> 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 <wuzhy@linux.vnet.ibm.com>
>> ---
>>  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();
>
> Since I/O throttling is still enabled, the restarted requests could
> enqueue again if they exceed the limit.  We could still hit the assert.
good catch.
>
> If the semantics of bdrv_drain_request() are that no requests are
> pending when it returns then we need a loop here.
OK.
>
> BTW bdrv_drain() would be a shorter name for this function.
OK
>
> Stefan



-- 
Regards,

Zhi Yong Wu

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

[Zhi Yong Wu]

On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> 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 <wuzhy@linux.vnet.ibm.com>
>> ---
>>  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();
>
> Since I/O throttling is still enabled, the restarted requests could
> enqueue again if they exceed the limit.  We could still hit the assert.
>
> If the semantics of bdrv_drain_request() are that no requests are
> pending when it returns then we need a loop here.
>
> BTW bdrv_drain() would be a shorter name for this function.
For this function's semantics, i have some concerns.
Is it used to drain all requests of one single disk or all disks for one guest?
which is more suitable?

>
> Stefan



-- 
Regards,

Zhi Yong Wu

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

[Stefan Hajnoczi]

On Fri, Feb 24, 2012 at 05:25:08PM +0800, Zhi Yong Wu wrote:
> On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.kernel@gmail.com wrote:
> >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> >>
> >> 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 <wuzhy@linux.vnet.ibm.com>
> >> ---
> >>  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();
> >
> > Since I/O throttling is still enabled, the restarted requests could
> > enqueue again if they exceed the limit.  We could still hit the assert.
> >
> > If the semantics of bdrv_drain_request() are that no requests are
> > pending when it returns then we need a loop here.
> >
> > BTW bdrv_drain() would be a shorter name for this function.
> For this function's semantics, i have some concerns.
> Is it used to drain all requests of one single disk or all disks for one guest?
> which is more suitable?

Both are useful:

/**
 * Complete all pending requests for a block device
 */
void bdrv_drain(BlockDriverState *bs);

/**
 * Complete pending requests for all block devices
 */
void bdrv_drain_all(void);

Stefan

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

[Paolo Bonzini]

On 02/24/2012 09:49 AM, Stefan Hajnoczi wrote:
>> > +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();
> Since I/O throttling is still enabled, the restarted requests could
> enqueue again if they exceed the limit.  We could still hit the assert.

Yes, and qemu_aio_flush() rightly doesn't know that there are pending
requests, because these are "not there" until the timer fires.

Perhaps the bug is simply that the assert is bogus.  If there are
throttled requests, we need to invoke qemu_aio_flush() again.  The
problem with this is that timers don't run inside qemu_aio_wait() so we
have to restart them all and we're effectively busy waiting.  Not a huge
problem since qemu_aio_flush() is rare, but not too nice either.

Paolo

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

[Zhi Yong Wu]

On Fri, Feb 24, 2012 at 7:18 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> On Fri, Feb 24, 2012 at 05:25:08PM +0800, Zhi Yong Wu wrote:
>> On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.kernel@gmail.com wrote:
>> >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> >>
>> >> 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 <wuzhy@linux.vnet.ibm.com>
>> >> ---
>> >>  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();
>> >
>> > Since I/O throttling is still enabled, the restarted requests could
>> > enqueue again if they exceed the limit.  We could still hit the assert.
>> >
>> > If the semantics of bdrv_drain_request() are that no requests are
>> > pending when it returns then we need a loop here.
>> >
>> > BTW bdrv_drain() would be a shorter name for this function.
>> For this function's semantics, i have some concerns.
>> Is it used to drain all requests of one single disk or all disks for one guest?
>> which is more suitable?
>
> Both are useful:
>
> /**
>  * Complete all pending requests for a block device
>  */
> void bdrv_drain(BlockDriverState *bs);
>
> /**
>  * Complete pending requests for all block devices
>  */
> void bdrv_drain_all(void);
Great, thanks.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu