On 12/12/2014 10:14 PM, John Ferlan wrote:
> 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.
Adding --type snapshot would be the bugfix here.
Adding support for thin pools sounds like a new feature to me.
> 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(-)
> @@ -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.
We should be optimizing for readability.
Repeating the run commands would take up about the same space, but with less
indentation and effort required to understand the flow.
Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list