[Qemu-devel] [PATCH 0/4] qemu-img: Fix preallocation with -S 0 for convert

[Qemu-devel] [PATCH 0/4] qemu-img: Fix preallocation with -S 0 for convert

[Max Reitz]

Using -S 0 is supposed to allocate everything in the output image; or at
least it is supposed to always explicitly write zeros even if the area
in question is known to only contain zeros. That doesn't always work
right now, so this series fixes it (patch 1, to be specific).

I only noticed after I had written the test added by patch 4 that we
already had an -S 0 test case which is included in the iotest 122.
However, the test added here works for all image formats and is maybe
more of a direct test (instead of querying the format whether it thinks
it allocated all of the data we directly ask du whether everything has
been allocated) so maybe it reflects better what users expect -S 0 to
do. Maybe.

Patches 2 and 3 are required for the test. I could have written the test
without doing a convert with null-co as the source, but that would have
been boring, so I did not.

If you want to argue that in light of the existence of test 122 the new
test added here is unnecessary and we therefore do not need patches 2, 3
and 4, please go ahead. I won't put up too much of a fight.


Max Reitz (4):
  qemu-img: Fix preallocation with -S 0 for convert
  block/null-{co,aio}: Return zeros when read
  block/null-{co,aio}: Implement get_block_status()
  iotests: Test qemu-img convert -S 0 behavior

 block/null.c               |  19 ++++++++
 qemu-img.c                 |  37 ++++++++++------
 tests/qemu-iotests/122.out |   6 +--
 tests/qemu-iotests/146     | 105 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/146.out |  14 ++++++
 tests/qemu-iotests/group   |   1 +
 6 files changed, 165 insertions(+), 17 deletions(-)
 create mode 100755 tests/qemu-iotests/146
 create mode 100644 tests/qemu-iotests/146.out

-- 
2.7.1

[Qemu-devel] [PATCH 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Max Reitz]

When passing -S 0 to qemu-img convert, the target image is supposed to
be fully allocated. Right now, this is not the case if the source image
contains areas which bdrv_get_block_status() reports as being zero.

This patch introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA
which is set by convert_iteration_sectors() if an area is detected to be
zero and min_sparse is 0. Simply using BLK_DATA is not optimal, because
knowing an area to be zero allows us to memset() the buffer to be
written directly rather than having to use blk_read().

Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA.

This patch changes the reference output for iotest 122; contrary to what
it assumed, -S 0 really should allocate everything in the output, not
just areas that are filled with zeros (as opposed to being zeroed).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c                 | 37 ++++++++++++++++++++++++-------------
 tests/qemu-iotests/122.out |  6 ++----
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index fb9ab1f..809ea3b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1232,6 +1232,10 @@ enum ImgConvertBlockStatus {
     BLK_DATA,
     BLK_ZERO,
     BLK_BACKING_FILE,
+
+    /* Will return zeros when read but must be written as data (i.e. is set for
+     * zeroed areas with min_sparse == 0) */
+    BLK_ZERO_DATA,
 };
 
 typedef struct ImgConvertState {
@@ -1282,7 +1286,13 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
         }
 
         if (ret & BDRV_BLOCK_ZERO) {
-            s->status = BLK_ZERO;
+            if (s->min_sparse) {
+                s->status = BLK_ZERO;
+            } else {
+                /* If min_sparse is 0, this area must be written to the target
+                 * image as data */
+                s->status = BLK_ZERO_DATA;
+            }
         } else if (ret & BDRV_BLOCK_DATA) {
             s->status = BLK_DATA;
         } else if (!s->target_has_backing) {
@@ -1300,7 +1310,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
     }
 
     n = MIN(n, s->sector_next_status - sector_num);
-    if (s->status == BLK_DATA) {
+    if (s->status == BLK_DATA || s->status == BLK_ZERO_DATA) {
         n = MIN(n, s->buf_sectors);
     }
 
@@ -1325,10 +1335,6 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors,
     int n;
     int ret;
 
-    if (s->status == BLK_ZERO || s->status == BLK_BACKING_FILE) {
-        return 0;
-    }
-
     assert(nb_sectors <= s->buf_sectors);
     while (nb_sectors > 0) {
         BlockBackend *blk;
@@ -1372,6 +1378,7 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
             break;
 
         case BLK_DATA:
+        case BLK_ZERO_DATA:
             /* We must always write compressed clusters as a whole, so don't
              * try to find zeroed parts in the buffer. We can only save the
              * write if the buffer is completely zeroed and we're allowed to
@@ -1466,7 +1473,7 @@ static int convert_do_copy(ImgConvertState *s)
             ret = n;
             goto fail;
         }
-        if (s->status == BLK_DATA) {
+        if (s->status == BLK_DATA || s->status == BLK_ZERO_DATA) {
             s->allocated_sectors += n;
         }
         sector_num += n;
@@ -1486,17 +1493,21 @@ static int convert_do_copy(ImgConvertState *s)
             ret = n;
             goto fail;
         }
-        if (s->status == BLK_DATA) {
+        if (s->status == BLK_DATA || s->status == BLK_ZERO_DATA) {
             allocated_done += n;
             qemu_progress_print(100.0 * allocated_done / s->allocated_sectors,
                                 0);
         }
 
-        ret = convert_read(s, sector_num, n, buf);
-        if (ret < 0) {
-            error_report("error while reading sector %" PRId64
-                         ": %s", sector_num, strerror(-ret));
-            goto fail;
+        if (s->status == BLK_DATA) {
+            ret = convert_read(s, sector_num, n, buf);
+            if (ret < 0) {
+                error_report("error while reading sector %" PRId64
+                             ": %s", sector_num, strerror(-ret));
+                goto fail;
+            }
+        } else if (s->status == BLK_ZERO_DATA) {
+            memset(buf, 0, n * BDRV_SECTOR_SIZE);
         }
 
         ret = convert_write(s, sector_num, n, buf);
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 0068e96..98814de 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -112,16 +112,14 @@ read 3145728/3145728 bytes at offset 0
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 63963136/63963136 bytes at offset 3145728
 61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": 327680},
-{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
 
 convert -c -S 0:
 read 3145728/3145728 bytes at offset 0
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 63963136/63963136 bytes at offset 3145728
 61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true},
-{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true}]
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
 wrote 33554432/33554432 bytes at offset 0
 32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.7.1

Re: [Qemu-devel] [PATCH 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Kevin Wolf]

Am 20.02.2016 um 18:39 hat Max Reitz geschrieben:
> When passing -S 0 to qemu-img convert, the target image is supposed to
> be fully allocated. Right now, this is not the case if the source image
> contains areas which bdrv_get_block_status() reports as being zero.
> 
> This patch introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA
> which is set by convert_iteration_sectors() if an area is detected to be
> zero and min_sparse is 0. Simply using BLK_DATA is not optimal, because
> knowing an area to be zero allows us to memset() the buffer to be
> written directly rather than having to use blk_read().
> 
> Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA.
> 
> This patch changes the reference output for iotest 122; contrary to what
> it assumed, -S 0 really should allocate everything in the output, not
> just areas that are filled with zeros (as opposed to being zeroed).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

I don't like how you touch the conversion code all over the place.
Specifically, convert_iteration_sectors() and convert_read() (and
consequently s->status) are supposed to be only about the source file.
-S 0 doesn't make a difference for what the source file looks like, so
we shouldn't need to change anything there.

The following change should do the same thing (it passes your test case
anyway) and is more contained to the actual writeout of image data.

Kevin


diff --git a/qemu-img.c b/qemu-img.c
index 2edb139..3b234cf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1540,14 +1540,21 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors,
 }
 
 static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
-                         const uint8_t *buf)
+                         uint8_t *buf)
 {
     int ret;
 
     while (nb_sectors > 0) {
         int n = nb_sectors;
+        int status = s->status;
 
-        switch (s->status) {
+        if (!s->min_sparse && status == BLK_ZERO) {
+            n = MIN(n, s->buf_sectors);
+            memset(buf, 0, n * BDRV_SECTOR_SIZE);
+            status = BLK_DATA;
+        }
+
+        switch (status) {
         case BLK_BACKING_FILE:
             /* If we have a backing file, leave clusters unallocated that are
              * unallocated in the source image, so that the backing file is
@@ -1602,7 +1609,9 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
 
         sector_num += n;
         nb_sectors -= n;
-        buf += n * BDRV_SECTOR_SIZE;
+        if (s->status == BLK_DATA) {
+            buf += n * BDRV_SECTOR_SIZE;
+        }
     }
 
     return 0;

Re: [Qemu-devel] [PATCH 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4518 bytes --]

On 22.02.2016 13:50, Kevin Wolf wrote:
> Am 20.02.2016 um 18:39 hat Max Reitz geschrieben:
>> When passing -S 0 to qemu-img convert, the target image is supposed to
>> be fully allocated. Right now, this is not the case if the source image
>> contains areas which bdrv_get_block_status() reports as being zero.
>>
>> This patch introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA
>> which is set by convert_iteration_sectors() if an area is detected to be
>> zero and min_sparse is 0. Simply using BLK_DATA is not optimal, because
>> knowing an area to be zero allows us to memset() the buffer to be
>> written directly rather than having to use blk_read().
>>
>> Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA.
>>
>> This patch changes the reference output for iotest 122; contrary to what
>> it assumed, -S 0 really should allocate everything in the output, not
>> just areas that are filled with zeros (as opposed to being zeroed).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> I don't like how you touch the conversion code all over the place.
> Specifically, convert_iteration_sectors() and convert_read() (and
> consequently s->status) are supposed to be only about the source file.
> -S 0 doesn't make a difference for what the source file looks like, so
> we shouldn't need to change anything there.
> 
> The following change should do the same thing (it passes your test case
> anyway) and is more contained to the actual writeout of image data.

Well, I briefly considered making @buf non-const in convert_write(), but
I discarded that idea, and I'm still not comfortable with that. If you
argue that convert_read() should only deal with the source image, I'll
argue that convert_write() should only deal with the target image. I
know that you're making @buf non-const because we need some scratch
buffer and, well, @buf is available, so why not use that? But it still
doesn't sit right with me.

So I'd like to pull that memset() out of convert_write() and just tell
convert_write() to write that buffer as data. In fact, that is basically
what my patch does. But why does it then not just use BLK_DATA but this
new status?

Because of two reasons: First, another issue with your patch is that
zeroed areas are not counted in the progress update. If we are writing
them as zeros, we should count them, however. Therefore, we need some
special-casing in convert_do_copy(). Effectively, we end up with stuff like:

> if (s->status == BLK_DATA ||
>     (!s->min_sparse && s->status == BLK_ZERO))

I found that combination of (min_sparse && BLK_ZERO) to be ugly, and
liked it much better if we could do that test a single time in
convert_read and be done with it. This is why I added the new status.*

And if you pull the memset() out of convert_write() and add a new
status, what you end up with is basically my patch.

*Note that I initially thought we'd have this test in many more places
than we actually would, as it turned out. I'll take a look at how much
simpler this patch becomes if I drop the BLK_ZERO_DATA status.

Max

> 
> Kevin
> 
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 2edb139..3b234cf 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1540,14 +1540,21 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors,
>  }
>  
>  static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
> -                         const uint8_t *buf)
> +                         uint8_t *buf)
>  {
>      int ret;
>  
>      while (nb_sectors > 0) {
>          int n = nb_sectors;
> +        int status = s->status;
>  
> -        switch (s->status) {
> +        if (!s->min_sparse && status == BLK_ZERO) {
> +            n = MIN(n, s->buf_sectors);
> +            memset(buf, 0, n * BDRV_SECTOR_SIZE);
> +            status = BLK_DATA;
> +        }
> +
> +        switch (status) {
>          case BLK_BACKING_FILE:
>              /* If we have a backing file, leave clusters unallocated that are
>               * unallocated in the source image, so that the backing file is
> @@ -1602,7 +1609,9 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
>  
>          sector_num += n;
>          nb_sectors -= n;
> -        buf += n * BDRV_SECTOR_SIZE;
> +        if (s->status == BLK_DATA) {
> +            buf += n * BDRV_SECTOR_SIZE;
> +        }
>      }
>  
>      return 0;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5680 bytes --]

On 22.02.2016 17:43, Max Reitz wrote:
> On 22.02.2016 13:50, Kevin Wolf wrote:
>> Am 20.02.2016 um 18:39 hat Max Reitz geschrieben:
>>> When passing -S 0 to qemu-img convert, the target image is supposed to
>>> be fully allocated. Right now, this is not the case if the source image
>>> contains areas which bdrv_get_block_status() reports as being zero.
>>>
>>> This patch introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA
>>> which is set by convert_iteration_sectors() if an area is detected to be
>>> zero and min_sparse is 0. Simply using BLK_DATA is not optimal, because
>>> knowing an area to be zero allows us to memset() the buffer to be
>>> written directly rather than having to use blk_read().
>>>
>>> Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA.
>>>
>>> This patch changes the reference output for iotest 122; contrary to what
>>> it assumed, -S 0 really should allocate everything in the output, not
>>> just areas that are filled with zeros (as opposed to being zeroed).
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>
>> I don't like how you touch the conversion code all over the place.
>> Specifically, convert_iteration_sectors() and convert_read() (and
>> consequently s->status) are supposed to be only about the source file.
>> -S 0 doesn't make a difference for what the source file looks like, so
>> we shouldn't need to change anything there.
>>
>> The following change should do the same thing (it passes your test case
>> anyway) and is more contained to the actual writeout of image data.
> 
> Well, I briefly considered making @buf non-const in convert_write(), but
> I discarded that idea, and I'm still not comfortable with that. If you
> argue that convert_read() should only deal with the source image, I'll
> argue that convert_write() should only deal with the target image. I
> know that you're making @buf non-const because we need some scratch
> buffer and, well, @buf is available, so why not use that? But it still
> doesn't sit right with me.
> 
> So I'd like to pull that memset() out of convert_write() and just tell
> convert_write() to write that buffer as data. In fact, that is basically
> what my patch does. But why does it then not just use BLK_DATA but this
> new status?
> 
> Because of two reasons: First, another issue with your patch is that
> zeroed areas are not counted in the progress update. If we are writing
> them as zeros, we should count them, however. Therefore, we need some
> special-casing in convert_do_copy(). Effectively, we end up with stuff like:
> 
>> if (s->status == BLK_DATA ||
>>     (!s->min_sparse && s->status == BLK_ZERO))
> 
> I found that combination of (min_sparse && BLK_ZERO) to be ugly, and
> liked it much better if we could do that test a single time in
> convert_read and be done with it. This is why I added the new status.*
> 
> And if you pull the memset() out of convert_write() and add a new
> status, what you end up with is basically my patch.
> 
> *Note that I initially thought we'd have this test in many more places
> than we actually would, as it turned out. I'll take a look at how much
> simpler this patch becomes if I drop the BLK_ZERO_DATA status.

Said patch would look like this:

diff --git a/qemu-img.c b/qemu-img.c
index 2edb139..b696ba4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1509,10 +1509,6 @@ static int convert_read(ImgConvertState *s,
int64_t sector_num, int nb_sectors,
     int n;
     int ret;

-    if (s->status == BLK_ZERO || s->status == BLK_BACKING_FILE) {
-        return 0;
-    }
-
     assert(nb_sectors <= s->buf_sectors);
     while (nb_sectors > 0) {
         BlockBackend *blk;
@@ -1650,7 +1646,8 @@ static int convert_do_copy(ImgConvertState *s)
             ret = n;
             goto fail;
         }
-        if (s->status == BLK_DATA) {
+        if (s->status == BLK_DATA || (!s->min_sparse && s->status ==
BLK_ZERO))
+        {
             s->allocated_sectors += n;
         }
         sector_num += n;
@@ -1670,17 +1667,24 @@ static int convert_do_copy(ImgConvertState *s)
             ret = n;
             goto fail;
         }
-        if (s->status == BLK_DATA) {
+        if (s->status == BLK_DATA || (!s->min_sparse && s->status ==
BLK_ZERO))
+        {
             allocated_done += n;
             qemu_progress_print(100.0 * allocated_done /
s->allocated_sectors,
                                 0);
         }

-        ret = convert_read(s, sector_num, n, buf);
-        if (ret < 0) {
-            error_report("error while reading sector %" PRId64
-                         ": %s", sector_num, strerror(-ret));
-            goto fail;
+        if (s->status == BLK_DATA) {
+            ret = convert_read(s, sector_num, n, buf);
+            if (ret < 0) {
+                error_report("error while reading sector %" PRId64
+                             ": %s", sector_num, strerror(-ret));
+                goto fail;
+            }
+        } else if (!s->min_sparse && s->status == BLK_ZERO) {
+            n = MIN(n, s->buf_sectors);
+            memset(buf, 0, n * BDRV_SECTOR_SIZE);
+            s->status = BLK_DATA;
         }

         ret = convert_write(s, sector_num, n, buf);



I'd get the diffcount even smaller by keeping convert_read() unchanged
and continuing to call it unconditionally, but I like that change in
itself because I find it makes the logic clearer. I can be persuaded to
split this patch into two, however (one pulling the condition out of
convert_read(), and another one doing the rest of this patch).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 2/4] block/null-{co, aio}: Return zeros when read

[Max Reitz]

Currently, we do not define exactly what is returned when read, but
having a reliable source of zeros is always nice.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/null.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/null.c b/block/null.c
index d90165d..ad883d9 100644
--- a/block/null.c
+++ b/block/null.c
@@ -90,6 +90,7 @@ static coroutine_fn int null_co_readv(BlockDriverState *bs,
                                       int64_t sector_num, int nb_sectors,
                                       QEMUIOVector *qiov)
 {
+    qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
     return null_co_common(bs);
 }
 
@@ -159,6 +160,7 @@ static BlockAIOCB *null_aio_readv(BlockDriverState *bs,
                                   BlockCompletionFunc *cb,
                                   void *opaque)
 {
+    qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
     return null_aio_common(bs, cb, opaque);
 }
 
-- 
2.7.1

Re: [Qemu-devel] [PATCH 2/4] block/null-{co, aio}: Return zeros when read

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

On 02/20/2016 10:39 AM, Max Reitz wrote:
> Currently, we do not define exactly what is returned when read, but
> having a reliable source of zeros is always nice.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/null.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/null.c b/block/null.c
> index d90165d..ad883d9 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -90,6 +90,7 @@ static coroutine_fn int null_co_readv(BlockDriverState *bs,
>                                        int64_t sector_num, int nb_sectors,
>                                        QEMUIOVector *qiov)
>  {
> +    qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
>      return null_co_common(bs);
>  }

Shouldn't this be made optional?  That is, one of the points of null-co
was to be as fast as possible, and the memset will slow us down.  So
having an optional bool as part of the null-co setup, which defaults off
for back-compat but can be turned on for guaranteed all-zero reads,
sounds like it would be nicer than forcing all-zero reads.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/4] block/null-{co, aio}: Return zeros when read

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

On 22.02.2016 17:31, Eric Blake wrote:
> On 02/20/2016 10:39 AM, Max Reitz wrote:
>> Currently, we do not define exactly what is returned when read, but
>> having a reliable source of zeros is always nice.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/null.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/block/null.c b/block/null.c
>> index d90165d..ad883d9 100644
>> --- a/block/null.c
>> +++ b/block/null.c
>> @@ -90,6 +90,7 @@ static coroutine_fn int null_co_readv(BlockDriverState *bs,
>>                                        int64_t sector_num, int nb_sectors,
>>                                        QEMUIOVector *qiov)
>>  {
>> +    qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
>>      return null_co_common(bs);
>>  }
> 
> Shouldn't this be made optional?  That is, one of the points of null-co
> was to be as fast as possible, and the memset will slow us down.  So
> having an optional bool as part of the null-co setup, which defaults off
> for back-compat but can be turned on for guaranteed all-zero reads,
> sounds like it would be nicer than forcing all-zero reads.

Can do, will do, thanks.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 3/4] block/null-{co, aio}: Implement get_block_status()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/null.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/block/null.c b/block/null.c
index ad883d9..0a8ff7b 100644
--- a/block/null.c
+++ b/block/null.c
@@ -186,6 +186,19 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state,
     return 0;
 }
 
+static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum,
+                                                     BlockDriverState **file)
+{
+    off_t start = sector_num * BDRV_SECTOR_SIZE;
+
+    *pnum = nb_sectors;
+    *file = bs;
+
+    return BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start;
+}
+
 static BlockDriver bdrv_null_co = {
     .format_name            = "null-co",
     .protocol_name          = "null-co",
@@ -199,6 +212,8 @@ static BlockDriver bdrv_null_co = {
     .bdrv_co_writev         = null_co_writev,
     .bdrv_co_flush_to_disk  = null_co_flush,
     .bdrv_reopen_prepare    = null_reopen_prepare,
+
+    .bdrv_co_get_block_status   = null_co_get_block_status,
 };
 
 static BlockDriver bdrv_null_aio = {
@@ -214,6 +229,8 @@ static BlockDriver bdrv_null_aio = {
     .bdrv_aio_writev        = null_aio_writev,
     .bdrv_aio_flush         = null_aio_flush,
     .bdrv_reopen_prepare    = null_reopen_prepare,
+
+    .bdrv_co_get_block_status   = null_co_get_block_status,
 };
 
 static void bdrv_null_init(void)
-- 
2.7.1

[Qemu-devel] [PATCH 4/4] iotests: Test qemu-img convert -S 0 behavior

[Max Reitz]

Passing -S 0 to qemu-img convert should result in all source data being
copied to the output, even if that source data is known to be 0. The
output image should therefore have exactly the same size on disk as an
image which we explicitly filled with data.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/146     | 105 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/146.out |  14 ++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 120 insertions(+)
 create mode 100755 tests/qemu-iotests/146
 create mode 100644 tests/qemu-iotests/146.out

diff --git a/tests/qemu-iotests/146 b/tests/qemu-iotests/146
new file mode 100755
index 0000000..611602e
--- /dev/null
+++ b/tests/qemu-iotests/146
@@ -0,0 +1,105 @@
+#!/bin/bash
+#
+# Test that qemu-img convert -S 0 fully allocates the target image
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+
+on_disk_size()
+{
+    du "$@" | sed -e 's/\t\+.*//'
+}
+
+
+img_size=1048576
+
+
+echo
+echo '=== Comparing empty image against sparse conversion ==='
+echo
+
+_make_test_img $img_size
+
+empty_size=$(on_disk_size "$TEST_IMG")
+
+
+$QEMU_IMG_PROG convert -O "$IMGFMT" -S 512 \
+    "json:{ 'driver': 'null-co', 'size': $img_size }" \
+    "$TEST_IMG"
+
+sparse_convert_size=$(on_disk_size "$TEST_IMG")
+
+
+if [ "$empty_size" -eq "$sparse_convert_size" ]; then
+    echo 'Equal image size'
+else
+    echo 'Different image size'
+fi
+
+
+echo
+echo '=== Comparing full image against non-sparse conversion ==='
+echo
+
+_make_test_img $img_size
+$QEMU_IO -c "write 0 $img_size" "$TEST_IMG" | _filter_qemu_io
+
+full_size=$(on_disk_size "$TEST_IMG")
+
+
+$QEMU_IMG convert -O "$IMGFMT" -S 0 \
+    "json:{ 'driver': 'null-co', 'size': $img_size }" \
+    "$TEST_IMG"
+
+non_sparse_convert_size=$(on_disk_size "$TEST_IMG")
+
+
+if [ "$full_size" -eq "$non_sparse_convert_size" ]; then
+    echo 'Equal image size'
+else
+    echo 'Different image size'
+fi
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/146.out b/tests/qemu-iotests/146.out
new file mode 100644
index 0000000..5464fba
--- /dev/null
+++ b/tests/qemu-iotests/146.out
@@ -0,0 +1,14 @@
+QA output created by 146
+
+=== Comparing empty image against sparse conversion ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+Equal image size
+
+=== Comparing full image against non-sparse conversion ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Equal image size
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 65df029..524cd30 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -147,3 +147,4 @@
 142 auto
 143 auto quick
 144 rw auto quick
+146 rw auto quick
-- 
2.7.1