[libvirt] [PATCH 0/4] Some logical pool/volume changes
This series is a result of looking at Joe's comments : http://www.redhat.com/archives/libvir-list/2016-January/msg00403.html To an old series I posted and has been stuck on the bottom of a todo list regarding adding/using "thin" logical volumes: http://www.redhat.com/archives/libvir-list/2014-December/msg00706.html Joe's patch is refactord into two patches to do essentially the same thing, but add a bit more context/thoughts. The first two patches have no functional changes, just some comment changes and some code motion. The result of Joe's patches is that libvirt can find a thin logical volume if the libvirt pool looking at the containing volume group has a thin logical volume pool. Still to be done is the (cap)ability to create a thin logical volume pool and the thin logical volume(s) in the pool. Just figured it'd be easier to post some patches and to let discussion happen. Joe Harvell (2): logical: Remove need for 'segtype' field logical: Adjust regex for devices John Ferlan (2): logical: Fix comment examples for virStorageBackendLogicalFindLVs logical: Create helper virStorageBackendLogicalParseVolDevice src/storage/storage_backend_logical.c | 237 +++++++++++++++++++--------------- 1 file changed, 130 insertions(+), 107 deletions(-) -- 2.5.0
When commit id '82c1740a' made changes to the output format (changing from using a ',' separator to '#'), the examples in the lvs output from the comments weren't changed. Additionally, the two new fields added ('segtype' and 'stripes') were not included in the output, leaving it well confusing. This patch fixes the sample output, adds a 'striped' example, and makes other comment related adjuments for long line and spacing between followup 'NB' remarks (while I'm there). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index f59684a..76ea00a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -289,24 +289,27 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { /* - * # lvs --separator , --noheadings --units b --unbuffered --nosuffix --options \ - * "lv_name,origin,uuid,devices,seg_size,vg_extent_size,size,lv_attr" VGNAME + * # lvs --separator # --noheadings --units b --unbuffered --nosuffix --options \ + * "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr" VGNAME * - * RootLV,,06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky,/dev/hda2(0),5234491392,33554432,5234491392,-wi-ao - * SwapLV,,oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M,/dev/hda2(156),1040187392,33554432,1040187392,-wi-ao - * Test2,,3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR,/dev/hda2(219),1073741824,33554432,1073741824,owi-a- - * Test3,,UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht,/dev/hda2(251),2181038080,33554432,2181038080,-wi-a- - * Test3,Test2,UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht,/dev/hda2(187),1040187392,33554432,1040187392,swi-a- + * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#linear#1#5234491392#33554432#5234491392#-wi-ao + * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#linear#1#1040187392#33554432#1040187392#-wi-ao + * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#linear#1#1073741824#33554432#1073741824#owi-a- + * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#linear#1#2181038080#33554432#2181038080#-wi-a- + * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#linear#1#1040187392#33554432#1040187392#swi-a- + * test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#/dev/sdc1(10240),/dev/sdd1(0)#striped#2#42949672960#4194304#-wi-a- * * Pull out name, origin, & uuid, device, device extent start #, * segment size, extent size, size, attrs * * NB can be multiple rows per volume if they have many extents * - * NB lvs from some distros (e.g. SLES10 SP2) outputs trailing "," on each line + * NB lvs from some distros (e.g. SLES10 SP2) outputs trailing "," + * on each line * * NB Encrypted logical volumes can print ':' in their name, so it is * 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). */ -- 2.5.0
On Fri, Jan 22, 2016 at 05:21:03PM -0500, John Ferlan wrote:
When commit id '82c1740a' made changes to the output format (changing from using a ',' separator to '#'), the examples in the lvs output from the comments weren't changed.
Additionally, the two new fields added ('segtype' and 'stripes') were not included in the output, leaving it well confusing.
This patch fixes the sample output, adds a 'striped' example, and makes other comment related adjuments for long line and spacing between followup
s/adjuments/adjustments/
'NB' remarks (while I'm there).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
ACK
On 01/26/2016 08:44 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 05:21:03PM -0500, John Ferlan wrote:
When commit id '82c1740a' made changes to the output format (changing from using a ',' separator to '#'), the examples in the lvs output from the comments weren't changed.
Additionally, the two new fields added ('segtype' and 'stripes') were not included in the output, leaving it well confusing.
This patch fixes the sample output, adds a 'striped' example, and makes other comment related adjuments for long line and spacing between followup
s/adjuments/adjustments/
'NB' remarks (while I'm there).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
ACK
OK - One extra adjustment - the copyright date change to 2016 and I'll push this separately before reposting the rest of my changes... John
Create a helper routine in order to parse the 'device' string contained within the generated 'lvs' output string. A future patch would then be able to avoid the code more cleanly Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 186 +++++++++++++++++++--------------- 1 file changed, 104 insertions(+), 82 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 76ea00a..bf67faf 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -72,21 +72,115 @@ struct virStorageBackendLogicalPoolVolData { }; static int -virStorageBackendLogicalMakeVol(char **const groups, - void *opaque) +virStorageBackendLogicalParseVolDevice(virStorageVolDefPtr vol, + char **const groups, + int nextents, + unsigned long long size, + unsigned long long length) { - struct virStorageBackendLogicalPoolVolData *data = opaque; - virStoragePoolObjPtr pool = data->pool; - virStorageVolDefPtr vol = NULL; - bool is_new_vol = false; - unsigned long long offset, size, length; + int ret = -1; const char *regex_unit = "(\\S+)\\((\\S+)\\)"; char *regex = NULL; regex_t *reg = NULL; regmatch_t *vars = NULL; char *p = NULL; size_t i; - int err, nextents, nvars, ret = -1; + int err, nvars; + unsigned long long offset; + + if (VIR_STRDUP(regex, regex_unit) < 0) + goto cleanup; + + for (i = 1; i < nextents; i++) { + if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2) < 0) + goto cleanup; + /* "," is the separator of "devices" field */ + strcat(regex, ","); + strncat(regex, regex_unit, strlen(regex_unit)); + } + + if (VIR_ALLOC(reg) < 0) + goto cleanup; + + /* Each extent has a "path:offset" pair, and vars[0] will + * be the whole matched string. + */ + nvars = (nextents * 2) + 1; + if (VIR_ALLOC_N(vars, nvars) < 0) + goto cleanup; + + err = regcomp(reg, regex, REG_EXTENDED); + if (err != 0) { + char error[100]; + regerror(err, reg, error, sizeof(error)); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to compile regex %s"), + error); + 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; + } + + p = groups[3]; + + /* vars[0] is skipped */ + for (i = 0; i < nextents; i++) { + size_t j; + int len; + char *offset_str = NULL; + + j = (i * 2) + 1; + len = vars[j].rm_eo - vars[j].rm_so; + p[vars[j].rm_eo] = '\0'; + + if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path, + p + vars[j].rm_so, len) < 0) + goto cleanup; + + len = vars[j + 1].rm_eo - vars[j + 1].rm_so; + if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0) + goto cleanup; + + if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent offset value")); + VIR_FREE(offset_str); + goto cleanup; + } + + VIR_FREE(offset_str); + + vol->source.extents[vol->source.nextent].start = offset * size; + vol->source.extents[vol->source.nextent].end = (offset * size) + length; + vol->source.nextent++; + } + + ret = 0; + + cleanup: + VIR_FREE(regex); + VIR_FREE(reg); + VIR_FREE(vars); + return ret; +} + + +static int +virStorageBackendLogicalMakeVol(char **const groups, + void *opaque) +{ + struct virStorageBackendLogicalPoolVolData *data = opaque; + virStoragePoolObjPtr pool = data->pool; + virStorageVolDefPtr vol = NULL; + bool is_new_vol = false; + unsigned long long size, length; + int nextents, ret = -1; const char *attrs = groups[9]; /* Skip inactive volume */ @@ -196,78 +290,9 @@ virStorageBackendLogicalMakeVol(char **const groups, } /* Now parse the "devices" field separately */ - if (VIR_STRDUP(regex, regex_unit) < 0) - goto cleanup; - - for (i = 1; i < nextents; i++) { - if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2) < 0) - goto cleanup; - /* "," is the separator of "devices" field */ - strcat(regex, ","); - strncat(regex, regex_unit, strlen(regex_unit)); - } - - if (VIR_ALLOC(reg) < 0) - goto cleanup; - - /* Each extent has a "path:offset" pair, and vars[0] will - * be the whole matched string. - */ - nvars = (nextents * 2) + 1; - if (VIR_ALLOC_N(vars, nvars) < 0) - goto cleanup; - - err = regcomp(reg, regex, REG_EXTENDED); - if (err != 0) { - char error[100]; - regerror(err, reg, error, sizeof(error)); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), - error); - 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")); + if (virStorageBackendLogicalParseVolDevice(vol, groups, nextents, + size, length) < 0) goto cleanup; - } - - p = groups[3]; - - /* vars[0] is skipped */ - for (i = 0; i < nextents; i++) { - size_t j; - int len; - char *offset_str = NULL; - - j = (i * 2) + 1; - len = vars[j].rm_eo - vars[j].rm_so; - p[vars[j].rm_eo] = '\0'; - - if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path, - p + vars[j].rm_so, len) < 0) - goto cleanup; - - len = vars[j + 1].rm_eo - vars[j + 1].rm_so; - if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0) - goto cleanup; - - if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed volume extent offset value")); - VIR_FREE(offset_str); - goto cleanup; - } - - VIR_FREE(offset_str); - - vol->source.extents[vol->source.nextent].start = offset * size; - vol->source.extents[vol->source.nextent].end = (offset * size) + length; - vol->source.nextent++; - } if (is_new_vol && VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) @@ -276,9 +301,6 @@ virStorageBackendLogicalMakeVol(char **const groups, ret = 0; cleanup: - VIR_FREE(regex); - VIR_FREE(reg); - VIR_FREE(vars); if (is_new_vol && (ret == -1)) virStorageVolDefFree(vol); return ret; -- 2.5.0
On Fri, Jan 22, 2016 at 05:21:04PM -0500, John Ferlan wrote:
Create a helper routine in order to parse the 'device' string contained within the generated 'lvs' output string.
A future patch would then be able to avoid the code more cleanly
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 186 +++++++++++++++++++--------------- 1 file changed, 104 insertions(+), 82 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 76ea00a..bf67faf 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -72,21 +72,115 @@ struct virStorageBackendLogicalPoolVolData { };
static int -virStorageBackendLogicalMakeVol(char **const groups, - void *opaque) +virStorageBackendLogicalParseVolDevice(virStorageVolDefPtr vol, + char **const groups, + int nextents, + unsigned long long size, + unsigned long long length) {
You've called the new helper *ParseVolDevice, but it actually does more than that. I would rather see it like ParseExtents and also move all the extents related code into this function (parsing length and size) [...]
+ /* vars[0] is skipped */ + for (i = 0; i < nextents; i++) { + size_t j; + int len; + char *offset_str = NULL; + + j = (i * 2) + 1; + len = vars[j].rm_eo - vars[j].rm_so; + p[vars[j].rm_eo] = '\0'; + + if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path, + p + vars[j].rm_so, len) < 0) + goto cleanup; + + len = vars[j + 1].rm_eo - vars[j + 1].rm_so; + if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0) + goto cleanup; + + if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent offset value")); + VIR_FREE(offset_str); + goto cleanup; + } + + VIR_FREE(offset_str); + + vol->source.extents[vol->source.nextent].start = offset * size; + vol->source.extents[vol->source.nextent].end = (offset * size) + length; + vol->source.nextent++;
This would be much nicer to be done using VIR_APPEND_ELEMENT(). I know that this commit is just a code movement so this should be done in separate commit.
On 01/26/2016 09:46 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 05:21:04PM -0500, John Ferlan wrote:
Create a helper routine in order to parse the 'device' string contained within the generated 'lvs' output string.
A future patch would then be able to avoid the code more cleanly
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 186 +++++++++++++++++++--------------- 1 file changed, 104 insertions(+), 82 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 76ea00a..bf67faf 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -72,21 +72,115 @@ struct virStorageBackendLogicalPoolVolData { };
static int -virStorageBackendLogicalMakeVol(char **const groups, - void *opaque) +virStorageBackendLogicalParseVolDevice(virStorageVolDefPtr vol, + char **const groups, + int nextents, + unsigned long long size, + unsigned long long length) {
You've called the new helper *ParseVolDevice, but it actually does more than that. I would rather see it like ParseExtents and also move all the extents related code into this function (parsing length and size)
OK - my goal was to just parse the devices/extents element so that the next patch could use the 'nextents' to decide whether to call it... The 'nextents' is just the count... Having more than 1 is seen in a 'striped' and 'mirror' lv. A 'thin' lv has 0... A 'thin-pool' and 'linear' each has 1... Names are found in the 'segtype' field, but it's otherwise useless. Perhaps originally the thought when adding a parse of that field was that the 'stripes' value only was valid for stripes and mirrors, but it seems it's set depending on the count of devices - so it seems safe to use it for the extents parsing. FWIW: The "extents" look like: /dev/hda2(0) <--- linear or /dev/sdc1(10240),/dev/sdd1(0) <--- striped or thinpool_lv_test_thin_tdata(0) <--- thin-pool The "extents" gets further 'regcomp' and 'regexec''d to grab that "(#)" which is part of the black magic that I cannot explain ;-)
[...]
+ /* vars[0] is skipped */ + for (i = 0; i < nextents; i++) { + size_t j; + int len; + char *offset_str = NULL; + + j = (i * 2) + 1; + len = vars[j].rm_eo - vars[j].rm_so; + p[vars[j].rm_eo] = '\0'; + + if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path, + p + vars[j].rm_so, len) < 0) + goto cleanup; + + len = vars[j + 1].rm_eo - vars[j + 1].rm_so; + if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0) + goto cleanup; + + if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent offset value")); + VIR_FREE(offset_str); + goto cleanup; + } + + VIR_FREE(offset_str); + + vol->source.extents[vol->source.nextent].start = offset * size; + vol->source.extents[vol->source.nextent].end = (offset * size) + length; + vol->source.nextent++;
This would be much nicer to be done using VIR_APPEND_ELEMENT(). I know that this commit is just a code movement so this should be done in separate commit.
Yes I pretty much blindly moved "just" the devices parsing code, but I will note that we're not appending here - it's filling it in. Back in the caller "vol->source.extents" is allocated using "vol->source.nextents + nextents". A "new" vol would have "source.nextents == 0". All this loop does is fill in each source.extents and bump the source.nextents. In any case, I will move the allocation of source.extents, fetch of length and size into the helper (and rename it). John
On Wed, Jan 27, 2016 at 09:04:45AM -0500, John Ferlan wrote:
On 01/26/2016 09:46 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 05:21:04PM -0500, John Ferlan wrote:
Create a helper routine in order to parse the 'device' string contained within the generated 'lvs' output string.
A future patch would then be able to avoid the code more cleanly
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 186 +++++++++++++++++++--------------- 1 file changed, 104 insertions(+), 82 deletions(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 76ea00a..bf67faf 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -72,21 +72,115 @@ struct virStorageBackendLogicalPoolVolData { };
static int -virStorageBackendLogicalMakeVol(char **const groups, - void *opaque) +virStorageBackendLogicalParseVolDevice(virStorageVolDefPtr vol, + char **const groups, + int nextents, + unsigned long long size, + unsigned long long length) {
You've called the new helper *ParseVolDevice, but it actually does more than that. I would rather see it like ParseExtents and also move all the extents related code into this function (parsing length and size)
OK - my goal was to just parse the devices/extents element so that the next patch could use the 'nextents' to decide whether to call it... The 'nextents' is just the count... Having more than 1 is seen in a 'striped' and 'mirror' lv. A 'thin' lv has 0... A 'thin-pool' and 'linear' each has 1... Names are found in the 'segtype' field, but it's otherwise useless. Perhaps originally the thought when adding a parse of that field was that the 'stripes' value only was valid for stripes and mirrors, but it seems it's set depending on the count of devices - so it seems safe to use it for the extents parsing.
FWIW: The "extents" look like:
/dev/hda2(0) <--- linear
or
/dev/sdc1(10240),/dev/sdd1(0) <--- striped
or
thinpool_lv_test_thin_tdata(0) <--- thin-pool
The "extents" gets further 'regcomp' and 'regexec''d to grab that "(#)" which is part of the black magic that I cannot explain ;-)
Yes, I understand why the next patch updates the way, that we don't assume there is always 1 extent. It just seemed to me that parsing length and size would be nice to move to that function as it's used only in that function.
[...]
+ /* vars[0] is skipped */ + for (i = 0; i < nextents; i++) { + size_t j; + int len; + char *offset_str = NULL; + + j = (i * 2) + 1; + len = vars[j].rm_eo - vars[j].rm_so; + p[vars[j].rm_eo] = '\0'; + + if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path, + p + vars[j].rm_so, len) < 0) + goto cleanup; + + len = vars[j + 1].rm_eo - vars[j + 1].rm_so; + if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0) + goto cleanup; + + if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent offset value")); + VIR_FREE(offset_str); + goto cleanup; + } + + VIR_FREE(offset_str); + + vol->source.extents[vol->source.nextent].start = offset * size; + vol->source.extents[vol->source.nextent].end = (offset * size) + length; + vol->source.nextent++;
This would be much nicer to be done using VIR_APPEND_ELEMENT(). I know that this commit is just a code movement so this should be done in separate commit.
Yes I pretty much blindly moved "just" the devices parsing code, but I will note that we're not appending here - it's filling it in. Back in the caller "vol->source.extents" is allocated using "vol->source.nextents + nextents". A "new" vol would have "source.nextents == 0". All this loop does is fill in each source.extents and bump the source.nextents.
Yes, I know that we are not appending but filling existing array that was allocated earlier, I just didn't mentioned it. But we don't need to pre-allocate the array but just simply use append_element. The main point was that the allocation is again outside of the new function and that's not a good practice.
In any case, I will move the allocation of source.extents, fetch of length and size into the helper (and rename it).
From: Joe Harvell <joe.harvell@tekcomms.com> Modify the virStorageBackendLogicalMakeVol parsing to use only the 'stripes' output as the value for 'nextents' rather than assuming there is at least 1 extent and then adjusting "only if" the 'segtype' was 'striped'. This avoids the chance that 'mirror' segtypes cause issues with the parsed output. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 47 ++++++++++++++++------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index bf67faf..3010f58 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -64,8 +64,6 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, } -#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" - struct virStorageBackendLogicalPoolVolData { virStoragePoolObjPtr pool; virStorageVolDefPtr vol; @@ -181,7 +179,7 @@ virStorageBackendLogicalMakeVol(char **const groups, bool is_new_vol = false; unsigned long long size, length; int nextents, ret = -1; - const char *attrs = groups[9]; + const char *attrs = groups[8]; /* Skip inactive volume */ if (attrs[4] != 'a') @@ -259,13 +257,11 @@ virStorageBackendLogicalMakeVol(char **const groups, VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 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[4], NULL, 10, &nextents) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed volume extent stripes value")); + goto cleanup; } /* Finally fill in extents information */ @@ -273,24 +269,25 @@ virStorageBackendLogicalMakeVol(char **const groups, vol->source.nextent + nextents) < 0) goto cleanup; - if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) { + if (virStrToLong_ull(groups[5], NULL, 10, &length) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent length value")); goto cleanup; } - if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) { + if (virStrToLong_ull(groups[6], NULL, 10, &size) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent size value")); goto cleanup; } - if (virStrToLong_ull(groups[8], NULL, 10, &vol->target.allocation) < 0) { + if (virStrToLong_ull(groups[7], NULL, 10, &vol->target.allocation) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume allocation value")); goto cleanup; } - /* Now parse the "devices" field separately */ - if (virStorageBackendLogicalParseVolDevice(vol, groups, nextents, + /* If we have extents, then parse the "devices" field separately */ + if (nextents && + virStorageBackendLogicalParseVolDevice(vol, groups, nextents, size, length) < 0) goto cleanup; @@ -312,14 +309,14 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, { /* * # lvs --separator # --noheadings --units b --unbuffered --nosuffix --options \ - * "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr" VGNAME + * "lv_name,origin,uuid,devices,stripes,seg_size,vg_extent_size,size,lv_attr" VGNAME * - * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#linear#1#5234491392#33554432#5234491392#-wi-ao - * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#linear#1#1040187392#33554432#1040187392#-wi-ao - * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#linear#1#1073741824#33554432#1073741824#owi-a- - * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#linear#1#2181038080#33554432#2181038080#-wi-a- - * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#linear#1#1040187392#33554432#1040187392#swi-a- - * test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#/dev/sdc1(10240),/dev/sdd1(0)#striped#2#42949672960#4194304#-wi-a- + * RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#1#5234491392#33554432#5234491392#-wi-ao + * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#1#1040187392#33554432#1040187392#-wi-ao + * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#1#1073741824#33554432#1073741824#owi-a- + * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#1#2181038080#33554432#2181038080#-wi-a- + * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#1#1040187392#33554432#1040187392#swi-a- + * test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#/dev/sdc1(10240),/dev/sdd1(0)#2#42949672960#4194304#-wi-a- * * Pull out name, origin, & uuid, device, device extent start #, * segment size, extent size, size, attrs @@ -336,10 +333,10 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * 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*$" + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" }; int vars[] = { - 10 + 9 }; int ret = -1; virCommandPtr cmd; @@ -355,7 +352,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, "--unbuffered", "--nosuffix", "--options", - "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr", + "lv_name,origin,uuid,devices,stripes,seg_size,vg_extent_size,size,lv_attr", pool->def->source.name, NULL); if (virCommandRunRegex(cmd, -- 2.5.0
From: Joe Harvell <joe.harvell@tekcomms.com> Since our 'devices' parsing logic now will use the 'nextents' (or lvs 'stripes' output) to decide whether or not to parse the field, use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)" (1 or more) when grabbing the 'groups[3]' or 'devices' field. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3010f58..2af3e69 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" +/* name orig uuid devs stripes segsz vgextsz sz lvattr */ + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" }; int vars[] = { 9 -- 2.5.0
On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote:
From: Joe Harvell <joe.harvell@tekcomms.com>
Since our 'devices' parsing logic now will use the 'nextents' (or lvs 'stripes' output) to decide whether or not to parse the field, use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)" (1 or more) when grabbing the 'groups[3]' or 'devices' field.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3010f58..2af3e69 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" +/* name orig uuid devs stripes segsz vgextsz sz lvattr */ + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" }; int vars[] = { 9
This could be squashed into the previous commit and if you are creating a comment with labels, please use the exact names that lvs uses.
On 01/26/2016 09:55 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote:
From: Joe Harvell <joe.harvell@tekcomms.com>
Since our 'devices' parsing logic now will use the 'nextents' (or lvs 'stripes' output) to decide whether or not to parse the field, use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)" (1 or more) when grabbing the 'groups[3]' or 'devices' field.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3010f58..2af3e69 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" +/* name orig uuid devs stripes segsz vgextsz sz lvattr */ + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" }; int vars[] = { 9
This could be squashed into the previous commit and if you are creating a comment with labels, please use the exact names that lvs uses.
I'll just remove the comment - yes it's shorthand, but "vg_extent_size" is a bit long for the "([0-9]+)"... BTW: I went separately mainly to make it more obvious of the change. Patch 3 was just dealing with removing the need for the 'segtype' field as it was made obsolete by just using the stripes field to fill in the 'nextents'. Patch 4 alters the regex to allow 0 or more, rather than at least 1. Although I do understand why combining them is also perfectly reasonable. I was trying not to do two things at one time... However, after some more "investigation" of the environment - I think perhaps there's still a couple of loose ends. Keeping the 'segtype' field may be necessary/useful... Details follow if interested ;-) A 'vol-dumpxml' on a "found" thin lv from a thin-pool will provide an empty "<source>" and the capacity == allocation; whereas, a 'vol-dumpxml' on a snapshot lv will provide the "<source>...<device path='/dev/sdX'> with the <extent start='#' end='#'/> where the size of the extent of course matches the allocation value when seen with vol-info. The <source> of a thin lv is a lv thin-pool and there is no extents data (e.g. nextents == 0). However, there is "pool_lv" data for a "thin" segtype which could then be used for display in the "<source>...<name>". There's still no way to "create" a thin pool from <name>, but finding/displaying one is still possible... The details of which "/dev/sdX" is used is hidden by the lv mgr thin_pool code. We don't display the thin_pool because by itself since it cannot be used as a volume, but that pool contains the information about the real sizes, so it's useful. Right now there's no way to get that useful information so going with the "size" (e.g. allocation) value would have to suffice. Future patches can perhaps adjust this. John
On Wed, Jan 27, 2016 at 10:05:36AM -0500, John Ferlan wrote:
On 01/26/2016 09:55 AM, Pavel Hrdina wrote:
On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote:
From: Joe Harvell <joe.harvell@tekcomms.com>
Since our 'devices' parsing logic now will use the 'nextents' (or lvs 'stripes' output) to decide whether or not to parse the field, use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)" (1 or more) when grabbing the 'groups[3]' or 'devices' field.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 3010f58..2af3e69 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, * striped, so "," is not a suitable separator either (rhbz 727474). */ const char *regexes[] = { - "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" +/* name orig uuid devs stripes segsz vgextsz sz lvattr */ + "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" }; int vars[] = { 9
This could be squashed into the previous commit and if you are creating a comment with labels, please use the exact names that lvs uses.
I'll just remove the comment - yes it's shorthand, but "vg_extent_size" is a bit long for the "([0-9]+)"...
BTW: I went separately mainly to make it more obvious of the change.
Patch 3 was just dealing with removing the need for the 'segtype' field as it was made obsolete by just using the stripes field to fill in the 'nextents'.
Patch 4 alters the regex to allow 0 or more, rather than at least 1. Although I do understand why combining them is also perfectly reasonable. I was trying not to do two things at one time...
However, after some more "investigation" of the environment - I think perhaps there's still a couple of loose ends. Keeping the 'segtype' field may be necessary/useful... Details follow if interested ;-)
Yes, you're right and I did a bad job during review. The segtype is required and we should always consider it, there is like 20 segtypes right now and some of them could also use 'stripes'.
A 'vol-dumpxml' on a "found" thin lv from a thin-pool will provide an empty "<source>" and the capacity == allocation; whereas, a 'vol-dumpxml' on a snapshot lv will provide the "<source>...<device path='/dev/sdX'> with the <extent start='#' end='#'/> where the size of the extent of course matches the allocation value when seen with vol-info.
The <source> of a thin lv is a lv thin-pool and there is no extents data (e.g. nextents == 0). However, there is "pool_lv" data for a "thin" segtype which could then be used for display in the "<source>...<name>". There's still no way to "create" a thin pool from <name>, but finding/displaying one is still possible... The details of which "/dev/sdX" is used is hidden by the lv mgr thin_pool code.
We don't display the thin_pool because by itself since it cannot be used as a volume, but that pool contains the information about the real sizes, so it's useful. Right now there's no way to get that useful information so going with the "size" (e.g. allocation) value would have to suffice. Future patches can perhaps adjust this.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
...
However, after some more "investigation" of the environment - I think perhaps there's still a couple of loose ends. Keeping the 'segtype' field may be necessary/useful... Details follow if interested ;-)
Yes, you're right and I did a bad job during review. The segtype is required and we should always consider it, there is like 20 segtypes right now and some of them could also use 'stripes'.
The 'segtype' is currently only required because commit id '82c1740a' added 'segtype' and 'stripes' to resolve a bz (or more than one depending on how far you chase) by using 'segtype' to determine whether more than one existed. It also did quite a few things in one step which by today's review standards is a bad thing ;-). However, that only worked for "striped" lv's. If there was a "mirror" lv, then while the mirror could be found, the vol-dumpxml output is wrong because it only parsed 1 extent (incorrectly at that): <source> <device path='lv_test_mirror_rimage_0(0),lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source> instead of something like (if using 'stripes' to get nsegments): <source> <device path='lv_test_mirror_rimage_0'> <extent start='0' end='33554432'/> </device> <device path='lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source> Linking 'nsegments' to 'striped' lv's is a "limitation". Anyway, dropping 'segtype' wasn't necessarily bad unless you needed "more information", such as perhaps the lv thin pool source when nsegments == 0. This becomes obvious once the change to use "(\\S*)" instead of "(\\S+)" hits. Now we can "see" thin lv's in a volume group. Not sure what else becomes visible, though. The problem is you then get: <source> </source> But that's fixable... The interesting part about <source> is that it's an output only (virStorageVolDefParseXML doesn't read it). So, by adding a new parse field 'pool_lv', we can then check 'segtype' to be "thin" and if so, store the new field in a new vol->source.thin_pool field. Still cannot create a thin lv pool/volume without using the lvcreate command. Nor can one create a mirror or stripe lv using libvirt's vol-create command. One just cannot "find" a thin lv right now... John
On Wed, Jan 27, 2016 at 03:26:12PM -0500, John Ferlan wrote:
...
However, after some more "investigation" of the environment - I think perhaps there's still a couple of loose ends. Keeping the 'segtype' field may be necessary/useful... Details follow if interested ;-)
Yes, you're right and I did a bad job during review. The segtype is required and we should always consider it, there is like 20 segtypes right now and some of them could also use 'stripes'.
The 'segtype' is currently only required because commit id '82c1740a' added 'segtype' and 'stripes' to resolve a bz (or more than one depending on how far you chase) by using 'segtype' to determine whether more than one existed. It also did quite a few things in one step which by today's review standards is a bad thing ;-).
However, that only worked for "striped" lv's. If there was a "mirror" lv, then while the mirror could be found, the vol-dumpxml output is wrong because it only parsed 1 extent (incorrectly at that):
That leads me to conclusion, that we should parse only the segtypes that we know how to parse. The rest should be ignored.
<source> <device path='lv_test_mirror_rimage_0(0),lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source>
instead of something like (if using 'stripes' to get nsegments):
<source> <device path='lv_test_mirror_rimage_0'> <extent start='0' end='33554432'/> </device> <device path='lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source>
Linking 'nsegments' to 'striped' lv's is a "limitation".
Anyway, dropping 'segtype' wasn't necessarily bad unless you needed "more information", such as perhaps the lv thin pool source when nsegments == 0. This becomes obvious once the change to use "(\\S*)" instead of "(\\S+)" hits. Now we can "see" thin lv's in a volume group. Not sure what else becomes visible, though. The problem is you then get:
But we need more information and we propagate those information to user via our API and that's why I think that we should parse only those volume which we know how to parse to pass correct values to user. Pavel
<source> </source>
But that's fixable... The interesting part about <source> is that it's an output only (virStorageVolDefParseXML doesn't read it). So, by adding a new parse field 'pool_lv', we can then check 'segtype' to be "thin" and if so, store the new field in a new vol->source.thin_pool field.
Still cannot create a thin lv pool/volume without using the lvcreate command. Nor can one create a mirror or stripe lv using libvirt's vol-create command. One just cannot "find" a thin lv right now...
John
On 01/28/2016 06:30 AM, Pavel Hrdina wrote:
On Wed, Jan 27, 2016 at 03:26:12PM -0500, John Ferlan wrote:
...
However, after some more "investigation" of the environment - I think perhaps there's still a couple of loose ends. Keeping the 'segtype' field may be necessary/useful... Details follow if interested ;-)
Yes, you're right and I did a bad job during review. The segtype is required and we should always consider it, there is like 20 segtypes right now and some of them could also use 'stripes'.
The 'segtype' is currently only required because commit id '82c1740a' added 'segtype' and 'stripes' to resolve a bz (or more than one depending on how far you chase) by using 'segtype' to determine whether more than one existed. It also did quite a few things in one step which by today's review standards is a bad thing ;-).
However, that only worked for "striped" lv's. If there was a "mirror" lv, then while the mirror could be found, the vol-dumpxml output is wrong because it only parsed 1 extent (incorrectly at that):
That leads me to conclusion, that we should parse only the segtypes that we know how to parse. The rest should be ignored.
<source> <device path='lv_test_mirror_rimage_0(0),lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source>
instead of something like (if using 'stripes' to get nsegments):
<source> <device path='lv_test_mirror_rimage_0'> <extent start='0' end='33554432'/> </device> <device path='lv_test_mirror_rimage_1'> <extent start='0' end='33554432'/> </device> </source>
Linking 'nsegments' to 'striped' lv's is a "limitation".
Anyway, dropping 'segtype' wasn't necessarily bad unless you needed "more information", such as perhaps the lv thin pool source when nsegments == 0. This becomes obvious once the change to use "(\\S*)" instead of "(\\S+)" hits. Now we can "see" thin lv's in a volume group. Not sure what else becomes visible, though. The problem is you then get:
But we need more information and we propagate those information to user via our API and that's why I think that we should parse only those volume which we know how to parse to pass correct values to user.
I understand your point, but what is that list? What happens if/when we "choose" something that works today? The issues with the code now: 1. It doesn't recognize a thin lv 2. It doesn't parse a mirror or raid segtype extent correctly Both are based around using the 'segtype' to make a decision to use the 'stripes' field in order to get the number of segments found. While it does accurately give the number of devices for striped, mirror, raid, thin, thin-pool, and linear lv's - it's not documented that way. So by using 'segtype' to limit we've already caused an issue, so I'm not sure using that to limit what we parse. Using it to make a decision about how to parse a specific segtype I think would be OK. Secondarily, using 'stripes' as the count of extents is probably wrong too. The count of segments should probably be made by actively parsing the devices field and doing the VIR_APPEND_ELEMENT as you originally suggested each time we parse something that has "string(#)", where string and (#) are the parsed elements. Let's see where this takes me ;-) John
Pavel
<source> </source>
But that's fixable... The interesting part about <source> is that it's an output only (virStorageVolDefParseXML doesn't read it). So, by adding a new parse field 'pool_lv', we can then check 'segtype' to be "thin" and if so, store the new field in a new vol->source.thin_pool field.
Still cannot create a thin lv pool/volume without using the lvcreate command. Nor can one create a mirror or stripe lv using libvirt's vol-create command. One just cannot "find" a thin lv right now...
John
participants (2)
-
John Ferlan -
Pavel Hrdina