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@redhat.com> --- 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

Use the output of the 'lvs' command in order to determine the current allocation value for logical volumes. The capacity value for these pools can still be obtained via the existing methodology to lseek to the end of the block file in virStorageBackendUpdateVolTargetInfoFD. The real need is for the thin logical volumes where the thin pool keeps track of the capacity and the thin logical volume keeps track of the allocation value. In the future someone could add a fetch for the 'data_percent' in order to determine the percentage of data used for the allocated space. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 97 +++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 45df38c..8b7325b 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -904,6 +904,102 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, return -1; } +struct virStorageBackendLogicalVolRefreshData { + virStorageVolDefPtr vol; + bool alloc_sent; +}; + +static int +virStorageBackendLogicalRefreshVolFunc(char **const groups, + void *opaque) +{ + struct virStorageBackendLogicalVolRefreshData *data = opaque; + virStorageVolDefPtr vol = data->vol; + + if (data->alloc_sent) { + /* The capacity of the thin logical volume is kept in the 'lv_size' + * of the pool which will be sent when the RefreshVol code determines + * that this is a thin logical volume + */ + if (virStrToLong_ull(groups[0], NULL, 10, &vol->target.capacity) < 0) + return -1; + } else { + if (virStrToLong_ull(groups[0], NULL, 10, &vol->target.allocation) < 0) + return -1; + data->alloc_sent = true; + } + + return 0; +} + +static int +virStorageBackendLogicalRefreshVol(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + /* + * # lvs --noheadings --units b --unbuffered --nosuffix \ + * --options "lv_size" VGNAME/LVNAME [VGNAME/thinpool_LVNAME] + * + * Pull out lv_size for volume capacity and if necessary pull + * out thinpool size for allocation. The output will either be + * one or two lines. The first line will be the capacity value. + * While, if there is a thin volume involved, the second line will + * be the allocation value for the thin lv from the thin pool + */ + const char *regexes[] = { + "^\\s*([0-9]+)\\s*$" + }; + int vars[] = { + 1 + }; + virCommandPtr cmd = NULL; + int ret = -1; + struct virStorageBackendLogicalVolRefreshData cbdata = { + .vol = vol, + .alloc_sent = false, + }; + + cmd = virCommandNewArgList(LVS, + "--noheadings", + "--units", "b", + "--unbuffered", + "--nosuffix", + "--options", "lv_size", + NULL); + virCommandAddArgFormat(cmd, "%s/%s", + pool->def->source.name, + vol->name); + + /* If this is a thin volume, let's add the fetch of the thin volume size + * This will gernate two lines of output, which we'll handle via the + * alloc_sent boolean - this is where the pool capacity is stored from + * the -V during the lvcreate process + */ + if (vol->target.thinVolume) + virCommandAddArgFormat(cmd, "%s/thinpool_%s", + pool->def->source.name, + vol->name); + + /* Now get allocation of the logical volume and perhaps the capacity + * of the thin volume + */ + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virStorageBackendLogicalRefreshVolFunc, + &cbdata, + "lvs") < 0) + goto cleanup; + + ret = 0; + + cleanup: + virCommandFree(cmd); + return ret; +} + static int virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -961,6 +1057,7 @@ virStorageBackend virStorageBackendLogical = { .buildVol = NULL, .buildVolFrom = virStorageBackendLogicalBuildVolFrom, .createVol = virStorageBackendLogicalCreateVol, + .refreshVol = virStorageBackendLogicalRefreshVol, .deleteVol = virStorageBackendLogicalDeleteVol, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, -- 1.9.3