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

2015-06-25 Thread Prerna Saxena 

On Tuesday 23 June 2015 06:29 PM, Ján Tomko wrote:
 On Mon, Jun 22, 2015 at 05:09:18PM +0530, Prerna Saxena wrote:
 From: Prerna Saxena pre...@linux.vnet.ibm.com
 Date: Mon, 22 Jun 2015 02:54:32 -0500

 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 pre...@linux.vnet.ibm.com
 ---
  src/storage/storage_backend.c | 10 +-
  src/storage/storage_driver.c  |  5 -
  2 files changed, 5 insertions(+), 10 deletions(-)

 diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
 index ce59f63..c99b718 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,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
bool reflink_copy)
  {
 -bool need_alloc = true;
 +bool need_alloc = !(inputvol  (inputvol-target.capacity  
 inputvol-target.allocation));
 Comparing 'inputvol-target.capacity  vol-target.allocation' would
 allow creating a sparse volume from a non-sparse one and vice-versa
 by specifying the correct allocation.

Ok, I was not sure if libvirt wanted to do that.
If the parent volume is a sparse volume, I'd expect the cloned volume to be 
sparse too. Likewise, for a non-sparse parent, the cloned volume should also be 
non-sparse. Should that not be something we
honour in libvirt, when we clone ?

  int ret = 0;
  unsigned long long remain;
  
 @@ -420,7 +420,7 @@ 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 (vol-target.allocation  need_alloc) {
  if (fallocate(fd, 0, 0, vol-target.allocation) == 0) {
  need_alloc = false;
  } else if (errno != ENOSYS  errno != EOPNOTSUPP) {
 @@ -433,14 +433,14 @@ createRawFile(int fd, virStorageVolDefPtr vol,
  }
  #endif
  
 -remain = vol-target.allocation;
 +remain = vol-target.capacity;
  
  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);
 +(inputvol-target.allocation  inputvol-target.capacity);
 If allocation  capacity, then need_alloc is already false.

I was trying to accomodate the original usecase of need_alloc, but you are 
right. This will go away in the next version of this series, which I will post 
shortly.

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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

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

2015-06-23 Thread Ján Tomko 
On Mon, Jun 22, 2015 at 05:09:18PM +0530, Prerna Saxena wrote:
 From: Prerna Saxena pre...@linux.vnet.ibm.com
 Date: Mon, 22 Jun 2015 02:54:32 -0500
 
 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 pre...@linux.vnet.ibm.com
 ---
  src/storage/storage_backend.c | 10 +-
  src/storage/storage_driver.c  |  5 -
  2 files changed, 5 insertions(+), 10 deletions(-)
 
 diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
 index ce59f63..c99b718 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,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
bool reflink_copy)
  {
 -bool need_alloc = true;
 +bool need_alloc = !(inputvol  (inputvol-target.capacity  
 inputvol-target.allocation));

Comparing 'inputvol-target.capacity  vol-target.allocation' would
allow creating a sparse volume from a non-sparse one and vice-versa
by specifying the correct allocation.

  int ret = 0;
  unsigned long long remain;
  
 @@ -420,7 +420,7 @@ 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 (vol-target.allocation  need_alloc) {
  if (fallocate(fd, 0, 0, vol-target.allocation) == 0) {
  need_alloc = false;
  } else if (errno != ENOSYS  errno != EOPNOTSUPP) {
 @@ -433,14 +433,14 @@ createRawFile(int fd, virStorageVolDefPtr vol,
  }
  #endif
  
 -remain = vol-target.allocation;
 +remain = vol-target.capacity;
  
  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);
 +(inputvol-target.allocation  inputvol-target.capacity);

If allocation  capacity, then need_alloc is already false.

Jan


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-06-22 Thread Prerna Saxena 
From: Prerna Saxena pre...@linux.vnet.ibm.com
Date: Mon, 22 Jun 2015 02:54:32 -0500

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 pre...@linux.vnet.ibm.com
---
 src/storage/storage_backend.c | 10 +-
 src/storage/storage_driver.c  |  5 -
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index ce59f63..c99b718 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,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
   virStorageVolDefPtr inputvol,
   bool reflink_copy)
 {
-bool need_alloc = true;
+bool need_alloc = !(inputvol  (inputvol-target.capacity  
inputvol-target.allocation));
 int ret = 0;
 unsigned long long remain;
 
@@ -420,7 +420,7 @@ 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 (vol-target.allocation  need_alloc) {
 if (fallocate(fd, 0, 0, vol-target.allocation) == 0) {
 need_alloc = false;
 } else if (errno != ENOSYS  errno != EOPNOTSUPP) {
@@ -433,14 +433,14 @@ createRawFile(int fd, virStorageVolDefPtr vol,
 }
 #endif
 
-remain = vol-target.allocation;
+remain = vol-target.capacity;
 
 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);
+(inputvol-target.allocation  inputvol-target.capacity);
 
 ret = virStorageBackendCopyToFD(vol, inputvol, fd, remain,
 want_sparse, reflink_copy);
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 4980546..c1dfe89 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1976,11 +1976,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 if (newvol-target.capacity  origvol-target.capacity)
 newvol-target.capacity = origvol-target.capacity;
 
-/* Make sure allocation is at least as large as the destination cap,
- * to make absolutely sure we copy all possible contents */
-if (newvol-target.allocation  origvol-target.capacity)
-newvol-target.allocation = origvol-target.capacity;
-
 if (!backend-buildVolFrom) {
 virReportError(VIR_ERR_NO_SUPPORT,
%s, _(storage pool does not support
-- 
1.8.3.1

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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

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

2015-05-05 Thread Ján Tomko 
On Tue, May 05, 2015 at 08:47:56AM +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.
 
 Reference: 
 https://www.redhat.com/archives/libvir-list/2014-September/msg00064.html
 
 Also fixes : https://bugzilla.redhat.com/show_bug.cgi?id=1130739
 
 Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
 ---
  src/storage/storage_backend.c | 2 +-
  src/storage/storage_driver.c  | 5 -
  2 files changed, 1 insertion(+), 6 deletions(-)
 
 diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
 index 355fc7f..1a7c0cc 100644
 --- a/src/storage/storage_backend.c
 +++ b/src/storage/storage_backend.c
 @@ -429,7 +429,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
  }
  #endif
  
 -remain = vol-target.allocation;
 +remain = inputvol-target.capacity;
  

If we're not cloning a volume, inputvol is NULL and this will crash.

  if (inputvol) {

After moving the assignment here, we will copy all the data from the input 
volume,
but we still use the original value of 'allocation' to fallocate the
start of the file.

So for an original 5GB file with 3 GB allocated (where + is an allocated
gigabyte):
  +_+_+
We'll end up with a file that has 4 GB allocated:
  +++_+
(We'll allocate the first 3 GB, and copy any remaining data)

I think we do not have to honor an allocation value less than the capacity
of the original volume and just assume it means 'create a sparse file'.

So I'd suggest:
1. setting the need_alloc to false if we're cloning a volume and the new
   allocation is less than the *original* capacity
2. making the fallocate call depend on need_alloc

  /* allow zero blocks to be skipped if we've requested sparse

Jan


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] Storage : Fix cloning of raw, sparse volumes.

2015-05-04 Thread Prerna Saxena 
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.

Reference: 
https://www.redhat.com/archives/libvir-list/2014-September/msg00064.html

Also fixes : https://bugzilla.redhat.com/show_bug.cgi?id=1130739

Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
---
 src/storage/storage_backend.c | 2 +-
 src/storage/storage_driver.c  | 5 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 355fc7f..1a7c0cc 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -429,7 +429,7 @@ createRawFile(int fd, virStorageVolDefPtr vol,
 }
 #endif
 
-remain = vol-target.allocation;
+remain = inputvol-target.capacity;
 
 if (inputvol) {
 /* allow zero blocks to be skipped if we've requested sparse
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ac4a74a..c511035 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1990,11 +1990,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 if (newvol-target.capacity  origvol-target.capacity)
 newvol-target.capacity = origvol-target.capacity;
 
-/* Make sure allocation is at least as large as the destination cap,
- * to make absolutely sure we copy all possible contents */
-if (newvol-target.allocation  origvol-target.capacity)
-newvol-target.allocation = origvol-target.capacity;
-
 if (!backend-buildVolFrom) {
 virReportError(VIR_ERR_NO_SUPPORT,
%s, _(storage pool does not support
-- 
1.8.3.1


-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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