Hi, On 02/09/2012 09:27 PM, Eric Blake wrote:

On 02/09/2012 01:04 PM, Wido den Hollander wrote:

...

Hi,

I'm still working on the RBD (RADOS / Ceph) storage driver for libvirt and I noticed the virStorageVolDownload and virStorageVolWipe methods.

I assumed those would be passed on to the storage backend, but it doesn't.

In the storageDriver the method storageVolumeDownload simply opens a file descriptor and reads the device.

Until now libvirt only had support for storage drivers who presented regular files or block devices, but RBD doesn't. (Well, RBD could, but I'm currently going for Qemu-RBD).

In the future we might see more storage drivers in libvirt for a project like Sheepdog as well.

Sheepdog and RBD both have drivers in Qemu.

What would the way be to approach this? Should the download, upload and wipe methods be moved to the storage backends?

There could also be an exception? If virStoragePoolType matches VIR_STORAGE_POOL_RBD or VIR_STORAGE_POOL_SHEEPDOG the storage backend could be invoked instead of opening the file descriptor?

Any thoughts on this?

I was just checking out this code again and I saw that both RBD and Sheepdog prefix their device paths RBD: rbd:pool/image Sheepdog: sheepdog:image In the storageDriver a check could be made. If 'path' is a file we use the regular methods. If it's a prefix, we send this down to the storage backend for further handling. In short-term this doesn't brake any exisiting code and applications, but does allow these functions to work with new storage backends. Wido

For at least virStorageVolDownload, I can see two useful behaviors, and think we need to introduce a flag to specify which behavior:

virStorageVolDownload(, 0) => verbatim copy of the raw volume, including metadata, as seen by the host; does not require any decryption keys to download, but may require secrets for accessing remote Sheepdog serves - great for cloning volumes

virStorageVolDownload(, CONTENTS) => copy the contents of the volume as they would be seen in the guest; for any encrypted storage formats, this requires the right virSecret object to access the contents - great for converting arbitrary volumes into a raw storage format

Conversely, virStorageVolUpload(, 0) installs a raw volume, and virStroageVolUpload(, CONTENTS) takes raw data and injects it into the contents that the guest will see (which may involve compression or encryption).

Even on just local file systems, this would be the difference between 'cp qcow2.img1 qcow2.img2' (copy the entire metadata, without knowing the contents as seen by the guest) and 'qemu-img conviert -f qcow2 -O qcow2 qcow2.img1 qcow2.img2' (copy the entire guest contents, but by creating new qcow2 metadata, which may result in a smaller img2 file as abandoned sectors in img1 are elided).

Very much worth doing!

I also think that virStorageVolWipe should have a raw vs. contents, as in:

virStorageVolWipe(, 0) => wipe the entire volume, including any storage metadata; a qcow2 file with 10M in use, 30M capacity, and occupying 20M on disk gets wiped into a raw file with 20M in use, capacity, and allocation

virStorageVolWipe(, CONTENTS) => ensure that the guest sees an empty volume, but preserve host metadata; a qcow2 file with 10M in use, 30M capacity, and occupying 20M on disk gets converted to a qcow2 with 0 bytes in use, 30M capacity; and we probably need a second flag to control whether disk usage is altered (that is, whether the image is thin or pre-allocated on disk)

It certainly sounds like we should be wiring more of these decisions through to the backends, rather than attempting to do them all from the main storage driver.