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

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.