Re: [PATCH v3 2/2] Storage : Fix cloning of raw, sparse volumes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 26, 2015 at 05:13:26PM +0530, Prerna Saxena wrote:
> When virsh vol-clone is attempted on a raw file where capacity > allocation,
> the resulting cloned volume has a size that matches the virtual-size of
> the parent; in place of matching its actual, disk size.
> This patch fixes the cloned disk to have same _allocated_size_ as
> the parent file from which it was cloned.
> 
> Ref: http://www.redhat.com/archives/libvir-list/2015-May/msg00050.html
> 
> Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1130739
> 
> Signed-off-by: Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx>
> ---
>  src/storage/storage_backend.c | 23 ++++++++++-------------
>  src/storage/storage_driver.c  |  5 -----
>  2 files changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index ce59f63..492b344 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -342,7 +342,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> -    remain = vol->target.allocation;
> +    remain = vol->target.capacity;
>  
>      if (inputvol) {
>          int res = virStorageBackendCopyToFD(vol, inputvol,
> @@ -397,7 +397,8 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>                virStorageVolDefPtr inputvol,
>                bool reflink_copy)
>  {
> -    bool need_alloc = true;
> +    bool want_sparse = (inputvol &&
> +             (inputvol->target.capacity > vol->target.allocation));
>      int ret = 0;
>      unsigned long long remain;
>  
> @@ -420,10 +421,9 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>       * to writing zeroes block by block in case fallocate isn't
>       * available, and since we're going to copy data from another
>       * file it doesn't make sense to write the file twice. */
> -    if (vol->target.allocation) {
> -        if (fallocate(fd, 0, 0, vol->target.allocation) == 0) {
> -            need_alloc = false;
> -        } else if (errno != ENOSYS && errno != EOPNOTSUPP) {
> +    if (vol->target.allocation && !want_sparse) {
> +        if ((fallocate(fd, 0, 0, vol->target.allocation) != 0) &&

If fallocate succeeds here, there's no need to copy zero bytes from the
source.

> +          (errno != ENOSYS && errno != EOPNOTSUPP)) {
>              ret = -errno;
>              virReportSystemError(errno,
>                                   _("cannot allocate %llu bytes in file '%s'"),
> @@ -433,22 +433,19 @@ createRawFile(int fd, virStorageVolDefPtr vol,
>      }
>  #endif
>  
> -    remain = vol->target.allocation;
> +    remain = vol->target.capacity;
>  

This breaks sparse file creation on systems without fallocate.

>      if (inputvol) {
>          /* allow zero blocks to be skipped if we've requested sparse
> -         * allocation (allocation < capacity) or we have already
> -         * been able to allocate the required space. */
> -        bool want_sparse = !need_alloc ||
> -            (vol->target.allocation < inputvol->target.capacity);
> -
> +         * allocation (allocation < capacity)
> +         */
>          ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain,
>                                          want_sparse, reflink_copy);

I'd rahter use !need_alloc instead of want_sparse here, because
sparseness only makes sense for cloning volumes, but the bool affects
creating new volumes as well.

>          if (ret < 0)
>              goto cleanup;
>      }
>  
> -    if (remain && need_alloc) {
> +    if (remain && !want_sparse) {
>          if (safezero(fd, vol->target.allocation - remain, remain) < 0) {

This calculation assumes that 'remain' contains the bytes remaining to
the target allocation, but after this patch it contains the bytes
remaining to the target capacity.

>              ret = -errno;
>              virReportSystemError(errno, _("cannot fill file '%s'"),

I'll send a v4 shortly.

Jan

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]