On 01/12/2016 11:03 AM, Joe Harvell wrote: I haven't forgotten, just have been sidetracked ;-)... I also wanted to "try it out" for myself (a visual learner)... The end result is an interesting way to recognize an existing "thin" volume in the volume group; however, I still believe we need a way to add/delete "thin" volumes in "thin" lv pools. Today all that would be created is a "snapshot" volume. If I wanted a real "thin" volume then a thinpool needs to be generated and the thin volume comes from that pool (that's how I understand it at least). The referenced patch generated a 1-to-1 mapping of thin volume and thin pool (although I may have got the -L and -V parameters wrong). If there's a better way, then please submit some patches for that. Feel free to use those old patches as a base. It may be that we need to create a new switch/flag when the volume is created in order to signify that a thinpool is desired as opposed to the option those patches took as "defaulting" to that. Since you included your patch as an attachment - my reply-all was less than happy to include it. I cut-n-paste what I have and make some comments after ">>". I also have more details afterwards... diff -ur a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c --- a/src/storage/storage_backend_logical.c 2015-10-29 01:35:32.000000000 -0500 +++ b/src/storage/storage_backend_logical.c 2016-01-11 10:22:36.807282623 -0600 @@ -64,8 +64,6 @@ } -#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" - struct virStorageBackendLogicalPoolVolData { virStoragePoolObjPtr pool; virStorageVolDefPtr vol; @@ -165,13 +163,11 @@ VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) goto cleanup; - nextents = 1; - if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) { - if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed volume extent stripes value")); - goto cleanup; - } + nextents = 0; + if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent stripes value")); + goto cleanup; } (always)... Same for "thin-pool"? It would be "1" or more for "striped" (although 1 doesn't make sense)... Similarly for "mirror"? I see it will be "0" for "thin". Are there other instances of "0" that would be important to note? I think this could be a "separate" patch unrelated the rest of the changes here. Does this also mean that getting the 'segtype' is unnecessary? Can (or should) it be removed? I'm still unclear when it is 0, then what does that mean for the following lines? The REALLOC, length, size, target.allocation, and device (group[3]) regex/parsing? In the patch I had posted, I just jumped around it (IOW: I punted seeing as it wasn't important nor was it listed by libvirt). /* Finally fill in extents information */ @@ -227,14 +223,15 @@ goto cleanup; } - err = regexec(reg, groups[3], nvars, vars, 0); - regfree(reg); - if (err != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed volume extent devices value")); - goto cleanup; + if(groups[3] != NULL && *groups[3] != '\0') { + err = regexec(reg, groups[3], nvars, vars, 0); + regfree(reg); + if (err != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent devices value")); + goto cleanup; + } "##" (when [4] is "thin"). In any case, if it's either NULL or "", then all the code adjusting the extents, setting up the regex, reg, etc. is essentially unnecessary (hence why I had skipped it in my code). What I do see though is an opportunity to move all that devices parsing code into a helper. Then from there make some decisions on whether to call based on the value of 'nextents'. I would "think" (assume) that if nextents == 0, then devices (or groups[3] would be empty). Is that a fair assumption? } - p = groups[3]; /* vars[0] is skipped */ @@ -311,7 +308,8 @@ * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" +// name orig uuid devs sgtype stripes segsz vgextsz sz lvattr + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" for field [3] (e.g. 'devices') where you're changing from "(\\S+)" to "(\\S*)". The "*" means match zero or more while "+" means match at least one. Would you consider this "related" to the nextents (eg 'stripes') value of 0? That is - I'm assuming a "*" which returns 0, means 'stripes == 0'. If so, then choosing to not parse devices based on nextents == 0 would mean the prior change related to groups[3] != NULL and *groups[3] != '\0' is then unnecessary, true? }; int vars[] = { 10 ... So while I was reviewing and thinking I generated a set of patches and ran some basic tests. I'm still concerned about the thin/thinpool stuff, but interestingly enough I can "see" the thin volume in a pool. For example, assume I have a volume group (and pool) "vg_test_thin" (it's a 50M iSCSI volume for which I did a pvcreate and vgcreate). If I run the following: # lvcreate --type thin-pool -L 20M \ --thinpool thinpool_lv_test_thin vg_test_thin # lvcreate --name lv_test_thin \ --thin vg_test_thin/thinpool_lv_test_thin \ --type thin -V 40M Then refresh my "vg_test_thin" libvirt pool, I can see a 40M "lv_test_thin" (both capacity and allocation have 40M - regardless what value I use for -V, the capacity and allocation remain whatever was provided). I guess I partially at least expected the model where allocation shows up as smaller (e.g. is what's presented to the guest), but allowed to go up to capacity (or do I have that backwards - it's Friday afternoon ;-)). IIRC, for these thin pool/volume's when the actual storage on the volume goes above 20M (eg. the pool -L value), then the pool goes 'inactive' until the pool is extended. That too could be concerning to a libvirt and lv novice. The other issue is how to handle/avoid the vol-wipe problem for thin volumes. I'll post the patches in a bit so you (and others) can have a look. John