From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1S8o-0008Lh-RS for qemu-devel@nongnu.org; Tue, 24 Nov 2015 23:57:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1S8k-0005Je-P4 for qemu-devel@nongnu.org; Tue, 24 Nov 2015 23:57:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35041) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1S8k-0005JZ-IA for qemu-devel@nongnu.org; Tue, 24 Nov 2015 23:57:10 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 590D932D3DD for ; Wed, 25 Nov 2015 04:57:09 +0000 (UTC) Date: Wed, 25 Nov 2015 12:57:04 +0800 From: Fam Zheng Message-ID: <20151125045704.GD17946@ad.usersys.redhat.com> References: <1448273262-13845-1-git-send-email-peterx@redhat.com> <56533D45.1060108@redhat.com> <20151123175759.GG3606@hawk.localdomain> <5653C422.3040307@redhat.com> <20151124031027.GC26733@ad.usersys.redhat.com> <56544694.3060309@redhat.com> <20151124113723.GA29832@ad.usersys.redhat.com> <56546ADC.5050402@redhat.com> <20151125024631.GA17946@ad.usersys.redhat.com> <56553DBA.1070504@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56553DBA.1070504@redhat.com> Subject: Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: Andrew Jones , qemu-devel@nongnu.org, armbru@redhat.com, lcapitulino@redhat.com, pbonzini@redhat.com, Laszlo Ersek On Wed, 11/25 12:48, Peter Xu wrote: > > > On 11/25/2015 10:46 AM, Fam Zheng wrote: > > On Tue, 11/24 06:49, Eric Blake wrote: > >> On 11/24/2015 04:37 AM, Fam Zheng wrote: > >> > >>>> I think the patch should be dropped, and periodic progress reports > >>>> should be emitted from within the dump loops that do the heavy lifting. > >>>> > >>>> For the ELF format dumps, that loop appears to reside in dump_iterate() > >>>> [dump.c]. > >>>> > >>>> For the compressed format dumps, the loop seems to live in > >>>> write_dump_pages() [dump.c]. > >>> > >>> This is a good idea! > >>> > >>> What I'm not sure is where to report the progress. Can it be the monitor where > >>> the dump-guest-memory command was issued? In other words, do we support raising > >>> events before the previous command returns? If yes, can libvirt handle this > >>> correctly? (But the worst case is using another channel to communicate the > >>> progress, it is ad-hocery but it must be better than all the risk and effort to > >>> enable multi-threaded dump.) > >>> > >>> Eric, Markus, have any idea with the progress reporting? > >> > >> I'm fairly certain we support raising events prior to completion of a > >> synchronous command; what I'm not sure of is whether the event hits the > >> wire right away or whether it piles up waiting for the next synchronous > >> command completion. If the latter, then we need to rework it (since the > >> whole point of this exercise is that we are trying to give progress of a > >> long-running synchronous command that hasn't completed yet). > > > > So in that case we may want some "flush" operation of events. That sounds OK to > > me. > > > >> But we > >> only have the one monitor connection for libvirt - the only way to pass > >> events through a second channel is to open a second monitor connection, > >> but that feels wrong to make libvirt have to track two monitors. > > > > OK, that's a fair point, but FWIW I was thinking about adding an optional > > argument: > > > > "*progress": "fd:dump-progress" > > > > into which dump.c talks in a mini-protocol, to send progress information. It's > > just an crazily hacky idea, not anything I'm advocating. > > If query status is necessary, what about adding one command: > "query-dump"? Which could be a simplified version of "query-migration": > > 1. before first dump: > > -> { "execute": "query-dump" } > <- { "return": {} } > > 2. one background dump in progress: > > -> { "execute": "query-dump" } > <- { > "return":{ > "status":"active", > "percentage": {0..99}, > } > } > > 3. after first dump, and not running background dump (substraction > of case 1 and 2) > > -> { "execute": "query-dump" } > <- { > "return": { > "status": "completed|failed", > } > } > > All these would be based on the fact that this patch might not be > dropped though. :) This is okay, if you're going to use threaded dump. And it needs an QAPI event like other async operations. See migrate_generate_event. Fam