Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Fam Zheng]

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