[libvirt] [PATCHv2 0/2] storage: Don't wipe volumes on remote filesystems with local tools
Version 2 now splits the stuff into separate driver backend funcs. Peter Krempa (2): storage: wipe: Move helper code into storage backend storage: Split out volume wiping as separate backend function src/storage/storage_backend.c | 203 +++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 12 ++ src/storage/storage_backend_disk.c | 1 + src/storage/storage_backend_fs.c | 3 + src/storage/storage_backend_iscsi.c | 1 + src/storage/storage_backend_logical.c | 1 + src/storage/storage_backend_mpath.c | 1 + src/storage/storage_backend_scsi.c | 1 + src/storage/storage_driver.c | 205 +--------------------------------- 9 files changed, 229 insertions(+), 199 deletions(-) -- 2.0.0
The next patch will move the storage volume wiping code into the individual backends. This patch splits out the common code to wipe a local volume into a separate backend helper so that the next patch is simpler. --- src/storage/storage_backend.c | 203 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 6 ++ src/storage/storage_driver.c | 199 +---------------------------------------- 3 files changed, 210 insertions(+), 198 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7b17ca4..8e62403 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1700,6 +1700,209 @@ virStorageBackendVolDownloadLocal(virConnectPtr conn ATTRIBUTE_UNUSED, return virFDStreamOpenFile(stream, vol->target.path, offset, len, O_RDONLY); } + +/* If the volume we're wiping is already a sparse file, we simply + * truncate and extend it to its original size, filling it with + * zeroes. This behavior is guaranteed by POSIX: + * + * http://www.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html + * + * If fildes refers to a regular file, the ftruncate() function shall + * cause the size of the file to be truncated to length. If the size + * of the file previously exceeded length, the extra data shall no + * longer be available to reads on the file. If the file previously + * was smaller than this size, ftruncate() shall increase the size of + * the file. If the file size is increased, the extended area shall + * appear as if it were zero-filled. + */ +static int +virStorageBackendVolZeroSparseFileLocal(virStorageVolDefPtr vol, + off_t size, + int fd) +{ + int ret = -1; + + ret = ftruncate(fd, 0); + if (ret == -1) { + virReportSystemError(errno, + _("Failed to truncate volume with " + "path '%s' to 0 bytes"), + vol->target.path); + return ret; + } + + ret = ftruncate(fd, size); + if (ret == -1) { + virReportSystemError(errno, + _("Failed to truncate volume with " + "path '%s' to %ju bytes"), + vol->target.path, (uintmax_t)size); + } + + return ret; +} + + +static int +virStorageBackendWipeExtentLocal(virStorageVolDefPtr vol, + int fd, + off_t extent_start, + off_t extent_length, + char *writebuf, + size_t writebuf_length, + size_t *bytes_wiped) +{ + int ret = -1, written = 0; + off_t remaining = 0; + size_t write_size = 0; + + VIR_DEBUG("extent logical start: %ju len: %ju", + (uintmax_t)extent_start, (uintmax_t)extent_length); + + if ((ret = lseek(fd, extent_start, SEEK_SET)) < 0) { + virReportSystemError(errno, + _("Failed to seek to position %ju in volume " + "with path '%s'"), + (uintmax_t)extent_start, vol->target.path); + goto cleanup; + } + + remaining = extent_length; + while (remaining > 0) { + + write_size = (writebuf_length < remaining) ? writebuf_length : remaining; + written = safewrite(fd, writebuf, write_size); + if (written < 0) { + virReportSystemError(errno, + _("Failed to write %zu bytes to " + "storage volume with path '%s'"), + write_size, vol->target.path); + + goto cleanup; + } + + *bytes_wiped += written; + remaining -= written; + } + + if (fdatasync(fd) < 0) { + ret = -errno; + virReportSystemError(errno, + _("cannot sync data to volume with path '%s'"), + vol->target.path); + goto cleanup; + } + + VIR_DEBUG("Wrote %zu bytes to volume with path '%s'", + *bytes_wiped, vol->target.path); + + ret = 0; + + cleanup: + return ret; +} + + +int +virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + unsigned int algorithm, + unsigned int flags) +{ + int ret = -1, fd = -1; + struct stat st; + char *writebuf = NULL; + size_t bytes_wiped = 0; + virCommandPtr cmd = NULL; + + virCheckFlags(0, -1); + + VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", + vol->target.path, algorithm); + + fd = open(vol->target.path, O_RDWR); + if (fd == -1) { + virReportSystemError(errno, + _("Failed to open storage volume with path '%s'"), + vol->target.path); + goto cleanup; + } + + if (fstat(fd, &st) == -1) { + virReportSystemError(errno, + _("Failed to stat storage volume with path '%s'"), + vol->target.path); + goto cleanup; + } + + if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) { + const char *alg_char ATTRIBUTE_UNUSED = NULL; + switch (algorithm) { + case VIR_STORAGE_VOL_WIPE_ALG_NNSA: + alg_char = "nnsa"; + break; + case VIR_STORAGE_VOL_WIPE_ALG_DOD: + alg_char = "dod"; + break; + case VIR_STORAGE_VOL_WIPE_ALG_BSI: + alg_char = "bsi"; + break; + case VIR_STORAGE_VOL_WIPE_ALG_GUTMANN: + alg_char = "gutmann"; + break; + case VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER: + alg_char = "schneier"; + break; + case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7: + alg_char = "pfitzner7"; + break; + case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33: + alg_char = "pfitzner33"; + break; + case VIR_STORAGE_VOL_WIPE_ALG_RANDOM: + alg_char = "random"; + break; + default: + virReportError(VIR_ERR_INVALID_ARG, + _("unsupported algorithm %d"), + algorithm); + } + cmd = virCommandNew(SCRUB); + virCommandAddArgList(cmd, "-f", "-p", alg_char, + vol->target.path, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + goto cleanup; + } else { + if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { + ret = virStorageBackendVolZeroSparseFileLocal(vol, st.st_size, fd); + } else { + + if (VIR_ALLOC_N(writebuf, st.st_blksize) < 0) + goto cleanup; + + ret = virStorageBackendWipeExtentLocal(vol, + fd, + 0, + vol->target.allocation, + writebuf, + st.st_blksize, + &bytes_wiped); + } + } + + cleanup: + virCommandFree(cmd); + VIR_FREE(writebuf); + VIR_FORCE_CLOSE(fd); + return ret; +} + + #ifdef GLUSTER_CLI int virStorageBackendFindGlusterPoolSources(const char *host, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 4d7d4b0..5e251d7 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -120,6 +120,12 @@ int virStorageBackendVolDownloadLocal(virConnectPtr conn, unsigned long long len, unsigned int flags); +int virStorageBackendVolWipeLocal(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned int algorithm, + unsigned int flags); + typedef struct _virStorageBackend virStorageBackend; typedef virStorageBackend *virStorageBackendPtr; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5407dfb..97571e8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2094,202 +2094,6 @@ storageVolResize(virStorageVolPtr obj, return ret; } -/* If the volume we're wiping is already a sparse file, we simply - * truncate and extend it to its original size, filling it with - * zeroes. This behavior is guaranteed by POSIX: - * - * http://www.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html - * - * If fildes refers to a regular file, the ftruncate() function shall - * cause the size of the file to be truncated to length. If the size - * of the file previously exceeded length, the extra data shall no - * longer be available to reads on the file. If the file previously - * was smaller than this size, ftruncate() shall increase the size of - * the file. If the file size is increased, the extended area shall - * appear as if it were zero-filled. - */ -static int -storageVolZeroSparseFile(virStorageVolDefPtr vol, - off_t size, - int fd) -{ - int ret = -1; - - ret = ftruncate(fd, 0); - if (ret == -1) { - virReportSystemError(errno, - _("Failed to truncate volume with " - "path '%s' to 0 bytes"), - vol->target.path); - return ret; - } - - ret = ftruncate(fd, size); - if (ret == -1) { - virReportSystemError(errno, - _("Failed to truncate volume with " - "path '%s' to %ju bytes"), - vol->target.path, (uintmax_t)size); - } - - return ret; -} - - -static int -storageWipeExtent(virStorageVolDefPtr vol, - int fd, - off_t extent_start, - off_t extent_length, - char *writebuf, - size_t writebuf_length, - size_t *bytes_wiped) -{ - int ret = -1, written = 0; - off_t remaining = 0; - size_t write_size = 0; - - VIR_DEBUG("extent logical start: %ju len: %ju", - (uintmax_t)extent_start, (uintmax_t)extent_length); - - if ((ret = lseek(fd, extent_start, SEEK_SET)) < 0) { - virReportSystemError(errno, - _("Failed to seek to position %ju in volume " - "with path '%s'"), - (uintmax_t)extent_start, vol->target.path); - goto cleanup; - } - - remaining = extent_length; - while (remaining > 0) { - - write_size = (writebuf_length < remaining) ? writebuf_length : remaining; - written = safewrite(fd, writebuf, write_size); - if (written < 0) { - virReportSystemError(errno, - _("Failed to write %zu bytes to " - "storage volume with path '%s'"), - write_size, vol->target.path); - - goto cleanup; - } - - *bytes_wiped += written; - remaining -= written; - } - - if (fdatasync(fd) < 0) { - ret = -errno; - virReportSystemError(errno, - _("cannot sync data to volume with path '%s'"), - vol->target.path); - goto cleanup; - } - - VIR_DEBUG("Wrote %zu bytes to volume with path '%s'", - *bytes_wiped, vol->target.path); - - ret = 0; - - cleanup: - return ret; -} - - -static int -storageVolWipeInternal(virStorageVolDefPtr def, - unsigned int algorithm) -{ - int ret = -1, fd = -1; - struct stat st; - char *writebuf = NULL; - size_t bytes_wiped = 0; - virCommandPtr cmd = NULL; - - VIR_DEBUG("Wiping volume with path '%s' and algorithm %u", - def->target.path, algorithm); - - fd = open(def->target.path, O_RDWR); - if (fd == -1) { - virReportSystemError(errno, - _("Failed to open storage volume with path '%s'"), - def->target.path); - goto cleanup; - } - - if (fstat(fd, &st) == -1) { - virReportSystemError(errno, - _("Failed to stat storage volume with path '%s'"), - def->target.path); - goto cleanup; - } - - if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) { - const char *alg_char ATTRIBUTE_UNUSED = NULL; - switch (algorithm) { - case VIR_STORAGE_VOL_WIPE_ALG_NNSA: - alg_char = "nnsa"; - break; - case VIR_STORAGE_VOL_WIPE_ALG_DOD: - alg_char = "dod"; - break; - case VIR_STORAGE_VOL_WIPE_ALG_BSI: - alg_char = "bsi"; - break; - case VIR_STORAGE_VOL_WIPE_ALG_GUTMANN: - alg_char = "gutmann"; - break; - case VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER: - alg_char = "schneier"; - break; - case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7: - alg_char = "pfitzner7"; - break; - case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33: - alg_char = "pfitzner33"; - break; - case VIR_STORAGE_VOL_WIPE_ALG_RANDOM: - alg_char = "random"; - break; - default: - virReportError(VIR_ERR_INVALID_ARG, - _("unsupported algorithm %d"), - algorithm); - } - cmd = virCommandNew(SCRUB); - virCommandAddArgList(cmd, "-f", "-p", alg_char, - def->target.path, NULL); - - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - goto cleanup; - } else { - if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { - ret = storageVolZeroSparseFile(def, st.st_size, fd); - } else { - - if (VIR_ALLOC_N(writebuf, st.st_blksize) < 0) - goto cleanup; - - ret = storageWipeExtent(def, - fd, - 0, - def->target.allocation, - writebuf, - st.st_blksize, - &bytes_wiped); - } - } - - cleanup: - virCommandFree(cmd); - VIR_FREE(writebuf); - VIR_FORCE_CLOSE(fd); - return ret; -} - static int storageVolWipePattern(virStorageVolPtr obj, @@ -2330,9 +2134,8 @@ storageVolWipePattern(virStorageVolPtr obj, goto cleanup; } - if (storageVolWipeInternal(vol, algorithm) == -1) { + if (virStorageBackendVolWipeLocal(obj->conn, pool, vol, algorithm, flags) < 0) goto cleanup; - } ret = 0; -- 2.0.0
For non-local storage drivers we can't expect to use the "scrub" tool to wipe the volume. Split the code into a separate backend function so that we can add protocol specific code later. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1118710 --- src/storage/storage_backend.h | 6 ++++++ src/storage/storage_backend_disk.c | 1 + src/storage/storage_backend_fs.c | 3 +++ src/storage/storage_backend_iscsi.c | 1 + src/storage/storage_backend_logical.c | 1 + src/storage/storage_backend_mpath.c | 1 + src/storage/storage_backend_scsi.c | 1 + src/storage/storage_driver.c | 10 +++++++--- 8 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 5e251d7..e48da5b 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -87,6 +87,11 @@ typedef int (*virStorageBackendVolumeUpload)(virConnectPtr conn, unsigned long long offset, unsigned long long len, unsigned int flags); +typedef int (*virStorageBackendVolumeWipe)(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned int algorithm, + unsigned int flags); /* File creation/cloning functions used for cloning between backends */ int virStorageBackendCreateRaw(virConnectPtr conn, @@ -150,6 +155,7 @@ struct _virStorageBackend { virStorageBackendVolumeResize resizeVol; virStorageBackendVolumeUpload uploadVol; virStorageBackendVolumeDownload downloadVol; + virStorageBackendVolumeWipe wipeVol; }; virStorageBackendPtr virStorageBackendForType(int type); diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index f900dee..d85f13f 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -794,4 +794,5 @@ virStorageBackend virStorageBackendDisk = { .buildVolFrom = virStorageBackendDiskBuildVolFrom, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 1615c12..58e849f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1291,6 +1291,7 @@ virStorageBackend virStorageBackendDirectory = { .resizeVol = virStorageBackendFileSystemVolResize, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; #if WITH_STORAGE_FS @@ -1311,6 +1312,7 @@ virStorageBackend virStorageBackendFileSystem = { .resizeVol = virStorageBackendFileSystemVolResize, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; virStorageBackend virStorageBackendNetFileSystem = { .type = VIR_STORAGE_POOL_NETFS, @@ -1330,6 +1332,7 @@ virStorageBackend virStorageBackendNetFileSystem = { .resizeVol = virStorageBackendFileSystemVolResize, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 7345571..1d0cf73 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -476,4 +476,5 @@ virStorageBackend virStorageBackendISCSI = { .findPoolSources = virStorageBackendISCSIFindPoolSources, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index faa9a4b..562f019 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -843,4 +843,5 @@ virStorageBackend virStorageBackendLogical = { .deleteVol = virStorageBackendLogicalDeleteVol, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 402faa0..f21ae4c 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -289,4 +289,5 @@ virStorageBackend virStorageBackendMpath = { .refreshPool = virStorageBackendMpathRefreshPool, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 0b44f71..dfb663b 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -730,4 +730,5 @@ virStorageBackend virStorageBackendSCSI = { .stopPool = virStorageBackendSCSIStopPool, .uploadVol = virStorageBackendVolUploadLocal, .downloadVol = virStorageBackendVolDownloadLocal, + .wipeVol = virStorageBackendVolWipeLocal, }; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 97571e8..441da21 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2100,6 +2100,7 @@ storageVolWipePattern(virStorageVolPtr obj, unsigned int algorithm, unsigned int flags) { + virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; int ret = -1; @@ -2113,7 +2114,7 @@ storageVolWipePattern(virStorageVolPtr obj, return -1; } - if (!(vol = virStorageVolDefFromVol(obj, &pool, NULL))) + if (!(vol = virStorageVolDefFromVol(obj, &pool, &backend))) return -1; @@ -2134,10 +2135,13 @@ storageVolWipePattern(virStorageVolPtr obj, goto cleanup; } - if (virStorageBackendVolWipeLocal(obj->conn, pool, vol, algorithm, flags) < 0) + if (!backend->wipeVol) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("storage pool doesn't support volume wiping")); goto cleanup; + } - ret = 0; + ret = backend->wipeVol(obj->conn, pool, vol, algorithm, flags); cleanup: virStoragePoolObjUnlock(pool); -- 2.0.0
On 07/11/2014 08:20 AM, Peter Krempa wrote:
Version 2 now splits the stuff into separate driver backend funcs.
Peter Krempa (2): storage: wipe: Move helper code into storage backend storage: Split out volume wiping as separate backend function
src/storage/storage_backend.c | 203 +++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 12 ++ src/storage/storage_backend_disk.c | 1 + src/storage/storage_backend_fs.c | 3 + src/storage/storage_backend_iscsi.c | 1 + src/storage/storage_backend_logical.c | 1 + src/storage/storage_backend_mpath.c | 1 + src/storage/storage_backend_scsi.c | 1 + src/storage/storage_driver.c | 205 +--------------------------------- 9 files changed, 229 insertions(+), 199 deletions(-)
ACK both as they seem to fulfill the changes requested from http://www.redhat.com/archives/libvir-list/2014-July/msg00556.html John BTW: Since I was looking at a bug (bz 1091866) in this area - this caught my attention... Not that these changes fix the problem, but perhaps they'll help since as it seems "sparse snapshot" logical volumes are "problematic"...
On 07/16/14 18:54, John Ferlan wrote:
On 07/11/2014 08:20 AM, Peter Krempa wrote:
Version 2 now splits the stuff into separate driver backend funcs.
Peter Krempa (2): storage: wipe: Move helper code into storage backend storage: Split out volume wiping as separate backend function
src/storage/storage_backend.c | 203 +++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 12 ++ src/storage/storage_backend_disk.c | 1 + src/storage/storage_backend_fs.c | 3 + src/storage/storage_backend_iscsi.c | 1 + src/storage/storage_backend_logical.c | 1 + src/storage/storage_backend_mpath.c | 1 + src/storage/storage_backend_scsi.c | 1 + src/storage/storage_driver.c | 205 +--------------------------------- 9 files changed, 229 insertions(+), 199 deletions(-)
ACK both as they seem to fulfill the changes requested from http://www.redhat.com/archives/libvir-list/2014-July/msg00556.html
Thanks; Pushed.
John
BTW: Since I was looking at a bug (bz 1091866) in this area - this caught my attention... Not that these changes fix the problem, but perhaps they'll help since as it seems "sparse snapshot" logical volumes are "problematic"...
Well with this stuff pushed, you can easily add a wrapper function for the logical pool that will check whether the volume is sparse and forbid destroying it before relaying to the wiping function. Peter
participants (2)
-
John Ferlan -
Peter Krempa