Commit id '1ffc78b5' was supposed to support for thin logical volumes;
however, instead it added support for thin snapshot volumes. This worked
fine for a few releases of LVM; however, more recent versions (not sure
exactly which one) made a differentiation between a thin snapshot volume
and a thin volume in a thin pool. Furthermore, the creation command
used by libvirt:
lvcreate --name <name> -L <allocation> --virtualsize <capacity> <VGname>
that used to create a thin snapshot volume will now create a thin volume
and a thin pool which libvirt doesn't know how to parse, for example:
# lvcreate --name test -L 2M -V 5M lvm_test
Rounding up size to full physical extent 4.00 MiB
Rounding up size to full physical extent 8.00 MiB
Logical volume "test" created.
# lvs lvm_test
LV VG Attr LSize Pool Origin Data% Meta% Move Log Cpy%Sync Convert
lvol1 lvm_test twi-a-tz-- 4.00m 0.00 0.98
test lvm_test Vwi-a-tz-- 8.00m lvol1 0.00
compared to the former code which had the following:
LV VG Attr LSize Pool Origin Data% Move Log Cpy%Sync Convert
test LVM_Test swi-a-s--- 4.00m [test_vorigin] 0.00
When using the virsh vol-create-as or vol-create xmlfile commands, this
will cause libvirt to not find the 'test' volume nor mark it as sparse.
It cannot find since the command used to find/parse returns a thin volume
'test' with no associated device, for example the output is:
lvol1##UgUwkp-fTFP-C0rc-ufue-xrYh-dkPr-FGPFPx#lvol1_tdata(0)#thin-pool#1#4194304#4194304#4194304#twi-a-tz--
test##NcaIoH-4YWJ-QKu3-sJc3-EOcS-goff-cThLIL##thin#0#8388608#4194304#8388608#Vwi-a-tz--
as compared to the former which had the following:
test#[test_vorigin]#Dt5Of3-4WE6-buvw-CWJ4-XOiz-ywOU-YULYw6#/dev/sda3(1300)#linear#1#4194304#4194304#4194304#swi-a-s---
The relevant changes being:
1. The field after the UUID and before "thin" is where it's expected
to return a device now has nothing. This is used in the vol-dumpxml
output; however, it doesn't seem to have other uses
2. The field after "thin" is a count of the number of extents. Which is
assumed to be at least 1 by the LV processing code, but is only ever
checked for a "striped" LV.
3. The first character of the last field is used to determine LV attributes.
A 'V' signifies a "thin Volume", while an 's' signifies a "snapshot"
(and in our usage a thin snapshot).
4. The LSize field in the new view is not the 'real' capacity of the
'test' LV - it is now kept in the pool. Formerly (and for the thin
snapshot), it's managed by LVM (it can be seen via a 'lvs -a' command).
While continuing to use a thin snapshot would be possible by simply changing
the "lvcreate" command to add a "--type snapshot" option, it's not clear
whether that was the desired result and if libvirt's model is the intended
usage for LVM of a thin shapshot. However, going forward the thin volume
support is what the following patch will utilize for new LV's created while
also still supporting the thin snapshots since it's not clear whether there
is a need to convert them and whether it's feasible/desired. Also we have
to support the old format for back-compat reasons, so it just seems safer
to keep things as they are.
This patch will adjust the lvcreate to be a sequence of two commands when
it's determined that the allocation and capacity do not match. First a
lvcreate --type thin-pool -L <allocation> --thinpool thinpool_<name> <VGname>
followed by a
lvcreate --name <name> --thin <VGname>/thinpool_<name> \
--type thin -V <capacity>
For non thin pools, it'll remain as it was:
lvcreate --name <name> -L <capacity> <VGname> (or -s <backingStorePath>)
Additionally, a new flag 'thinVolume' will be set to indicate the type
of volume. This will be essential during delete and will also be useful
for perhaps a new VolumeRefresh option to get the "correct" sizes for
thin volumes.
The volume processing callback (virStorageBackendLogicalMakeVol) will
be changed to handle/recognize the thinVolume. It will ignore the
extents and device 'source'. The effect of this is to have an empty
source in the vol-dumpxml output. Being able to ignore the devices
field also requires a change to the regex since it previously required
something in the field. For non thin volumes that don't have the
device field, the parsing algorithm already handles with a "malformed
volume extent devices value" failure.
The volume deletion code will now have to delete not only the LV, but
the thin pool associated with the volume. NB: If we didn't provide our
own name, LVM would generate one and it's at this point we'd have to
figure that out; otherwise, we'd leave around thin pools in the volume
group and eventually with enough of them, the VG would be needlessly
exhausted.
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/storage/storage_backend_logical.c | 119 +++++++++++++++++++++++++++++-----
src/util/virstoragefile.h | 1 +
2 files changed, 102 insertions(+), 18 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
index 4959985..45df38c 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -140,6 +140,20 @@ virStorageBackendLogicalMakeVol(char **const groups,
if (attrs[0] == 's')
vol->target.sparse = true;
+ /* The initial implementation mistakenly used thin snapshots (the 's')
+ * so we're stuck with back-compat issues dealing with two types of
+ * sparse or thin volumes. We'll mark those that have the 'V' in
+ * the attrs[0] as ones using the correct methodology of adding
+ * "--type thin" and a "--thin thinpool_<name>" as having both
+ * the sparse attribute (thus inhibiting volume wipe functionality)
+ * and the thin attribute indicing more information about the volume
+ * is available from the thin pool.
+ */
+ if (attrs[0] == 'V') {
+ vol->target.sparse = true;
+ vol->target.thinVolume = true;
+ }
+
/* Skips the backingStore of lv created with "--virtualsize",
* its original device "/dev/$vgname/$lvname_vorigin" is
* just for lvm internal use, one should never use it.
@@ -161,10 +175,20 @@ virStorageBackendLogicalMakeVol(char **const groups,
if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
goto cleanup;
+ /* Updates capacity and allocation. The allocation is overwritten
+ * later in the processing of extents
+ */
if (virStorageBackendUpdateVolInfo(vol, true, false,
VIR_STORAGE_VOL_OPEN_DEFAULT) < 0)
goto cleanup;
+ /* Thin LV's are managed by LVM, thus there's no "devices" field and
+ * the extents are managed by the thin pool which we've conveniently
+ * ignored (attrs[0] == 't'), so let's just skip the extents mgmt here.
+ */
+ if (vol->target.thinVolume)
+ goto skip_extents;
+
nextents = 1;
if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) {
if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) {
@@ -269,6 +293,7 @@ virStorageBackendLogicalMakeVol(char **const groups,
vol->source.nextent++;
}
+ skip_extents:
if (is_new_vol &&
VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0)
goto cleanup;
@@ -309,9 +334,10 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
* not a suitable separator (rhbz 470693).
* NB "devices" field has multiple device paths and "," if the volume is
* striped, so "," is not a suitable separator either (rhbz 727474).
+ * NB "devices" field is empty for thin logical volumes (rhbz 1166592)
*/
const char *regexes[] = {
- "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
+ "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
};
int vars[] = {
10
@@ -690,11 +716,12 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn ATTRIBUTE_UNUSED,
static int
virStorageBackendLogicalDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
- virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
+ virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
unsigned int flags)
{
int ret = -1;
+ bool del_pool_try;
virCommandPtr lvchange_cmd = NULL;
virCommandPtr lvremove_cmd = NULL;
@@ -706,14 +733,37 @@ virStorageBackendLogicalDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
lvchange_cmd = virCommandNewArgList(LVCHANGE, "-aln", vol->target.path, NULL);
lvremove_cmd = virCommandNewArgList(LVREMOVE, "-f", vol->target.path, NULL);
- if (virCommandRun(lvremove_cmd, NULL) < 0) {
- if (virCommandRun(lvchange_cmd, NULL) < 0) {
- goto cleanup;
- } else {
- if (virCommandRun(lvremove_cmd, NULL) < 0)
+
+ do {
+ if (virCommandRun(lvremove_cmd, NULL) < 0) {
+ if (virCommandRun(lvchange_cmd, NULL) < 0) {
goto cleanup;
+ } else {
+ if (virCommandRun(lvremove_cmd, NULL) < 0)
+ goto cleanup;
+ }
}
- }
+
+ /* When a thin volume is created, there are two elements added to the
+ * logical volume - the thin volume by name and a thin volume pool.
+ * We need to try to remove both of them - only once though - the
+ * do {} while; is just an optimization to avoid copying the above
+ * run commands again.
+ */
+ del_pool_try = false;
+ if (vol->target.thinVolume) {
+ virCommandFree(lvchange_cmd);
+ virCommandFree(lvremove_cmd);
+ lvchange_cmd = virCommandNewArgList(LVCHANGE, "-aln", NULL);
+ virCommandAddArgFormat(lvchange_cmd, "%s/thinpool_%s",
+ pool->def->source.name, vol->name);
+ lvremove_cmd = virCommandNewArgList(LVREMOVE, "-f", NULL);
+ virCommandAddArgFormat(lvremove_cmd, "%s/thinpool_%s",
+ pool->def->source.name, vol->name);
+ del_pool_try = true;
+ vol->target.thinVolume = false; /* To avoid another attempt */
+ }
+ } while (del_pool_try);
ret = 0;
cleanup:
@@ -750,23 +800,56 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
vol->name) == -1)
return -1;
+ /* Creation of thin/sparse volumes is a two step process (it cannot be
+ * done in one command). First we need to create a thin pool into which
+ * the logical volume will be placed. Rather than having LVM pick a name
+ * for us, we'll go with the volume name prefixed by "thinpool_".
+ * This way when we go to delete/remove the volume - we don't have to
+ * fish around LVM in order to figure out the name created for us and
+ * ensure nothing else is using it.
+ */
+ if (vol->target.capacity != vol->target.allocation) {
+ cmd = virCommandNewArgList(LVCREATE,
+ "--type", "thin-pool",
+ NULL);
+ virCommandAddArg(cmd, "-L");
+ virCommandAddArgFormat(cmd, "%lluK",
+ VIR_DIV_UP(vol->target.allocation
+ ? vol->target.allocation : 1, 1024));
+ virCommandAddArg(cmd, "--thinpool");
+ virCommandAddArgFormat(cmd, "thinpool_%s", vol->name);
+ virCommandAddArg(cmd, pool->def->source.name);
+ if (virCommandRun(cmd, NULL) < 0)
+ goto error;
+
+ virCommandFree(cmd);
+ cmd = NULL;
+ }
+
cmd = virCommandNewArgList(LVCREATE,
"--name", vol->name,
NULL);
- virCommandAddArg(cmd, "-L");
if (vol->target.capacity != vol->target.allocation) {
+ virCommandAddArg(cmd, "--thin");
+ virCommandAddArgFormat(cmd, "%s/thinpool_%s",
+ pool->def->source.name, vol->name);
+ virCommandAddArgList(cmd, "--type", "thin", NULL);
+ virCommandAddArg(cmd, "-V");
virCommandAddArgFormat(cmd, "%lluK",
- VIR_DIV_UP(vol->target.allocation
- ? vol->target.allocation : 1, 1024));
- virCommandAddArg(cmd, "--virtualsize");
+ VIR_DIV_UP(vol->target.capacity, 1024));
+
+ /* Set the sparse and thinVolume flags for later use */
vol->target.sparse = true;
+ vol->target.thinVolume = true;
+ } else {
+ virCommandAddArg(cmd, "-L");
+ virCommandAddArgFormat(cmd, "%lluK",
+ VIR_DIV_UP(vol->target.capacity, 1024));
+ if (vol->target.backingStore)
+ virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL);
+ else
+ virCommandAddArg(cmd, pool->def->source.name);
}
- virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity,
- 1024));
- if (vol->target.backingStore)
- virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL);
- else
- virCommandAddArg(cmd, pool->def->source.name);
if (virCommandRun(cmd, NULL) < 0)
goto error;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index e05b843..7522dcd 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -254,6 +254,7 @@ struct _virStorageSource {
char *compat;
bool nocow;
bool sparse;
+ bool thinVolume;
virStoragePermsPtr perms;
virStorageTimestampsPtr timestamps;
--
1.9.3
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list