[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/6] Add new API virDomainStreamDisk[Info] to header and drivers



On Mon, May 02, 2011 at 04:29:49PM -0500, Adam Litke wrote:
> After several long distractions, I am back to working on disk streaming.
> Before I hit the list with a new series of patches, I was hoping to
> reach some middle ground on the proposed streaming API.
> 
> On Fri, 2011-04-08 at 14:31 +0100, Daniel P. Berrange wrote:
> > On Thu, Apr 07, 2011 at 04:31:59PM -0500, Adam Litke wrote:
> > >  /*
> > > + * Disk Streaming
> > > + */
> > > +typedef enum {
> > > +    VIR_STREAM_DISK_ONE   = 1,  /* Stream a single disk unit */
> > > +    VIR_STREAM_DISK_START = 2,  /* Stream the entire disk */
> > > +    VIR_STREAM_DISK_STOP  = 4,  /* Stop streaming a disk */
> > > +} virDomainStreamDiskFlags;
> > 
> > Using flags to combine two separate tasks into one single API
> > is rather unpleasant. As raised in the previous patch, the API
> > should also be taking a offset+length in bytes, then there is
> > no need for a special case transfer of an individual sector.
> 
> This is a fair point.  I will work with Stefan to support an
> offset/length qemu API.  Since there doesn't seem to be a great way to
> query device sizes, I think we do need a convenient way to request that
> the entire disk be streamed.  We could either do this with a flag or by
> overriding (offset==0 && length==0) to mean stream the entire device.
> Do you have a preference?

Since length==0 is otherwise meaningless, it is fine to  use that
to indicate "until end of disk". This is consistent with
what we do for virStorageVolUpload/Download which allow length=0
to indicate "until end of disk".

> > > +#define VIR_STREAM_PATH_BUFLEN 1024
> > > +#define VIR_STREAM_DISK_MAX_STREAMS 10
> > > +
> > > +typedef struct _virStreamDiskState virStreamDiskState;
> > > +struct _virStreamDiskState {
> > > +    char path[VIR_STREAM_PATH_BUFLEN];
> > > +    /*
> > > +     * The unit of measure for size and offset is unspecified.  These fields
> > > +     * are meant to indicate the progress of a continuous streaming operation.
> > > +     */
> > > +    unsigned long long offset; /* Current offset of active streaming */
> > > +    unsigned long long size;   /* Disk size */
> > > +};
> > > +typedef virStreamDiskState *virStreamDiskStatePtr;
> > > +
> > > +unsigned long long       virDomainStreamDisk(virDomainPtr dom,
> > > +                                             const char *path,
> > > +                                             unsigned long long offset,
> > > +                                             unsigned int flags);
> > > +
> > > +int                      virDomainStreamDiskInfo(virDomainPtr dom,
> > > +                                                 virStreamDiskStatePtr states,
> > > +                                                 unsigned int nr_states,
> > > +                                                 unsigned int flags);
> > 
> > I would have liked it if we could use the existing JobInfo APIs for
> > getting progress information, but if we need to allow concurrent
> > usage for multiple disks per-VM, we can't. I think we should still
> > use a similar style of API though.
> 
> The goal is to eventually support concurrent streams.  Therefore, we
> will probably need to roll our own Status and Abort APIs (see my
> proposed API below).
> 
> > There also doesn't appear to be a precise way to determine if the
> > copying of an entire disk failed part way through, and if so, how
> > much was actually copied.
> > 
> > Taking all the previous points together, I think the API needs to
> > look like this:
> > 
> >     typedef enum {
> >         /* If set, virDomainBlockAllocate() will return immediately
> >          * allowing polling for operation completion & status
> >          */
> >         VIR_DOMAIN_DISK_ALLOCATE_NONBLOCK,
> >     } virDomainBlockAllocateFlags;
> > 
> >     /*
> >      * @path: fully qualified filename of the virtual disk
> 
> I probably misnamed it, but path is actually the device alias, not a
> path to an image file.

Hmm, I wonder whether that is a good choice or not. The other
APIs all use the disk path. Perhaps we could use that as default
and have a flag to indicate whether the path should be treated as
a device alias instead. Thus getting both options.

> 
> >      * @offset: logical position in bytes, within the virtual disk
> >      * @length: amount of data to copy, in bytes starting from @offset
> >      * @flags: One of virDomainBlockAllocateFlags, or zero
> >      *
> >      * Ensure the virtual disk @path is fully allocated
> >      * between @offset and @offset+ length  If a backing
> >      * store is present, data will be filled from the
> >      * backing store, otherwise data will be fileld with
> >      * zeros
> >      *
> >      * If @flags contains VIR_DOMAIN_DISK_ALLOCATE_NONBLOCK,
> >      * this API will return immediately after initiating the
> >      * copy, otherwise it will block until copying is complete
> >      *
> >      */
> >     int virDomainBlockAllocate(virDomainPtr dom,
> >                                const char *path,
> >                                unsigned long long offset,
> >                                unsigned long long length,
> >                                unsigned int flags);
> > 
> > 
> >     /*
> >      * @path: fully qualified filename of the virtual disk
> >      * @info: allocated struct to return progress info
> >      *
> >      * Query the progress of a disk allocation job. This
> >      * API must be used when virDomainBlockAllocate() was
> >      * invoked with the VIR_DOMAIN_DISK_ALLOCATE_NONBLOCK
> >      * flag set.
> >      *
> >      * The @info.type field will indicate whether the job
> >      * was completed successfully, or failed part way
> >      * through.
> >      *
> >      * The @info data progress fields will contain current
> >      * progress information.
> >      *
> >      * The hypervisor driver may optionally chose to also
> >      * fillin a time estimate for completion.
> >      */
> >     int virDomainBlockGetJobInfo(virDomainPtr dom,
> >                                  const char *path,
> >                                  virDomainJobInfoPtr info);
> > 
> >     /*
> >      * @path: fully qualified filename of the virtual disk
> >      *
> >      * Request that a disk allocation job be aborted at
> >      * the soonest opportunity. This API can only be used
> >      * when virDomainBlockAllocate() was invoked with the
> >      * VIR_DOMAIN_DISK_ALLOCATE_NONBLOCK flag set.
> >      */
> >     int virDomainBlockAbortJob(virDomainPtr dom,
> >                                const char *path);
> > 
> > 
> >     typedef struct _virDomainBlockRegion virDomainBlockRegion;
> >     typedef virDomainBlockRegion *virDomainBlockRegionPtr;
> >     struct _virDomainBlockRegion {
> >         /* The logical offset within the file of the allocated region */
> >         unsigned long long offset;
> >         /* The length of the allocated region */
> >         unsigned long long length;
> >     };
> > 
> >     /*
> >      * @path: fully qualified filename of the virtual disk
> >      * @nregions: filled in the number of @region structs
> >      * @regions: filled with a list of allocated regions
> >      *
> >      * Query the extents of allocated regions within the
> >      * virtual disk file. The offsets in the list of regions
> >      * are not guarenteed to be sorted in any explicit order.
> >      */
> >     int virDomainBlockGetAllocationMap(virDomainPtr dom,
> >                                        const char *path,
> >                                        unsigned int *nregions,
> >                                        virDomainBlockRegionPtr *regions);
> 
> I am not convinced that we really need the block allocation map stuff.
> It's a fun toy, but the layout of data within a device is far too
> low-level of a concept to be exposing to users.  In my opinion,
> streaming should really be done sequentially from the start of the
> device to the end (with arbitrary chunk sizes allowed for throttling
> purposes).  If an application wants to stream according to some special
> pattern, let them maintain a data structure outside of libvirt to manage
> that extra complexity.  It's not an error to stream a chunk that is
> already present.  The populated areas will just complete immediately.
> Therefore, there isn't a need to maintain a strict record of outstanding
> regions.

Well we can always add something like this in at a later
date, so it is fine to drop it.

> > This takes care of things for running guests. It would be
> > desirable to have the same functionality available when a
> > guest is not running, via the virStorageVol APIs. Indeed,
> > this would allow access to the allocation functionality
> > for disks not explicitly associated with any VM yet.
> 
> In light of previous discussion about the complexities around storage
> formats and filesystems, I am not sure how useful such an offline API is
> going to be in practice.  At any rate, I would prefer to consider that
> issue separately.
> 
> So, with my points taken into account I would like to counter with the
> following API proposal:
> 
> ==== snip ====
> 
>     typedef enum {
>          /* If set, virDomainBlockAllocate() will return immediately
>           * allowing polling for operation completion & status
>           */
>          VIR_DOMAIN_DISK_STREAM_NONBLOCK,
>      } virDomainBlockStreamFlags;
> 
>      /*
>       * @device: alias of the target block device
>       * @offset: logical position in bytes, within the virtual disk
>       * @length: amount of data to copy, in bytes starting from @offset
>       * @flags: One of virDomainBlockAllocateFlags, or zero
>       *
>       * Ensure the virtual disk @device is fully allocated
>       * between @offset and @offset+ length  If a backing
>       * store is present, data will be filled from the
>       * backing store, otherwise data will be fileld with
>       * zeros.
>       *
>       * If @flags contains VIR_DOMAIN_DISK_STREAM_NONBLOCK,
>       * this API will return immediately after initiating the
>       * copy, otherwise it will block until copying is complete
>       *
>       */
>      int virDomainBlockStream(virDomainPtr dom,
>                               const char *device,
>                               unsigned long long offset,
>                               unsigned long long length,
>                               unsigned int flags);
> 
>      /*
>       * @device: alias of the target block device
>       *
>       * Request that a disk streaming job be aborted at
>       * the soonest opportunity. This API can only be used
>       * when virDomainBlockStream() was invoked with the
>       * VIR_DOMAIN_DISK_STREAM_NONBLOCK flag set.
>       */
>      int virDomainBlockStreamAbort(virDomainPtr dom,
>                                    const char *device);
> 
>      typedef enum {
>          VIR_BLOCK_STREAM_STATE_COMPLETED = 0,
>          VIR_BLOCK_STREAM_STATE_ACTIVE = 1,
>          VIR_BLOCK_STREAM_STATE_FAILED = 2,
>      } virBlockStreamStatus;
> 
>      typedef struct _virBlockStreamInfo virBlockStreamInfo;
>      struct _virBlockStreamInfo {
>          virBlockStreamStatus status;

Coding guidelines don't allow use of enum types in structs
or as API parameters, instead requiring 'int'.

>          unsigned long long remaining; /* number of bytes remaining */ 
>      };
>      typedef virBlockStreamInfo *virBlockStreamInfoPtr;

s/virBlock/virDomainBlock/ in several places here.

> 
>      /*
>       * @device: alias of the target block device
>       * @info: allocated struct to return progress info
>       *
>       * Query the progress of a disk streaming job. This
>       * API must be used when virDomainBlockStream() was
>       * invoked with the VIR_DOMAIN_DISK_STREAM_NONBLOCK
>       * flag set.
>       *
>       */
>      int virDomainBlockStreamInfo(virDomainPtr dom,
>                                   const char *device,
>                                   virBlockStreamInfoPtr info);

Any reason you didn't just use the existing 'virDomainJobInfoPtr'
struct, with this new API ? In particular I think we want many
of the other fields from that struct beyond just 'remaining'.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]