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 <address@hidden> wrote:
> > On Mon, Feb 20, 2012 at 12:50:30PM +0800, address@hidden wrote:
> >> 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();
> >
> > 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