During virStorageBackendDiskCreateVol if virStorageBackendDiskReadPartitions fails, then we were leaving with an error and a partition on the disk for which there was no corresponding volume and used space on the disk which could be reclaimable through direct parted activity. On a subsequent restart, reload, or refresh the volume may magically appear too. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_disk.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 60f8393..0c4126a 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -654,6 +654,13 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); + if (!vol->target.path) { + virReportError(VIR_ERR_INVALID_ARG, + _("volume target path empty for source path '%s'"), + pool->def->source.devices[0].path); + return -1; + } + if (virFileResolveLink(vol->target.path, &devpath) < 0) { virReportSystemError(errno, _("Couldn't read volume target path '%s'"), @@ -709,7 +716,7 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, static int -virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendDiskCreateVol(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { @@ -756,8 +763,16 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_FREE(vol->target.path); /* Fetch actual extent info, generate key */ - if (virStorageBackendDiskReadPartitions(pool, vol) < 0) + if (virStorageBackendDiskReadPartitions(pool, vol) < 0) { + /* Best effort to remove the partition. Ignore any errors + * since we could be calling this with vol->target.path == NULL + */ + virErrorPtr save_err = virSaveLastError(); + ignore_value(virStorageBackendDiskDeleteVol(conn, pool, vol, 0)); + virSetError(save_err); + virFreeError(save_err); goto cleanup; + } res = 0; -- 2.1.0

During virStorageBackendDiskMakeDataVol processing, if we find an extended partition, then handle it specially when updating the capacity/allocation rather than calling virStorageBackendUpdateVolInfo. As it turns out, once a logical partition exists, any attempt to refresh the pool or after libvirtd restart/reload will result in a failure to open the extended partition device resulting in the inability to start the pool. The downside to this is we will lose the <permissions> and <timestamps> for the extended partition upon subsequent restart, refresh, reload since the stat() in virStorageBackendUpdateVolTargetInfoFD will not be called. However, since it's really only a container and shouldn't directly be used for storage that seems reasonable. Therefore, only use the existing code that already had a comment about getting the allocation wrong for extended partitions for just the setting of the extended partition data. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend.c | 4 ++++ src/storage/storage_backend_disk.c | 32 +++++++++++++++++++++++--------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b990a82..d7bccfe 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1385,6 +1385,10 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, VIR_WARN("ignoring missing file '%s'", path); return -2; } + if (errno == ENXIO && noerror) { + VIR_WARN("ignoring missing fifo '%s'", path); + return -2; + } virReportSystemError(errno, _("cannot open volume '%s'"), path); return -1; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 31b6025..233e293 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -110,11 +110,6 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, return -1; } - /* Refresh allocation/capacity/perms */ - if (virStorageBackendUpdateVolInfo(vol, true, false, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) - return -1; - /* set partition type */ if (STREQ(groups[1], "normal")) vol->source.partType = VIR_STORAGE_VOL_DISK_TYPE_PRIMARY; @@ -127,10 +122,29 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, vol->type = VIR_STORAGE_VOL_BLOCK; - /* The above gets allocation wrong for - * extended partitions, so overwrite it */ - vol->target.allocation = vol->target.capacity = - (vol->source.extents[0].end - vol->source.extents[0].start); + /* Refresh allocation/capacity/perms + * + * For an extended partition, virStorageBackendUpdateVolInfo will + * return incorrect values for allocation and capacity, so use the + * extent information captured above instead. + * + * Also once a logical partition exists or another primary partition + * after an extended partition is created an open on the extended + * partition will fail, so pass the NOERROR flag and only error if a + * -1 was returned indicating some other error than an open error. + */ + if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { + if (virStorageBackendUpdateVolInfo(vol, true, false, + VIR_STORAGE_VOL_OPEN_DEFAULT | + VIR_STORAGE_VOL_OPEN_NOERROR) == -1) + return -1; + vol->target.allocation = vol->target.capacity = + (vol->source.extents[0].end - vol->source.extents[0].start); + } else { + if (virStorageBackendUpdateVolInfo(vol, true, false, + VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + return -1; + } if (STRNEQ(groups[2], "metadata")) pool->def->allocation += vol->target.allocation; -- 2.1.0

https://bugzilla.redhat.com/show_bug.cgi?id=1138516 If the provided volume name doesn't match what parted generated as the partition name, then return a failure. Update virsh.pod and formatstorage.html.in to describe the 'name' restriction for disk pools as well as the usage of the <target>'s <format type='value'>. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatstorage.html.in | 12 ++++++++++-- src/storage/storage_backend_disk.c | 30 ++++++++++++++++++++++++------ tools/virsh.pod | 17 ++++++++++++++--- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 933268c..d2e2bb8 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -463,7 +463,13 @@ <dl> <dt><code>name</code></dt> <dd>Providing a name for the volume which is unique to the pool. - This is mandatory when defining a volume. <span class="since">Since 0.4.1</span></dd> + This is mandatory when defining a volume. For a disk pool, the + name must be combination of the <code>source</code> device path + device and next partition number to be created. For example, if + the <code>source</code> device path is /dev/sdb and there are no + partitions on the disk, then the name must be sdb1 with the next + name being sdb2 and so on. + <span class="since">Since 0.4.1</span></dd> <dt><code>key</code></dt> <dd>Providing an identifier for the volume which identifies a single volume. In some cases it's possible to have two distinct keys @@ -553,7 +559,9 @@ <span class="since">Since 0.4.1</span></dd> <dt><code>format</code></dt> <dd>Provides information about the pool specific volume format. - For disk pools it will provide the partition type. For filesystem + For disk pools it will provide the partition table format type, but is + not preserved after a pool refresh or libvirtd restart. Use extended + in order to create an extended disk extent partition. For filesystem or directory pools it will provide the file format type, eg cow, qcow, vmdk, raw. If omitted when creating a volume, the pool's default format will be used. The actual format is specified via diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 300aab3..9f4d76a 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -47,16 +47,23 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, char **const groups, virStorageVolDefPtr vol) { - char *tmp, *devpath; + char *tmp, *devpath, *partname; + + /* Prepended path will be same for all partitions, so we can + * strip the path to form a reasonable pool-unique name + */ + if ((tmp = strrchr(groups[0], '/'))) + partname = tmp + 1; + else + partname = groups[0]; if (vol == NULL) { + /* This is typically a reload/restart/refresh path where + * we're discovering the existing partitions for the pool + */ if (VIR_ALLOC(vol) < 0) return -1; - /* Prepended path will be same for all partitions, so we can - * strip the path to form a reasonable pool-unique name - */ - tmp = strrchr(groups[0], '/'); - if (VIR_STRDUP(vol->name, tmp ? tmp + 1 : groups[0]) < 0 || + if (VIR_STRDUP(vol->name, partname) < 0 || VIR_APPEND_ELEMENT_COPY(pool->volumes.objs, pool->volumes.count, vol) < 0) { virStorageVolDefFree(vol); @@ -80,6 +87,17 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, return -1; } + /* Enforce provided vol->name is the same as what parted created. + * We do this after filling target.path so that we have a chance at + * deleting the partition with this failure from CreateVol path + */ + if (STRNEQ(vol->name, partname)) { + virReportError(VIR_ERR_INVALID_ARG, + _("invalid partition name '%s', expected '%s'"), + vol->name, partname); + return -1; + } + if (vol->key == NULL) { /* XXX base off a unique key of the underlying disk */ if (VIR_STRDUP(vol->key, vol->target.path) < 0) diff --git a/tools/virsh.pod b/tools/virsh.pod index abe80c2..1591341 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2983,7 +2983,11 @@ Create and start a pool object from the XML I<file>. Create and start a pool object I<name> from the raw parameters. If I<--print-xml> is specified, then print the XML of the pool object without creating the pool. Otherwise, the pool has the specified -I<type>. +I<type>. When using B<pool-create-as> for a pool of I<type> "disk", +the existing partitions found on the I<--source-dev path> will be used +to populate the disk pool. Therefore, it is suggested to use +B<pool-define-as> and B<pool-build> with the I<--overwrite> in order +to properly initialize the disk pool. [I<--source-host hostname>] provides the source hostname for pools backed by storage from a remote server (pool types netfs, iscsi, rbd, sheepdog, @@ -3175,13 +3179,20 @@ I<vol-name-or-key-or-path>] [I<--backing-vol-format> I<string>] Create a volume from a set of arguments. I<pool-or-uuid> is the name or UUID of the storage pool to create the volume in. -I<name> is the name of the new volume. +I<name> is the name of the new volume. For a disk pool, this must match the +partition name as determined from the pool's source device path and the next +available partition. For example, a source device path of /dev/sdb and there +are no partitions on the disk, then the name must be sdb1 with the next +name being sdb2 and so on. I<capacity> is the size of the volume to be created, as a scaled integer (see B<NOTES> above), defaulting to bytes if there is no suffix. I<--allocation> I<size> is the initial size to be allocated in the volume, also as a scaled integer defaulting to bytes. I<--format> I<string> is used in file based storage pools to specify the volume -file format to use; raw, bochs, qcow, qcow2, vmdk, qed. +file format to use; raw, bochs, qcow, qcow2, vmdk, qed. Use extended for disk +storage pools in order to create an extended partition (other values are +validity checked but not preserved when libvirtd is restarted or the pool +is refreshed). I<--backing-vol> I<vol-name-or-key-or-path> is the source backing volume to be used if taking a snapshot of an existing volume. I<--backing-vol-format> I<string> is the format of the snapshot backing volume; -- 2.1.0