Re: [libvirt] RFC New virDomainBlockPull API family to libvirt

On Fri, Jul 15, 2011 at 12:05 PM, Kevin Wolf <kwolf(a)redhat.com&gt; wrote: > Am 15.07.2011 12:50, schrieb Stefan Hajnoczi: >> On Thu, Jul 14, 2011 at 7:54 PM, Adam Litke <agl(a)us.ibm.com&gt; wrote: >>> >>> >>> On 07/14/2011 05:33 AM, Stefan Hajnoczi wrote: >>>> On Thu, Jul 14, 2011 at 11:19 AM, Jiri Denemark <jdenemar(a)redhat.com&gt; wrote: >>>>> On Thu, Jul 14, 2011 at 10:58:31 +0100, Stefan Hajnoczi wrote: >>>>>> On Thu, Jul 14, 2011 at 10:21 AM, Jiri Denemark <jdenemar(a)redhat.com&gt; wrote: >>>>>>> On Wed, Jul 13, 2011 at 15:46:30 -0500, Adam Litke wrote: >>>>>>> ... >>>>>>>> /* >>>>>>>>  * BlockPull API >>>>>>>>  */ >>>>>>>> >>>>>>>> /* An iterator for initiating and monitoring block pull operations */ >>>>>>>> typedef unsigned long long virDomainBlockPullCursor; >>>>>>>> >>>>>>>> typedef struct _virDomainBlockPullInfo virDomainBlockPullInfo; >>>>>>>> struct _virDomainBlockPullInfo { >>>>>>>>     /* >>>>>>>>      * The following fields provide an indication of block pull progress.  @cur >>>>>>>>      * indicates the current position and will be between 0 and @end.  @end is >>>>>>>>      * the final cursor position for this operation and represents completion. >>>>>>>>      * To approximate progress, divide @cur by @end. >>>>>>>>      */ >>>>>>>>     virDomainBlockPullCursor cur; >>>>>>>>     virDomainBlockPullCursor end; >>>>>>>> }; >>>>>>>> typedef virDomainBlockPullInfo *virDomainBlockPullInfoPtr; >>>>>>> ... >>>>>>>> /** >>>>>>>>  * virDomainBlockPullAbort: >>>>>>>>  * @dom: pointer to domain object >>>>>>>>  * @path: fully-qualified filename of disk >>>>>>>>  * @flags: currently unused, for future extension >>>>>>>>  * >>>>>>>>  * Cancel a pull operation previously started by virDomainBlockPullAll(). >>>>>>>>  * >>>>>>>>  * Returns -1 in case of failure, 0 when successful. >>>>>>>>  */ >>>>>>>> int                 virDomainBlockPullAbort(virDomainPtr dom, >>>>>>>>                                             const char *path, >>>>>>>>                                             unsigned int flags); >>>>>>>> >>>>>>>> /** >>>>>>>>  * virDomainGetBlockPullInfo: >>>>>>>>  * @dom: pointer to domain object >>>>>>>>  * @path: fully-qualified filename of disk >>>>>>>>  * @info: pointer to a virDomainBlockPullInfo structure >>>>>>>>  * @flags: currently unused, for future extension >>>>>>>>  * >>>>>>>>  * Request progress information on a block pull operation that has been started >>>>>>>>  * with virDomainBlockPull().  If an operation is active for the given >>>>>>>>  * parameters, @info will be updated with the current progress. >>>>>>>>  * >>>>>>>>  * Returns -1 in case of failure, 0 when successful. >>>>>>>>  */ >>>>>>>> int                 virDomainGetBlockPullInfo(virDomainPtr dom, >>>>>>>>                                               const char *path, >>>>>>>>                                               virDomainBlockPullInfoPtr info, >>>>>>>>                                               unsigned int flags); >>>>>>>> >>>>>>>> /** >>>>>>>>  * virConnectDomainEventBlockPullStatus: >>>>>>>>  * >>>>>>>>  * The final status of a virDomainBlockPull() operation >>>>>>>>  */ >>>>>>>> typedef enum { >>>>>>>>     VIR_DOMAIN_BLOCK_PULL_COMPLETED = 0, >>>>>>>>     VIR_DOMAIN_BLOCK_PULL_FAILED = 1, >>>>>>>> } virConnectDomainEventBlockPullStatus; >>>>>>>> >>>>>>>> /** >>>>>>>>  * virConnectDomainEventBlockPullCallback: >>>>>>>>  * @conn: connection object >>>>>>>>  * @dom: domain on which the event occurred >>>>>>>>  * @path: fully-qualified filename of the affected disk >>>>>>>>  * @status: final status of the operation (virConnectDomainEventBlockPullStatus >>>>>>>>  * >>>>>>>>  * The callback signature to use when registering for an event of type >>>>>>>>  * VIR_DOMAIN_EVENT_ID_BLOCK_PULL with virConnectDomainEventRegisterAny() >>>>>>>>  */ >>>>>>>> typedef void (*virConnectDomainEventBlockPullCallback)(virConnectPtr conn, >>>>>>>>                                                        virDomainPtr dom, >>>>>>>>                                                        const char *path, >>>>>>>>                                                        int status, >>>>>>>>                                                        void *opaque); >>>>>>> >>>>>>> Could these managing functions and event callback become general and usable by >>>>>>> other block operations such as block copy? >>>>>> >>>>>> Hi Jiri, >>>>>> Live block copy will certainly be possible using the streaming API. >>>>>> Before you start streaming you need to use the snapshot_blkdev command >>>>>> to create the destination file with the source file as its backing >>>>>> image.  You can then use the streaming API to copy data from the >>>>>> source file into the destination file.  On completion the source file >>>>>> is no longer needed. >>>>> >>>>> Well, I'm not talking about using the same API for block copy or implementing >>>>> block copy internally as block streaming. I'm talking about making GetInfo, >>>>> Abort and event callback general to be usable not only for block streaming or >>>>> block copy but also for other possible block operations in the future. >>>>> >>>>> The reason is that starting a block operation (streaming, copy, whatever) may >>>>> need different parameters so they should be different APIs. But once the >>>>> operation is started, we just need to know how far it got and we need the >>>>> ability to abort the job. So this can share the same APIs for all operations. >>>>> It doesn't make sense to require any block operation to provide their own set >>>>> of managing APIs. >>>>> >>>>> It's analogous to virDomainSave, virDomainMigrate, virDomainCoreDump. They are >>>>> all different jobs but all of them can be monitored and aborted using >>>>> virDomainGetJobInfo and virDomainAbortJob. >>>> >>>> I understand.  I can't comment on libvirt API specifics but yes, >>>> there's a similarity here and a chance we'll have other operations in >>>> the future too. >>>> >>>> I'm thinking things like compacting images, compressing images, etc. >>> >>> From a libvirt API perspective, I would only want to merge the abort and >>> info commands if the underlying qemu monitor commands are also merged. >>> Otherwise, we'll need to maintain state on which types of jobs are >>> active on which devices and the added complexity erases any potential >>> benefits IMO. >>> >>> Stefan, any chance that block operation info and cancellation could be >>> unified at the qemu level? >> >> The commands would become "block_job_cancel" and "info block-job". >> >> block_stream would start a job, which basically means: >> BlockJob { >>     BlockDriverState *bs; /* the block device */ >>     BlockJobType type; /* BLOCK_JOB_STREAMING */ >>     int64_t offset;    /* progress info */ >>     int64_t end; >> }; >> >> We probably want to generalize the offset/end pair to have no units. >> They are simply used to indicate progress information.  When the job >> completes offset == end. >> >> The BLOCK_STREAM_COMPLETED event also needs to be generalized to >> BLOCK_JOB_COMPLETED. >> >> This approach is reasonable although I do worry that we don't >> understand the requirements for future block device background jobs. >> For example, jobs that perform an operation on two block devices >> instead of just one.  With QMP we are extensible and can add fields >> for specific job types, so hopefully we can get by without finding the >> API inadequate for future block jobs. >> >> Kevin: What do you think about providing a job interface instead of >> tying the cancel and info operations specifically to block_stream? > > I guess it's a reasonable thing to do. We shouldn't overengineer here > and try to solve all problems in qemu at once, but as long as we're > basically talking about renaming what our current streaming/block copy > concept needs anyway, I don't see a problem. I would leave block_stream as-is because the job start function is where most of the variability comes in.  I think it's not worth doing a generic block_job_start function where you have to say type=streaming. /me is off to update the image streaming API wiki page (and have lunch).