From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35714) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YwCy7-00042b-Ur for qemu-devel@nongnu.org; Sat, 23 May 2015 13:12:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YwCy3-0002nF-7t for qemu-devel@nongnu.org; Sat, 23 May 2015 13:12:15 -0400 Message-ID: <5560B4DA.8000908@redhat.com> Date: Sat, 23 May 2015 19:11:54 +0200 From: Max Reitz MIME-Version: 1.0 References: <1432190583-10518-1-git-send-email-famz@redhat.com> <1432190583-10518-13-git-send-email-famz@redhat.com> In-Reply-To: <1432190583-10518-13-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 12/13] block: Block "device IO" during bdrv_drain and bdrv_drain_all List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, jcody@redhat.com, armbru@redhat.com, Stefan Hajnoczi , amit.shah@redhat.com, pbonzini@redhat.com 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 > --- > 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); > } > > /**