diff mbox

[v3,1/3] block: resize backing file image during offline commit, if necessary

Message ID cbd2d430e1eb75264289b243a7863f8ed6570ea2.1390513348.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody Jan. 23, 2014, 9:48 p.m. UTC 
Currently, if an image file is logically larger than its backing file,
commiting it via 'qemu-img commit' will fail.

For instance, if we have a base image with a virtual size 10G, and a
snapshot image of size 20G, then committing the snapshot offline with
'qemu-img commit' will likely fail.

This will automatically attempt to resize the base image, if the
snapshot image to be committed is larger.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

Benoît Canet Jan. 23, 2014, 10 p.m. UTC | #1 
Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
> Currently, if an image file is logically larger than its backing file,
> commiting it via 'qemu-img commit' will fail.
> 
> For instance, if we have a base image with a virtual size 10G, and a
> snapshot image of size 20G, then committing the snapshot offline with
> 'qemu-img commit' will likely fail.
> 
> This will automatically attempt to resize the base image, if the
> snapshot image to be committed is larger.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 64e7d22..dea7591 100644
> --- a/block.c
> +++ b/block.c
> @@ -1876,10 +1876,10 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
>  int bdrv_commit(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> -    int64_t sector, total_sectors;
> +    int64_t sector, total_sectors, length, backing_length;
>      int n, ro, open_flags;
>      int ret = 0;
> -    uint8_t *buf;
> +    uint8_t *buf = NULL;

Why assign NULL to buf ? Is it related to the rest of the patch ?

Reviewed-by: Benoit Canet <benoit@irqsave.net>

>      char filename[PATH_MAX];
>  
>      if (!drv)
> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
>          }
>      }
>  
> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +    length = bdrv_getlength(bs);
> +    backing_length = bdrv_getlength(bs->backing_hd);
> +
> +    if (length < 0 || backing_length < 0) {
> +        goto ro_cleanup;
> +    }
> +
> +    /* If our top snapshot is larger than the backing file image,
> +     * grow the backing file image if possible.  If not possible,
> +     * we must return an error */
> +    if (length > backing_length) {
> +        ret = bdrv_truncate(bs->backing_hd, length);
> +        if (ret < 0) {
> +            goto ro_cleanup;
> +        }
> +    }
> +
> +    total_sectors = length >> BDRV_SECTOR_BITS;
>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
>  
>      for (sector = 0; sector < total_sectors; sector += n) {
> -- 
> 1.8.3.1
> 
>
Eric Blake Jan. 23, 2014, 10:07 p.m. UTC | #2 
On 01/23/2014 03:00 PM, Benoît Canet wrote:
> Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
>> Currently, if an image file is logically larger than its backing file,
>> commiting it via 'qemu-img commit' will fail.

s/commiting/committing/


>> +    uint8_t *buf = NULL;
> 
> Why assign NULL to buf ? Is it related to the rest of the patch ?
> 
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> 
>>      char filename[PATH_MAX];
>>  
>>      if (!drv)
>> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
>>          }
>>      }
>>  
>> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
>> +    length = bdrv_getlength(bs);
>> +    backing_length = bdrv_getlength(bs->backing_hd);
>> +
>> +    if (length < 0 || backing_length < 0) {
>> +        goto ro_cleanup;

Because this goto now reaches the ro_cleanup label with buf
uninitialized, if we don't assign NULL originally.

>> +    total_sectors = length >> BDRV_SECTOR_BITS;
>>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);

The old code only ever reached ro_cleanup after assigning buf, and
ro_cleanup blindly frees buf.
Jeff Cody Jan. 23, 2014, 10:14 p.m. UTC | #3 
On Thu, Jan 23, 2014 at 03:07:44PM -0700, Eric Blake wrote:
> On 01/23/2014 03:00 PM, Benoît Canet wrote:
> > Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
> >> Currently, if an image file is logically larger than its backing file,
> >> commiting it via 'qemu-img commit' will fail.
> 
> s/commiting/committing/
> 

I could respin, or Kevin / Stefan could fix this up whenever it is
applied to their branches.

> 
> >> +    uint8_t *buf = NULL;
> > 
> > Why assign NULL to buf ? Is it related to the rest of the patch ?
> > 
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > 
> >>      char filename[PATH_MAX];
> >>  
> >>      if (!drv)
> >> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
> >>          }
> >>      }
> >>  
> >> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> >> +    length = bdrv_getlength(bs);
> >> +    backing_length = bdrv_getlength(bs->backing_hd);
> >> +
> >> +    if (length < 0 || backing_length < 0) {
> >> +        goto ro_cleanup;
> 
> Because this goto now reaches the ro_cleanup label with buf
> uninitialized, if we don't assign NULL originally.
> 

Yup, exactly.

> >> +    total_sectors = length >> BDRV_SECTOR_BITS;
> >>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
> 
> The old code only ever reached ro_cleanup after assigning buf, and
> ro_cleanup blindly frees buf.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Benoît Canet Jan. 23, 2014, 10:35 p.m. UTC | #4 
Le Thursday 23 Jan 2014 à 15:07:44 (-0700), Eric Blake a écrit :
> On 01/23/2014 03:00 PM, Benoît Canet wrote:
> > Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
> >> Currently, if an image file is logically larger than its backing file,
> >> commiting it via 'qemu-img commit' will fail.
> 
> s/commiting/committing/
> 
> 
> >> +    uint8_t *buf = NULL;
> > 
> > Why assign NULL to buf ? Is it related to the rest of the patch ?
> > 
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > 
> >>      char filename[PATH_MAX];
> >>  
> >>      if (!drv)
> >> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
> >>          }
> >>      }
> >>  
> >> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> >> +    length = bdrv_getlength(bs);
> >> +    backing_length = bdrv_getlength(bs->backing_hd);
> >> +
> >> +    if (length < 0 || backing_length < 0) {
> >> +        goto ro_cleanup;
> 
> Because this goto now reaches the ro_cleanup label with buf
> uninitialized, if we don't assign NULL originally.

Thanks for the explanation.

Best regards

Benoît
> 
> >> +    total_sectors = length >> BDRV_SECTOR_BITS;
> >>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
> 
> The old code only ever reached ro_cleanup after assigning buf, and
> ro_cleanup blindly frees buf.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
diff mbox

Patch

diff --git a/block.c b/block.c
index 64e7d22..dea7591 100644
--- a/block.c
+++ b/block.c
@@ -1876,10 +1876,10 @@  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
 int bdrv_commit(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
-    int64_t sector, total_sectors;
+    int64_t sector, total_sectors, length, backing_length;
     int n, ro, open_flags;
     int ret = 0;
-    uint8_t *buf;
+    uint8_t *buf = NULL;
     char filename[PATH_MAX];
 
     if (!drv)
@@ -1904,7 +1904,24 @@  int bdrv_commit(BlockDriverState *bs)
         }
     }
 
-    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+    length = bdrv_getlength(bs);
+    backing_length = bdrv_getlength(bs->backing_hd);
+
+    if (length < 0 || backing_length < 0) {
+        goto ro_cleanup;
+    }
+
+    /* If our top snapshot is larger than the backing file image,
+     * grow the backing file image if possible.  If not possible,
+     * we must return an error */
+    if (length > backing_length) {
+        ret = bdrv_truncate(bs->backing_hd, length);
+        if (ret < 0) {
+            goto ro_cleanup;
+        }
+    }
+
+    total_sectors = length >> BDRV_SECTOR_BITS;
     buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
 
     for (sector = 0; sector < total_sectors; sector += n) {