From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43405) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxez0-0006cY-LR for qemu-devel@nongnu.org; Fri, 21 Oct 2016 14:55:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxeyz-0004zx-UK for qemu-devel@nongnu.org; Fri, 21 Oct 2016 14:55:58 -0400 References: <99a0ff7379a345ed60a6092866c9a42f667b37b5.1476450059.git.berto@igalia.com> <20161019171120.GH5336@noname.redhat.com> From: John Snow Message-ID: Date: Fri, 21 Oct 2016 14:55:51 -0400 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Alberto Garcia , Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Eric Blake , Max Reitz , Markus Armbruster , pbonzini@redhat.com On 10/20/2016 04:25 AM, Alberto Garcia wrote: > On Wed 19 Oct 2016 07:11:20 PM CEST, Kevin Wolf wrote: > >>> bdrv_drain_all() doesn't allow the caller to do anything after all >>> pending requests have been completed but before block jobs are >>> resumed. >>> >>> This patch splits bdrv_drain_all() into _begin() and _end() for that >>> purpose. It also adds aio_{disable,enable}_external() calls to >>> disable external clients in the meantime. >>> >>> Signed-off-by: Alberto Garcia >> >> This looks okay as a first step, possibly enough for this series >> (we'll have to review this carefully), but it leaves us with a rather >> limited version of bdrv_drain_all_begin/end that excludes many useful >> cases. One of them is that John wants to use it around QMP >> transactions. >> >> Specifically, you can't add a new BDS or a new block job in a >> drain_all section because then bdrv_drain_all_end() would try to >> unpause the new thing even though it has never been paused. Depending >> on what else we did with it, this will either corrupt the pause >> counters or just directly fail an assertion. > > The problem is: do you want to be able to create a new block job and let > it run? Because then you can end up having the same problem that this > patch is trying to prevent if the new job attempts to reopen a > BlockDriverState. > > Berto > The plan was to create jobs in a pre-started mode and only to engage them after the drained section. Do any jobs re-open a BDS prior to the creation of their coroutine? --js