From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btuu6-0008WJ-4h for qemu-devel@nongnu.org; Tue, 11 Oct 2016 07:07:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1btuu4-0001GR-05 for qemu-devel@nongnu.org; Tue, 11 Oct 2016 07:07:24 -0400 Date: Tue, 11 Oct 2016 13:07:14 +0200 From: Kevin Wolf Message-ID: <20161011110714.GD6334@noname.redhat.com> References: <950ab25638c15b988aa336474ab21f6020028828.1475757437.git.berto@igalia.com> <20161010153756.GI6775@noname.redhat.com> <0c822c44-052e-768b-0257-ed1f6b9a43ea@redhat.com> <20161011093949.GB6334@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v10 01/16] block: Pause all jobs during bdrv_reopen_multiple() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Alberto Garcia , qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Markus Armbruster , Stefan Hajnoczi Am 11.10.2016 um 11:54 hat Paolo Bonzini geschrieben: > On 11/10/2016 11:39, Kevin Wolf wrote: > > Am 10.10.2016 um 18:41 hat Paolo Bonzini geschrieben: > >> On 10/10/2016 17:37, Kevin Wolf wrote: > >>>> + while ((job = block_job_next(job))) { > >>>> + AioContext *aio_context = blk_get_aio_context(job->blk); > >>>> + > >>>> + aio_context_acquire(aio_context); > >>>> + block_job_pause(job); > >>>> + aio_context_release(aio_context); > >>>> + } > >>>> + > >>>> bdrv_drain_all(); > >>> > >>> We already have a bdrv_drain_all() here, which does the same thing (and > >>> more) internally, except that it resumes all jobs before it returns. > >>> Maybe what we should do is split bdrv_drain_all() in a begin/end pair, > >>> too. > >> > >> Hey, haven't I just suggested the same? :) I swear I hadn't read this > >> before. > >> > >>> The other point I'm wondering now is whether bdrv_drain_all() should > >>> have the aio_disable/enable_external() pair that bdrv_drain() has. > >> > >> bdrv_drain_all need not have it, but its start/end replacement should. > > > > Doesn't need it because it holds the AioContext lock? > > No, because as soon as bdrv_drain_all exits, external file descriptors > can be triggered again so I don't think the aio_disable/enable_external > actually provides any protection. Right, what I was thinking of was more like new requests coming in while bdrv_drain_all() is still running. If that happened, the result after bdrv_drain_all() wouldn't be different, but if external file descriptors are still active and the guest keeps sending requests, it could take forever until it returns. But that part is addressed by the AioContext lock, I think. > bdrv_drain_all should really only be used in cases where you already > have some kind of "hold" on external file descriptors, like bdrv_close > uses bdrv_drain(): > > bdrv_drained_begin(bs); /* complete I/O */ > bdrv_flush(bs); > bdrv_drain(bs); /* in case flush left pending I/O */ > > That said, because the simplest implementation of bdrv_drain_all() does > > bdrv_drained_all_start(); > bdrv_drained_all_end(); > > just like bdrv_drain() does it, it's not a very interesting point. > bdrv_drain_all will probably disable/enable external file descriptors, > it just doesn't need that. Yes, of course. Kevin