[libvirt] [PATCH 8/9] Simplified version of volume zeroing based on feedback from the list.

[Daniel P. Berrange]

On Tue, Mar 02, 2010 at 05:13:33PM -0500, David Allan wrote:
> @@ -1518,6 +1521,220 @@ cleanup:
>      return ret;
> +
> +static int
> +storageZeroExtent(virStorageVolDefPtr vol,
> +                  struct stat *st,
> +                  int fd,
> +                  size_t extent_start,
> +                  size_t extent_length,
> +                  char *writebuf,
> +                  size_t *bytes_zeroed)
> +{
> +    int ret = -1, written;
> +    size_t remaining, write_size;
> +    char errbuf[64];
> +
> +    VIR_DEBUG("extent logical start: %zu len: %zu ",
> +              extent_start, extent_length);
> +
> +    if ((ret = lseek(fd, extent_start, SEEK_SET)) < 0) {
> +        virReportSystemError(ret, "Failed to seek to position %zu in volume "
> +                             "with path '%s': '%s'",
> +                             extent_start, vol->target.path,
> +                             virStrerror(errno, errbuf, sizeof(errbuf)));
> +        goto out;
> +    }
> +
> +    remaining = extent_length;
> +    while (remaining > 0) {
> +
> +        write_size = (st->st_blksize < remaining) ? st->st_blksize : remaining;
> +        written = safewrite(fd, writebuf, write_size);

This is a little fragile, because its making storageZeroExtent() assume that the
passed in 'writebuf' is st->st_blksize. I think it'd be better if a 'writebuflen'
were passed in from the caller

> +        if (written < 0) {
> +            virReportSystemError(written,
> +                                 _("Failed to write to storage volume with "
> +                                   "path '%s': '%s' "
> +                                   "(attempted to write %d bytes)"),
> +                                 vol->target.path,
> +                                 virStrerror(errno, errbuf, sizeof(errbuf)),
> +                                 write_size);
> +            goto out;
> +        }
> +
> +        *bytes_zeroed += written;
> +        remaining -= written;
> +    }
> +
> +    VIR_DEBUG("Wrote %zu bytes to volume with path '%s'",
> +              *bytes_zeroed, vol->target.path);
> +
> +    ret = 0;
> +
> +out:
> +    return ret;
> +}
> +
> +
> +static int
> +storageVolumeZeroOutInternal(virStorageVolDefPtr def)
> +{
> +    int ret = -1, fd = -1;
> +    char errbuf[64];
> +    struct stat st;
> +    char *writebuf;
> +    size_t bytes_zeroed = 0;
> +
> +    VIR_DEBUG("Zeroing out 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;
> +    }
> +
> +    memset(&st, 0, sizeof(st));

That's redundant I believe - at least no other fstat calls ever memset
their buffer ahead of time.

> +
> +    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, fd);
> +    } else {
> +
> +        if (VIR_ALLOC_N(writebuf, st.st_blksize) != 0) {
> +            virReportOOMError();
> +            goto out;
> +        }
> +
> +        ret = storageZeroExtent(def,
> +                                &st,
> +                                fd,
> +                                0,
> +                                def->allocation,
> +                                writebuf,
> +                                &bytes_zeroed);

Add st.st_blksize as a param here

> +    }
> +
> +out:
> +    VIR_FREE(writebuf);
> +
> +    if (fd != -1) {
> +        close(fd);
> +    }
> +
> +    return ret;
> +}
> +
> +
> +static int
> +storageVolumeZeroOut(virStorageVolPtr obj,
> +                     unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    virStorageDriverStatePtr driver = obj->conn->storagePrivateData;
> +    virStoragePoolObjPtr pool;
> +    virStorageVolDefPtr vol = NULL;
> +    int ret = -1;

As Eric suggested, this would be a good time to do an explicit 'flags == 0'
check, so that callers can detect whether a flag is supported or not.

> +
> +    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 (storageVolumeZeroOutInternal(vol) == -1) {
> +        goto out;
> +    }
> +
> +    ret = 0;
> +
> +out:
> +    if (pool) {
> +        virStoragePoolObjUnlock(pool);
> +    }
> +
> +    return ret;
> +
> +}
> +
>  static int
>  storageVolumeDelete(virStorageVolPtr obj,
>                      unsigned int flags) {
> @@ -1775,6 +1992,7 @@ static virStorageDriver storageDriver = {
>      .volCreateXML = storageVolumeCreateXML,
>      .volCreateXMLFrom = storageVolumeCreateXMLFrom,
>      .volDelete = storageVolumeDelete,
> +    .volZeroOut = storageVolumeZeroOut,
>      .volGetInfo = storageVolumeGetInfo,
>      .volGetXMLDesc = storageVolumeGetXMLDesc,
>      .volGetPath = storageVolumeGetPath,
> -- 


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|