[libvirt] [PATCH 1/2] logical: Use correct syntax for thin/sparse pool creation
John Ferlan
jferlan at redhat.com
Fri Jan 22 21:54:42 UTC 2016
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 at 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
>
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;
}
>>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
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
More information about the libvir-list
mailing list