[libvirt] [PATCH 0/2] Disallow wiping a sparse logical volume
The first patch just changes 'building' into a bool as that's how I found it used when going to add a new bool for patch 2 Patch 2 addresses the following bz: https://bugzilla.redhat.com/show_bug.cgi?id=1091866 Essentially, wiping the lv caused it to disappear after a vol-refresh. This was because the lv that was created was a sparse (or thin or snapshot via --virtualsize of -V) logical volume. The multiple names are the ways I found the feature described. When the sparse volume was written to by the wipe algorithms it actually filled the volume beyond it's capacity rendering it INACTIVE which causes pool-refresh to ignore it. As it turns out a sparse lv created of size (for example) 4 MiB has metadata contained within the size created. While we could adjust the size of the metadata, control over it's location becomes the issue. It seems there is also some guard data at each end of the sparse lv as when the wipe was done lost I/O messages were sent to the system log. After much searching it just seems that writing a sparse lv with some sort of wipe algorithm will not work. Although if anyone has suggestions I'm willing to try. I have found writing '0x00''s, the scrub utility, and a "dd if=/dev/zero of=/dev/lv_pool/lv_test bs=4096 count=1024" doesn't work. It's also of interest to note that the 'scrub' utility doesn't seem to recognize the logical volume format properly as none of the nonzero algorithms are run - each fails to open the file - for example: $ /usr/sbin/lvcreate --name lv_test -L 4096K lv_pool $ /usr/bin/scrub -f -p bsi /dev/lv_pool/lv_test scrub: using BSI patterns scrub: warning: /dev/lv_pool/lv_test is zero length $ John Ferlan (2): storage: Convert 'building' into a bool storage: Disallow vol_wipe for sparse logical volumes src/conf/storage_conf.h | 2 +- src/storage/storage_backend_logical.c | 39 ++++++++++++++++++++++++++++++++++- src/storage/storage_driver.c | 8 +++---- src/util/virstoragefile.h | 1 + 4 files changed, 44 insertions(+), 6 deletions(-) -- 1.9.3
Rather than a unsigned int, use a 'bool' since that's how it was used. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.h | 2 +- src/storage/storage_driver.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index badf7a3..54c978d 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -63,7 +63,7 @@ struct _virStorageVolDef { char *key; int type; /* virStorageVolType */ - unsigned int building; + bool building; unsigned int in_use; virStorageVolSource source; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 441da21..3830867 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1691,7 +1691,7 @@ storageVolCreateXML(virStoragePoolPtr obj, /* Drop the pool lock during volume allocation */ pool->asyncjobs++; - voldef->building = 1; + voldef->building = true; virStoragePoolObjUnlock(pool); buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags); @@ -1700,7 +1700,7 @@ storageVolCreateXML(virStoragePoolPtr obj, virStoragePoolObjLock(pool); storageDriverUnlock(driver); - voldef->building = 0; + voldef->building = false; pool->asyncjobs--; if (buildret < 0) { @@ -1858,7 +1858,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, /* Drop the pool lock during volume allocation */ pool->asyncjobs++; - newvol->building = 1; + newvol->building = true; origvol->in_use++; virStoragePoolObjUnlock(pool); @@ -1876,7 +1876,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, storageDriverUnlock(driver); origvol->in_use--; - newvol->building = 0; + newvol->building = false; allocation = newvol->target.allocation; pool->asyncjobs--; -- 1.9.3
On 07/17/2014 12:10 PM, John Ferlan wrote:
Rather than a unsigned int, use a 'bool' since that's how it was used.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.h | 2 +- src/storage/storage_driver.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
https://bugzilla.redhat.com/show_bug.cgi?id=1091866 Add a new boolean 'sparse'. This will be used by the logical backend storage driver to determine whether the target volume is sparse or not (also known by a snapshot or thin logical volume). Although setting sparse to true at creation could be seen as duplicitous to setting during virStorageBackendLogicalMakeVol() in case there are ever other code paths between Create and FindLVs that need to know about the volume be sparse. Use the 'sparse' in a new virStorageBackendLogicalVolWipe() to decide whether to attempt to wipe the logical volume or not. For now, I have found no means to wipe the volume without writing to it. Writing to the sparse volume causes it to be filled. A sparse logical volume is not complely writeable as there exists metadata which if overwritten will cause the sparse lv to go INACTIVE which means pool-refresh will not find it. Access to whatever lvm uses to manage data blocks is not provided by any API I could find. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 39 ++++++++++++++++++++++++++++++++++- src/util/virstoragefile.h | 1 + 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ab1e64b..ed62c2f 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -131,6 +131,15 @@ virStorageBackendLogicalMakeVol(char **const groups, goto cleanup; } + /* Mark the (s) sparse/snapshot lv, e.g. the lv created using + * the --virtualsize/-V option. We've already ignored the (t)hin + * pool definition. In the manner libvirt defines these, the + * thin pool is hidden to the lvs output, except as the name + * in brackets [] described for the groups[1] (backingStore). + */ + if (attrs[0] == 's') + vol->target.sparse = true; + /* Skips the backingStore of lv created with "--virtualsize", * its original device "/dev/$vgname/$lvname_vorigin" is * just for lvm internal use, one should never use it. @@ -752,6 +761,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, VIR_DIV_UP(vol->target.allocation ? vol->target.allocation : 1, 1024)); virCommandAddArg(cmd, "--virtualsize"); + vol->target.sparse = true; } virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, 1024)); @@ -829,6 +839,33 @@ virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, return build_func(conn, pool, vol, inputvol, flags); } +static int +virStorageBackendLogicalVolWipe(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned int algorithm, + unsigned int flags) +{ + if (!vol->target.sparse) + return virStorageBackendVolWipeLocal(conn, pool, vol, algorithm, flags); + + /* The wiping algorithms will write something to the logical volume. + * Writing to a sparse logical volume causes it to be filled resulting + * in the volume becoming INACTIVE because there is some amount of + * metadata contained within the sparse lv. Choosing to only write + * a wipe pattern to the already written portion lv based on what + * 'lvs' shows in the "Data%" column/field for the sparse lv was + * considered. However, there is no guarantee that sparse lv could + * grow or shrink outside of libvirt's knowledge and thus still render + * the volume INACTIVE. Until there is some sort of wipe function + * implemented by lvm for one of these sparse lv, we'll just return + * unsupported. + */ + virReportError(VIR_ERR_NO_SUPPORT, + _("logical volue '%s' is sparse, volume wipe not supported"), + vol->target.path); + return -1; +} virStorageBackend virStorageBackendLogical = { .type = VIR_STORAGE_POOL_LOGICAL, @@ -846,5 +883,5 @@ virStorageBackend virStorageBackendLogical = { .deleteVol = virStorageBackendLogicalDeleteVol, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, - .wipeVol = virStorageBackendVolWipeLocal, + .wipeVol = virStorageBackendLogicalVolWipe, }; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c840aa0..eccbf4e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -248,6 +248,7 @@ struct _virStorageSource { virBitmapPtr features; char *compat; bool nocow; + bool sparse; virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; -- 1.9.3
On 07/17/2014 12:10 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1091866
Add a new boolean 'sparse'. This will be used by the logical backend storage driver to determine whether the target volume is sparse or not (also known by a snapshot or thin logical volume). Although setting sparse to true at creation could be seen as duplicitous to setting during virStorageBackendLogicalMakeVol() in case there are ever other code paths between Create and FindLVs that need to know about the volume be sparse.
Use the 'sparse' in a new virStorageBackendLogicalVolWipe() to decide whether to attempt to wipe the logical volume or not. For now, I have found no means to wipe the volume without writing to it. Writing to the sparse volume causes it to be filled. A sparse logical volume is not complely
s/complely/completely/
writeable as there exists metadata which if overwritten will cause the sparse lv to go INACTIVE which means pool-refresh will not find it. Access to whatever lvm uses to manage data blocks is not provided by any API I could find.
I wonder if lvm developers could point us to an alternative; maybe even lvreduce (shrink the allocated size) followed by lvresize (regrow it, but the new growth starts life empty) might behave semantically like a wipe. But yeah, we don't need to solve that today.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 39 ++++++++++++++++++++++++++++++++++- src/util/virstoragefile.h | 1 + 2 files changed, 39 insertions(+), 1 deletion(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/17/2014 02:10 PM, John Ferlan wrote:
John Ferlan (2): storage: Convert 'building' into a bool storage: Disallow vol_wipe for sparse logical volumes
src/conf/storage_conf.h | 2 +- src/storage/storage_backend_logical.c | 39 ++++++++++++++++++++++++++++++++++- src/storage/storage_driver.c | 8 +++---- src/util/virstoragefile.h | 1 + 4 files changed, 44 insertions(+), 6 deletions(-)
Thanks for the review - I adjusted the completely... funny when I typed it and looked at it I thought it looked funny but my fingers were more insistent that they had done the right thing :-) Not sure lvremove/lvresize will do the trick here - read the 'lvmthin' man page... Also online I found here was a reading or two online that suggested one cannot reduce the size of a thin pool due to some lvm bug. There's another doc that says in order to reduce the size use trim, but I'm not quite sure how to access that. In my "example" case - my extent size is 4096 and my thin/sparse size is 4096. Attemping to lvreduce results in a "New size given (xxx) not less than existing size (1 extents)." So even though I've filled half of it with 0's, there doesn't seem to be a way to do what we want. Maybe someone has more "experience" dealing with these In any case, I pushed what I have... John
participants (2)
-
Eric Blake -
John Ferlan