From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8UBX-0003Hc-39 for qemu-devel@nongnu.org; Fri, 26 Jun 2015 10:00:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8UBS-0002JB-G5 for qemu-devel@nongnu.org; Fri, 26 Jun 2015 10:00:51 -0400 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:41161) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8UBR-0002Hk-Od for qemu-devel@nongnu.org; Fri, 26 Jun 2015 10:00:46 -0400 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 26 Jun 2015 15:00:41 +0100 From: Alexander Yarygin References: <1434537440-28236-1-git-send-email-yarygin@linux.vnet.ibm.com> <1434537440-28236-3-git-send-email-yarygin@linux.vnet.ibm.com> <87mvzm2536.fsf@blackfin.pond.sub.org> Date: Fri, 26 Jun 2015 17:00:31 +0300 In-Reply-To: <87mvzm2536.fsf@blackfin.pond.sub.org> (Markus Armbruster's message of "Fri, 26 Jun 2015 11:26:21 +0200") Message-ID: <87egky4lj4.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v3 2/2] virtio-blk: Use blk_drain() to drain IO requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org, "Michael S. Tsirkin" , Ekaterina Tumanova , qemu-devel@nongnu.org, Christian Borntraeger , Stefan Hajnoczi , Cornelia Huck , Paolo Bonzini Markus Armbruster writes: > Just spotted this in my git-pull... > > Alexander Yarygin writes: > >> Each call of the virtio_blk_reset() function calls blk_drain_all(), >> which works for all existing BlockDriverStates, while draining only >> one is needed. >> >> This patch replaces blk_drain_all() by blk_drain() in >> virtio_blk_reset(). virtio_blk_data_plane_stop() should be called >> after draining because it restores vblk->complete_request. >> >> Cc: "Michael S. Tsirkin" >> Cc: Christian Borntraeger >> Cc: Cornelia Huck >> Cc: Kevin Wolf >> Cc: Paolo Bonzini >> Cc: Stefan Hajnoczi >> Signed-off-by: Alexander Yarygin >> --- >> hw/block/virtio-blk.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index e6afe97..d8a906f 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -651,16 +651,21 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running, >> static void virtio_blk_reset(VirtIODevice *vdev) >> { >> VirtIOBlock *s = VIRTIO_BLK(vdev); >> - >> - if (s->dataplane) { >> - virtio_blk_data_plane_stop(s->dataplane); >> - } >> + AioContext *ctx; >> >> /* >> * This should cancel pending requests, but can't do nicely until there >> * are per-device request lists. >> */ >> - blk_drain_all(); >> + ctx = blk_get_aio_context(s->blk); >> + aio_context_acquire(ctx); >> + blk_drain(s->blk); >> + >> + if (s->dataplane) { >> + virtio_blk_data_plane_stop(s->dataplane); >> + } >> + aio_context_release(ctx); >> + >> blk_set_enable_write_cache(s->blk, s->original_wce); >> } > > From bdrv_drain_all()'s comment: > > * 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. > > From bdrv_drain()'s comment: > > * See the warning in bdrv_drain_all(). This function can only be called if > * you are sure nothing can generate I/O because you have op blockers > * installed. > > blk_drain() and blk_drain_all() are trivial wrappers. > > Ignorant questions: > > * Why does blk_drain() suffice here? > > * Is blk_drain() (created in PATCH 1) even a safe interface? * We want to drain requests from only one bdrv and blk_drain() can do that. * Ignorant answer: I was told that the bdrv_drain_all()'s comment is obsolete and we can use bdrv_drain(). Here is a link to the old thread: http://marc.info/?l=qemu-devel&m=143154211017926&w=2. Since I don't see the full picture of this area yet, I'm just relying on other people's opinion.