[libvirt] [PATCH 0/9] Storage cloning for LVM and Disk backends
This following series implements cloning for LVM and disk backends. Most of the functionality is already here, it just needed some reorganization to be accessible for every backend. I verified the following scenarios produced a bootable image: - Clone within a disk pool - Clone within a logical pool - Clone a raw file to a disk pool - Clone a disk pool to a logical pool Cole Robinson (9): storage: Refactor FS backend 'create' function choosing. storage: Always assume we are tracking alloc progress in FS backend storage: Move most of the FS creation functions to common backend. storage: cleanup: do away with 'createFile' storage: Break out actual raw cloning to separate function. storage: Implement 'CreateBlockFrom' helper. storage: Don't try sparse detection if writing to block device. storage: Implement CreateVolFrom for logical and disk backend. storage: Fix deadlock when cloning across pools. src/storage_backend.c | 471 +++++++++++++++++++++++++++++++++++++++++ src/storage_backend.h | 15 ++ src/storage_backend_disk.c | 15 ++ src/storage_backend_fs.c | 378 ++------------------------------- src/storage_backend_logical.c | 17 ++ src/storage_driver.c | 6 +- 6 files changed, 540 insertions(+), 362 deletions(-)
Break out separate functions for - Determining the supported '*-img' tool, - The tool's associated create function, - Desired function for cloning (CreateXMLFrom). This will be eventually used to unify cloning across all backends. --- src/storage_backend_fs.c | 104 ++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 86 insertions(+), 18 deletions(-) diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index c3d66b5..64a7130 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -55,6 +55,12 @@ enum { BACKING_STORE_ERROR, }; +enum { + TOOL_QEMU_IMG, + TOOL_KVM_IMG, + TOOL_QCOW_CREATE, +}; + static int cowGetBackingStore(virConnectPtr, char **, const unsigned char *, size_t); static int qcowXGetBackingStore(virConnectPtr, char **, @@ -1363,6 +1369,77 @@ static int createQemuCreate(virConnectPtr conn, return 0; } +static createFile +toolTypeToFunction(virConnectPtr conn, int tool_type) +{ + switch (tool_type) { + case TOOL_KVM_IMG: + case TOOL_QEMU_IMG: + return createQemuImg; + case TOOL_QCOW_CREATE: + return createQemuCreate; + default: + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unknown file create tool type '%d'."), + tool_type); + } + + return NULL; +} + +static int +findImageTool(char **tool) +{ + int tool_type = -1; + char *tmp = NULL; + + if ((tmp = virFindFileInPath("kvm-img")) != NULL) { + tool_type = TOOL_KVM_IMG; + } else if ((tmp = virFindFileInPath("qemu-img")) != NULL) { + tool_type = TOOL_QEMU_IMG; + } else if ((tmp = virFindFileInPath("qcow-create")) != NULL) { + tool_type = TOOL_QCOW_CREATE; + } + + if (tool) + *tool = tmp; + else + VIR_FREE(tmp); + + return tool_type; +} + +static createFile +buildVolFromFunction(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol) +{ + int tool_type; + + if (!inputvol) + return NULL; + + /* If either volume is a non-raw file vol, we need to use an external + * tool for converting + */ + if ((vol->type == VIR_STORAGE_VOL_FILE && + vol->target.format != VIR_STORAGE_VOL_FILE_RAW) || + (inputvol->type == VIR_STORAGE_VOL_FILE && + inputvol->target.format != VIR_STORAGE_VOL_FILE_RAW)) { + + if ((tool_type = findImageTool(NULL)) != -1) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("creation of non-raw file images is " + "not supported without qemu-img.")); + return NULL; + } + + return toolTypeToFunction(conn, tool_type); + } + + return createRaw; +} + static int _virStorageBackendFileSystemVolBuild(virConnectPtr conn, virStorageVolDefPtr vol, @@ -1370,28 +1447,19 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, { int fd; createFile create_func; - char *create_tool; + int tool_type; - if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW && - (!inputvol || - (inputvol->type == VIR_STORAGE_VOL_BLOCK || - inputvol->target.format == VIR_STORAGE_VOL_FILE_RAW))) { - /* Raw file creation - * Raw -> Raw copying - * Block dev -> Raw copying - */ + if (inputvol) { + create_func = buildVolFromFunction(conn, vol, inputvol); + if (!create_func) + return -1; + } else if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW) { create_func = createRaw; } else if (vol->target.format == VIR_STORAGE_VOL_FILE_DIR) { create_func = createFileDir; - } else if ((create_tool = virFindFileInPath("kvm-img")) != NULL) { - VIR_FREE(create_tool); - create_func = createQemuImg; - } else if ((create_tool = virFindFileInPath("qemu-img")) != NULL) { - VIR_FREE(create_tool); - create_func = createQemuImg; - } else if ((create_tool = virFindFileInPath("qcow-create")) != NULL) { - VIR_FREE(create_tool); - create_func = createQemuCreate; + } else if ((tool_type = findImageTool(NULL)) != -1) { + if ((create_func = toolTypeToFunction(conn, tool_type)) == NULL) + return -1; } else { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("creation of non-raw images " -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:46:58PM -0400, Cole Robinson wrote:
Break out separate functions for
- Determining the supported '*-img' tool, - The tool's associated create function, - Desired function for cloning (CreateXMLFrom).
This will be eventually used to unify cloning across all backends. --- src/storage_backend_fs.c | 104 ++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 86 insertions(+), 18 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
We do support allocation progress now, so have the code accomodate. --- src/storage_backend_fs.c | 49 +++++++++++++++------------------------------- 1 files changed, 16 insertions(+), 33 deletions(-) diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 64a7130..c7c1c9c 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -72,8 +72,6 @@ typedef int (*createFile)(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol); -static int track_allocation_progress = 0; - /* Either 'magic' or 'extension' *must* be provided */ struct FileTypeInfo { int type; /* One of the constants above */ @@ -1108,38 +1106,23 @@ static int createRaw(virConnectPtr conn, /* Pre-allocate any data if requested */ /* XXX slooooooooooooooooow on non-extents-based file systems */ - if (remain) { - if (track_allocation_progress) { - - while (remain) { - /* Allocate in chunks of 512MiB: big-enough chunk - * size and takes approx. 9s on ext3. A progress - * update every 9s is a fair-enough trade-off - */ - unsigned long long bytes = 512 * 1024 * 1024; - int r; - - if (bytes > remain) - bytes = remain; - if ((r = safezero(fd, 0, vol->allocation - remain, - bytes)) != 0) { - virReportSystemError(conn, r, - _("cannot fill file '%s'"), - vol->target.path); - goto cleanup; - } - remain -= bytes; - } - } else { /* No progress bars to be shown */ - int r; - - if ((r = safezero(fd, 0, 0, remain)) != 0) { - virReportSystemError(conn, r, - _("cannot fill file '%s'"), - vol->target.path); - goto cleanup; - } + while (remain) { + /* Allocate in chunks of 512MiB: big-enough chunk + * size and takes approx. 9s on ext3. A progress + * update every 9s is a fair-enough trade-off + */ + unsigned long long bytes = 512 * 1024 * 1024; + int r; + + if (bytes > remain) + bytes = remain; + if ((r = safezero(fd, 0, vol->allocation - remain, bytes)) != 0) { + virReportSystemError(conn, r, + _("cannot fill file '%s'"), + vol->target.path); + goto cleanup; } + remain -= bytes; } if (close(fd) < 0) { -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:46:59PM -0400, Cole Robinson wrote:
We do support allocation progress now, so have the code accomodate. --- src/storage_backend_fs.c | 49 +++++++++++++++------------------------------- 1 files changed, 16 insertions(+), 33 deletions(-)
Do later patches have a dependancy on this ? The allocation progress tracking you mention is actually WRT to a 2nd thread calling virStorageVolGetInfo while the first thread is doing the work. AFAIK, that would not be impacted by either of these 2 code paths. The 'track_allocation_progress' variable was a placeholder for the case where we do proper async non-blocking vol creation from a single thread. By switching this code to use the iterative allocation, it will increase the number of ext4 extents required for large files which is not so desirable. So I think its better to just leave this code in as is, or remove the other code path. Daniel
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 64a7130..c7c1c9c 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -72,8 +72,6 @@ typedef int (*createFile)(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol);
-static int track_allocation_progress = 0; - /* Either 'magic' or 'extension' *must* be provided */ struct FileTypeInfo { int type; /* One of the constants above */ @@ -1108,38 +1106,23 @@ static int createRaw(virConnectPtr conn,
/* Pre-allocate any data if requested */ /* XXX slooooooooooooooooow on non-extents-based file systems */ - if (remain) { - if (track_allocation_progress) { - - while (remain) { - /* Allocate in chunks of 512MiB: big-enough chunk - * size and takes approx. 9s on ext3. A progress - * update every 9s is a fair-enough trade-off - */ - unsigned long long bytes = 512 * 1024 * 1024; - int r; - - if (bytes > remain) - bytes = remain; - if ((r = safezero(fd, 0, vol->allocation - remain, - bytes)) != 0) { - virReportSystemError(conn, r, - _("cannot fill file '%s'"), - vol->target.path); - goto cleanup; - } - remain -= bytes; - } - } else { /* No progress bars to be shown */ - int r; - - if ((r = safezero(fd, 0, 0, remain)) != 0) { - virReportSystemError(conn, r, - _("cannot fill file '%s'"), - vol->target.path); - goto cleanup; - } + while (remain) { + /* Allocate in chunks of 512MiB: big-enough chunk + * size and takes approx. 9s on ext3. A progress + * update every 9s is a fair-enough trade-off + */ + unsigned long long bytes = 512 * 1024 * 1024; + int r; + + if (bytes > remain) + bytes = remain; + if ((r = safezero(fd, 0, vol->allocation - remain, bytes)) != 0) { + virReportSystemError(conn, r, + _("cannot fill file '%s'"), + vol->target.path); + goto cleanup; } + remain -= bytes; }
if (close(fd) < 0) { -- 1.6.0.6
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Fri, Jul 10, 2009 at 04:46:59PM -0400, Cole Robinson wrote:
We do support allocation progress now, so have the code accomodate. --- src/storage_backend_fs.c | 49 +++++++++++++++------------------------------- 1 files changed, 16 insertions(+), 33 deletions(-)
Do later patches have a dependancy on this ?
The allocation progress tracking you mention is actually WRT to a 2nd thread calling virStorageVolGetInfo while the first thread is doing the work. AFAIK, that would not be impacted by either of these 2 code paths. The 'track_allocation_progress' variable was a placeholder for the case where we do proper async non-blocking vol creation from a single thread.
By switching this code to use the iterative allocation, it will increase the number of ext4 extents required for large files which is not so desirable. So I think its better to just leave this code in as is, or remove the other code path.
This change was just a (attempted) cleanup. I'll drop it. Thanks, Cole
These will be used by other pool cloning implementations. --- src/storage_backend.c | 397 ++++++++++++++++++++++++++++++++++++++++++++ src/storage_backend.h | 17 ++ src/storage_backend_fs.c | 408 +--------------------------------------------- 3 files changed, 421 insertions(+), 401 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index 953928e..59a24e1 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -95,6 +95,403 @@ static virStorageBackendPtr backends[] = { NULL }; +enum { + TOOL_QEMU_IMG, + TOOL_KVM_IMG, + TOOL_QCOW_CREATE, +}; + +int +virStorageBackendCreateRaw(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol) +{ + int fd = -1; + int inputfd = -1; + int ret = -1; + unsigned long long remain; + char *buf = NULL; + + if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, + vol->target.perms.mode)) < 0) { + virReportSystemError(conn, errno, + _("cannot create path '%s'"), + vol->target.path); + goto cleanup; + } + + if (inputvol) { + if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { + virReportSystemError(conn, errno, + _("could not open input path '%s'"), + inputvol->target.path); + goto cleanup; + } + } + + /* Seek to the final size, so the capacity is available upfront + * for progress reporting */ + if (ftruncate(fd, vol->capacity) < 0) { + virReportSystemError(conn, errno, + _("cannot extend file '%s'"), + vol->target.path); + goto cleanup; + } + + remain = vol->allocation; + + if (inputfd != -1) { + int amtread = -1; + size_t bytes = 1024 * 1024; + char zerobuf[512]; + + bzero(&zerobuf, sizeof(zerobuf)); + + if (VIR_ALLOC_N(buf, bytes) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + while (amtread != 0) { + int amtleft; + + if (remain < bytes) + bytes = remain; + + if ((amtread = saferead(inputfd, buf, bytes)) < 0) { + virReportSystemError(conn, errno, + _("failed reading from file '%s'"), + inputvol->target.path); + goto cleanup; + } + remain -= amtread; + + /* Loop over amt read in 512 byte increments, looking for sparse + * blocks */ + amtleft = amtread; + do { + int interval = ((512 > amtleft) ? amtleft : 512); + int offset = amtread - amtleft; + + if (memcmp(buf+offset, zerobuf, interval) == 0) { + if (lseek(fd, interval, SEEK_CUR) < 0) { + virReportSystemError(conn, errno, + _("cannot extend file '%s'"), + vol->target.path); + goto cleanup; + } + } else if (safewrite(fd, buf+offset, interval) < 0) { + virReportSystemError(conn, errno, + _("failed writing to file '%s'"), + vol->target.path); + goto cleanup; + + } + } while ((amtleft -= 512) > 0); + } + } + + /* Pre-allocate any data if requested */ + /* XXX slooooooooooooooooow on non-extents-based file systems */ + while (remain) { + /* Allocate in chunks of 512MiB: big-enough chunk + * size and takes approx. 9s on ext3. A progress + * update every 9s is a fair-enough trade-off + */ + unsigned long long bytes = 512 * 1024 * 1024; + int r; + + if (bytes > remain) + bytes = remain; + if ((r = safezero(fd, 0, vol->allocation - remain, bytes)) != 0) { + virReportSystemError(conn, r, _("cannot fill file '%s'"), + vol->target.path); + goto cleanup; + } + remain -= bytes; + } + + if (close(fd) < 0) { + virReportSystemError(conn, errno, + _("cannot close file '%s'"), + vol->target.path); + goto cleanup; + } + fd = -1; + + if (inputfd != -1 && close(inputfd) < 0) { + virReportSystemError(conn, errno, + _("cannot close file '%s'"), + inputvol->target.path); + goto cleanup; + } + inputfd = -1; + + ret = 0; +cleanup: + if (fd != -1) + close(fd); + if (inputfd != -1) + close(inputfd); + VIR_FREE(buf); + + return ret; +} + +static int +virStorageBackendCreateQemuImg(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol) +{ + char size[100]; + char *create_tool; + short use_kvmimg; + + const char *type = virStorageVolFormatFileSystemTypeToString(vol->target.format); + const char *backingType = vol->backingStore.path ? + virStorageVolFormatFileSystemTypeToString(vol->backingStore.format) : NULL; + + const char *inputBackingPath = (inputvol ? inputvol->backingStore.path + : NULL); + const char *inputPath = inputvol ? inputvol->target.path : NULL; + /* Treat input block devices as 'raw' format */ + const char *inputType = inputPath ? + virStorageVolFormatFileSystemTypeToString(inputvol->type == VIR_STORAGE_VOL_BLOCK ? VIR_STORAGE_VOL_FILE_RAW : inputvol->target.format) : + NULL; + + const char **imgargv; + const char *imgargvnormal[] = { + NULL, "create", + "-f", type, + vol->target.path, + size, + NULL, + }; + /* Extra NULL fields are for including "backingType" when using + * kvm-img. It's -F backingType + */ + const char *imgargvbacking[] = { + NULL, "create", + "-f", type, + "-b", vol->backingStore.path, + vol->target.path, + size, + NULL, + NULL, + NULL + }; + const char *convargv[] = { + NULL, "convert", + "-f", inputType, + "-O", type, + inputPath, + vol->target.path, + NULL, + }; + + if (type == NULL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol type %d"), + vol->target.format); + return -1; + } + if (inputvol && inputType == NULL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol type %d"), + inputvol->target.format); + return -1; + } + + if (vol->backingStore.path) { + + /* XXX: Not strictly required: qemu-img has an option a different + * backing store, not really sure what use it serves though, and it + * may cause issues with lvm. Untested essentially. + */ + if (inputvol && + (!inputBackingPath || + STRNEQ(inputBackingPath, vol->backingStore.path))) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("a different backing store can not " + "be specified.")); + return -1; + } + + if (backingType == NULL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol backing store type %d"), + vol->backingStore.format); + return -1; + } + if (access(vol->backingStore.path, R_OK) != 0) { + virReportSystemError(conn, errno, + _("inaccessible backing store volume %s"), + vol->backingStore.path); + return -1; + } + } + + if ((create_tool = virFindFileInPath("kvm-img")) != NULL) + use_kvmimg = 1; + else if ((create_tool = virFindFileInPath("qemu-img")) != NULL) + use_kvmimg = 0; + else { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unable to find kvm-img or qemu-img")); + return -1; + } + + if (inputvol) { + convargv[0] = create_tool; + imgargv = convargv; + } else if (vol->backingStore.path) { + imgargvbacking[0] = create_tool; + if (use_kvmimg) { + imgargvbacking[6] = "-F"; + imgargvbacking[7] = backingType; + imgargvbacking[8] = vol->target.path; + imgargvbacking[9] = size; + } + imgargv = imgargvbacking; + } else { + imgargvnormal[0] = create_tool; + imgargv = imgargvnormal; + } + + + /* Size in KB */ + snprintf(size, sizeof(size), "%llu", vol->capacity/1024); + + if (virRun(conn, imgargv, NULL) < 0) { + VIR_FREE(imgargv[0]); + return -1; + } + + VIR_FREE(imgargv[0]); + + return 0; +} + +/* + * Xen removed the fully-functional qemu-img, and replaced it + * with a partially functional qcow-create. Go figure ??!? + */ +static int +virStorageBackendCreateQcowCreate(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol) +{ + char size[100]; + const char *imgargv[4]; + + if (inputvol) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", + _("cannot copy from volume with qcow-create")); + return -1; + } + + if (vol->target.format != VIR_STORAGE_VOL_FILE_QCOW2) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unsupported storage vol type %d"), + vol->target.format); + return -1; + } + if (vol->backingStore.path != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + _("copy-on-write image not supported with " + "qcow-create")); + return -1; + } + + /* Size in MB - yes different units to qemu-img :-( */ + snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024); + + imgargv[0] = virFindFileInPath("qcow-create"); + imgargv[1] = size; + imgargv[2] = vol->target.path; + imgargv[3] = NULL; + + if (virRun(conn, imgargv, NULL) < 0) { + VIR_FREE(imgargv[0]); + return -1; + } + + VIR_FREE(imgargv[0]); + + return 0; +} + +createFile +virStorageBackendFSImageToolTypeToFunc(virConnectPtr conn, int tool_type) +{ + switch (tool_type) { + case TOOL_KVM_IMG: + case TOOL_QEMU_IMG: + return virStorageBackendCreateQemuImg; + case TOOL_QCOW_CREATE: + return virStorageBackendCreateQcowCreate; + default: + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unknown file create tool type '%d'."), + tool_type); + } + + return NULL; +} + +int +virStorageBackendFindFSImageTool(char **tool) +{ + int tool_type = -1; + char *tmp = NULL; + + if ((tmp = virFindFileInPath("kvm-img")) != NULL) { + tool_type = TOOL_KVM_IMG; + } else if ((tmp = virFindFileInPath("qemu-img")) != NULL) { + tool_type = TOOL_QEMU_IMG; + } else if ((tmp = virFindFileInPath("qcow-create")) != NULL) { + tool_type = TOOL_QCOW_CREATE; + } + + if (tool) + *tool = tmp; + else + VIR_FREE(tmp); + + return tool_type; +} + +createFile +virStorageBackendGetBuildVolFromFunction(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol) +{ + int tool_type; + + if (!inputvol) + return NULL; + + /* If either volume is a non-raw file vol, we need to use an external + * tool for converting + */ + if ((vol->type == VIR_STORAGE_VOL_FILE && + vol->target.format != VIR_STORAGE_VOL_FILE_RAW) || + (inputvol->type == VIR_STORAGE_VOL_FILE && + inputvol->target.format != VIR_STORAGE_VOL_FILE_RAW)) { + + if ((tool_type = virStorageBackendFindFSImageTool(NULL)) != -1) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("creation of non-raw file images is " + "not supported without qemu-img.")); + return NULL; + } + + return virStorageBackendFSImageToolTypeToFunc(conn, tool_type); + } + + return virStorageBackendCreateRaw; +} #if defined(UDEVADM) || defined(UDEVSETTLE) void virWaitForDevices(virConnectPtr conn) diff --git a/src/storage_backend.h b/src/storage_backend.h index a3f441c..ec75f41 100644 --- a/src/storage_backend.h +++ b/src/storage_backend.h @@ -40,6 +40,23 @@ typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObj typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, unsigned int flags); +typedef int (*createFile)(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol); + +/* File creation/cloning functions used for cloning between backends */ +int virStorageBackendCreateRaw(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol); +createFile +virStorageBackendGetBuildVolFromFunction(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol); +int virStorageBackendFindFSImageTool(char **tool); +createFile virStorageBackendFSImageToolTypeToFunc(virConnectPtr conn, + int tool_type); + + typedef struct _virStorageBackend virStorageBackend; typedef virStorageBackend *virStorageBackendPtr; diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index c7c1c9c..3b2af59 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -55,12 +55,6 @@ enum { BACKING_STORE_ERROR, }; -enum { - TOOL_QEMU_IMG, - TOOL_KVM_IMG, - TOOL_QCOW_CREATE, -}; - static int cowGetBackingStore(virConnectPtr, char **, const unsigned char *, size_t); static int qcowXGetBackingStore(virConnectPtr, char **, @@ -68,10 +62,6 @@ static int qcowXGetBackingStore(virConnectPtr, char **, static int vmdk4GetBackingStore(virConnectPtr, char **, const unsigned char *, size_t); -typedef int (*createFile)(virConnectPtr conn, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol); - /* Either 'magic' or 'extension' *must* be provided */ struct FileTypeInfo { int type; /* One of the constants above */ @@ -1016,142 +1006,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, return 0; } -static int createRaw(virConnectPtr conn, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) { - int fd = -1; - int inputfd = -1; - int ret = -1; - unsigned long long remain; - char *buf = NULL; - - if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode)) < 0) { - virReportSystemError(conn, errno, - _("cannot create path '%s'"), - vol->target.path); - goto cleanup; - } - - if (inputvol) { - if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { - virReportSystemError(conn, errno, - _("could not open input path '%s'"), - inputvol->target.path); - goto cleanup; - } - } - - /* Seek to the final size, so the capacity is available upfront - * for progress reporting */ - if (ftruncate(fd, vol->capacity) < 0) { - virReportSystemError(conn, errno, - _("cannot extend file '%s'"), - vol->target.path); - goto cleanup; - } - - remain = vol->allocation; - - if (inputfd != -1) { - int amtread = -1; - size_t bytes = 1024 * 1024; - char zerobuf[512]; - - bzero(&zerobuf, sizeof(zerobuf)); - - if (VIR_ALLOC_N(buf, bytes) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - while (amtread != 0) { - int amtleft; - - if (remain < bytes) - bytes = remain; - - if ((amtread = saferead(inputfd, buf, bytes)) < 0) { - virReportSystemError(conn, errno, - _("failed reading from file '%s'"), - inputvol->target.path); - goto cleanup; - } - remain -= amtread; - - /* Loop over amt read in 512 byte increments, looking for sparse - * blocks */ - amtleft = amtread; - do { - int interval = ((512 > amtleft) ? amtleft : 512); - int offset = amtread - amtleft; - - if (memcmp(buf+offset, zerobuf, interval) == 0) { - if (lseek(fd, interval, SEEK_CUR) < 0) { - virReportSystemError(conn, errno, - _("cannot extend file '%s'"), - vol->target.path); - goto cleanup; - } - } else if (safewrite(fd, buf+offset, interval) < 0) { - virReportSystemError(conn, errno, - _("failed writing to file '%s'"), - vol->target.path); - goto cleanup; - - } - } while ((amtleft -= 512) > 0); - } - } - - /* Pre-allocate any data if requested */ - /* XXX slooooooooooooooooow on non-extents-based file systems */ - while (remain) { - /* Allocate in chunks of 512MiB: big-enough chunk - * size and takes approx. 9s on ext3. A progress - * update every 9s is a fair-enough trade-off - */ - unsigned long long bytes = 512 * 1024 * 1024; - int r; - - if (bytes > remain) - bytes = remain; - if ((r = safezero(fd, 0, vol->allocation - remain, bytes)) != 0) { - virReportSystemError(conn, r, - _("cannot fill file '%s'"), - vol->target.path); - goto cleanup; - } - remain -= bytes; - } - - if (close(fd) < 0) { - virReportSystemError(conn, errno, - _("cannot close file '%s'"), - vol->target.path); - goto cleanup; - } - fd = -1; - - if (inputfd != -1 && close(inputfd) < 0) { - virReportSystemError(conn, errno, - _("cannot close file '%s'"), - inputvol->target.path); - goto cleanup; - } - inputfd = -1; - - ret = 0; -cleanup: - if (fd != -1) - close(fd); - if (inputfd != -1) - close(inputfd); - VIR_FREE(buf); - - return ret; -} - static int createFileDir(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol) { @@ -1172,257 +1026,6 @@ static int createFileDir(virConnectPtr conn, return 0; } -static int createQemuImg(virConnectPtr conn, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) { - char size[100]; - char *create_tool; - short use_kvmimg; - - const char *type = virStorageVolFormatFileSystemTypeToString(vol->target.format); - const char *backingType = vol->backingStore.path ? - virStorageVolFormatFileSystemTypeToString(vol->backingStore.format) : NULL; - - const char *inputBackingPath = (inputvol ? inputvol->backingStore.path - : NULL); - const char *inputPath = inputvol ? inputvol->target.path : NULL; - /* Treat input block devices as 'raw' format */ - const char *inputType = inputPath ? - virStorageVolFormatFileSystemTypeToString(inputvol->type == VIR_STORAGE_VOL_BLOCK ? VIR_STORAGE_VOL_FILE_RAW : inputvol->target.format) : - NULL; - - const char **imgargv; - const char *imgargvnormal[] = { - NULL, "create", - "-f", type, - vol->target.path, - size, - NULL, - }; - /* Extra NULL fields are for including "backingType" when using - * kvm-img. It's -F backingType - */ - const char *imgargvbacking[] = { - NULL, "create", - "-f", type, - "-b", vol->backingStore.path, - vol->target.path, - size, - NULL, - NULL, - NULL - }; - const char *convargv[] = { - NULL, "convert", - "-f", inputType, - "-O", type, - inputPath, - vol->target.path, - NULL, - }; - - if (type == NULL) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol type %d"), - vol->target.format); - return -1; - } - if (inputvol && inputType == NULL) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol type %d"), - inputvol->target.format); - return -1; - } - - if (vol->backingStore.path) { - - /* XXX: Not strictly required: qemu-img has an option a different - * backing store, not really sure what use it serves though, and it - * may cause issues with lvm. Untested essentially. - */ - if (inputvol && - (!inputBackingPath || - STRNEQ(inputBackingPath, vol->backingStore.path))) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - "%s", _("a different backing store can not " - "be specified.")); - return -1; - } - - if (backingType == NULL) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol backing store type %d"), - vol->backingStore.format); - return -1; - } - if (access(vol->backingStore.path, R_OK) != 0) { - virReportSystemError(conn, errno, - _("inaccessible backing store volume %s"), - vol->backingStore.path); - return -1; - } - } - - if ((create_tool = virFindFileInPath("kvm-img")) != NULL) - use_kvmimg = 1; - else if ((create_tool = virFindFileInPath("qemu-img")) != NULL) - use_kvmimg = 0; - else { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("unable to find kvm-img or qemu-img")); - return -1; - } - - if (inputvol) { - convargv[0] = create_tool; - imgargv = convargv; - } else if (vol->backingStore.path) { - imgargvbacking[0] = create_tool; - if (use_kvmimg) { - imgargvbacking[6] = "-F"; - imgargvbacking[7] = backingType; - imgargvbacking[8] = vol->target.path; - imgargvbacking[9] = size; - } - imgargv = imgargvbacking; - } else { - imgargvnormal[0] = create_tool; - imgargv = imgargvnormal; - } - - - /* Size in KB */ - snprintf(size, sizeof(size), "%llu", vol->capacity/1024); - - if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - - VIR_FREE(imgargv[0]); - - return 0; -} - -/* - * Xen removed the fully-functional qemu-img, and replaced it - * with a partially functional qcow-create. Go figure ??!? - */ -static int createQemuCreate(virConnectPtr conn, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) { - char size[100]; - const char *imgargv[4]; - - if (inputvol) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - "%s", - _("cannot copy from volume with qcow-create")); - return -1; - } - - if (vol->target.format != VIR_STORAGE_VOL_FILE_QCOW2) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("unsupported storage vol type %d"), - vol->target.format); - return -1; - } - if (vol->backingStore.path != NULL) { - virStorageReportError(conn, VIR_ERR_NO_SUPPORT, - _("copy-on-write image not supported with " - "qcow-create")); - return -1; - } - - /* Size in MB - yes different units to qemu-img :-( */ - snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024); - - imgargv[0] = virFindFileInPath("qcow-create"); - imgargv[1] = size; - imgargv[2] = vol->target.path; - imgargv[3] = NULL; - - if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - - VIR_FREE(imgargv[0]); - - return 0; -} - -static createFile -toolTypeToFunction(virConnectPtr conn, int tool_type) -{ - switch (tool_type) { - case TOOL_KVM_IMG: - case TOOL_QEMU_IMG: - return createQemuImg; - case TOOL_QCOW_CREATE: - return createQemuCreate; - default: - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Unknown file create tool type '%d'."), - tool_type); - } - - return NULL; -} - -static int -findImageTool(char **tool) -{ - int tool_type = -1; - char *tmp = NULL; - - if ((tmp = virFindFileInPath("kvm-img")) != NULL) { - tool_type = TOOL_KVM_IMG; - } else if ((tmp = virFindFileInPath("qemu-img")) != NULL) { - tool_type = TOOL_QEMU_IMG; - } else if ((tmp = virFindFileInPath("qcow-create")) != NULL) { - tool_type = TOOL_QCOW_CREATE; - } - - if (tool) - *tool = tmp; - else - VIR_FREE(tmp); - - return tool_type; -} - -static createFile -buildVolFromFunction(virConnectPtr conn, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) -{ - int tool_type; - - if (!inputvol) - return NULL; - - /* If either volume is a non-raw file vol, we need to use an external - * tool for converting - */ - if ((vol->type == VIR_STORAGE_VOL_FILE && - vol->target.format != VIR_STORAGE_VOL_FILE_RAW) || - (inputvol->type == VIR_STORAGE_VOL_FILE && - inputvol->target.format != VIR_STORAGE_VOL_FILE_RAW)) { - - if ((tool_type = findImageTool(NULL)) != -1) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - "%s", _("creation of non-raw file images is " - "not supported without qemu-img.")); - return NULL; - } - - return toolTypeToFunction(conn, tool_type); - } - - return createRaw; -} - static int _virStorageBackendFileSystemVolBuild(virConnectPtr conn, virStorageVolDefPtr vol, @@ -1433,15 +1036,18 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, int tool_type; if (inputvol) { - create_func = buildVolFromFunction(conn, vol, inputvol); + create_func = virStorageBackendGetBuildVolFromFunction(conn, vol, + inputvol); if (!create_func) return -1; } else if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW) { - create_func = createRaw; + create_func = virStorageBackendCreateRaw; } else if (vol->target.format == VIR_STORAGE_VOL_FILE_DIR) { create_func = createFileDir; - } else if ((tool_type = findImageTool(NULL)) != -1) { - if ((create_func = toolTypeToFunction(conn, tool_type)) == NULL) + } else if ((tool_type = virStorageBackendFindFSImageTool(NULL)) != -1) { + create_func = virStorageBackendFSImageToolTypeToFunc(conn, tool_type); + + if (!create_func) return -1; } else { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:47:00PM -0400, Cole Robinson wrote:
These will be used by other pool cloning implementations. --- src/storage_backend.c | 397 ++++++++++++++++++++++++++++++++++++++++++++ src/storage_backend.h | 17 ++ src/storage_backend_fs.c | 408 +--------------------------------------------- 3 files changed, 421 insertions(+), 401 deletions(-)
ACK, good to avoid dependancies between specific backend impls Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Have storage building functions be definitions of virStorageBackendBuildVolFrom: we will need to do this in the future anyways if we ever support the flags attribute. --- src/storage_backend.c | 13 ++++++++----- src/storage_backend.h | 14 ++++++-------- src/storage_backend_fs.c | 7 ++++--- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index 59a24e1..aea6105 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -104,7 +104,8 @@ enum { int virStorageBackendCreateRaw(virConnectPtr conn, virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) { int fd = -1; int inputfd = -1; @@ -241,7 +242,8 @@ cleanup: static int virStorageBackendCreateQemuImg(virConnectPtr conn, virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) { char size[100]; char *create_tool; @@ -379,7 +381,8 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, static int virStorageBackendCreateQcowCreate(virConnectPtr conn, virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) { char size[100]; const char *imgargv[4]; @@ -422,7 +425,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn, return 0; } -createFile +virStorageBackendBuildVolFrom virStorageBackendFSImageToolTypeToFunc(virConnectPtr conn, int tool_type) { switch (tool_type) { @@ -462,7 +465,7 @@ virStorageBackendFindFSImageTool(char **tool) return tool_type; } -createFile +virStorageBackendBuildVolFrom virStorageBackendGetBuildVolFromFunction(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol) diff --git a/src/storage_backend.h b/src/storage_backend.h index ec75f41..a948c01 100644 --- a/src/storage_backend.h +++ b/src/storage_backend.h @@ -40,21 +40,19 @@ typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObj typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, unsigned int flags); -typedef int (*createFile)(virConnectPtr conn, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol); - /* File creation/cloning functions used for cloning between backends */ int virStorageBackendCreateRaw(virConnectPtr conn, virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol); -createFile + virStorageVolDefPtr inputvol, + unsigned int flags); +virStorageBackendBuildVolFrom virStorageBackendGetBuildVolFromFunction(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol); int virStorageBackendFindFSImageTool(char **tool); -createFile virStorageBackendFSImageToolTypeToFunc(virConnectPtr conn, - int tool_type); +virStorageBackendBuildVolFrom +virStorageBackendFSImageToolTypeToFunc(virConnectPtr conn, + int tool_type); diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 3b2af59..8cfc462 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -1008,7 +1008,8 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, static int createFileDir(virConnectPtr conn, virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) { + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) { if (inputvol) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", @@ -1032,7 +1033,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, virStorageVolDefPtr inputvol) { int fd; - createFile create_func; + virStorageBackendBuildVolFrom create_func; int tool_type; if (inputvol) { @@ -1056,7 +1057,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, return -1; } - if (create_func(conn, vol, inputvol) < 0) + if (create_func(conn, vol, inputvol, 0) < 0) return -1; if ((fd = open(vol->target.path, O_RDONLY)) < 0) { -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:47:01PM -0400, Cole Robinson wrote:
Have storage building functions be definitions of virStorageBackendBuildVolFrom: we will need to do this in the future anyways if we ever support the flags attribute. --- src/storage_backend.c | 13 ++++++++----- src/storage_backend.h | 14 ++++++-------- src/storage_backend_fs.c | 7 ++++--- 3 files changed, 18 insertions(+), 16 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
The CreateRaw function has some 'file' only assumptions, so break the agnostic cloning bits to a separate function. --- src/storage_backend.c | 160 ++++++++++++++++++++++++++++--------------------- 1 files changed, 92 insertions(+), 68 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index aea6105..868c297 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -101,6 +101,95 @@ enum { TOOL_QCOW_CREATE, }; +static int +virStorageBackendCopyToFD(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + int fd, + unsigned long long *total) +{ + int inputfd = -1; + int amtread = -1; + int ret = -1; + unsigned long long remain; + size_t bytes = 1024 * 1024; + char zerobuf[512]; + char *buf = NULL; + + if (inputvol) { + if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { + virReportSystemError(conn, errno, + _("could not open input path '%s'"), + inputvol->target.path); + goto cleanup; + } + } + + bzero(&zerobuf, sizeof(zerobuf)); + + if (VIR_ALLOC_N(buf, bytes) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + remain = *total; + + while (amtread != 0) { + int amtleft; + + if (remain < bytes) + bytes = remain; + + if ((amtread = saferead(inputfd, buf, bytes)) < 0) { + virReportSystemError(conn, errno, + _("failed reading from file '%s'"), + inputvol->target.path); + goto cleanup; + } + remain -= amtread; + + /* Loop over amt read in 512 byte increments, looking for sparse + * blocks */ + amtleft = amtread; + do { + int interval = ((512 > amtleft) ? amtleft : 512); + int offset = amtread - amtleft; + + if (memcmp(buf+offset, zerobuf, interval) == 0) { + if (lseek(fd, interval, SEEK_CUR) < 0) { + virReportSystemError(conn, errno, + _("cannot extend file '%s'"), + vol->target.path); + goto cleanup; + } + } else if (safewrite(fd, buf+offset, interval) < 0) { + virReportSystemError(conn, errno, + _("failed writing to file '%s'"), + vol->target.path); + goto cleanup; + + } + } while ((amtleft -= 512) > 0); + } + + if (inputfd != -1 && close(inputfd) < 0) { + virReportSystemError(conn, errno, + _("cannot close file '%s'"), + inputvol->target.path); + goto cleanup; + } + inputfd = -1; + + *total -= remain; + ret = 0; + +cleanup: + if (inputfd != -1) + close(inputfd); + + return ret; +} + int virStorageBackendCreateRaw(virConnectPtr conn, virStorageVolDefPtr vol, @@ -108,7 +197,6 @@ virStorageBackendCreateRaw(virConnectPtr conn, unsigned int flags ATTRIBUTE_UNUSED) { int fd = -1; - int inputfd = -1; int ret = -1; unsigned long long remain; char *buf = NULL; @@ -121,15 +209,6 @@ virStorageBackendCreateRaw(virConnectPtr conn, goto cleanup; } - if (inputvol) { - if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { - virReportSystemError(conn, errno, - _("could not open input path '%s'"), - inputvol->target.path); - goto cleanup; - } - } - /* Seek to the final size, so the capacity is available upfront * for progress reporting */ if (ftruncate(fd, vol->capacity) < 0) { @@ -141,55 +220,10 @@ virStorageBackendCreateRaw(virConnectPtr conn, remain = vol->allocation; - if (inputfd != -1) { - int amtread = -1; - size_t bytes = 1024 * 1024; - char zerobuf[512]; - - bzero(&zerobuf, sizeof(zerobuf)); - - if (VIR_ALLOC_N(buf, bytes) < 0) { - virReportOOMError(conn); + if (inputvol) { + int res = virStorageBackendCopyToFD(conn, vol, inputvol, fd, &remain); + if (res < 0) goto cleanup; - } - - while (amtread != 0) { - int amtleft; - - if (remain < bytes) - bytes = remain; - - if ((amtread = saferead(inputfd, buf, bytes)) < 0) { - virReportSystemError(conn, errno, - _("failed reading from file '%s'"), - inputvol->target.path); - goto cleanup; - } - remain -= amtread; - - /* Loop over amt read in 512 byte increments, looking for sparse - * blocks */ - amtleft = amtread; - do { - int interval = ((512 > amtleft) ? amtleft : 512); - int offset = amtread - amtleft; - - if (memcmp(buf+offset, zerobuf, interval) == 0) { - if (lseek(fd, interval, SEEK_CUR) < 0) { - virReportSystemError(conn, errno, - _("cannot extend file '%s'"), - vol->target.path); - goto cleanup; - } - } else if (safewrite(fd, buf+offset, interval) < 0) { - virReportSystemError(conn, errno, - _("failed writing to file '%s'"), - vol->target.path); - goto cleanup; - - } - } while ((amtleft -= 512) > 0); - } } /* Pre-allocate any data if requested */ @@ -220,20 +254,10 @@ virStorageBackendCreateRaw(virConnectPtr conn, } fd = -1; - if (inputfd != -1 && close(inputfd) < 0) { - virReportSystemError(conn, errno, - _("cannot close file '%s'"), - inputvol->target.path); - goto cleanup; - } - inputfd = -1; - ret = 0; cleanup: if (fd != -1) close(fd); - if (inputfd != -1) - close(inputfd); VIR_FREE(buf); return ret; -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:47:02PM -0400, Cole Robinson wrote:
The CreateRaw function has some 'file' only assumptions, so break the agnostic cloning bits to a separate function. --- src/storage_backend.c | 160 ++++++++++++++++++++++++++++--------------------- 1 files changed, 92 insertions(+), 68 deletions(-)
ACK. One thing occurs to me for the future. We should probably use the fallocate() call to attempt to pre-allocate the target volume and then copy the data across afterwards. fallocate() will only succeeed on filesystems supporting extents (eg ext4), so it is guareteened always fast (unlike posix_fallocate() which falls back to writing zeros. Anyway, by attempting fallocate() before cloning we get good extents allocation for filesystems where this matters. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Add a 'CreateBlockFrom' in the global storage_backend, which sets up the destination block device: CopyFromFD does the rest of the cloning. --- src/storage_backend.c | 46 +++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 45 insertions(+), 1 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index 868c297..37c227f 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -190,6 +190,47 @@ cleanup: return ret; } +static int +virStorageBackendCreateBlockFrom(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int fd = -1; + int ret = -1; + unsigned long long remain; + + if ((fd = open(vol->target.path, O_RDWR)) < 0) { + virReportSystemError(conn, errno, + _("cannot create path '%s'"), + vol->target.path); + goto cleanup; + } + + remain = vol->allocation; + + if (inputvol) { + int res = virStorageBackendCopyToFD(conn, vol, inputvol, fd, &remain); + if (res < 0) + goto cleanup; + } + + if (close(fd) < 0) { + virReportSystemError(conn, errno, + _("cannot close file '%s'"), + vol->target.path); + goto cleanup; + } + fd = -1; + + ret = 0; +cleanup: + if (fd != -1) + close(fd); + + return ret; +} + int virStorageBackendCreateRaw(virConnectPtr conn, virStorageVolDefPtr vol, @@ -517,7 +558,10 @@ virStorageBackendGetBuildVolFromFunction(virConnectPtr conn, return virStorageBackendFSImageToolTypeToFunc(conn, tool_type); } - return virStorageBackendCreateRaw; + if (vol->type == VIR_STORAGE_VOL_BLOCK) + return virStorageBackendCreateBlockFrom; + else + return virStorageBackendCreateRaw; } #if defined(UDEVADM) || defined(UDEVSETTLE) -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:47:03PM -0400, Cole Robinson wrote:
Add a 'CreateBlockFrom' in the global storage_backend, which sets up the destination block device: CopyFromFD does the rest of the cloning. --- src/storage_backend.c | 46 +++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 45 insertions(+), 1 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
We don't gain any space savings, so skip the detection to speed up the cloning operation. --- src/storage_backend.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index 37c227f..b5e4ac7 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -106,7 +106,8 @@ virStorageBackendCopyToFD(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, int fd, - unsigned long long *total) + unsigned long long *total, + int is_dest_file) { int inputfd = -1; int amtread = -1; @@ -155,7 +156,7 @@ virStorageBackendCopyToFD(virConnectPtr conn, int interval = ((512 > amtleft) ? amtleft : 512); int offset = amtread - amtleft; - if (memcmp(buf+offset, zerobuf, interval) == 0) { + if (is_dest_file && memcmp(buf+offset, zerobuf, interval) == 0) { if (lseek(fd, interval, SEEK_CUR) < 0) { virReportSystemError(conn, errno, _("cannot extend file '%s'"), @@ -210,7 +211,8 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn, remain = vol->allocation; if (inputvol) { - int res = virStorageBackendCopyToFD(conn, vol, inputvol, fd, &remain); + int res = virStorageBackendCopyToFD(conn, vol, inputvol, + fd, &remain, 0); if (res < 0) goto cleanup; } @@ -262,7 +264,8 @@ virStorageBackendCreateRaw(virConnectPtr conn, remain = vol->allocation; if (inputvol) { - int res = virStorageBackendCopyToFD(conn, vol, inputvol, fd, &remain); + int res = virStorageBackendCopyToFD(conn, vol, inputvol, + fd, &remain, 1); if (res < 0) goto cleanup; } -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:47:04PM -0400, Cole Robinson wrote:
We don't gain any space savings, so skip the detection to speed up the cloning operation. --- src/storage_backend.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
With the previous refactoring, this is a simple process, since the global 'CreateBlockFrom' in storage_backend does all the work. --- src/storage_backend_disk.c | 15 +++++++++++++++ src/storage_backend_logical.c | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 0 deletions(-) diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index eb694a7..1eb3eac 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -587,6 +587,20 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, return 0; } +static int +virStorageBackendDiskBuildVolFrom(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags) +{ + virStorageBackendBuildVolFrom build_func; + + build_func = virStorageBackendGetBuildVolFromFunction(conn, vol, inputvol); + if (!build_func) + return -1; + + return build_func(conn, vol, inputvol, flags); +} static int virStorageBackendDiskDeleteVol(virConnectPtr conn, @@ -655,4 +669,5 @@ virStorageBackend virStorageBackendDisk = { .createVol = virStorageBackendDiskCreateVol, .deleteVol = virStorageBackendDiskDeleteVol, + .buildVolFrom = virStorageBackendDiskBuildVolFrom, }; diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c index 1bd00d4..6c123ae 100644 --- a/src/storage_backend_logical.c +++ b/src/storage_backend_logical.c @@ -654,6 +654,21 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, } static int +virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags) +{ + virStorageBackendBuildVolFrom build_func; + + build_func = virStorageBackendGetBuildVolFromFunction(conn, vol, inputvol); + if (!build_func) + return -1; + + return build_func(conn, vol, inputvol, flags); +} + +static int virStorageBackendLogicalDeleteVol(virConnectPtr conn, virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol, @@ -679,6 +694,8 @@ virStorageBackend virStorageBackendLogical = { .refreshPool = virStorageBackendLogicalRefreshPool, .stopPool = virStorageBackendLogicalStopPool, .deletePool = virStorageBackendLogicalDeletePool, + .buildVol = NULL, + .buildVolFrom = virStorageBackendLogicalBuildVolFrom, .createVol = virStorageBackendLogicalCreateVol, .deleteVol = virStorageBackendLogicalDeleteVol, }; -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:47:05PM -0400, Cole Robinson wrote:
With the previous refactoring, this is a simple process, since the global 'CreateBlockFrom' in storage_backend does all the work. --- src/storage_backend_disk.c | 15 +++++++++++++++ src/storage_backend_logical.c | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 0 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
We need to unlock the first pool before looking up the second, since the search locks every pool it checks. --- src/storage_driver.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/storage_driver.c b/src/storage_driver.c index c27534c..e9ecb20 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -1327,9 +1327,11 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, storageDriverLock(driver); pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - if (diffpool) + if (diffpool) { + virStoragePoolObjUnlock(pool); origpool = virStoragePoolObjFindByName(&driver->pools, vobj->pool); - else + virStoragePoolObjLock(pool); + } else origpool = pool; storageDriverUnlock(driver); -- 1.6.0.6
On Fri, Jul 10, 2009 at 04:47:06PM -0400, Cole Robinson wrote:
We need to unlock the first pool before looking up the second, since the search locks every pool it checks. --- src/storage_driver.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
ACK, though this patch should surely be first in the series so you can test & bisect safely without deadlocking Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Jul 10, 2009 at 04:46:57PM -0400, Cole Robinson wrote:
This following series implements cloning for LVM and disk backends. Most of the functionality is already here, it just needed some reorganization to be accessible for every backend.
I verified the following scenarios produced a bootable image:
- Clone within a disk pool - Clone within a logical pool - Clone a raw file to a disk pool - Clone a disk pool to a logical pool
Cole Robinson (9): storage: Refactor FS backend 'create' function choosing. storage: Always assume we are tracking alloc progress in FS backend storage: Move most of the FS creation functions to common backend. storage: cleanup: do away with 'createFile' storage: Break out actual raw cloning to separate function. storage: Implement 'CreateBlockFrom' helper. storage: Don't try sparse detection if writing to block device. storage: Implement CreateVolFrom for logical and disk backend. storage: Fix deadlock when cloning across pools.
Sounds good, I went through the set of patches and didn't detect anything, Dan's review is more thorough ! ACK for me, thanks, that will improve users experience I'm sure :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Fri, Jul 10, 2009 at 04:46:57PM -0400, Cole Robinson wrote:
This following series implements cloning for LVM and disk backends. Most of the functionality is already here, it just needed some reorganization to be accessible for every backend.
I verified the following scenarios produced a bootable image:
- Clone within a disk pool - Clone within a logical pool - Clone a raw file to a disk pool - Clone a disk pool to a logical pool
Cole Robinson (9): storage: Refactor FS backend 'create' function choosing. storage: Always assume we are tracking alloc progress in FS backend storage: Move most of the FS creation functions to common backend. storage: cleanup: do away with 'createFile' storage: Break out actual raw cloning to separate function. storage: Implement 'CreateBlockFrom' helper. storage: Don't try sparse detection if writing to block device. storage: Implement CreateVolFrom for logical and disk backend. storage: Fix deadlock when cloning across pools.
Sounds good, I went through the set of patches and didn't detect anything, Dan's review is more thorough !
ACK for me,
thanks, that will improve users experience I'm sure :-)
Thanks, pushed now (with patch 2 dropped). - Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard