[Qemu-devel] Qemu-img convert with -B

[Qemu-devel] Qemu-img convert with -B

[Brad Campbell]

G'day all,

I see there is a bug raised about the behaviour of qemu-img when used to 
convert using an output backing file. It allocates every sector whether 
or not it already exists in the output backing file.

I'm walking my way through the block driver to try and get a handle on 
why this is the case.

The comment in qemu-img.c is :

                 /* If the output image is being created as a copy on 
write image,
                    copy all sectors even the ones containing only NUL 
bytes,
                    because they may differ from the sectors in the base 
image.

Can someone verify these assumptions for me please?
- I can bdrv_open() a file that has a chain of backing files, and the 
following is true :
	- bdrv_read() returns the most recently allocated sector contents (or 0)
	- bdrv_is_allocated() will return false only if that sector is not 
allocated in _any_ of the files in the chain

If these assumptions are true, can anyone see a logical error in 
bdrv_open() both the input file and the out_baseimg, and where sectors 
are allocated comparing them to see if the are the same. If so, not 
allocating those in the output file?

I'm assuming the comment above is to allow for the case that someone 
specifies the output backing file being different from the last input 
backing file.

Am I missing something?

Regards,
Brad

Re: [Qemu-devel] Qemu-img convert with -B

[Stefan Hajnoczi]

On Wed, Apr 27, 2011 at 4:05 AM, Brad Campbell
<lists2009@fnarfbargle.com> wrote:
> I see there is a bug raised about the behaviour of qemu-img when used to convert using an output backing file. It allocates every sector whether or not it already exists in the output backing file.

Please post the link to the bug report.

> Can someone verify these assumptions for me please?
> - I can bdrv_open() a file that has a chain of backing files, and the
> following is true :
>        - bdrv_read() returns the most recently allocated sector contents (or
> 0)

Correct.

>        - bdrv_is_allocated() will return false only if that sector is not
> allocated in _any_ of the files in the chain

Incorrect.  It returns true if the sector is allocated in the top-most
file, false otherwise.  In other words bdrv_is_allocated() is flat, it
does not traverse a chain of backing files.

Stefan

Re: [Qemu-devel] Qemu-img convert with -B

[Brad Campbell]

On 27/04/11 16:10, Stefan Hajnoczi wrote:
> On Wed, Apr 27, 2011 at 4:05 AM, Brad Campbell
> <lists2009@fnarfbargle.com>  wrote:
>> I see there is a bug raised about the behaviour of qemu-img when used to convert using an output backing file. It allocates every sector whether or not it already exists in the output backing file.
> Please post the link to the bug report.
>
Yeah, sorry about that. Not very clever of me.

https://bugs.launchpad.net/qemu/+bug/660366
>> Can someone verify these assumptions for me please?
>> - I can bdrv_open() a file that has a chain of backing files, and the
>> following is true :
>>         - bdrv_read() returns the most recently allocated sector contents (or
>> 0)
> Correct.
>
>>         - bdrv_is_allocated() will return false only if that sector is not
>> allocated in _any_ of the files in the chain
> Incorrect.  It returns true if the sector is allocated in the top-most
> file, false otherwise.  In other words bdrv_is_allocated() is flat, it
> does not traverse a chain of backing files.

Right.

I guess the correct way to do this is to open and traverse all the input and output backing files, 
but I don't see why that should be necessary as the output file is created O_RDWR.

Now as the output file is created with the backing_file option, can I simply bdrv_read() both input 
and output files, and only write to the output file if the sector differs or != 0? Seems like that 
would be the logical way to do everything right while leveraging the complexity of the block 
drivers. It would also allow for maximum "compression" of the output file if the filesystem has all 
unused space wiped (which is my desired usage case).

Brad

Re: [Qemu-devel] Qemu-img convert with -B

[Kevin Wolf]

Am 27.04.2011 10:56, schrieb Brad Campbell:
> On 27/04/11 16:10, Stefan Hajnoczi wrote:
>> On Wed, Apr 27, 2011 at 4:05 AM, Brad Campbell
>> <lists2009@fnarfbargle.com>  wrote:
>>> I see there is a bug raised about the behaviour of qemu-img when used to convert using an output backing file. It allocates every sector whether or not it already exists in the output backing file.
>> Please post the link to the bug report.
>>
> Yeah, sorry about that. Not very clever of me.
> 
> https://bugs.launchpad.net/qemu/+bug/660366

I think this bug is fixed by commit a18953fb.

>>> Can someone verify these assumptions for me please?
>>> - I can bdrv_open() a file that has a chain of backing files, and the
>>> following is true :
>>>         - bdrv_read() returns the most recently allocated sector contents (or
>>> 0)
>> Correct.
>>
>>>         - bdrv_is_allocated() will return false only if that sector is not
>>> allocated in _any_ of the files in the chain
>> Incorrect.  It returns true if the sector is allocated in the top-most
>> file, false otherwise.  In other words bdrv_is_allocated() is flat, it
>> does not traverse a chain of backing files.
> 
> Right.
> 
> I guess the correct way to do this is to open and traverse all the input and output backing files, 
> but I don't see why that should be necessary as the output file is created O_RDWR.
> 
> Now as the output file is created with the backing_file option, can I simply bdrv_read() both input 
> and output files, and only write to the output file if the sector differs or != 0? Seems like that 
> would be the logical way to do everything right while leveraging the complexity of the block 
> drivers. It would also allow for maximum "compression" of the output file if the filesystem has all 
> unused space wiped (which is my desired usage case).

qemu-img convert -B is supposed to work only with unchanged backing
files! I'm not aware of any major use cases besides renaming the backing
file (for which rebase -u exists today), so it's only there for
compatibility reasons.  What you describe looks much more like qemu-img
rebase.

Kevin

Re: [Qemu-devel] Qemu-img convert with -B

[Brad Campbell]

On 27/04/11 18:06, Kevin Wolf wrote:
> Am 27.04.2011 10:56, schrieb Brad Campbell:
>> On 27/04/11 16:10, Stefan Hajnoczi wrote:
>>> On Wed, Apr 27, 2011 at 4:05 AM, Brad Campbell
>>> <lists2009@fnarfbargle.com>   wrote:
>>>> I see there is a bug raised about the behaviour of qemu-img when used to convert using an output backing file. It allocates every sector whether or not it already exists in the output backing file.
>>> Please post the link to the bug report.
>>>
>> Yeah, sorry about that. Not very clever of me.
>>
>> https://bugs.launchpad.net/qemu/+bug/660366
>
> I think this bug is fixed by commit a18953fb.

And indeed it is. Thus while the issue I'm facing looks like that bug, 
it's either another one or my misunderstanding about how backing files work.

So what is happening here then?

- create 1.img as a 20G qcow2
- 1.img is 193k
- Install windows XP into 1.img
- 1.img is 1.5G
- create 2.img as a qcow2 with 1.img as a backing file.
- 2.img is ~150k
- Install/uninstall and generally use 2.img
- 2.img is 7G
- Mount 2.img with systemrescuecd and use ntfswipe -a which zero's all 
unused data and cluster tails.
- 2.img is 20G
- qemu-img convert -O qcow2 -o backing_file 1.img 2.img 3.img
- 3.img is 20G

If I do the same process without the backing file..

- create 1.img as a 20G qcow2
- 1.img is 193k
- Install windows XP into 1.img
- 1.img is 1.5G
- Install/Uninstall and generally use 1.img
- 1.img is 7G
- Mount 1.img with systemrescuecd and use ntfswipe
- 1.img is 20G
- qemu-img convert -O qcow2 1.img 3.img
- 3.img is 4G

Why does the first example write out all the zeroed sectors into the 
image while the second one doesn't?

Regards,
Brad

Re: [Qemu-devel] Qemu-img convert with -B

[Kevin Wolf]

Am 27.04.2011 15:45, schrieb Brad Campbell:
> On 27/04/11 18:06, Kevin Wolf wrote:
>> Am 27.04.2011 10:56, schrieb Brad Campbell:
>>> On 27/04/11 16:10, Stefan Hajnoczi wrote:
>>>> On Wed, Apr 27, 2011 at 4:05 AM, Brad Campbell
>>>> <lists2009@fnarfbargle.com>   wrote:
>>>>> I see there is a bug raised about the behaviour of qemu-img when used to convert using an output backing file. It allocates every sector whether or not it already exists in the output backing file.
>>>> Please post the link to the bug report.
>>>>
>>> Yeah, sorry about that. Not very clever of me.
>>>
>>> https://bugs.launchpad.net/qemu/+bug/660366
>>
>> I think this bug is fixed by commit a18953fb.
> 
> And indeed it is. Thus while the issue I'm facing looks like that bug, 
> it's either another one or my misunderstanding about how backing files work.
> 
> So what is happening here then?
> 
> - create 1.img as a 20G qcow2
> - 1.img is 193k
> - Install windows XP into 1.img
> - 1.img is 1.5G
> - create 2.img as a qcow2 with 1.img as a backing file.
> - 2.img is ~150k
> - Install/uninstall and generally use 2.img
> - 2.img is 7G
> - Mount 2.img with systemrescuecd and use ntfswipe -a which zero's all 
> unused data and cluster tails.
> - 2.img is 20G
> - qemu-img convert -O qcow2 -o backing_file 1.img 2.img 3.img
> - 3.img is 20G
> 
> If I do the same process without the backing file..
> 
> - create 1.img as a 20G qcow2
> - 1.img is 193k
> - Install windows XP into 1.img
> - 1.img is 1.5G
> - Install/Uninstall and generally use 1.img
> - 1.img is 7G
> - Mount 1.img with systemrescuecd and use ntfswipe
> - 1.img is 20G
> - qemu-img convert -O qcow2 1.img 3.img
> - 3.img is 4G
> 
> Why does the first example write out all the zeroed sectors into the 
> image while the second one doesn't?

When you don't have a backing file, leaving an cluster unallocated means
that it's zero. When you have a backing file, it could be anything. So
if qemu-img convert wanted to save this space, it would have to read
from the backing file and leave the cluster unallocated if it reads as zero.

This is something that qemu-img doesn't do today.

Kevin

Re: [Qemu-devel] Qemu-img convert with -B

[Brad Campbell]

On 27/04/11 21:56, Kevin Wolf wrote:

> When you don't have a backing file, leaving an cluster unallocated means
> that it's zero. When you have a backing file, it could be anything. So
> if qemu-img convert wanted to save this space, it would have to read
> from the backing file and leave the cluster unallocated if it reads as zero.
>
> This is something that qemu-img doesn't do today.

... which would explain the behaviour I'm seeing then. Great, time to 
dust off my Kernighan & Ritchie I guess.

Thanks for the explanation.

Regards,
Brad

Re: [Qemu-devel] Qemu-img convert with -B

[Brad Campbell]

On 27/04/11 22:02, Brad Campbell wrote:
> On 27/04/11 21:56, Kevin Wolf wrote:
>
>> When you don't have a backing file, leaving an cluster unallocated means
>> that it's zero. When you have a backing file, it could be anything. So
>> if qemu-img convert wanted to save this space, it would have to read
>> from the backing file and leave the cluster unallocated if it reads as
>> zero.
>>
>> This is something that qemu-img doesn't do today.
>

This passes cursory testing, but I'm just wondering if this is along the 
right lines?

diff --git a/qemu-img.c b/qemu-img.c
index d9c2c12..ab4c70c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1,4 +1,4 @@
-/*
+ /*
   * QEMU disk image utility
   *
   * Copyright (c) 2003-2008 Fabrice Bellard
@@ -571,11 +571,12 @@ static int img_convert(int argc, char **argv)
      int progress = 0;
      const char *fmt, *out_fmt, *out_baseimg, *out_filename;
      BlockDriver *drv, *proto_drv;
-    BlockDriverState **bs = NULL, *out_bs = NULL;
+    BlockDriverState **bs = NULL, *out_bs = NULL, *out_bf = NULL;
      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
      uint64_t bs_sectors;
      uint8_t * buf = NULL;
      const uint8_t *buf1;
+    uint8_t * buf3 = NULL;
      BlockDriverInfo bdi;
      QEMUOptionParameter *param = NULL, *create_options = NULL;
      QEMUOptionParameter *out_baseimg_param;
@@ -719,6 +720,12 @@ static int img_convert(int argc, char **argv)
      out_baseimg_param = get_option_parameter(param, 
BLOCK_OPT_BACKING_FILE);
      if (out_baseimg_param) {
          out_baseimg = out_baseimg_param->value.s;
+       out_bf = bdrv_new_open(out_baseimg, NULL, BDRV_O_FLAGS);
+       if (!out_bf) {
+            error_report("Could not open backing file '%s'", out_baseimg);
+            ret = -1;
+            goto out;
+        }
      }

      /* Check if compression is supported */
@@ -767,6 +774,9 @@ static int img_convert(int argc, char **argv)
      bs_offset = 0;
      bdrv_get_geometry(bs[0], &bs_sectors);
      buf = qemu_malloc(IO_BUF_SIZE);
+    if (out_baseimg) {
+        buf3 = qemu_malloc(IO_BUF_SIZE);
+    }

      if (compress) {
          ret = bdrv_get_info(out_bs, &bdi);
@@ -889,9 +899,17 @@ static int img_convert(int argc, char **argv)
                     are present in both the output's and input's base 
images (no
                     need to copy them). */
                  if (out_baseimg) {
-                    if (!bdrv_is_allocated(bs[bs_i], sector_num - 
bs_offset,
-                                           n, &n1)) {
+                    if (bdrv_read(bs[bs_i], sector_num - bs_offset, 
buf, n) < 0) {
+                        error_report("error while reading input file");
+                        goto out;
+                    }
+                    if (bdrv_read(out_bf, sector_num - bs_offset, buf3, 
n) < 0) {
+                        error_report("error while reading backing file");
+                        goto out;
+                    }
+                    if (!compare_sectors(buf, buf3, n, &n1)) {
                          sector_num += n1;
+    //                    printf("Skipping %u sectors\n",n1);
                          continue;
                      }
                      /* The next 'n1' sectors are allocated in the 
input image. Copy
@@ -939,9 +957,16 @@ out:
      free_option_parameters(create_options);
      free_option_parameters(param);
      qemu_free(buf);
+    if (buf3) {
+        qemu_free(buf3);
+    }
      if (out_bs) {
          bdrv_delete(out_bs);
      }
+    if (out_bf) {
+        bdrv_delete(out_bf);
+    }
+
      if (bs) {
          for (bs_i = 0; bs_i < bs_n; bs_i++) {
              if (bs[bs_i]) {

Re: [Qemu-devel] Qemu-img convert with -B

[Kevin Wolf]

Am 28.04.2011 04:06, schrieb Brad Campbell:
> On 27/04/11 22:02, Brad Campbell wrote:
>> On 27/04/11 21:56, Kevin Wolf wrote:
>>
>>> When you don't have a backing file, leaving an cluster unallocated means
>>> that it's zero. When you have a backing file, it could be anything. So
>>> if qemu-img convert wanted to save this space, it would have to read
>>> from the backing file and leave the cluster unallocated if it reads as
>>> zero.
>>>
>>> This is something that qemu-img doesn't do today.
>>
> 
> This passes cursory testing, but I'm just wondering if this is along the 
> right lines?

I haven't checked all details, but it looks like what I would have done.
(Though something is wrong with your indentations, I suspect that the
patch wouldn't apply)

> @@ -939,9 +957,16 @@ out:
>       free_option_parameters(create_options);
>       free_option_parameters(param);
>       qemu_free(buf);
> +    if (buf3) {
> +        qemu_free(buf3);
> +    }

qemu_free (and the libc free, too) work just fine with NULL, so the
check isn't needed.

Kevin

Re: [Qemu-devel] Qemu-img convert with -B

[Brad Campbell]

On 28/04/11 14:36, Kevin Wolf wrote:
> Am 28.04.2011 04:06, schrieb Brad Campbell:
>> On 27/04/11 22:02, Brad Campbell wrote:
>>> On 27/04/11 21:56, Kevin Wolf wrote:
>>>
>>>> When you don't have a backing file, leaving an cluster unallocated means
>>>> that it's zero. When you have a backing file, it could be anything. So
>>>> if qemu-img convert wanted to save this space, it would have to read
>>>> from the backing file and leave the cluster unallocated if it reads as
>>>> zero.
>>>>
>>>> This is something that qemu-img doesn't do today.
>> This passes cursory testing, but I'm just wondering if this is along the
>> right lines?
> I haven't checked all details, but it looks like what I would have done.
> (Though something is wrong with your indentations, I suspect that the
> patch wouldn't apply)
>

Odd, I generated it with git diff. Must have lost something in the copy & paste from the terminal.

I'm using tabs=spaces and a ts of 4 in vim. It seemed to fit in with the rest of the file.

>> @@ -939,9 +957,16 @@ out:
>>        free_option_parameters(create_options);
>>        free_option_parameters(param);
>>        qemu_free(buf);
>> +    if (buf3) {
>> +        qemu_free(buf3);
>> +    }
> qemu_free (and the libc free, too) work just fine with NULL, so the
> check isn't needed.

I ran some more tests on it and there are some small issues I need to fix up, but it does what it 
says on the tin.

Cheers for the feedback, I'll do some cleanups and prepare something for submission.

Regards,
Brad