From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35811) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz3GG-0002aU-Vt for qemu-devel@nongnu.org; Tue, 25 Oct 2016 11:03:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz3GC-0001oZ-Aa for qemu-devel@nongnu.org; Tue, 25 Oct 2016 11:03:33 -0400 Date: Tue, 25 Oct 2016 17:03:19 +0200 From: Kevin Wolf Message-ID: <20161025150319.GM4695@noname.str.redhat.com> References: <99a0ff7379a345ed60a6092866c9a42f667b37b5.1476450059.git.berto@igalia.com> <20161019171120.GH5336@noname.redhat.com> <4aa7bd58-f80a-946b-f94d-65065943a8dd@redhat.com> <20161025143827.GK4695@noname.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}() 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, Eric Blake , Max Reitz , Markus Armbruster , jsnow@redhat.com Am 25.10.2016 um 16:41 hat Paolo Bonzini geschrieben: > > > On 25/10/2016 16:38, Kevin Wolf wrote: > > Am 25.10.2016 um 15:53 hat Paolo Bonzini geschrieben: > >> > >> > >> On 25/10/2016 15:39, Alberto Garcia wrote: > >>> On Mon 24 Oct 2016 12:53:41 PM CEST, Paolo Bonzini wrote: > >>> > >>>>> My first thoughts were about how to let an unpause succeed without a > >>>>> previous pause for these objects, but actually I think this isn't > >>>>> what we should do. We rather want to actually do the pause instead > >>>>> because even new BDSes and block jobs should probably start in a > >>>>> quiesced state when created inside a drain_all section. > >>>> > >>>> Yes, I agree with this. It shouldn't be too hard to implement it. It > >>>> would require a BlockDriverState to look at the global "inside > >>>> bdrv_drain_all_begin" state, and ask its BlockBackend to disable > >>>> itself upon bdrv_replace_child. > >>> > >>> Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but > >>> if you add one with "blockdev-add" it's not going to be disabled using > >>> this method. > >> > >> You only need to disable it when blk_insert_bs is called. In fact... > > > > This assumes that the block driver doesn't issue internal background I/O > > by itself. Probably true for everything that we have today, but it would > > probably be cleaner to quiesce it directly in bdrv_open_common(). > > So > > bs->quiesce_counter = all_quiesce_counter; > > ? That would work. Yes, that's what I had in mind. > Should bdrv_close assert bs->quiesce_counter==0 > (which implies all_quiesce_counter == 0), since it usually has to do I/O? Hm... Not sure about that. We're still using bdrv_drain_all_begin/end as a function to isolate the BDSes, so some I/O from the caller of drain_all is expected, and that could involve deleting a BDS. But once we fully implemented what you proposed, probably yes. Kevin