[Qemu-devel] [PATCH] qemu-img rebase: Fix for undersized backing files

[Qemu-devel] [PATCH] qemu-img rebase: Fix for undersized backing files

[Kevin Wolf]

Backing files may be smaller than the corresponding COW file. When
reading directly from the backing file, qemu-img rebase must consider
this and assume zero sectors after the end of backing files.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c |   42 +++++++++++++++++++++++++++++++++---------
 1 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 8bdae66..01cc0d3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1420,6 +1420,8 @@ static int img_rebase(int argc, char **argv)
      */
     if (!unsafe) {
         uint64_t num_sectors;
+        uint64_t old_backing_num_sectors;
+        uint64_t new_backing_num_sectors;
         uint64_t sector;
         int n;
         uint8_t * buf_old;
@@ -1430,6 +1432,8 @@ static int img_rebase(int argc, char **argv)
         buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
 
         bdrv_get_geometry(bs, &num_sectors);
+        bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
+        bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
 
         local_progress = (float)100 /
             (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512));
@@ -1448,16 +1452,36 @@ static int img_rebase(int argc, char **argv)
                 continue;
             }
 
-            /* Read old and new backing file */
-            ret = bdrv_read(bs_old_backing, sector, buf_old, n);
-            if (ret < 0) {
-                error_report("error while reading from old backing file");
-                goto out;
+            /*
+             * Read old and new backing file and take into consideration that
+             * backing files may be smaller than the COW image.
+             */
+            if (sector >= old_backing_num_sectors) {
+                memset(buf_old, 0, n * BDRV_SECTOR_SIZE);
+            } else {
+                if (sector + n > old_backing_num_sectors) {
+                    n = old_backing_num_sectors - sector;
+                }
+
+                ret = bdrv_read(bs_old_backing, sector, buf_old, n);
+                if (ret < 0) {
+                    error_report("error while reading from old backing file");
+                    goto out;
+                }
             }
-            ret = bdrv_read(bs_new_backing, sector, buf_new, n);
-            if (ret < 0) {
-                error_report("error while reading from new backing file");
-                goto out;
+
+            if (sector >= new_backing_num_sectors) {
+                memset(buf_new, 0, n * BDRV_SECTOR_SIZE);
+            } else {
+                if (sector + n > new_backing_num_sectors) {
+                    n = new_backing_num_sectors - sector;
+                }
+
+                ret = bdrv_read(bs_new_backing, sector, buf_new, n);
+                if (ret < 0) {
+                    error_report("error while reading from new backing file");
+                    goto out;
+                }
             }
 
             /* If they differ, we need to write to the COW file */
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH] qemu-img rebase: Fix for undersized backing files

[Stefan Hajnoczi]

On Wed, Dec 7, 2011 at 11:45 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Backing files may be smaller than the corresponding COW file. When
> reading directly from the backing file, qemu-img rebase must consider
> this and assume zero sectors after the end of backing files.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-img.c |   42 +++++++++++++++++++++++++++++++++---------
>  1 files changed, 33 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

It almost feels like we want a bdrv_read() variation that does the
zeroing beyond end of image instead of duplicating this.

Stefan

Re: [Qemu-devel] [PATCH] qemu-img rebase: Fix for undersized backing files

[Kevin Wolf]

Am 07.12.2011 12:50, schrieb Stefan Hajnoczi:
> On Wed, Dec 7, 2011 at 11:45 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Backing files may be smaller than the corresponding COW file. When
>> reading directly from the backing file, qemu-img rebase must consider
>> this and assume zero sectors after the end of backing files.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  qemu-img.c |   42 +++++++++++++++++++++++++++++++++---------
>>  1 files changed, 33 insertions(+), 9 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> 
> It almost feels like we want a bdrv_read() variation that does the
> zeroing beyond end of image instead of duplicating this.

Almost. If we have a third user, it's probably worth it.

Actually, if we make a bdrv_co_readv_backing() instead of a synchronous
one, qcow2 would be the third user (and maybe VMDK could use it, too).
That would require moving qemu-img into a coroutine first, which I think
you wanted to do anyway?

Kevin

Re: [Qemu-devel] [PATCH] qemu-img rebase: Fix for undersized backing files

[Stefan Hajnoczi]

On Wed, Dec 7, 2011 at 12:02 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 07.12.2011 12:50, schrieb Stefan Hajnoczi:
>> On Wed, Dec 7, 2011 at 11:45 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Backing files may be smaller than the corresponding COW file. When
>>> reading directly from the backing file, qemu-img rebase must consider
>>> this and assume zero sectors after the end of backing files.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  qemu-img.c |   42 +++++++++++++++++++++++++++++++++---------
>>>  1 files changed, 33 insertions(+), 9 deletions(-)
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>
>> It almost feels like we want a bdrv_read() variation that does the
>> zeroing beyond end of image instead of duplicating this.
>
> Almost. If we have a third user, it's probably worth it.
>
> Actually, if we make a bdrv_co_readv_backing() instead of a synchronous
> one, qcow2 would be the third user (and maybe VMDK could use it, too).
> That would require moving qemu-img into a coroutine first, which I think
> you wanted to do anyway?

If we can make qemu-img the last user of synchronous interfaces then
it would be nice to convert it.  Right now I'm happy that we're
solidifying coroutines and converting synchronous hardware emulation,
hopefully we'll get to the point where there is no real reason to keep
the synchronous API around anymore.

Stefan

Re: [Qemu-devel] [PATCH] qemu-img rebase: Fix for undersized backing files

[Kevin Wolf]

Am 07.12.2011 13:06, schrieb Stefan Hajnoczi:
> On Wed, Dec 7, 2011 at 12:02 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 07.12.2011 12:50, schrieb Stefan Hajnoczi:
>>> On Wed, Dec 7, 2011 at 11:45 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>>> Backing files may be smaller than the corresponding COW file. When
>>>> reading directly from the backing file, qemu-img rebase must consider
>>>> this and assume zero sectors after the end of backing files.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>> ---
>>>>  qemu-img.c |   42 +++++++++++++++++++++++++++++++++---------
>>>>  1 files changed, 33 insertions(+), 9 deletions(-)
>>>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>>
>>> It almost feels like we want a bdrv_read() variation that does the
>>> zeroing beyond end of image instead of duplicating this.
>>
>> Almost. If we have a third user, it's probably worth it.
>>
>> Actually, if we make a bdrv_co_readv_backing() instead of a synchronous
>> one, qcow2 would be the third user (and maybe VMDK could use it, too).
>> That would require moving qemu-img into a coroutine first, which I think
>> you wanted to do anyway?
> 
> If we can make qemu-img the last user of synchronous interfaces then
> it would be nice to convert it.  Right now I'm happy that we're
> solidifying coroutines and converting synchronous hardware emulation,
> hopefully we'll get to the point where there is no real reason to keep
> the synchronous API around anymore.

The point is, if I were to introduce a bdrv_co_readv_backing() function,
I certainly wouldn't like to add all the bdrv_(aio_)read/write emulation
code for it, too.

Kevin