[libvirt] [PATCH 8/9] Simplified version of volume wiping based on feedback from the list.
Daniel Veillard
veillard at redhat.com
Wed Mar 17 14:13:12 UTC 2010
On Mon, Mar 15, 2010 at 10:13:30PM -0400, David Allan wrote:
> ---
> src/storage/storage_driver.c | 224 ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 224 insertions(+), 0 deletions(-)
>
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index ca2540f..1961fa0 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -26,6 +26,9 @@
> #include <stdio.h>
> #include <unistd.h>
> #include <sys/types.h>
> +#include <sys/param.h>
> +#include <fcntl.h>
> +
> #if HAVE_PWD_H
> # include <pwd.h>
> #endif
> @@ -1518,6 +1521,226 @@ cleanup:
> 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
> +storageVolumeZeroSparseFile(virStorageVolDefPtr vol,
> + off_t size,
> + int fd)
> +{
> + int ret = -1;
no need to initialize here
> + char errbuf[64];
While the 1024 used many places in the code is probably a bit too
much, 64 chars might be a bit small for an errno return description
I would bump it to 128,
> + ret = ftruncate(fd, 0);
> + if (ret == -1) {
> + virReportSystemError(ret,
> + _("Failed to truncate volume with "
> + "path '%s' to 0 bytes: '%s'"),
> + vol->target.path,
> + virStrerror(errno, errbuf, sizeof(errbuf)));
> + goto out;
> + }
> +
> + ret = ftruncate(fd, size);
> + if (ret == -1) {
> + virReportSystemError(ret,
> + _("Failed to truncate volume with "
> + "path '%s' to %ju bytes: '%s'\n"),
> + vol->target.path, (intmax_t)size,
> + virStrerror(errno, errbuf, sizeof(errbuf)));
> + }
> +
> +out:
> + 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;
no need to initialize ret here
> + off_t remaining = 0;
> + size_t write_size = 0;
> + char errbuf[64];
same 128 vs 64
> + VIR_DEBUG("extent logical start: %ju len: %ju",
> + (intmax_t)extent_start, (intmax_t)extent_length);
> +
> + if ((ret = lseek(fd, extent_start, SEEK_SET)) < 0) {
> + virReportSystemError(ret,
> + _("Failed to seek to position %ju in volume "
> + "with path '%s': '%s'"),
> + (intmax_t)extent_start, vol->target.path,
> + virStrerror(errno, errbuf, sizeof(errbuf)));
> + goto out;
> + }
> +
> + remaining = extent_length;
> + while (remaining > 0) {
> +
> + write_size = (writebuf_length < remaining) ? writebuf_length : remaining;
> + written = safewrite(fd, writebuf, write_size);
> + if (written < 0) {
> + virReportSystemError(written,
> + _("Failed to write to storage volume with "
> + "path '%s': '%s' "
> + "(attempted to write %zu bytes)"),
> + vol->target.path,
> + virStrerror(errno, errbuf, sizeof(errbuf)),
> + write_size);
> + goto out;
> + }
> +
> + *bytes_wiped += written;
> + remaining -= written;
> + }
> +
> + VIR_DEBUG("Wrote %zu bytes to volume with path '%s'",
> + *bytes_wiped, vol->target.path);
> +
> + ret = 0;
> +
> +out:
> + return ret;
> +}
> +
> +
> +static int
> +storageVolumeWipeInternal(virStorageVolDefPtr def)
> +{
> + int ret = -1, fd = -1;
> + char errbuf[64];
> + struct stat st;
> + char *writebuf = NULL;
> + size_t bytes_wiped = 0;
> +
> + VIR_DEBUG("Wiping volume with path '%s'", def->target.path);
> +
> + fd = open(def->target.path, O_RDWR);
> + if (fd == -1) {
> + VIR_ERROR("Failed to open storage volume with path '%s': '%s'",
> + def->target.path,
> + virStrerror(errno, errbuf, sizeof(errbuf)));
> + goto out;
> + }
> +
> + if (fstat(fd, &st) == -1) {
> + VIR_ERROR("Failed to stat storage volume with path '%s': '%s'",
> + def->target.path,
> + virStrerror(errno, errbuf, sizeof(errbuf)));
> + goto out;
> + }
> +
> + if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) {
> + ret = storageVolumeZeroSparseFile(def, st.st_size, fd);
> + } else {
> +
> + if (VIR_ALLOC_N(writebuf, st.st_blksize) != 0) {
> + virReportOOMError();
> + goto out;
> + }
Urgh .... that sounds quite inefficient to me. Assuming a block size of
512, that means 2000 safewrite/write/syscall() per megabyte. Can we at
least increase that to 64K, the loop on remaining on the called function
should insure that there is no problem at the end of the extent
> + ret = storageWipeExtent(def,
> + fd,
> + 0,
> + def->allocation,
> + writebuf,
> + st.st_blksize,
> + &bytes_wiped);
> + }
> +
> +out:
> + VIR_FREE(writebuf);
> +
> + if (fd != -1) {
> + close(fd);
> + }
> +
> + return ret;
> +}
> +
> +
> +static int
> +storageVolumeWipe(virStorageVolPtr obj,
> + unsigned int flags)
> +{
> + virStorageDriverStatePtr driver = obj->conn->storagePrivateData;
> + virStoragePoolObjPtr pool = NULL;
> + virStorageVolDefPtr vol = NULL;
> + int ret = -1;
> +
> + if (flags != 0) {
> + virStorageReportError(VIR_ERR_INVALID_ARG,
> + _("Unsupported flags (0x%x) passed to '%s'"), flags, __FUNCTION__);
> + goto out;
> + }
> +
> + storageDriverLock(driver);
> + pool = virStoragePoolObjFindByName(&driver->pools, obj->pool);
> + storageDriverUnlock(driver);
> +
> + if (!pool) {
> + virStorageReportError(VIR_ERR_INVALID_STORAGE_POOL,
> + "%s", _("no storage pool with matching uuid"));
> + goto out;
> + }
> +
> + if (!virStoragePoolObjIsActive(pool)) {
> + virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("storage pool is not active"));
> + goto out;
> + }
> +
> + vol = virStorageVolDefFindByName(pool, obj->name);
> +
> + if (vol == NULL) {
> + virStorageReportError(VIR_ERR_NO_STORAGE_VOL,
> + _("no storage vol with matching name '%s'"),
> + obj->name);
> + goto out;
> + }
> +
> + if (vol->building) {
> + virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> + _("volume '%s' is still being allocated."),
> + vol->name);
> + goto out;
> + }
> +
> + if (storageVolumeWipeInternal(vol) == -1) {
> + goto out;
> + }
> +
> + ret = 0;
> +
> +out:
> + if (pool) {
> + virStoragePoolObjUnlock(pool);
> + }
> +
> + return ret;
> +
> +}
Hum, where did we lock the pool ? Does virStoragePoolObjFindByName()
lock it automatically, I think I remember seomthing like that but I
would prefer to be sure.
> static int
> storageVolumeDelete(virStorageVolPtr obj,
> unsigned int flags) {
> @@ -1775,6 +1998,7 @@ static virStorageDriver storageDriver = {
> .volCreateXML = storageVolumeCreateXML,
> .volCreateXMLFrom = storageVolumeCreateXMLFrom,
> .volDelete = storageVolumeDelete,
> + .volWipe = storageVolumeWipe,
> .volGetInfo = storageVolumeGetInfo,
> .volGetXMLDesc = storageVolumeGetXMLDesc,
> .volGetPath = storageVolumeGetPath,
ACK once the wiping buffer size is incremented to something decent :-)
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list