On 01/12/2016 11:03 AM, Joe Harvell wrote:

Le 11/01/2016 17:32, John Ferlan a écrit :

...

On 01/11/2016 12:50 PM, Joe Harvell wrote:

...

...

John/Jan:

I actually made a patch similar to this and was about to submit it for review. Now I see there is a more thorough version here already submitted just over a year ago. I don't see any evidence of this in the git repo. Are there any plans to commit it?

--- Joe Harvell

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list I was referring to John's patch posted on 12 Dec 2014.

For those that don't keep that much history in their mailbox...

http://www.redhat.com/archives/libvir-list/2014-December/msg00706.html

There's also a related bz:

https://bugzilla.redhat.com/show_bug.cgi?id=1060287

So from my perspective, this has languished in one of my local git branches. I got busy with other things and this has never really surfaced to anywhere near the top of my todo list.

John

That's what I suspected. If the testing part is why it's languishing, here is a simpler patch with less functionality. This patch just makes Thin LVs and Snapshot Thin LVs (once you activate them) detectable by libvirt. Would it be worth me submitting this as a patch?

--- Joe

...

So, it's a fair assumption that this field will be "1" for "linear" (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

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; } 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; + }

...

So the "if" you're showing means we could be passed #""#? I can see "##" (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? } -

...

Spurrious change that shouldn't be made.

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*$"

...

To make it more obvious for the sight challenged - the change here is 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