Re: [Qemu-block] [PATCH v6 12/13] block: Block "device IO" during bdrv_drain and bdrv_drain_all

[Max Reitz]

On 21.05.2015 08:43, Fam Zheng wrote:
> We don't want new requests from guest, so block the operation around the
> nested poll.
>
> It also avoids looping forever when iothread is submitting a lot of requests.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>   block/io.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)

Hm, I don't know about this. When I see someone calling 
bdrv_drain()/bdrv_drain_all(), I'm expecting that every request has been 
drained afterwards. This patch implies that this is not necessarily the 
case, because apparently in some configurations the guest can still 
submit I/O even while bdrv_drain() is running, but this means that even 
after this patch, the same can happen if I/O is submitted after 
bdrv_op_unblock() and before anything the caller of bdrv_drain() wants 
to do while the BDS is still drained. So this looks to me more like the 
caller must ensure that the BDS won't receive new requests, and do so 
before bdrv_drain() is called.

Maybe it works anyway because I'm once again just confused by qemu's 
threading model, and the problem here is only that bdrv_drain_one() may 
yield which may result in new I/O requests being submitted. If apart 
from yielding no new I/O can be submitted, I guess this patch is good.

Max

> diff --git a/block/io.c b/block/io.c
> index 1ce62c4..b23a83f 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -286,12 +286,21 @@ static bool bdrv_drain_one(BlockDriverState *bs)
>    *
>    * Note that unlike bdrv_drain_all(), the caller must hold the 
> BlockDriverState
>    * AioContext.
> + *
> + * Devices are paused to avoid looping forever because otherwise they could
> + * keep submitting more requests.
>    */
>   void bdrv_drain(BlockDriverState *bs)
>   {
> +    Error *blocker = NULL;
> +
> +    error_setg(&blocker, "bdrv_drain in progress");
> +    bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
>       while (bdrv_drain_one(bs)) {
>           /* Keep iterating */
>       }
> +    bdrv_op_unblock(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
> +    error_free(blocker);
>   }
>   
>   /*
> @@ -303,14 +312,20 @@ void bdrv_drain(BlockDriverState *bs)
>    * Note that completion of an asynchronous I/O operation can trigger any
>    * number of other I/O operations on other devices---for example a coroutine
>    * can be arbitrarily complex and a constant flow of I/O can come until the
> - * coroutine is complete.  Because of this, it is not possible to have a
> - * function to drain a single device's I/O queue.
> + * coroutine is complete.  Because of this, we must call bdrv_drain_one in a
> + * loop.
> + *
> + * We explicitly pause block jobs and devices to prevent them from submitting
> + * more requests.
>    */
>   void bdrv_drain_all(void)
>   {
>       /* Always run first iteration so any pending completion BHs run */
>       bool busy = true;
>       BlockDriverState *bs = NULL;
> +    Error *blocker = NULL;
> +
> +    error_setg(&blocker, "bdrv_drain_all in progress");
>   
>       while ((bs = bdrv_next(bs))) {
>           AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -319,6 +334,7 @@ void bdrv_drain_all(void)
>           if (bs->job) {
>               block_job_pause(bs->job);
>           }
> +        bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
>           aio_context_release(aio_context);
>       }
>   
> @@ -343,8 +359,10 @@ void bdrv_drain_all(void)
>           if (bs->job) {
>               block_job_resume(bs->job);
>           }
> +        bdrv_op_unblock(bs, BLOCK_OP_TYPE_DEVICE_IO, blocker);
>           aio_context_release(aio_context);
>       }
> +    error_free(blocker);
>   }
>   
>   /**