[Qemu-devel] [PATCH v3 00/19] Add qemu-img subcommand to dump file metadata

[Qemu-devel] [PATCH v3 00/19] Add qemu-img subcommand to dump file metadata

[Paolo Bonzini]

This series adds a subcommand to "map" that can dump file metadata.
Metadata that is dumped includes:

- whether blocks are allocated in bs->file and, if so, where

- whether blocks are zero

- whether data is read from bs or bs->backing_hd

Metadata is dumped for an entire chain of images.  One example usage is
(for non-compressed, non-encrypted images) to transform the metadata
into a Linux device-mapper setup, and make a qcow2 image available (for
read only) as a block device.  Another possible usage is to determine
the used areas of a file, and convert it in place to another format.
Alternatively, the richer data can be used by block jobs to quickly
determine if a block is zero without reading it.

The patches implement a new operation, bdrv_co_get_block_status, that
supersedes bdrv_co_is_allocated.  The bdrv_is_allocated API is still
available of course, and implemented on top of the new operation.

Patches 1-3 touch cow, which uses bdrv_co_is_allocated during its reads
and writes.  I optimized it a bit because it was unbearably slow during
testing.  With these patches (also tested with blkverify), booting of
a cow guest image is not particularly slow.

Patch 4 fixes the bs->total_sectors file size cache, which is used in
bdrv_co_get_block_status to clamp invalid input.

Patches 5 to 7 clean up the bdrv_is_allocated and bdrv_is_allocated_above
implementation, eliminating some code duplication.

Patches 8 and 9 tweak bdrv_has_zero_init and its usage in qemu-img in
a way that helps when implementing the new API.

Patches 10 to 12 implement bdrv_get_block_status and change the formats
to report all the available information.

Patch 13 and 14 adds the "map" subcommand to qemu-img, and the relevant
documentation.

Finally, patches 15 to 19 tweak the implementation to extract more
information from protocols, and combine this information with format
metadata whenever possible.

v2->v3:
        new patch 4 to fix qemu-iotests regression
        new patch 14 for documentation

Paolo Bonzini (19):
  cow: make reads go at a decent speed
  cow: make writes go at a less indecent speed
  cow: do not call bdrv_co_is_allocated
  block: update bs->total_sectors on writes
  block: make bdrv_co_is_allocated static
  block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above
    distinction
  block: expect errors from bdrv_co_is_allocated
  qemu-img: always probe the input image for allocated sectors
  block: make bdrv_has_zero_init return false for copy-on-write-images
  block: introduce bdrv_get_block_status API
  block: define get_block_status return value
  block: return get_block_status data and flags for formats
  qemu-img: add a "map" subcommand
  docs, qapi: document qemu-img map
  block: use bdrv_has_zero_init to return BDRV_BLOCK_ZERO
  raw-posix: return get_block_status data and flags
  raw-posix: detect XFS unwritten extents
  block: add default get_block_status implementation for protocols
  block: look for zero blocks in bs->file

 block.c                   | 140 ++++++++++++++--------------
 block/commit.c            |   6 +-
 block/cow.c               |  90 ++++++++++++------
 block/mirror.c            |   4 +-
 block/qcow.c              |  13 ++-
 block/qcow2.c             |  24 +++--
 block/qed.c               |  39 ++++++--
 block/raw-posix.c         |  24 +++--
 block/raw.c               |   6 +-
 block/sheepdog.c          |  14 +--
 block/stream.c            |  10 +-
 block/vdi.c               |  17 +++-
 block/vmdk.c              |  23 ++++-
 block/vvfat.c             |  15 +--
 include/block/block.h     |  34 +++++--
 include/block/block_int.h |   2 +-
 qapi-schema.json          |  29 ++++++
 qemu-img-cmds.hx          |   6 ++
 qemu-img.c                | 226 +++++++++++++++++++++++++++++++++++++++++-----
 qemu-img.texi             |  46 ++++++++++
 20 files changed, 585 insertions(+), 183 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 01/19] cow: make reads go at a decent speed

[Paolo Bonzini]

Do not do two reads for each sector; load each sector of the bitmap
and use bitmap operations to process it.

Writes are still dog slow!

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c | 54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index 1cc2e89..fa3d41f 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -126,18 +126,31 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
     return 0;
 }
 
-static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
+#define BITS_PER_BITMAP_SECTOR (512 * 8)
+
+/* Cannot use bitmap.c on big-endian machines.  */
+static int cow_test_bit(int64_t bitnum, const uint8_t *bitmap)
 {
-    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
-    uint8_t bitmap;
-    int ret;
+    return (bitmap[bitnum / 8] & (1 << (bitnum & 7))) != 0;
+}
 
-    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
-    if (ret < 0) {
-       return ret;
+static int cow_find_streak(const uint8_t *bitmap, int value, int start, int nb_sectors)
+{
+    int streak_value = value ? 0xFF : 0;
+    int last = MIN(start + nb_sectors, BITS_PER_BITMAP_SECTOR);
+    int bitnum = start;
+    while (bitnum < last) {
+        if ((bitnum & 7) == 0 && bitmap[bitnum / 8] == streak_value) {
+            bitnum += 8;
+            continue;
+        }
+        if (cow_test_bit(bitnum, bitmap) == value) {
+            bitnum++;
+            continue;
+        }
+        break;
     }
-
-    return !!(bitmap & (1 << (bitnum % 8)));
+    return MIN(bitnum, last) - start;
 }
 
 /* Return true if first block has been changed (ie. current version is
@@ -146,23 +159,20 @@ static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
 static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *num_same)
 {
+    int64_t bitnum = sector_num + sizeof(struct cow_header_v2) * 8;
+    uint64_t offset = (bitnum / 8) & -BDRV_SECTOR_SIZE;
+    uint8_t bitmap[BDRV_SECTOR_SIZE];
+    int ret;
     int changed;
 
-    if (nb_sectors == 0) {
-	*num_same = nb_sectors;
-	return 0;
-    }
-
-    changed = is_bit_set(bs, sector_num);
-    if (changed < 0) {
-        return 0; /* XXX: how to return I/O errors? */
-    }
-
-    for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
-	if (is_bit_set(bs, sector_num + *num_same) != changed)
-	    break;
+    ret = bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap));
+    if (ret < 0) {
+        return ret;
     }
 
+    bitnum &= BITS_PER_BITMAP_SECTOR - 1;
+    changed = cow_test_bit(bitnum, bitmap);
+    *num_same = cow_find_streak(bitmap, changed, bitnum, nb_sectors);
     return changed;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 02/19] cow: make writes go at a less indecent speed

[Paolo Bonzini]

Only sync once per write, rather than once per sector.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index fa3d41f..9ae2d6a 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -106,7 +106,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags)
  * XXX(hch): right now these functions are extremely inefficient.
  * We should just read the whole bitmap we'll need in one go instead.
  */
-static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
+static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum, bool *first)
 {
     uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
     uint8_t bitmap;
@@ -117,9 +117,21 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
        return ret;
     }
 
+    if (bitmap & (1 << (bitnum % 8))) {
+        return 0;
+    }
+
+    if (*first) {
+        ret = bdrv_flush(bs->file);
+        if (ret < 0) {
+            return ret;
+        }
+        *first = false;
+    }
+
     bitmap |= (1 << (bitnum % 8));
 
-    ret = bdrv_pwrite_sync(bs->file, offset, &bitmap, sizeof(bitmap));
+    ret = bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap));
     if (ret < 0) {
        return ret;
     }
@@ -181,9 +193,10 @@ static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
 {
     int error = 0;
     int i;
+    bool first = true;
 
     for (i = 0; i < nb_sectors; i++) {
-        error = cow_set_bit(bs, sector_num + i);
+        error = cow_set_bit(bs, sector_num + i, &first);
         if (error) {
             break;
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 03/19] cow: do not call bdrv_co_is_allocated

[Paolo Bonzini]

As we change bdrv_is_allocated to gather more information from bs and
bs->file, it will become a bit slower.  It is still appropriate for online
jobs, but not for reads/writes.  Call the internal function instead.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/cow.c b/block/cow.c
index 9ae2d6a..9797f71 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -46,6 +46,9 @@ typedef struct BDRVCowState {
     int64_t cow_sectors_offset;
 } BDRVCowState;
 
+static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *num_same);
+
 static int cow_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     const struct cow_header_v2 *cow_header = (const void *)buf;
@@ -212,7 +215,7 @@ static int coroutine_fn cow_read(BlockDriverState *bs, int64_t sector_num,
     int ret, n;
 
     while (nb_sectors > 0) {
-        if (bdrv_co_is_allocated(bs, sector_num, nb_sectors, &n)) {
+        if (cow_co_is_allocated(bs, sector_num, nb_sectors, &n)) {
             ret = bdrv_pread(bs->file,
                         s->cow_sectors_offset + sector_num * 512,
                         buf, n * 512);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 03/19] cow: do not call bdrv_co_is_allocated

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> As we change bdrv_is_allocated to gather more information from bs and
> bs->file, it will become a bit slower.  It is still appropriate for online
> jobs, but not for reads/writes.  Call the internal function instead.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/cow.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 9ae2d6a..9797f71 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -46,6 +46,9 @@ typedef struct BDRVCowState {
>      int64_t cow_sectors_offset;
>  } BDRVCowState;
>  
> +static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, int *num_same);

This forward declaration seems unnecessary.

Kevin

[Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Paolo Bonzini]

If a BlockDriverState is growable, after every write we need to
check if bs->total_sectors might have changed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block.c b/block.c
index 6cd39fa..ebac2fa 100644
--- a/block.c
+++ b/block.c
@@ -2651,6 +2651,9 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
     if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
         bs->wr_highest_sector = sector_num + nb_sectors - 1;
     }
+    if (bs->growable && ret >= 0) {
+        bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
+    }
 
     tracked_request_end(&req);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> If a BlockDriverState is growable, after every write we need to
> check if bs->total_sectors might have changed.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 6cd39fa..ebac2fa 100644
> --- a/block.c
> +++ b/block.c
> @@ -2651,6 +2651,9 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>      if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>          bs->wr_highest_sector = sector_num + nb_sectors - 1;
>      }
> +    if (bs->growable && ret >= 0) {
> +        bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
> +    }

Can we change bdrv_getlength() to use bs->total_sectors even for
growable images after this patch?

Kevin

Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Paolo Bonzini]

Il 29/07/2013 15:13, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> If a BlockDriverState is growable, after every write we need to
>> check if bs->total_sectors might have changed.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 6cd39fa..ebac2fa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2651,6 +2651,9 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>>      if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>>          bs->wr_highest_sector = sector_num + nb_sectors - 1;
>>      }
>> +    if (bs->growable && ret >= 0) {
>> +        bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
>> +    }
> 
> Can we change bdrv_getlength() to use bs->total_sectors even for
> growable images after this patch?

Probably, but not in 1.6. :)

Paolo

Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Kevin Wolf]

Am 29.07.2013 um 15:47 hat Paolo Bonzini geschrieben:
> Il 29/07/2013 15:13, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> If a BlockDriverState is growable, after every write we need to
> >> check if bs->total_sectors might have changed.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  block.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/block.c b/block.c
> >> index 6cd39fa..ebac2fa 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -2651,6 +2651,9 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> >>      if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
> >>          bs->wr_highest_sector = sector_num + nb_sectors - 1;
> >>      }
> >> +    if (bs->growable && ret >= 0) {
> >> +        bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
> >> +    }
> > 
> > Can we change bdrv_getlength() to use bs->total_sectors even for
> > growable images after this patch?
> 
> Probably, but not in 1.6. :)

"Probably" was my conclusion as well. The answer to this question is the
answer to whether this patch makes sense, I think. So I can give you a
Probably-reviewed-by if that's of any use. ;-)

FWIW, I've got the feeling that the whole series might be better suited
for block-next. Is there anything urgent in it?

Kevin

Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Paolo Bonzini]

Il 29/07/2013 16:10, Kevin Wolf ha scritto:
> Am 29.07.2013 um 15:47 hat Paolo Bonzini geschrieben:
>> Il 29/07/2013 15:13, Kevin Wolf ha scritto:
>>> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>>>> If a BlockDriverState is growable, after every write we need to
>>>> check if bs->total_sectors might have changed.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>  block.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 6cd39fa..ebac2fa 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -2651,6 +2651,9 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>>>>      if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>>>>          bs->wr_highest_sector = sector_num + nb_sectors - 1;
>>>>      }
>>>> +    if (bs->growable && ret >= 0) {
>>>> +        bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
>>>> +    }
>>>
>>> Can we change bdrv_getlength() to use bs->total_sectors even for
>>> growable images after this patch?
>>
>> Probably, but not in 1.6. :)
> 
> "Probably" was my conclusion as well. The answer to this question is the
> answer to whether this patch makes sense, I think. So I can give you a
> Probably-reviewed-by if that's of any use. ;-)
> 
> FWIW, I've got the feeling that the whole series might be better suited
> for block-next. Is there anything urgent in it?

No, I don't think so.

Paolo

Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Peter Lieven]

On 29.07.2013 16:18, Paolo Bonzini wrote:
> Il 29/07/2013 16:10, Kevin Wolf ha scritto:
>> Am 29.07.2013 um 15:47 hat Paolo Bonzini geschrieben:
>>> Il 29/07/2013 15:13, Kevin Wolf ha scritto:
>>>> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>>>>> If a BlockDriverState is growable, after every write we need to
>>>>> check if bs->total_sectors might have changed.
>>>>>
>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> ---
>>>>>   block.c | 3 +++
>>>>>   1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 6cd39fa..ebac2fa 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -2651,6 +2651,9 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>>>>>       if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>>>>>           bs->wr_highest_sector = sector_num + nb_sectors - 1;
>>>>>       }
>>>>> +    if (bs->growable && ret >= 0) {
>>>>> +        bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
>>>>> +    }
>>>> Can we change bdrv_getlength() to use bs->total_sectors even for
>>>> growable images after this patch?
>>> Probably, but not in 1.6. :)
>> "Probably" was my conclusion as well. The answer to this question is the
>> answer to whether this patch makes sense, I think. So I can give you a
>> Probably-reviewed-by if that's of any use. ;-)
>>
>> FWIW, I've got the feeling that the whole series might be better suited
>> for block-next. Is there anything urgent in it?
> No, I don't think so.
can you give an update what are to current plans/schedule to merge this series? I have
a few patches in the queue that in their current version depend on this series being merged.

Peter

Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Paolo Bonzini]

Il 02/08/2013 09:05, Peter Lieven ha scritto:
> can you give an update what are to current plans/schedule to merge this
> series? I have
> a few patches in the queue that in their current version depend on this
> series being merged.

It should go in soon, perhaps a couple of weeks.

Paolo

Re: [Qemu-devel] [PATCH v3 04/19] block: update bs->total_sectors on writes

[Peter Lieven]

On 17.08.2013 08:27, Paolo Bonzini wrote:
> Il 02/08/2013 09:05, Peter Lieven ha scritto:
>> can you give an update what are to current plans/schedule to merge this
>> series? I have
>> a few patches in the queue that in their current version depend on this
>> series being merged.
> It should go in soon, perhaps a couple of weeks.
That would be good. I would like to make the relevant changes to iscsi soon and
as I promised to convert everything in block/iscsi to coroutines this will be a lot.

Peter

[Qemu-devel] [PATCH v3 05/19] block: make bdrv_co_is_allocated static

[Paolo Bonzini]

bdrv_is_allocated can detect coroutine context and go through a fast
path, similar to other block layer functions.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c               | 24 +++++++++++++++---------
 block/raw.c           |  2 +-
 block/stream.c        |  4 ++--
 include/block/block.h |  2 --
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index ebac2fa..dd4c570 100644
--- a/block.c
+++ b/block.c
@@ -2527,7 +2527,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
     if (flags & BDRV_REQ_COPY_ON_READ) {
         int pnum;
 
-        ret = bdrv_co_is_allocated(bs, sector_num, nb_sectors, &pnum);
+        ret = bdrv_is_allocated(bs, sector_num, nb_sectors, &pnum);
         if (ret < 0) {
             goto out;
         }
@@ -2981,8 +2981,9 @@ typedef struct BdrvCoIsAllocatedData {
  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
  * beyond the end of the disk image it will be clamped.
  */
-int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                                      int nb_sectors, int *pnum)
+static int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs,
+                                             int64_t sector_num,
+                                             int nb_sectors, int *pnum)
 {
     int64_t n;
 
@@ -3032,10 +3033,15 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
         .done = false,
     };
 
-    co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
-    qemu_coroutine_enter(co, &data);
-    while (!data.done) {
-        qemu_aio_wait();
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_is_allocated_co_entry(&data);
+    } else {
+        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
+        qemu_coroutine_enter(co, &data);
+        while (!data.done) {
+            qemu_aio_wait();
+        }
     }
     return data.ret;
 }
@@ -3063,8 +3069,8 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
     intermediate = top;
     while (intermediate && intermediate != base) {
         int pnum_inter;
-        ret = bdrv_co_is_allocated(intermediate, sector_num, nb_sectors,
-                                   &pnum_inter);
+        ret = bdrv_is_allocated(intermediate, sector_num, nb_sectors,
+                                &pnum_inter);
         if (ret < 0) {
             return ret;
         } else if (ret) {
diff --git a/block/raw.c b/block/raw.c
index f1682d4..4517a81 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -39,7 +39,7 @@ static int coroutine_fn raw_co_is_allocated(BlockDriverState *bs,
                                             int64_t sector_num,
                                             int nb_sectors, int *pnum)
 {
-    return bdrv_co_is_allocated(bs->file, sector_num, nb_sectors, pnum);
+    return bdrv_is_allocated(bs->file, sector_num, nb_sectors, pnum);
 }
 
 static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
diff --git a/block/stream.c b/block/stream.c
index 7fe9e48..fb1f9c3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -115,8 +115,8 @@ wait:
             break;
         }
 
-        ret = bdrv_co_is_allocated(bs, sector_num,
-                                   STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
+        ret = bdrv_is_allocated(bs, sector_num,
+                                STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
         if (ret == 1) {
             /* Allocated in the top, no need to copy.  */
             copy = false;
diff --git a/include/block/block.h b/include/block/block.h
index 742fce5..dde745f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -181,8 +181,6 @@ int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
  */
 int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
     int nb_sectors);
-int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t sector_num,
-    int nb_sectors, int *pnum);
 int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
                                             BlockDriverState *base,
                                             int64_t sector_num,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 05/19] block: make bdrv_co_is_allocated static

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> bdrv_is_allocated can detect coroutine context and go through a fast
> path, similar to other block layer functions.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

It can, but why is this a good idea?

The other block layer functions do this because existing drivers call
bdrv_(p)read/write(v) and we don't want them to block the vcpu. So it's
basically just a method of avoiding massive changes to existing callers.

The only such caller of bdrv_is_allocated() that I can see is vvfat, and
I don't think optimising that is our top priority.

Charlie is working in exactly the opposite direction, namely splitting
the existing functions in a coroutine-only and a synchronous-only part.
We should be clear what we really want here.

Kevin

Re: [Qemu-devel] [PATCH v3 05/19] block: make bdrv_co_is_allocated static

[Paolo Bonzini]

Il 29/07/2013 15:21, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> bdrv_is_allocated can detect coroutine context and go through a fast
>> path, similar to other block layer functions.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> It can, but why is this a good idea?

What exactly?  Dropping bdrv_co_is_allocated from the public API, or not
creating always a new coroutine while still dropping bdrv_co_is_allocated?

In the end, the answer is simply for consistency.  bdrv_co_is_allocated
is different from all other APIs in this respect, and I didn't want to
choose between making get_block_status the same as is_allocated vs. the
same as everything else.

> Charlie is working in exactly the opposite direction, namely splitting
> the existing functions in a coroutine-only and a synchronous-only part.
> We should be clear what we really want here.

That's fine---but whatever we do we should do it for all APIs.  So this
patch is just making things consistent before adding another API.

Paolo

[Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction

[Paolo Bonzini]

Now that bdrv_is_allocated detects coroutine context, the two can
use the same code.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c               | 46 ++++------------------------------------------
 block/commit.c        |  6 +++---
 block/mirror.c        |  4 ++--
 block/stream.c        |  4 ++--
 include/block/block.h |  4 ----
 5 files changed, 11 insertions(+), 53 deletions(-)

diff --git a/block.c b/block.c
index dd4c570..1ee1d93 100644
--- a/block.c
+++ b/block.c
@@ -3058,10 +3058,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
  *  allocated/unallocated state.
  *
  */
-int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
-                                            BlockDriverState *base,
-                                            int64_t sector_num,
-                                            int nb_sectors, int *pnum)
+int coroutine_fn bdrv_is_allocated_above(BlockDriverState *top,
+                                         BlockDriverState *base,
+                                         int64_t sector_num,
+                                         int nb_sectors, int *pnum)
 {
     BlockDriverState *intermediate;
     int ret, n = nb_sectors;
@@ -3097,44 +3097,6 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
-/* Coroutine wrapper for bdrv_is_allocated_above() */
-static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
-{
-    BdrvCoIsAllocatedData *data = opaque;
-    BlockDriverState *top = data->bs;
-    BlockDriverState *base = data->base;
-
-    data->ret = bdrv_co_is_allocated_above(top, base, data->sector_num,
-                                           data->nb_sectors, data->pnum);
-    data->done = true;
-}
-
-/*
- * Synchronous wrapper around bdrv_co_is_allocated_above().
- *
- * See bdrv_co_is_allocated_above() for details.
- */
-int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
-                            int64_t sector_num, int nb_sectors, int *pnum)
-{
-    Coroutine *co;
-    BdrvCoIsAllocatedData data = {
-        .bs = top,
-        .base = base,
-        .sector_num = sector_num,
-        .nb_sectors = nb_sectors,
-        .pnum = pnum,
-        .done = false,
-    };
-
-    co = qemu_coroutine_create(bdrv_is_allocated_above_co_entry);
-    qemu_coroutine_enter(co, &data);
-    while (!data.done) {
-        qemu_aio_wait();
-    }
-    return data.ret;
-}
-
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
 {
     if (bs->backing_hd && bs->backing_hd->encrypted)
diff --git a/block/commit.c b/block/commit.c
index 2227fc2..37572f0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -108,9 +108,9 @@ wait:
             break;
         }
         /* Copy if allocated above the base */
-        ret = bdrv_co_is_allocated_above(top, base, sector_num,
-                                         COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
-                                         &n);
+        ret = bdrv_is_allocated_above(top, base, sector_num,
+                                      COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
+                                      &n);
         copy = (ret == 1);
         trace_commit_one_iteration(s, sector_num, n, ret);
         if (copy) {
diff --git a/block/mirror.c b/block/mirror.c
index bed4a7e..0b3c2f6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -338,8 +338,8 @@ static void coroutine_fn mirror_run(void *opaque)
         base = s->mode == MIRROR_SYNC_MODE_FULL ? NULL : bs->backing_hd;
         for (sector_num = 0; sector_num < end; ) {
             int64_t next = (sector_num | (sectors_per_chunk - 1)) + 1;
-            ret = bdrv_co_is_allocated_above(bs, base,
-                                             sector_num, next - sector_num, &n);
+            ret = bdrv_is_allocated_above(bs, base,
+                                          sector_num, next - sector_num, &n);
 
             if (ret < 0) {
                 goto immediate_exit;
diff --git a/block/stream.c b/block/stream.c
index fb1f9c3..60136fb 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -123,8 +123,8 @@ wait:
         } else {
             /* Copy if allocated in the intermediate images.  Limit to the
              * known-unallocated area [sector_num, sector_num+n).  */
-            ret = bdrv_co_is_allocated_above(bs->backing_hd, base,
-                                             sector_num, n, &n);
+            ret = bdrv_is_allocated_above(bs->backing_hd, base,
+                                          sector_num, n, &n);
 
             /* Finish early if end of backing file has been reached */
             if (ret == 0 && n == 0) {
diff --git a/include/block/block.h b/include/block/block.h
index dde745f..64f7730 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -181,10 +181,6 @@ int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
  */
 int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
     int nb_sectors);
-int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
-                                            BlockDriverState *base,
-                                            int64_t sector_num,
-                                            int nb_sectors, int *pnum);
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Now that bdrv_is_allocated detects coroutine context, the two can
> use the same code.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c               | 46 ++++------------------------------------------
>  block/commit.c        |  6 +++---
>  block/mirror.c        |  4 ++--
>  block/stream.c        |  4 ++--
>  include/block/block.h |  4 ----
>  5 files changed, 11 insertions(+), 53 deletions(-)
> 
> diff --git a/block.c b/block.c
> index dd4c570..1ee1d93 100644
> --- a/block.c
> +++ b/block.c
> @@ -3058,10 +3058,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>   *  allocated/unallocated state.
>   *
>   */
> -int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
> -                                            BlockDriverState *base,
> -                                            int64_t sector_num,
> -                                            int nb_sectors, int *pnum)
> +int coroutine_fn bdrv_is_allocated_above(BlockDriverState *top,
> +                                         BlockDriverState *base,
> +                                         int64_t sector_num,
> +                                         int nb_sectors, int *pnum)

This is no longer a coroutine_fn.

However, if this was the only reason for making bdrv_is_allocated() a
hybrid function, it may be easier to simply let the only caller
(img_compare in qemu-img) run in a coroutine and drop the synchronous
version entirely, keeping only a coroutine_fn.

Kevin

Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction

[Paolo Bonzini]

Il 29/07/2013 15:34, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> Now that bdrv_is_allocated detects coroutine context, the two can
>> use the same code.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block.c               | 46 ++++------------------------------------------
>>  block/commit.c        |  6 +++---
>>  block/mirror.c        |  4 ++--
>>  block/stream.c        |  4 ++--
>>  include/block/block.h |  4 ----
>>  5 files changed, 11 insertions(+), 53 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index dd4c570..1ee1d93 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3058,10 +3058,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>>   *  allocated/unallocated state.
>>   *
>>   */
>> -int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
>> -                                            BlockDriverState *base,
>> -                                            int64_t sector_num,
>> -                                            int nb_sectors, int *pnum)
>> +int coroutine_fn bdrv_is_allocated_above(BlockDriverState *top,
>> +                                         BlockDriverState *base,
>> +                                         int64_t sector_num,
>> +                                         int nb_sectors, int *pnum)
> 
> This is no longer a coroutine_fn.

I'm always confused about must-yield vs. may-yield. :(

> However, if this was the only reason for making bdrv_is_allocated() a
> hybrid function, it may be easier to simply let the only caller
> (img_compare in qemu-img) run in a coroutine and drop the synchronous
> version entirely, keeping only a coroutine_fn.

The reason is to avoid having the same (IMHO pretty gratuitous)
distinction in get_block_status.  "qemu-img map" will also add another
synchronous user of get_block_status.

If we want to keep only the coroutine_fn, we should make all of qemu-img
run in a coroutine.

Paolo

Re: [Qemu-devel] [PATCH v3 06/19] block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above distinction

[Kevin Wolf]

Am 29.07.2013 um 15:59 hat Paolo Bonzini geschrieben:
> Il 29/07/2013 15:34, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Now that bdrv_is_allocated detects coroutine context, the two can
> >> use the same code.
> >>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  block.c               | 46 ++++------------------------------------------
> >>  block/commit.c        |  6 +++---
> >>  block/mirror.c        |  4 ++--
> >>  block/stream.c        |  4 ++--
> >>  include/block/block.h |  4 ----
> >>  5 files changed, 11 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index dd4c570..1ee1d93 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3058,10 +3058,10 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> >>   *  allocated/unallocated state.
> >>   *
> >>   */
> >> -int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
> >> -                                            BlockDriverState *base,
> >> -                                            int64_t sector_num,
> >> -                                            int nb_sectors, int *pnum)
> >> +int coroutine_fn bdrv_is_allocated_above(BlockDriverState *top,
> >> +                                         BlockDriverState *base,
> >> +                                         int64_t sector_num,
> >> +                                         int nb_sectors, int *pnum)
> > 
> > This is no longer a coroutine_fn.
> 
> I'm always confused about must-yield vs. may-yield. :(

A coroutine_fn may yield without checking whether it runs in a
coroutine. Or phrased differently, coroutine_fns may only be called from
coroutine context.

> > However, if this was the only reason for making bdrv_is_allocated() a
> > hybrid function, it may be easier to simply let the only caller
> > (img_compare in qemu-img) run in a coroutine and drop the synchronous
> > version entirely, keeping only a coroutine_fn.
> 
> The reason is to avoid having the same (IMHO pretty gratuitous)
> distinction in get_block_status.  "qemu-img map" will also add another
> synchronous user of get_block_status.
> 
> If we want to keep only the coroutine_fn, we should make all of qemu-img
> run in a coroutine.

I think in this case that's really a good option, because qemu-img is
the only caller for the synchronous version (if I didn't miss any
other). It may turn out useful for other functions as well.

And letting qemu-img's main() call into a coroutine doesn't really seem
too bad compared to doing the optional coroutine dance for each block.c
function. And it finally gets us a main loop in qemu-img, too.

Kevin

[Qemu-devel] [PATCH v3 07/19] block: expect errors from bdrv_co_is_allocated

[Paolo Bonzini]

Some bdrv_is_allocated callers do not expect errors, but the fallback
in qcow2.c might make other callers trip on assertion failures or
infinite loops.

Fix the callers to always look for errors.

Cc: qemu-stable@nongnu.org
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow2.c  | 4 +---
 block/stream.c | 2 +-
 qemu-img.c     | 5 +++++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0eceefe..e2b4202 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -648,13 +648,11 @@ static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,
     int ret;
 
     *pnum = nb_sectors;
-    /* FIXME We can get errors here, but the bdrv_co_is_allocated interface
-     * can't pass them on today */
     qemu_co_mutex_lock(&s->lock);
     ret = qcow2_get_cluster_offset(bs, sector_num << 9, pnum, &cluster_offset);
     qemu_co_mutex_unlock(&s->lock);
     if (ret < 0) {
-        *pnum = 0;
+        return ret;
     }
 
     return (cluster_offset != 0) || (ret == QCOW2_CLUSTER_ZERO);
diff --git a/block/stream.c b/block/stream.c
index 60136fb..1579cd3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -120,7 +120,7 @@ wait:
         if (ret == 1) {
             /* Allocated in the top, no need to copy.  */
             copy = false;
-        } else {
+        } else if (ret >= 0) {
             /* Copy if allocated in the intermediate images.  Limit to the
              * known-unallocated area [sector_num, sector_num+n).  */
             ret = bdrv_is_allocated_above(bs->backing_hd, base,
diff --git a/qemu-img.c b/qemu-img.c
index c55ca5c..a4957eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2073,6 +2073,11 @@ static int img_rebase(int argc, char **argv)
 
             /* If the cluster is allocated, we don't need to take action */
             ret = bdrv_is_allocated(bs, sector, n, &n);
+            if (ret < 0) {
+                error_report("error while reading image metadata: %s",
+                             strerror(-ret));
+                goto out;
+            }
             if (ret) {
                 continue;
             }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 07/19] block: expect errors from bdrv_co_is_allocated

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Some bdrv_is_allocated callers do not expect errors, but the fallback
> in qcow2.c might make other callers trip on assertion failures or
> infinite loops.
> 
> Fix the callers to always look for errors.
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Good idea, but there seem to be a few more callers that don't handle
errors yet. Not sure what your criterion for fixing callers or not was.
Did you simply miss the rest?

Kevin

Re: [Qemu-devel] [PATCH v3 07/19] block: expect errors from bdrv_co_is_allocated

[Paolo Bonzini]

Il 29/07/2013 15:43, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> Some bdrv_is_allocated callers do not expect errors, but the fallback
>> in qcow2.c might make other callers trip on assertion failures or
>> infinite loops.
>>
>> Fix the callers to always look for errors.
>>
>> Cc: qemu-stable@nongnu.org
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Good idea, but there seem to be a few more callers that don't handle
> errors yet. Not sure what your criterion for fixing callers or not was.
> Did you simply miss the rest?

In general, I didn't bother about callers that didn't handle *pnum = 0
correctly (for example block-migration.c and commit).  But I see now
that the ones in qemu-io-cmds.c are due to a bad rebase.

(BTW, I just noticed now that both block-migration.c and commit also run
outside a coroutine, and thus rely on bdrv_is_allocated creating one).

Paolo

Re: [Qemu-devel] [PATCH v3 07/19] block: expect errors from bdrv_co_is_allocated

[Kevin Wolf]

Am 29.07.2013 um 16:03 hat Paolo Bonzini geschrieben:
> Il 29/07/2013 15:43, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Some bdrv_is_allocated callers do not expect errors, but the fallback
> >> in qcow2.c might make other callers trip on assertion failures or
> >> infinite loops.
> >>
> >> Fix the callers to always look for errors.
> >>
> >> Cc: qemu-stable@nongnu.org
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Good idea, but there seem to be a few more callers that don't handle
> > errors yet. Not sure what your criterion for fixing callers or not was.
> > Did you simply miss the rest?
> 
> In general, I didn't bother about callers that didn't handle *pnum = 0
> correctly (for example block-migration.c and commit).  But I see now
> that the ones in qemu-io-cmds.c are due to a bad rebase.

Would you mind fixing the already broken ones as well?

> (BTW, I just noticed now that both block-migration.c and commit also run
> outside a coroutine, and thus rely on bdrv_is_allocated creating one).

Ah well, I missed those, too bad. I won't insist on removing the
synchronous path then.

Kevin

[Qemu-devel] [PATCH v3 08/19] qemu-img: always probe the input image for allocated sectors

[Paolo Bonzini]

qemu-img convert is assuming "that sectors which are unallocated in the input
image are present in both the output's and input's base images", but it is
only doing this if the output image is zero initialized.  And checking if
the output image is zero initialized does not make much sense if the
output image is copy-on-write.  Always do the test.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-img.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index a4957eb..3faffee 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1476,21 +1476,19 @@ static int img_convert(int argc, char **argv)
                 n = bs_offset + bs_sectors - sector_num;
             }
 
-            if (has_zero_init) {
-                /* If the output image is being created as a copy on write image,
-                   assume that sectors which are unallocated in the input image
-                   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)) {
-                        sector_num += n1;
-                        continue;
-                    }
-                    /* The next 'n1' sectors are allocated in the input image. Copy
-                       only those as they may be followed by unallocated sectors. */
-                    n = n1;
+            /* If the output image is being created as a copy on write image,
+               assume that sectors which are unallocated in the input image
+               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)) {
+                    sector_num += n1;
+                    continue;
                 }
+                /* The next 'n1' sectors are allocated in the input image. Copy
+                   only those as they may be followed by unallocated sectors. */
+                n = n1;
             } else {
                 n1 = n;
             }
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 09/19] block: make bdrv_has_zero_init return false for copy-on-write-images

[Paolo Bonzini]

This helps implementing is_allocated on top of get_block_status.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c    | 5 +++++
 qemu-img.c | 9 +--------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 1ee1d93..cfb3785 100644
--- a/block.c
+++ b/block.c
@@ -2948,6 +2948,11 @@ int bdrv_has_zero_init(BlockDriverState *bs)
 {
     assert(bs->drv);
 
+    /* If BS is a copy on write image, it is initialized to
+       the contents of the base image, which may not be zeroes.  */
+    if (bs->backing_hd) {
+        return 0;
+    }
     if (bs->drv->bdrv_has_zero_init) {
         return bs->drv->bdrv_has_zero_init(bs);
     }
diff --git a/qemu-img.c b/qemu-img.c
index 3faffee..c5c8ebc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1504,14 +1504,7 @@ static int img_convert(int argc, char **argv)
                should add a specific call to have the info to go faster */
             buf1 = buf;
             while (n > 0) {
-                /* 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.
-
-                   If the output is to a host device, we also write out
-                   sectors that are entirely 0, since whatever data was
-                   already there is garbage, not 0s. */
-                if (!has_zero_init || out_baseimg ||
+                if (!has_zero_init ||
                     is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
                     ret = bdrv_write(out_bs, sector_num, buf1, n1);
                     if (ret < 0) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 10/19] block: introduce bdrv_get_block_status API

[Paolo Bonzini]

For now, bdrv_get_block_status is just another name for bdrv_is_allocated.
The next patches will add more flags.

This also touches all block drivers with a mostly mechanical rename.  The
sole exception is cow; because it calls cow_co_is_allocated from the read
code, we keep that function and make cow_co_get_block_status a wrapper.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c                   | 46 ++++++++++++++++++++++++++--------------------
 block/cow.c               |  8 +++++++-
 block/qcow.c              |  4 ++--
 block/qcow2.c             |  4 ++--
 block/qed.c               |  4 ++--
 block/raw-posix.c         |  4 ++--
 block/raw.c               |  6 +++---
 block/sheepdog.c          | 12 ++++++------
 block/vdi.c               |  4 ++--
 block/vmdk.c              |  4 ++--
 block/vvfat.c             |  4 ++--
 include/block/block.h     |  2 ++
 include/block/block_int.h |  2 +-
 13 files changed, 59 insertions(+), 45 deletions(-)

diff --git a/block.c b/block.c
index cfb3785..f533c36 100644
--- a/block.c
+++ b/block.c
@@ -2961,15 +2961,15 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
-typedef struct BdrvCoIsAllocatedData {
+typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
     int64_t sector_num;
     int nb_sectors;
     int *pnum;
-    int ret;
+    int64_t ret;
     bool done;
-} BdrvCoIsAllocatedData;
+} BdrvCoGetBlockStatusData;
 
 /*
  * Returns true iff the specified sector is present in the disk image. Drivers
@@ -2986,9 +2986,9 @@ typedef struct BdrvCoIsAllocatedData {
  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
  * beyond the end of the disk image it will be clamped.
  */
-static int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs,
-                                             int64_t sector_num,
-                                             int nb_sectors, int *pnum)
+static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum)
 {
     int64_t n;
 
@@ -3002,35 +3002,35 @@ static int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs,
         nb_sectors = n;
     }
 
-    if (!bs->drv->bdrv_co_is_allocated) {
+    if (!bs->drv->bdrv_co_get_block_status) {
         *pnum = nb_sectors;
         return 1;
     }
 
-    return bs->drv->bdrv_co_is_allocated(bs, sector_num, nb_sectors, pnum);
+    return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
 }
 
-/* Coroutine wrapper for bdrv_is_allocated() */
-static void coroutine_fn bdrv_is_allocated_co_entry(void *opaque)
+/* Coroutine wrapper for bdrv_get_block_status() */
+static void coroutine_fn bdrv_get_block_status_co_entry(void *opaque)
 {
-    BdrvCoIsAllocatedData *data = opaque;
+    BdrvCoGetBlockStatusData *data = opaque;
     BlockDriverState *bs = data->bs;
 
-    data->ret = bdrv_co_is_allocated(bs, data->sector_num, data->nb_sectors,
-                                     data->pnum);
+    data->ret = bdrv_co_get_block_status(bs, data->sector_num, data->nb_sectors,
+                                         data->pnum);
     data->done = true;
 }
 
 /*
- * Synchronous wrapper around bdrv_co_is_allocated().
+ * Synchronous wrapper around bdrv_co_get_block_status().
  *
- * See bdrv_co_is_allocated() for details.
+ * See bdrv_co_get_block_status() for details.
  */
-int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
-                      int *pnum)
+int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
+                              int nb_sectors, int *pnum)
 {
     Coroutine *co;
-    BdrvCoIsAllocatedData data = {
+    BdrvCoGetBlockStatusData data = {
         .bs = bs,
         .sector_num = sector_num,
         .nb_sectors = nb_sectors,
@@ -3040,9 +3040,9 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 
     if (qemu_in_coroutine()) {
         /* Fast-path if already in coroutine context */
-        bdrv_is_allocated_co_entry(&data);
+        bdrv_get_block_status_co_entry(&data);
     } else {
-        co = qemu_coroutine_create(bdrv_is_allocated_co_entry);
+        co = qemu_coroutine_create(bdrv_get_block_status_co_entry);
         qemu_coroutine_enter(co, &data);
         while (!data.done) {
             qemu_aio_wait();
@@ -3051,6 +3051,12 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return data.ret;
 }
 
+int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
+                                   int nb_sectors, int *pnum)
+{
+    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
+}
+
 /*
  * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
  *
diff --git a/block/cow.c b/block/cow.c
index 9797f71..e738b96 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -191,6 +191,12 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
     return changed;
 }
 
+static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *num_same)
+{
+    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
+}
+
 static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
         int nb_sectors)
 {
@@ -370,7 +376,7 @@ static BlockDriver bdrv_cow = {
 
     .bdrv_read              = cow_co_read,
     .bdrv_write             = cow_co_write,
-    .bdrv_co_is_allocated   = cow_co_is_allocated,
+    .bdrv_co_get_block_status   = cow_co_get_block_status,
 
     .create_options = cow_create_options,
 };
diff --git a/block/qcow.c b/block/qcow.c
index 5239bd6..acd1aeb 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -395,7 +395,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
     return cluster_offset;
 }
 
-static int coroutine_fn qcow_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
     BDRVQcowState *s = bs->opaque;
@@ -896,7 +896,7 @@ static BlockDriver bdrv_qcow = {
 
     .bdrv_co_readv          = qcow_co_readv,
     .bdrv_co_writev         = qcow_co_writev,
-    .bdrv_co_is_allocated   = qcow_co_is_allocated,
+    .bdrv_co_get_block_status   = qcow_co_get_block_status,
 
     .bdrv_set_key           = qcow_set_key,
     .bdrv_make_empty        = qcow_make_empty,
diff --git a/block/qcow2.c b/block/qcow2.c
index e2b4202..d35a134 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -640,7 +640,7 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
     return 0;
 }
 
-static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
     BDRVQcowState *s = bs->opaque;
@@ -1784,7 +1784,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
     .bdrv_create        = qcow2_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
-    .bdrv_co_is_allocated = qcow2_co_is_allocated,
+    .bdrv_co_get_block_status = qcow2_co_get_block_status,
     .bdrv_set_key       = qcow2_set_key,
     .bdrv_make_empty    = qcow2_make_empty,
 
diff --git a/block/qed.c b/block/qed.c
index f767b05..b0978ba 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -667,7 +667,7 @@ static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t l
     }
 }
 
-static int coroutine_fn bdrv_qed_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
                                                  int64_t sector_num,
                                                  int nb_sectors, int *pnum)
 {
@@ -1575,7 +1575,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_reopen_prepare      = bdrv_qed_reopen_prepare,
     .bdrv_create              = bdrv_qed_create,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
-    .bdrv_co_is_allocated     = bdrv_qed_co_is_allocated,
+    .bdrv_co_get_block_status = bdrv_qed_co_get_block_status,
     .bdrv_make_empty          = bdrv_qed_make_empty,
     .bdrv_aio_readv           = bdrv_qed_aio_readv,
     .bdrv_aio_writev          = bdrv_qed_aio_writev,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ba721d3..dbc65b0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1084,7 +1084,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
  * beyond the end of the disk image it will be clamped.
  */
-static int coroutine_fn raw_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                             int64_t sector_num,
                                             int nb_sectors, int *pnum)
 {
@@ -1200,7 +1200,7 @@ static BlockDriver bdrv_file = {
     .bdrv_close = raw_close,
     .bdrv_create = raw_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
-    .bdrv_co_is_allocated = raw_co_is_allocated,
+    .bdrv_co_get_block_status = raw_co_get_block_status,
 
     .bdrv_aio_readv = raw_aio_readv,
     .bdrv_aio_writev = raw_aio_writev,
diff --git a/block/raw.c b/block/raw.c
index 4517a81..757bd36 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -35,11 +35,11 @@ static void raw_close(BlockDriverState *bs)
 {
 }
 
-static int coroutine_fn raw_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                             int64_t sector_num,
                                             int nb_sectors, int *pnum)
 {
-    return bdrv_is_allocated(bs->file, sector_num, nb_sectors, pnum);
+    return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
 }
 
 static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
@@ -139,7 +139,7 @@ static BlockDriver bdrv_raw = {
 
     .bdrv_co_readv          = raw_co_readv,
     .bdrv_co_writev         = raw_co_writev,
-    .bdrv_co_is_allocated   = raw_co_is_allocated,
+    .bdrv_co_get_block_status   = raw_co_get_block_status,
     .bdrv_co_write_zeroes   = raw_co_write_zeroes,
     .bdrv_co_discard        = raw_co_discard,
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 6a41ad9..31cc573 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2287,9 +2287,9 @@ static coroutine_fn int sd_co_discard(BlockDriverState *bs, int64_t sector_num,
     return acb->ret;
 }
 
-static coroutine_fn int
-sd_co_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
-                   int *pnum)
+static coroutine_fn int64_t
+sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
+                       int *pnum)
 {
     BDRVSheepdogState *s = bs->opaque;
     SheepdogInode *inode = &s->inode;
@@ -2354,7 +2354,7 @@ static BlockDriver bdrv_sheepdog = {
     .bdrv_co_writev = sd_co_writev,
     .bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
     .bdrv_co_discard = sd_co_discard,
-    .bdrv_co_is_allocated = sd_co_is_allocated,
+    .bdrv_co_get_block_status = sd_co_get_block_status,
 
     .bdrv_snapshot_create   = sd_snapshot_create,
     .bdrv_snapshot_goto     = sd_snapshot_goto,
@@ -2381,7 +2381,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
     .bdrv_co_writev = sd_co_writev,
     .bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
     .bdrv_co_discard = sd_co_discard,
-    .bdrv_co_is_allocated = sd_co_is_allocated,
+    .bdrv_co_get_block_status = sd_co_get_block_status,
 
     .bdrv_snapshot_create   = sd_snapshot_create,
     .bdrv_snapshot_goto     = sd_snapshot_goto,
@@ -2409,7 +2409,7 @@ static BlockDriver bdrv_sheepdog_unix = {
     .bdrv_co_writev = sd_co_writev,
     .bdrv_co_flush_to_disk  = sd_co_flush_to_disk,
     .bdrv_co_discard = sd_co_discard,
-    .bdrv_co_is_allocated = sd_co_is_allocated,
+    .bdrv_co_get_block_status = sd_co_get_block_status,
 
     .bdrv_snapshot_create   = sd_snapshot_create,
     .bdrv_snapshot_goto     = sd_snapshot_goto,
diff --git a/block/vdi.c b/block/vdi.c
index 8a91525..7ab2567 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -470,7 +470,7 @@ static int vdi_reopen_prepare(BDRVReopenState *state,
     return 0;
 }
 
-static int coroutine_fn vdi_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
     /* TODO: Check for too large sector_num (in bdrv_is_allocated or here). */
@@ -780,7 +780,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_reopen_prepare = vdi_reopen_prepare,
     .bdrv_create = vdi_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
-    .bdrv_co_is_allocated = vdi_co_is_allocated,
+    .bdrv_co_get_block_status = vdi_co_get_block_status,
     .bdrv_make_empty = vdi_make_empty,
 
     .bdrv_read = vdi_co_read,
diff --git a/block/vmdk.c b/block/vmdk.c
index 3756333..026840c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -998,7 +998,7 @@ static VmdkExtent *find_extent(BDRVVmdkState *s,
     return NULL;
 }
 
-static int coroutine_fn vmdk_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
     BDRVVmdkState *s = bs->opaque;
@@ -1790,7 +1790,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_close                   = vmdk_close,
     .bdrv_create                  = vmdk_create,
     .bdrv_co_flush_to_disk        = vmdk_co_flush,
-    .bdrv_co_is_allocated         = vmdk_co_is_allocated,
+    .bdrv_co_get_block_status     = vmdk_co_get_block_status,
     .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
     .bdrv_has_zero_init           = vmdk_has_zero_init,
 
diff --git a/block/vvfat.c b/block/vvfat.c
index cd3b8ed..4c30620 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2874,7 +2874,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
-static int coroutine_fn vvfat_co_is_allocated(BlockDriverState *bs,
+static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
 	int64_t sector_num, int nb_sectors, int* n)
 {
     BDRVVVFATState* s = bs->opaque;
@@ -2984,7 +2984,7 @@ static BlockDriver bdrv_vvfat = {
 
     .bdrv_read              = vvfat_co_read,
     .bdrv_write             = vvfat_co_write,
-    .bdrv_co_is_allocated   = vvfat_co_is_allocated,
+    .bdrv_co_get_block_status = vvfat_co_get_block_status,
 };
 
 static void bdrv_vvfat_init(void)
diff --git a/include/block/block.h b/include/block/block.h
index 64f7730..e41854e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -271,6 +271,8 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
+int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
+                              int nb_sectors, int *pnum);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
                       int *pnum);
 int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..8f2d2c7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -134,7 +134,7 @@ struct BlockDriver {
         int64_t sector_num, int nb_sectors);
     int coroutine_fn (*bdrv_co_discard)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors);
-    int coroutine_fn (*bdrv_co_is_allocated)(BlockDriverState *bs,
+    int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum);
 
     /*
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 11/19] block: define get_block_status return value

[Paolo Bonzini]

Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
are left for future extensions.

The return code is compatible with the old is_allocated API: if a driver
only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
clients of is_allocated will not have any change in behavior.  Still,
we will return more precise information in the next patches and the
new definition of bdrv_is_allocated is already prepared for this.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c               | 10 ++++++++--
 include/block/block.h | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index f533c36..7cfbf71 100644
--- a/block.c
+++ b/block.c
@@ -3004,7 +3004,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
 
     if (!bs->drv->bdrv_co_get_block_status) {
         *pnum = nb_sectors;
-        return 1;
+        return BDRV_BLOCK_DATA;
     }
 
     return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
@@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
 int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
                                    int nb_sectors, int *pnum)
 {
-    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
+    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
+    if (ret < 0) {
+        return ret;
+    }
+    return
+        (ret & BDRV_BLOCK_DATA) ||
+        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
 }
 
 /*
diff --git a/include/block/block.h b/include/block/block.h
index e41854e..d044b31 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -81,6 +81,32 @@ typedef struct BlockDevOps {
 #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
 #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
+/* BDRV_BLOCK_DATA: data is read from bs->file or another file
+ * BDRV_BLOCK_ZERO: sectors read as zero
+ * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
+ *
+ * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
+ * bs->file where sector data can be read from as raw data.
+ *
+ * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
+ *
+ * DATA ZERO OFFSET_VALID
+ *  t    t        t       sectors read as zero, bs->file is zero at offset
+ *  t    f        t       sectors read as valid from bs->file at offset
+ *  f    t        t       sectors preallocated, read as zero, bs->file not
+ *                        necessarily zero at offset
+ *  f    f        t       sectors preallocated but read from backing_hd,
+ *                        bs->file contains garbage at offset
+ *  t    t        f       sectors preallocated, read as zero, unknown offset
+ *  t    f        f       sectors read from unknown file or offset
+ *  f    t        f       not allocated or unknown offset, read as zero
+ *  f    f        f       not allocated or unknown offset, read from backing_hd
+ */
+#define BDRV_BLOCK_DATA         1
+#define BDRV_BLOCK_ZERO         2
+#define BDRV_BLOCK_OFFSET_VALID 4
+#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
+
 typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockErrorAction;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 11/19] block: define get_block_status return value

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
> are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
> are left for future extensions.
> 
> The return code is compatible with the old is_allocated API: if a driver
> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
> clients of is_allocated will not have any change in behavior.  Still,
> we will return more precise information in the next patches and the
> new definition of bdrv_is_allocated is already prepared for this.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c               | 10 ++++++++--
>  include/block/block.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f533c36..7cfbf71 100644
> --- a/block.c
> +++ b/block.c
> @@ -3004,7 +3004,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>  
>      if (!bs->drv->bdrv_co_get_block_status) {
>          *pnum = nb_sectors;
> -        return 1;
> +        return BDRV_BLOCK_DATA;
>      }
>  
>      return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> @@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                                     int nb_sectors, int *pnum)
>  {
> -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return
> +        (ret & BDRV_BLOCK_DATA) ||
> +        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
>  }
>  
>  /*
> diff --git a/include/block/block.h b/include/block/block.h
> index e41854e..d044b31 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>  
> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
> + * BDRV_BLOCK_ZERO: sectors read as zero
> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> + *
> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> + * bs->file where sector data can be read from as raw data.
> + *
> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
> + *
> + * DATA ZERO OFFSET_VALID
> + *  t    t        t       sectors read as zero, bs->file is zero at offset
> + *  t    f        t       sectors read as valid from bs->file at offset
> + *  f    t        t       sectors preallocated, read as zero, bs->file not
> + *                        necessarily zero at offset
> + *  f    f        t       sectors preallocated but read from backing_hd,
> + *                        bs->file contains garbage at offset
> + *  t    t        f       sectors preallocated, read as zero, unknown offset
> + *  t    f        f       sectors read from unknown file or offset
> + *  f    t        f       not allocated or unknown offset, read as zero
> + *  f    f        f       not allocated or unknown offset, read from backing_hd
> + */
> +#define BDRV_BLOCK_DATA         1
> +#define BDRV_BLOCK_ZERO         2
> +#define BDRV_BLOCK_OFFSET_VALID 4
> +#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK

When are block driver supposed to set the BDRV_BLOCK_OFFSET_VALID flag?

For example, qcow2 could in theory set the flag, it has all of the
information already in memory. But with a fragmented image this might
mean that it returns only one cluster instead of a large area with one
bdrv_get_block_status() call.

Should the caller pass a flag that tells whether he is interested in the
offset or not?

Kevin

Re: [Qemu-devel] [PATCH v3 11/19] block: define get_block_status return value

[Paolo Bonzini]

Il 30/07/2013 16:14, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
>> are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
>> are left for future extensions.
>>
>> The return code is compatible with the old is_allocated API: if a driver
>> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
>> clients of is_allocated will not have any change in behavior.  Still,
>> we will return more precise information in the next patches and the
>> new definition of bdrv_is_allocated is already prepared for this.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block.c               | 10 ++++++++--
>>  include/block/block.h | 26 ++++++++++++++++++++++++++
>>  2 files changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f533c36..7cfbf71 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3004,7 +3004,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>  
>>      if (!bs->drv->bdrv_co_get_block_status) {
>>          *pnum = nb_sectors;
>> -        return 1;
>> +        return BDRV_BLOCK_DATA;
>>      }
>>  
>>      return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
>> @@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
>>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
>>                                     int nb_sectors, int *pnum)
>>  {
>> -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
>> +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    return
>> +        (ret & BDRV_BLOCK_DATA) ||
>> +        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
>>  }
>>  
>>  /*
>> diff --git a/include/block/block.h b/include/block/block.h
>> index e41854e..d044b31 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
>>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>>  
>> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
>> + * BDRV_BLOCK_ZERO: sectors read as zero
>> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
>> + *
>> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
>> + * bs->file where sector data can be read from as raw data.
>> + *
>> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
>> + *
>> + * DATA ZERO OFFSET_VALID
>> + *  t    t        t       sectors read as zero, bs->file is zero at offset
>> + *  t    f        t       sectors read as valid from bs->file at offset
>> + *  f    t        t       sectors preallocated, read as zero, bs->file not
>> + *                        necessarily zero at offset
>> + *  f    f        t       sectors preallocated but read from backing_hd,
>> + *                        bs->file contains garbage at offset
>> + *  t    t        f       sectors preallocated, read as zero, unknown offset
>> + *  t    f        f       sectors read from unknown file or offset
>> + *  f    t        f       not allocated or unknown offset, read as zero
>> + *  f    f        f       not allocated or unknown offset, read from backing_hd
>> + */
>> +#define BDRV_BLOCK_DATA         1
>> +#define BDRV_BLOCK_ZERO         2
>> +#define BDRV_BLOCK_OFFSET_VALID 4
>> +#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
> 
> When are block driver supposed to set the BDRV_BLOCK_OFFSET_VALID flag?

Always if they can provide the information.

> For example, qcow2 could in theory set the flag, it has all of the
> information already in memory.

In fact it does later in the series.

> But with a fragmented image this might
> mean that it returns only one cluster instead of a large area with one
> bdrv_get_block_status() call.

This is just theoretical, right?  qcow2_get_cluster_offset only works on
areas that are contiguous in the raw image.

> Should the caller pass a flag that tells whether he is interested in the
> offset or not?

That would complicate the API (and the implementation too if you want to
honor it in the formats).  Since is_allocated works the same way and it
wasn't a problem so far, I decided not to have such a flag.

Paolo

Re: [Qemu-devel] [PATCH v3 11/19] block: define get_block_status return value

[Kevin Wolf]

Am 30.07.2013 um 16:19 hat Paolo Bonzini geschrieben:
> Il 30/07/2013 16:14, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
> >> are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
> >> are left for future extensions.
> >>
> >> The return code is compatible with the old is_allocated API: if a driver
> >> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
> >> clients of is_allocated will not have any change in behavior.  Still,
> >> we will return more precise information in the next patches and the
> >> new definition of bdrv_is_allocated is already prepared for this.
> >>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  block.c               | 10 ++++++++--
> >>  include/block/block.h | 26 ++++++++++++++++++++++++++
> >>  2 files changed, 34 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index f533c36..7cfbf71 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3004,7 +3004,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >>  
> >>      if (!bs->drv->bdrv_co_get_block_status) {
> >>          *pnum = nb_sectors;
> >> -        return 1;
> >> +        return BDRV_BLOCK_DATA;
> >>      }
> >>  
> >>      return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> @@ -3054,7 +3054,13 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
> >>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
> >>                                     int nb_sectors, int *pnum)
> >>  {
> >> -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> >> +    if (ret < 0) {
> >> +        return ret;
> >> +    }
> >> +    return
> >> +        (ret & BDRV_BLOCK_DATA) ||
> >> +        ((ret & BDRV_BLOCK_ZERO) && !bdrv_has_zero_init(bs));
> >>  }
> >>  
> >>  /*
> >> diff --git a/include/block/block.h b/include/block/block.h
> >> index e41854e..d044b31 100644
> >> --- a/include/block/block.h
> >> +++ b/include/block/block.h
> >> @@ -81,6 +81,32 @@ typedef struct BlockDevOps {
> >>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
> >>  #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
> >>  
> >> +/* BDRV_BLOCK_DATA: data is read from bs->file or another file
> >> + * BDRV_BLOCK_ZERO: sectors read as zero
> >> + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> >> + *
> >> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> >> + * bs->file where sector data can be read from as raw data.
> >> + *
> >> + * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
> >> + *
> >> + * DATA ZERO OFFSET_VALID
> >> + *  t    t        t       sectors read as zero, bs->file is zero at offset
> >> + *  t    f        t       sectors read as valid from bs->file at offset
> >> + *  f    t        t       sectors preallocated, read as zero, bs->file not
> >> + *                        necessarily zero at offset
> >> + *  f    f        t       sectors preallocated but read from backing_hd,
> >> + *                        bs->file contains garbage at offset
> >> + *  t    t        f       sectors preallocated, read as zero, unknown offset
> >> + *  t    f        f       sectors read from unknown file or offset
> >> + *  f    t        f       not allocated or unknown offset, read as zero
> >> + *  f    f        f       not allocated or unknown offset, read from backing_hd
> >> + */
> >> +#define BDRV_BLOCK_DATA         1
> >> +#define BDRV_BLOCK_ZERO         2
> >> +#define BDRV_BLOCK_OFFSET_VALID 4
> >> +#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
> > 
> > When are block driver supposed to set the BDRV_BLOCK_OFFSET_VALID flag?
> 
> Always if they can provide the information.
> 
> > For example, qcow2 could in theory set the flag, it has all of the
> > information already in memory.
> 
> In fact it does later in the series.
> 
> > But with a fragmented image this might
> > mean that it returns only one cluster instead of a large area with one
> > bdrv_get_block_status() call.
> 
> This is just theoretical, right?  qcow2_get_cluster_offset only works on
> areas that are contiguous in the raw image.

Right, somehow I was thinking that .bdrv_is_allocated() used something
more clever, but it doesn't. So this isn't worse than what we have today.

Kevin

[Qemu-devel] [PATCH v3 12/19] block: return get_block_status data and flags for formats

[Paolo Bonzini]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/cow.c      |  8 +++++++-
 block/qcow.c     |  9 ++++++++-
 block/qcow2.c    | 16 ++++++++++++++--
 block/qed.c      | 35 ++++++++++++++++++++++++++++-------
 block/sheepdog.c |  2 +-
 block/vdi.c      | 13 ++++++++++++-
 block/vmdk.c     | 19 ++++++++++++++++++-
 block/vvfat.c    | 11 ++++++-----
 8 files changed, 94 insertions(+), 19 deletions(-)

diff --git a/block/cow.c b/block/cow.c
index e738b96..1e90413 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -194,7 +194,13 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
 static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *num_same)
 {
-    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
+    BDRVCowState *s = bs->opaque;
+    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
+    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
+    if (ret < 0) {
+        return ret;
+    }
+    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
 }
 
 static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
diff --git a/block/qcow.c b/block/qcow.c
index acd1aeb..1a65822 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -410,7 +410,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
     if (n > nb_sectors)
         n = nb_sectors;
     *pnum = n;
-    return (cluster_offset != 0);
+    if (!cluster_offset) {
+        return 0;
+    }
+    if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypt_method) {
+        return BDRV_BLOCK_DATA;
+    }
+    cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+    return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | cluster_offset;
 }
 
 static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
diff --git a/block/qcow2.c b/block/qcow2.c
index d35a134..cac7ee8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -645,7 +645,8 @@ static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t cluster_offset;
-    int ret;
+    int index_in_cluster, ret;
+    int64_t status = 0;
 
     *pnum = nb_sectors;
     qemu_co_mutex_lock(&s->lock);
@@ -655,7 +656,18 @@ static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
         return ret;
     }
 
-    return (cluster_offset != 0) || (ret == QCOW2_CLUSTER_ZERO);
+    if (cluster_offset != 0 && ret != QCOW2_CLUSTER_COMPRESSED &&
+        !s->crypt_method) {
+        index_in_cluster = sector_num & (s->cluster_sectors - 1);
+        cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+        status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
+    }
+    if (ret == QCOW2_CLUSTER_ZERO) {
+        status |= BDRV_BLOCK_ZERO;
+    } else if (ret != QCOW2_CLUSTER_UNALLOCATED) {
+        status |= BDRV_BLOCK_DATA;
+    }
+    return status;
 }
 
 /* handle reading after the end of the backing file */
diff --git a/block/qed.c b/block/qed.c
index b0978ba..ea41036 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -652,16 +652,36 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
 }
 
 typedef struct {
+    BlockDriverState *bs;
     Coroutine *co;
-    int is_allocated;
+    uint64_t pos;
+    int64_t status;
     int *pnum;
 } QEDIsAllocatedCB;
 
 static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
 {
     QEDIsAllocatedCB *cb = opaque;
+    BDRVQEDState *s = cb->bs->opaque;
     *cb->pnum = len / BDRV_SECTOR_SIZE;
-    cb->is_allocated = (ret == QED_CLUSTER_FOUND || ret == QED_CLUSTER_ZERO);
+    switch (ret) {
+    case QED_CLUSTER_FOUND:
+        offset |= qed_offset_into_cluster(s, cb->pos);
+        cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+        break;
+    case QED_CLUSTER_ZERO:
+        cb->status = BDRV_BLOCK_ZERO;
+        break;
+    case QED_CLUSTER_L2:
+    case QED_CLUSTER_L1:
+        cb->status = 0;
+        break;
+    default:
+        assert(ret < 0);
+        cb->status = ret;
+        break;
+    }
+
     if (cb->co) {
         qemu_coroutine_enter(cb->co, NULL);
     }
@@ -672,25 +692,26 @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
                                                  int nb_sectors, int *pnum)
 {
     BDRVQEDState *s = bs->opaque;
-    uint64_t pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE;
     size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
     QEDIsAllocatedCB cb = {
-        .is_allocated = -1,
+        .bs = bs,
+        .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
+        .status = BDRV_BLOCK_OFFSET_MASK,
         .pnum = pnum,
     };
     QEDRequest request = { .l2_table = NULL };
 
-    qed_find_cluster(s, &request, pos, len, qed_is_allocated_cb, &cb);
+    qed_find_cluster(s, &request, cb.pos, len, qed_is_allocated_cb, &cb);
 
     /* Now sleep if the callback wasn't invoked immediately */
-    while (cb.is_allocated == -1) {
+    while (cb.status == BDRV_BLOCK_OFFSET_MASK) {
         cb.co = qemu_coroutine_self();
         qemu_coroutine_yield();
     }
 
     qed_unref_l2_cache_entry(request.l2_table);
 
-    return cb.is_allocated;
+    return cb.status;
 }
 
 static int bdrv_qed_make_empty(BlockDriverState *bs)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 31cc573..b0ff8f1 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2297,7 +2297,7 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
                   end = DIV_ROUND_UP((sector_num + nb_sectors) *
                                      BDRV_SECTOR_SIZE, SD_DATA_OBJ_SIZE);
     unsigned long idx;
-    int ret = 1;
+    int64_t ret = BDRV_BLOCK_DATA;
 
     for (idx = start; idx < end; idx++) {
         if (inode->data_vdi_id[idx] == 0) {
diff --git a/block/vdi.c b/block/vdi.c
index 7ab2567..1bf7dc5 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -479,12 +479,23 @@ static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs,
     size_t sector_in_block = sector_num % s->block_sectors;
     int n_sectors = s->block_sectors - sector_in_block;
     uint32_t bmap_entry = le32_to_cpu(s->bmap[bmap_index]);
+    uint64_t offset;
+    int result;
+
     logout("%p, %" PRId64 ", %d, %p\n", bs, sector_num, nb_sectors, pnum);
     if (n_sectors > nb_sectors) {
         n_sectors = nb_sectors;
     }
     *pnum = n_sectors;
-    return VDI_IS_ALLOCATED(bmap_entry);
+    result = VDI_IS_ALLOCATED(bmap_entry);
+    if (!result) {
+        return 0;
+    }
+
+    offset = s->header.offset_data +
+                              (uint64_t)bmap_entry * s->block_size +
+                              sector_in_block * SECTOR_SIZE;
+    return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
 }
 
 static int vdi_co_read(BlockDriverState *bs,
diff --git a/block/vmdk.c b/block/vmdk.c
index 026840c..cd8db57 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1015,7 +1015,24 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
                             sector_num * 512, 0, &offset);
     qemu_co_mutex_unlock(&s->lock);
 
-    ret = (ret == VMDK_OK || ret == VMDK_ZEROED);
+    switch (ret) {
+    case VMDK_ERROR:
+        ret = -EIO;
+        break;
+    case VMDK_UNALLOC:
+        ret = 0;
+        break;
+    case VMDK_ZEROED:
+        ret = BDRV_BLOCK_ZERO;
+        break;
+    case VMDK_OK:
+        ret = BDRV_BLOCK_DATA;
+        if (extent->file == bs->file) {
+            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
+        }
+
+        break;
+    }
 
     index_in_cluster = sector_num % extent->cluster_sectors;
     n = extent->cluster_sectors - index_in_cluster;
diff --git a/block/vvfat.c b/block/vvfat.c
index 4c30620..dd0efca 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2879,11 +2879,12 @@ static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
 {
     BDRVVVFATState* s = bs->opaque;
     *n = s->sector_count - sector_num;
-    if (*n > nb_sectors)
-	*n = nb_sectors;
-    else if (*n < 0)
-	return 0;
-    return 1;
+    if (*n > nb_sectors) {
+        *n = nb_sectors;
+    } else if (*n < 0) {
+        return 0;
+    }
+    return BDRV_BLOCK_DATA;
 }
 
 static int write_target_commit(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 12/19] block: return get_block_status data and flags for formats

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/cow.c      |  8 +++++++-
>  block/qcow.c     |  9 ++++++++-
>  block/qcow2.c    | 16 ++++++++++++++--
>  block/qed.c      | 35 ++++++++++++++++++++++++++++-------
>  block/sheepdog.c |  2 +-
>  block/vdi.c      | 13 ++++++++++++-
>  block/vmdk.c     | 19 ++++++++++++++++++-
>  block/vvfat.c    | 11 ++++++-----
>  8 files changed, 94 insertions(+), 19 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index e738b96..1e90413 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -194,7 +194,13 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>  static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors, int *num_same)
>  {
> -    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> +    BDRVCowState *s = bs->opaque;
> +    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> +    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
>  }
>  
>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
> diff --git a/block/qcow.c b/block/qcow.c
> index acd1aeb..1a65822 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -410,7 +410,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>      if (n > nb_sectors)
>          n = nb_sectors;
>      *pnum = n;
> -    return (cluster_offset != 0);
> +    if (!cluster_offset) {
> +        return 0;

If you take your comment in patch 11 serious, you should return
bs->backing_hd ? 0 : BDRV_BLOCK_ZERO instead. (I think it would be
useful behaviour, too, because knowing that a sector is zero enables
optimisations in several places.)

Of course, this is something that could be done in the block.c
implementation of bdrv_co_get_block_status() instead of each single
driver.

Kevin

Re: [Qemu-devel] [PATCH v3 12/19] block: return get_block_status data and flags for formats

[Paolo Bonzini]

Il 30/07/2013 16:40, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/cow.c      |  8 +++++++-
>>  block/qcow.c     |  9 ++++++++-
>>  block/qcow2.c    | 16 ++++++++++++++--
>>  block/qed.c      | 35 ++++++++++++++++++++++++++++-------
>>  block/sheepdog.c |  2 +-
>>  block/vdi.c      | 13 ++++++++++++-
>>  block/vmdk.c     | 19 ++++++++++++++++++-
>>  block/vvfat.c    | 11 ++++++-----
>>  8 files changed, 94 insertions(+), 19 deletions(-)
>>
>> diff --git a/block/cow.c b/block/cow.c
>> index e738b96..1e90413 100644
>> --- a/block/cow.c
>> +++ b/block/cow.c
>> @@ -194,7 +194,13 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
>>  static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
>>          int64_t sector_num, int nb_sectors, int *num_same)
>>  {
>> -    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
>> +    BDRVCowState *s = bs->opaque;
>> +    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
>> +    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
>>  }
>>  
>>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
>> diff --git a/block/qcow.c b/block/qcow.c
>> index acd1aeb..1a65822 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -410,7 +410,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>>      if (n > nb_sectors)
>>          n = nb_sectors;
>>      *pnum = n;
>> -    return (cluster_offset != 0);
>> +    if (!cluster_offset) {
>> +        return 0;
> 
> If you take your comment in patch 11 serious, you should return
> bs->backing_hd ? 0 : BDRV_BLOCK_ZERO instead. (I think it would be
> useful behaviour, too, because knowing that a sector is zero enables
> optimisations in several places.)
> 
> Of course, this is something that could be done in the block.c
> implementation of bdrv_co_get_block_status() instead of each single
> driver.

And it is, in patch 15 ("block: use bdrv_has_zero_init to return
BDRV_BLOCK_ZERO"). :)  Should I reorder the patches?

Paolo

Re: [Qemu-devel] [PATCH v3 12/19] block: return get_block_status data and flags for formats

[Kevin Wolf]

Am 30.07.2013 um 17:15 hat Paolo Bonzini geschrieben:
> Il 30/07/2013 16:40, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  block/cow.c      |  8 +++++++-
> >>  block/qcow.c     |  9 ++++++++-
> >>  block/qcow2.c    | 16 ++++++++++++++--
> >>  block/qed.c      | 35 ++++++++++++++++++++++++++++-------
> >>  block/sheepdog.c |  2 +-
> >>  block/vdi.c      | 13 ++++++++++++-
> >>  block/vmdk.c     | 19 ++++++++++++++++++-
> >>  block/vvfat.c    | 11 ++++++-----
> >>  8 files changed, 94 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/block/cow.c b/block/cow.c
> >> index e738b96..1e90413 100644
> >> --- a/block/cow.c
> >> +++ b/block/cow.c
> >> @@ -194,7 +194,13 @@ static int coroutine_fn cow_co_is_allocated(BlockDriverState *bs,
> >>  static int64_t coroutine_fn cow_co_get_block_status(BlockDriverState *bs,
> >>          int64_t sector_num, int nb_sectors, int *num_same)
> >>  {
> >> -    return cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> >> +    BDRVCowState *s = bs->opaque;
> >> +    int ret = cow_co_is_allocated(bs, sector_num, nb_sectors, num_same);
> >> +    int64_t offset = s->cow_sectors_offset + (sector_num << BDRV_SECTOR_BITS);
> >> +    if (ret < 0) {
> >> +        return ret;
> >> +    }
> >> +    return (ret ? BDRV_BLOCK_DATA : 0) | offset | BDRV_BLOCK_OFFSET_VALID;
> >>  }
> >>  
> >>  static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
> >> diff --git a/block/qcow.c b/block/qcow.c
> >> index acd1aeb..1a65822 100644
> >> --- a/block/qcow.c
> >> +++ b/block/qcow.c
> >> @@ -410,7 +410,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
> >>      if (n > nb_sectors)
> >>          n = nb_sectors;
> >>      *pnum = n;
> >> -    return (cluster_offset != 0);
> >> +    if (!cluster_offset) {
> >> +        return 0;
> > 
> > If you take your comment in patch 11 serious, you should return
> > bs->backing_hd ? 0 : BDRV_BLOCK_ZERO instead. (I think it would be
> > useful behaviour, too, because knowing that a sector is zero enables
> > optimisations in several places.)
> > 
> > Of course, this is something that could be done in the block.c
> > implementation of bdrv_co_get_block_status() instead of each single
> > driver.
> 
> And it is, in patch 15 ("block: use bdrv_has_zero_init to return
> BDRV_BLOCK_ZERO"). :)  Should I reorder the patches?

No, I don't mind. I did look at the final state, but I missed it because
I expected something with bs->backing_hd instead of bdrv_has_zero_init()
and didn't remember that the former is already included in the latter.

Kevin

[Qemu-devel] [PATCH v3 13/19] qemu-img: add a "map" subcommand

[Paolo Bonzini]

This command dumps the metadata of an entire chain, in either tabular or JSON
format.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-img-cmds.hx |   6 ++
 qemu-img.c       | 186 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 192 insertions(+)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 4ca7e95..c97a1f4 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -45,6 +45,12 @@ STEXI
 @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 ETEXI
 
+DEF("map", img_map,
+    "map [-f fmt] [--output=ofmt] filename")
+STEXI
+@item map [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
+ETEXI
+
 DEF("snapshot", img_snapshot,
     "snapshot [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
 STEXI
diff --git a/qemu-img.c b/qemu-img.c
index c5c8ebc..b28d388 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1768,6 +1768,192 @@ static int img_info(int argc, char **argv)
     return 0;
 }
 
+
+typedef struct MapEntry {
+    int flags;
+    int depth;
+    int64_t start;
+    int64_t length;
+    int64_t offset;
+} MapEntry;
+
+static void dump_map_entry(OutputFormat output_format, MapEntry *e,
+                           MapEntry *next)
+{
+    switch (output_format) {
+    case OFORMAT_HUMAN:
+        if ((e->flags & BDRV_BLOCK_DATA) &&
+            !(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
+            error_report("File contains external, encrypted or compressed clusters.");
+            exit(1);
+        }
+        if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
+            printf("%"PRId64" %"PRId64" %d %"PRId64"\n",
+                   e->start, e->length, e->depth, e->offset);
+        }
+        /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
+         * Modify the flags here to allow more coalescing.
+         */
+        if (next &&
+            (next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) != BDRV_BLOCK_DATA) {
+            next->flags &= ~BDRV_BLOCK_DATA;
+            next->flags |= BDRV_BLOCK_ZERO;
+        }
+        break;
+    case OFORMAT_JSON:
+        printf("%s{ 'start': %"PRId64", 'length': %"PRId64", 'depth': %d, "
+               "'zero': %s, 'data': %s",
+               (e->start == 0 ? "[" : ",\n"),
+               e->start, e->length, e->depth,
+               (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false",
+               (e->flags & BDRV_BLOCK_DATA) ? "true" : "false");
+        if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
+            printf(", 'offset': %"PRId64"", e->offset);
+        }
+        putchar('}');
+
+        if (!next) {
+            printf("]\n");
+        }
+        break;
+    }
+}
+
+static int64_t get_block_status(BlockDriverState *bs, int64_t sector_num,
+                                int *nb_sectors, int *depth)
+{
+    int64_t ret;
+
+    /* As an optimization, we could cache the current range of unallocated
+     * clusters in each file of the chain, and avoid querying the same
+     * range repeatedly.
+     */
+
+    *depth = 0;
+    for (;;) {
+        int orig_nb_sectors = *nb_sectors;
+
+        ret = bdrv_get_block_status(bs, sector_num, *nb_sectors, nb_sectors);
+        if (ret < 0) {
+            return ret;
+        }
+        if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
+            return ret;
+        }
+        if (!*nb_sectors) {
+            /* Beyond the end of this image.  The extra data is read as zeroes.
+             * We're in the range of the BlockDriverState above this one, so
+             * adjust depth.
+             */
+            *nb_sectors = orig_nb_sectors;
+            (*depth)--;
+            return BDRV_BLOCK_ZERO;
+        }
+
+        bs = bs->backing_hd;
+        if (bs == NULL) {
+            return 0;
+        }
+
+        (*depth)++;
+    }
+}
+
+static int img_map(int argc, char **argv)
+{
+    int c;
+    OutputFormat output_format = OFORMAT_HUMAN;
+    BlockDriverState *bs;
+    const char *filename, *fmt, *output;
+    int64_t length;
+    MapEntry curr = { .length = 0 }, next;
+
+    fmt = NULL;
+    output = NULL;
+    for (;;) {
+        int option_index = 0;
+        static const struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"format", required_argument, 0, 'f'},
+            {"output", required_argument, 0, OPTION_OUTPUT},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, "f:h",
+                        long_options, &option_index);
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case '?':
+        case 'h':
+            help();
+            break;
+        case 'f':
+            fmt = optarg;
+            break;
+        case OPTION_OUTPUT:
+            output = optarg;
+            break;
+        }
+    }
+    if (optind >= argc) {
+        help();
+    }
+    filename = argv[optind++];
+
+    if (output && !strcmp(output, "json")) {
+        output_format = OFORMAT_JSON;
+    } else if (output && !strcmp(output, "human")) {
+        output_format = OFORMAT_HUMAN;
+    } else if (output) {
+        error_report("--output must be used with human or json as argument.");
+        return 1;
+    }
+
+    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS, true, false);
+    if (!bs) {
+        return 1;
+    }
+
+    length = bdrv_getlength(bs);
+    while (curr.start + curr.length < length) {
+        int64_t nsectors_left, ret;
+        int64_t sector_num;
+        int n, depth, flags;
+
+        /* Probe up to 1 G at a time.  */
+        sector_num = (curr.start + curr.length) >> BDRV_SECTOR_BITS;
+        nsectors_left = DIV_ROUND_UP(length, BDRV_SECTOR_SIZE) - sector_num;
+        n = MIN(1 << (30 - BDRV_SECTOR_BITS), nsectors_left);
+        ret = get_block_status(bs, sector_num, &n, &depth);
+
+        if (ret < 0) {
+            error_report("Could not read file metadata: %s", strerror(-ret));
+            return 1;
+        }
+
+        flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
+        ret &= BDRV_BLOCK_OFFSET_MASK;
+        if (curr.length == 0 || curr.flags != flags || curr.depth != depth ||
+            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) &&
+             curr.offset + curr.length != ret)) {
+            next.flags = flags;
+            next.depth = depth;
+            next.start = sector_num << BDRV_SECTOR_BITS;
+            next.offset = ret;
+            next.length = 0;
+            if (curr.length > 0) {
+                dump_map_entry(output_format, &curr, &next);
+            }
+            curr = next;
+        }
+        curr.length += n << BDRV_SECTOR_BITS;
+    }
+
+    dump_map_entry(output_format, &curr, NULL);
+    return 0;
+}
+
 #define SNAPSHOT_LIST   1
 #define SNAPSHOT_CREATE 2
 #define SNAPSHOT_APPLY  3
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 13/19] qemu-img: add a "map" subcommand

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> This command dumps the metadata of an entire chain, in either tabular or JSON
> format.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Hm, we have a 'map' command in qemu-io, which isn't exactly the same,
but then not much different either.

Depending on the use cases, should we move this to qemu-io, should we
remove the old function from qemu-io, or should we really keep both?

> diff --git a/qemu-img.c b/qemu-img.c
> index c5c8ebc..b28d388 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1768,6 +1768,192 @@ static int img_info(int argc, char **argv)
>      return 0;
>  }
>  
> +
> +typedef struct MapEntry {
> +    int flags;
> +    int depth;
> +    int64_t start;
> +    int64_t length;
> +    int64_t offset;
> +} MapEntry;
> +
> +static void dump_map_entry(OutputFormat output_format, MapEntry *e,
> +                           MapEntry *next)
> +{
> +    switch (output_format) {
> +    case OFORMAT_HUMAN:
> +        if ((e->flags & BDRV_BLOCK_DATA) &&
> +            !(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
> +            error_report("File contains external, encrypted or compressed clusters.");
> +            exit(1);
> +        }
> +        if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
> +            printf("%"PRId64" %"PRId64" %d %"PRId64"\n",
> +                   e->start, e->length, e->depth, e->offset);

Is this really human-readable output?

> +        }
> +        /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
> +         * Modify the flags here to allow more coalescing.
> +         */
> +        if (next &&
> +            (next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) != BDRV_BLOCK_DATA) {
> +            next->flags &= ~BDRV_BLOCK_DATA;
> +            next->flags |= BDRV_BLOCK_ZERO;
> +        }
> +        break;
> +    case OFORMAT_JSON:
> +        printf("%s{ 'start': %"PRId64", 'length': %"PRId64", 'depth': %d, "
> +               "'zero': %s, 'data': %s",
> +               (e->start == 0 ? "[" : ",\n"),
> +               e->start, e->length, e->depth,
> +               (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false",
> +               (e->flags & BDRV_BLOCK_DATA) ? "true" : "false");

Correct JSON uses double quotes.

> +        if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
> +            printf(", 'offset': %"PRId64"", e->offset);
> +        }
> +        putchar('}');
> +
> +        if (!next) {
> +            printf("]\n");
> +        }
> +        break;
> +    }
> +}
> +
> +static int64_t get_block_status(BlockDriverState *bs, int64_t sector_num,
> +                                int *nb_sectors, int *depth)
> +{
> +    int64_t ret;
> +
> +    /* As an optimization, we could cache the current range of unallocated
> +     * clusters in each file of the chain, and avoid querying the same
> +     * range repeatedly.
> +     */
> +
> +    *depth = 0;
> +    for (;;) {
> +        int orig_nb_sectors = *nb_sectors;
> +
> +        ret = bdrv_get_block_status(bs, sector_num, *nb_sectors, nb_sectors);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
> +            return ret;
> +        }
> +        if (!*nb_sectors) {
> +            /* Beyond the end of this image.  The extra data is read as zeroes.
> +             * We're in the range of the BlockDriverState above this one, so
> +             * adjust depth.
> +             */
> +            *nb_sectors = orig_nb_sectors;
> +            (*depth)--;
> +            return BDRV_BLOCK_ZERO;

If you implement my suggestion that bdrv_co_get_block_status() returns
BDRV_BLOCK_ZERO instead of 0 if the image has no backing file, it might
also make sense to put this check there and return BDRV_BLOCK_ZERO even
if it has a backing file, but we're after its end.

> +        }
> +
> +        bs = bs->backing_hd;
> +        if (bs == NULL) {
> +            return 0;
> +        }
> +
> +        (*depth)++;
> +    }
> +}
> +
> +static int img_map(int argc, char **argv)
> +{
> +    int c;
> +    OutputFormat output_format = OFORMAT_HUMAN;
> +    BlockDriverState *bs;
> +    const char *filename, *fmt, *output;
> +    int64_t length;
> +    MapEntry curr = { .length = 0 }, next;
> +
> +    fmt = NULL;
> +    output = NULL;
> +    for (;;) {
> +        int option_index = 0;
> +        static const struct option long_options[] = {
> +            {"help", no_argument, 0, 'h'},
> +            {"format", required_argument, 0, 'f'},
> +            {"output", required_argument, 0, OPTION_OUTPUT},
> +            {0, 0, 0, 0}
> +        };
> +        c = getopt_long(argc, argv, "f:h",
> +                        long_options, &option_index);
> +        if (c == -1) {
> +            break;
> +        }
> +        switch (c) {
> +        case '?':
> +        case 'h':
> +            help();
> +            break;
> +        case 'f':
> +            fmt = optarg;
> +            break;
> +        case OPTION_OUTPUT:
> +            output = optarg;
> +            break;
> +        }
> +    }
> +    if (optind >= argc) {
> +        help();
> +    }
> +    filename = argv[optind++];
> +
> +    if (output && !strcmp(output, "json")) {
> +        output_format = OFORMAT_JSON;
> +    } else if (output && !strcmp(output, "human")) {
> +        output_format = OFORMAT_HUMAN;
> +    } else if (output) {
> +        error_report("--output must be used with human or json as argument.");
> +        return 1;
> +    }
> +
> +    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS, true, false);
> +    if (!bs) {
> +        return 1;
> +    }
> +
> +    length = bdrv_getlength(bs);

bdrv_getlength() can fail.

> +    while (curr.start + curr.length < length) {
> +        int64_t nsectors_left, ret;
> +        int64_t sector_num;
> +        int n, depth, flags;
> +
> +        /* Probe up to 1 G at a time.  */
> +        sector_num = (curr.start + curr.length) >> BDRV_SECTOR_BITS;
> +        nsectors_left = DIV_ROUND_UP(length, BDRV_SECTOR_SIZE) - sector_num;
> +        n = MIN(1 << (30 - BDRV_SECTOR_BITS), nsectors_left);
> +        ret = get_block_status(bs, sector_num, &n, &depth);
> +
> +        if (ret < 0) {
> +            error_report("Could not read file metadata: %s", strerror(-ret));
> +            return 1;

This leaks bs.

> +        }
> +
> +        flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
> +        ret &= BDRV_BLOCK_OFFSET_MASK;
> +        if (curr.length == 0 || curr.flags != flags || curr.depth != depth ||
> +            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) &&
> +             curr.offset + curr.length != ret)) {
> +            next.flags = flags;
> +            next.depth = depth;
> +            next.start = sector_num << BDRV_SECTOR_BITS;
> +            next.offset = ret;
> +            next.length = 0;
> +            if (curr.length > 0) {
> +                dump_map_entry(output_format, &curr, &next);
> +            }
> +            curr = next;
> +        }
> +        curr.length += n << BDRV_SECTOR_BITS;
> +    }
> +
> +    dump_map_entry(output_format, &curr, NULL);
> +    return 0;
> +}
> +
>  #define SNAPSHOT_LIST   1
>  #define SNAPSHOT_CREATE 2
>  #define SNAPSHOT_APPLY  3

Kevin

Re: [Qemu-devel] [PATCH v3 13/19] qemu-img: add a "map" subcommand

[Paolo Bonzini]

Il 30/07/2013 17:13, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> This command dumps the metadata of an entire chain, in either tabular or JSON
>> format.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Hm, we have a 'map' command in qemu-io, which isn't exactly the same,
> but then not much different either.
> 
> Depending on the use cases, should we move this to qemu-io, should we
> remove the old function from qemu-io, or should we really keep both?

I would remove the one in qemu-io (but I haven't checked if qemu-iotests
uses it).

>> diff --git a/qemu-img.c b/qemu-img.c
>> index c5c8ebc..b28d388 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1768,6 +1768,192 @@ static int img_info(int argc, char **argv)
>>      return 0;
>>  }
>>  
>> +
>> +typedef struct MapEntry {
>> +    int flags;
>> +    int depth;
>> +    int64_t start;
>> +    int64_t length;
>> +    int64_t offset;
>> +} MapEntry;
>> +
>> +static void dump_map_entry(OutputFormat output_format, MapEntry *e,
>> +                           MapEntry *next)
>> +{
>> +    switch (output_format) {
>> +    case OFORMAT_HUMAN:
>> +        if ((e->flags & BDRV_BLOCK_DATA) &&
>> +            !(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
>> +            error_report("File contains external, encrypted or compressed clusters.");
>> +            exit(1);
>> +        }
>> +        if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
>> +            printf("%"PRId64" %"PRId64" %d %"PRId64"\n",
>> +                   e->start, e->length, e->depth, e->offset);
> 
> Is this really human-readable output?

I will change it to use tabs and add a heading line.

>> +        }
>> +        /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
>> +         * Modify the flags here to allow more coalescing.
>> +         */
>> +        if (next &&
>> +            (next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) != BDRV_BLOCK_DATA) {
>> +            next->flags &= ~BDRV_BLOCK_DATA;
>> +            next->flags |= BDRV_BLOCK_ZERO;
>> +        }
>> +        break;
>> +    case OFORMAT_JSON:
>> +        printf("%s{ 'start': %"PRId64", 'length': %"PRId64", 'depth': %d, "
>> +               "'zero': %s, 'data': %s",
>> +               (e->start == 0 ? "[" : ",\n"),
>> +               e->start, e->length, e->depth,
>> +               (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false",
>> +               (e->flags & BDRV_BLOCK_DATA) ? "true" : "false");
> 
> Correct JSON uses double quotes.

Will fix.

>> +        if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
>> +            printf(", 'offset': %"PRId64"", e->offset);
>> +        }
>> +        putchar('}');
>> +
>> +        if (!next) {
>> +            printf("]\n");
>> +        }
>> +        break;
>> +    }
>> +}
>> +
>> +static int64_t get_block_status(BlockDriverState *bs, int64_t sector_num,
>> +                                int *nb_sectors, int *depth)
>> +{
>> +    int64_t ret;
>> +
>> +    /* As an optimization, we could cache the current range of unallocated
>> +     * clusters in each file of the chain, and avoid querying the same
>> +     * range repeatedly.
>> +     */
>> +
>> +    *depth = 0;
>> +    for (;;) {
>> +        int orig_nb_sectors = *nb_sectors;
>> +
>> +        ret = bdrv_get_block_status(bs, sector_num, *nb_sectors, nb_sectors);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +        if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
>> +            return ret;
>> +        }
>> +        if (!*nb_sectors) {
>> +            /* Beyond the end of this image.  The extra data is read as zeroes.
>> +             * We're in the range of the BlockDriverState above this one, so
>> +             * adjust depth.
>> +             */
>> +            *nb_sectors = orig_nb_sectors;
>> +            (*depth)--;
>> +            return BDRV_BLOCK_ZERO;
> 
> If you implement my suggestion that bdrv_co_get_block_status() returns
> BDRV_BLOCK_ZERO instead of 0 if the image has no backing file, it might
> also make sense to put this check there and return BDRV_BLOCK_ZERO even
> if it has a backing file, but we're after its end.

Good idea.  I'll make this change in a separate patch.

>> +        }
>> +
>> +        bs = bs->backing_hd;
>> +        if (bs == NULL) {
>> +            return 0;
>> +        }
>> +
>> +        (*depth)++;
>> +    }
>> +}
>> +
>> +static int img_map(int argc, char **argv)
>> +{
>> +    int c;
>> +    OutputFormat output_format = OFORMAT_HUMAN;
>> +    BlockDriverState *bs;
>> +    const char *filename, *fmt, *output;
>> +    int64_t length;
>> +    MapEntry curr = { .length = 0 }, next;
>> +
>> +    fmt = NULL;
>> +    output = NULL;
>> +    for (;;) {
>> +        int option_index = 0;
>> +        static const struct option long_options[] = {
>> +            {"help", no_argument, 0, 'h'},
>> +            {"format", required_argument, 0, 'f'},
>> +            {"output", required_argument, 0, OPTION_OUTPUT},
>> +            {0, 0, 0, 0}
>> +        };
>> +        c = getopt_long(argc, argv, "f:h",
>> +                        long_options, &option_index);
>> +        if (c == -1) {
>> +            break;
>> +        }
>> +        switch (c) {
>> +        case '?':
>> +        case 'h':
>> +            help();
>> +            break;
>> +        case 'f':
>> +            fmt = optarg;
>> +            break;
>> +        case OPTION_OUTPUT:
>> +            output = optarg;
>> +            break;
>> +        }
>> +    }
>> +    if (optind >= argc) {
>> +        help();
>> +    }
>> +    filename = argv[optind++];
>> +
>> +    if (output && !strcmp(output, "json")) {
>> +        output_format = OFORMAT_JSON;
>> +    } else if (output && !strcmp(output, "human")) {
>> +        output_format = OFORMAT_HUMAN;
>> +    } else if (output) {
>> +        error_report("--output must be used with human or json as argument.");
>> +        return 1;
>> +    }
>> +
>> +    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS, true, false);
>> +    if (!bs) {
>> +        return 1;
>> +    }
>> +
>> +    length = bdrv_getlength(bs);
> 
> bdrv_getlength() can fail.

I'll make it always use bs->total_sectors instead of fixing this.

>> +    while (curr.start + curr.length < length) {
>> +        int64_t nsectors_left, ret;
>> +        int64_t sector_num;
>> +        int n, depth, flags;
>> +
>> +        /* Probe up to 1 G at a time.  */
>> +        sector_num = (curr.start + curr.length) >> BDRV_SECTOR_BITS;
>> +        nsectors_left = DIV_ROUND_UP(length, BDRV_SECTOR_SIZE) - sector_num;
>> +        n = MIN(1 << (30 - BDRV_SECTOR_BITS), nsectors_left);
>> +        ret = get_block_status(bs, sector_num, &n, &depth);
>> +
>> +        if (ret < 0) {
>> +            error_report("Could not read file metadata: %s", strerror(-ret));
>> +            return 1;
> 
> This leaks bs.

Right.

Paolo

>> +        }
>> +
>> +        flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
>> +        ret &= BDRV_BLOCK_OFFSET_MASK;
>> +        if (curr.length == 0 || curr.flags != flags || curr.depth != depth ||
>> +            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) &&
>> +             curr.offset + curr.length != ret)) {
>> +            next.flags = flags;
>> +            next.depth = depth;
>> +            next.start = sector_num << BDRV_SECTOR_BITS;
>> +            next.offset = ret;
>> +            next.length = 0;
>> +            if (curr.length > 0) {
>> +                dump_map_entry(output_format, &curr, &next);
>> +            }
>> +            curr = next;
>> +        }
>> +        curr.length += n << BDRV_SECTOR_BITS;
>> +    }
>> +
>> +    dump_map_entry(output_format, &curr, NULL);
>> +    return 0;
>> +}
>> +
>>  #define SNAPSHOT_LIST   1
>>  #define SNAPSHOT_CREATE 2
>>  #define SNAPSHOT_APPLY  3
> 
> Kevin
> 
> 

Re: [Qemu-devel] [PATCH v3 13/19] qemu-img: add a "map" subcommand

[Kevin Wolf]

Am 30.07.2013 um 17:22 hat Paolo Bonzini geschrieben:
> Il 30/07/2013 17:13, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> This command dumps the metadata of an entire chain, in either tabular or JSON
> >> format.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Hm, we have a 'map' command in qemu-io, which isn't exactly the same,
> > but then not much different either.
> > 
> > Depending on the use cases, should we move this to qemu-io, should we
> > remove the old function from qemu-io, or should we really keep both?
> 
> I would remove the one in qemu-io (but I haven't checked if qemu-iotests
> uses it).

qemu-iotests does use it in some block job tests (comparing the map
output for source and target), but I guess it can use this one instead.

Kevin

Re: [Qemu-devel] [PATCH v3 13/19] qemu-img: add a "map" subcommand

[Kevin Wolf]

Am 30.07.2013 um 17:22 hat Paolo Bonzini geschrieben:
> Il 30/07/2013 17:13, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> This command dumps the metadata of an entire chain, in either tabular or JSON
> >> format.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Hm, we have a 'map' command in qemu-io, which isn't exactly the same,
> > but then not much different either.
> > 
> > Depending on the use cases, should we move this to qemu-io, should we
> > remove the old function from qemu-io, or should we really keep both?
> 
> I would remove the one in qemu-io (but I haven't checked if qemu-iotests
> uses it).
> 
> >> diff --git a/qemu-img.c b/qemu-img.c
> >> index c5c8ebc..b28d388 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -1768,6 +1768,192 @@ static int img_info(int argc, char **argv)
> >>      return 0;
> >>  }
> >>  
> >> +
> >> +typedef struct MapEntry {
> >> +    int flags;
> >> +    int depth;
> >> +    int64_t start;
> >> +    int64_t length;
> >> +    int64_t offset;
> >> +} MapEntry;
> >> +
> >> +static void dump_map_entry(OutputFormat output_format, MapEntry *e,
> >> +                           MapEntry *next)
> >> +{
> >> +    switch (output_format) {
> >> +    case OFORMAT_HUMAN:
> >> +        if ((e->flags & BDRV_BLOCK_DATA) &&
> >> +            !(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
> >> +            error_report("File contains external, encrypted or compressed clusters.");
> >> +            exit(1);
> >> +        }
> >> +        if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
> >> +            printf("%"PRId64" %"PRId64" %d %"PRId64"\n",
> >> +                   e->start, e->length, e->depth, e->offset);
> > 
> > Is this really human-readable output?
> 
> I will change it to use tabs and add a heading line.

The documentation patch contains a line like this:

0       131072       2        327680

A heading line and tabs (or even better, fixed printf column widths)
sounds good, but I think if it's really only for human users and not for
shell scripts, we can further improve the output:

Offset      Length      Mapped to       File

0         + 128k     -> 320k            /tmp/backing.qcow2
128k      + 256k     -> 2M              /tmp/overlay.qcow2

We could add another exact, more technical format like this, which could
leave out things like + and -> so that it is easier to parse from shell
scripts, too:

Offset      Length      Mapped to       Depth   File

        0    0x20000       0x50000      1       /tmp/backing.qcow2
  0x20000    0x40000      0x200000      0       /tmp/overlay.qcow2

What do you think?

Kevin

Re: [Qemu-devel] [PATCH v3 13/19] qemu-img: add a "map" subcommand

[Paolo Bonzini]

> The documentation patch contains a line like this:
> 
> 0       131072       2        327680
> 
> A heading line and tabs (or even better, fixed printf column widths)
> sounds good, but I think if it's really only for human users and not for
> shell scripts, we can further improve the output:
> 
> Offset      Length      Mapped to       File
> 
> 0         + 128k     -> 320k            /tmp/backing.qcow2
> 128k      + 256k     -> 2M              /tmp/overlay.qcow2

Changing depth to file is a good idea, but it rules out any
possibility of using it in shell scripts due to newlines in
files.  I don't think + and -> add much and I'd rather leave
them out.

I'm quite ambivalent with respect to hexadecimal vs. decimal,
of course hex is more readable.  Some tools may prefer decimal,
but then x=`eval echo "\$(($x))"` is an easy way to convert.

Any user of this stuff is going to be quite technical, so in
the end I would go for this:

Offset  Length      Mapped to       File
0x0     0x20000     0x50000         /tmp/backup.qcow2

Paolo

Re: [Qemu-devel] [PATCH v3 13/19] qemu-img: add a "map" subcommand

[Kevin Wolf]

Am 31.07.2013 um 14:13 hat Paolo Bonzini geschrieben:
> > The documentation patch contains a line like this:
> > 
> > 0       131072       2        327680
> > 
> > A heading line and tabs (or even better, fixed printf column widths)
> > sounds good, but I think if it's really only for human users and not for
> > shell scripts, we can further improve the output:
> > 
> > Offset      Length      Mapped to       File
> > 
> > 0         + 128k     -> 320k            /tmp/backing.qcow2
> > 128k      + 256k     -> 2M              /tmp/overlay.qcow2
> 
> Changing depth to file is a good idea, but it rules out any
> possibility of using it in shell scripts due to newlines in
> files.  I don't think + and -> add much and I'd rather leave
> them out.
> 
> I'm quite ambivalent with respect to hexadecimal vs. decimal,
> of course hex is more readable.  Some tools may prefer decimal,
> but then x=`eval echo "\$(($x))"` is an easy way to convert.
> 
> Any user of this stuff is going to be quite technical, so in
> the end I would go for this:
> 
> Offset  Length      Mapped to       File
> 0x0     0x20000     0x50000         /tmp/backup.qcow2

Okay, fine with me.

Kevin

[Qemu-devel] [PATCH v3 14/19] docs, qapi: document qemu-img map

[Paolo Bonzini]

Eric Blake also requested including the output in qapi-schema.json,
so that it is published through the introspection mechanism.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi-schema.json | 29 +++++++++++++++++++++++++++++
 qemu-img.texi    | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 592bb9c..421ea8a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -803,6 +803,35 @@
 { 'enum': 'BlockDeviceIoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
 
 ##
+# @BlockDeviceMapEntry:
+#
+# Entry in the metadata map of the device (returned by "qemu-img map")
+#
+# @start: Offset in the image of the first byte described by this entry
+#         (in bytes)
+#
+# @length: Length of the range described by this entry (in bytes)
+#
+# @depth: Number of layers (0 = top image, 1 = top image's backing file, etc.)
+#         before reaching one for which the range is allocated.  The value is
+#         in the range 0 to the depth of the image chain - 1.
+#
+# @zero: the sectors in this range read as zeros
+#
+# @data: reading the image will actually read data from a file (in particular,
+#        if @offset is present this means that the sectors are not simply
+#        preallocated, but contain actual data in raw format)
+#
+# @offset: if present, the image file stores the data for this range in
+#          raw format at the given offset.
+#
+# Since 1.6
+##
+{ 'type': 'BlockDeviceMapEntry',
+  'data': { 'start': 'int', 'length': 'int', 'depth': 'int', 'zero': 'bool',
+            'data': 'bool', '*offset': 'int' } }
+
+##
 # @BlockDirtyInfo:
 #
 # Block dirty bitmap information.
diff --git a/qemu-img.texi b/qemu-img.texi
index 69f1bda..43f0b31 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -213,6 +213,52 @@ To enumerate information about each disk image in the above chain, starting from
 qemu-img info --backing-chain snap2.qcow2
 @end example
 
+@item map [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
+
+Dump the metadata of image @var{filename} and its backing file chain.
+In particular, this commands dumps the allocation state of every sector
+of @var{filename}, together with the topmost file that allocates it in
+the backing file chain.
+
+Two option formats are possible.  The default format (@code{human})
+only dumps known-nonzero areas of the file.  Known-zero parts of the
+file are omitted altogether, and likewise for parts that are not allocated
+throughout the chain.  @command{qemu-img} output will identify a file
+from where the data can be read, and the offset in the file.  Each line
+will include four fields; for example:
+@example
+0       131072       2        327680
+@end example
+@noindent
+means that 131072 bytes starting at offset 0 in the image are available at
+depth 2 (i.e. by opening in @code{raw} format the backing file of the
+backing file of @var{filename}) starting at offset 327680.  Data that
+is compressed, encrypted, or otherwise not available in raw format will
+cause an error if @code{human} format is in use.
+
+The alternative format @code{json} will return an array of dictionaries
+in JSON format.  It will include similar information in
+the @code{start}, @code{length}, @code{depth}, @code{offset} fields;
+it will also include other more specific information:
+@itemize @minus
+@item
+whether the sectors contain actual data or not (boolean field @code{data};
+if false, the sectors are either unallocated or stored as optimized
+all-zero clusters);
+
+@item
+whether the data is known to read as zero (boolean field @code{zero});
+@end itemize
+
+In JSON format, the @code{offset} field is optional; it is absent in
+cases where @code{human} format would omit the entry or exit with an error.
+If @code{data} is false and the @code{offset} field is present, the
+corresponding sectors in the file are not yet in use, but they are
+preallocated.
+
+For more information, consult @file{include/block/block.h} in QEMU's
+source code.
+
 @item snapshot [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot} ] @var{filename}
 
 List, apply, create or delete snapshots in image @var{filename}.
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 14/19] docs, qapi: document qemu-img map

[Eric Blake]

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

On 07/25/2013 08:23 AM, Paolo Bonzini wrote:
> Eric Blake also requested including the output in qapi-schema.json,
> so that it is published through the introspection mechanism.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi-schema.json | 29 +++++++++++++++++++++++++++++
>  qemu-img.texi    | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 

> +++ b/qapi-schema.json
> @@ -803,6 +803,35 @@
>  { 'enum': 'BlockDeviceIoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
>  
>  ##
> +# @BlockDeviceMapEntry:
> +#
> +# Entry in the metadata map of the device (returned by "qemu-img map")

Thanks for also referencing where this type is relevant (if we later add
a QMP command that also exposes the map via this type, we would remove
the parenthetical comment at that time).

> +#
> +# Since 1.6

Are we still shooting for 1.6, or has this missed the freeze?

>  
> +@item map [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
> +
> +Dump the metadata of image @var{filename} and its backing file chain.
> +In particular, this commands dumps the allocation state of every sector

s/commands/command/

> +of @var{filename}, together with the topmost file that allocates it in
> +the backing file chain.
> +
> +Two option formats are possible.  The default format (@code{human})
> +only dumps known-nonzero areas of the file.  Known-zero parts of the
> +file are omitted altogether, and likewise for parts that are not allocated
> +throughout the chain.  @command{qemu-img} output will identify a file
> +from where the data can be read, and the offset in the file.  Each line
> +will include four fields; for example:
> +@example
> +0       131072       2        327680
> +@end example
> +@noindent
> +means that 131072 bytes starting at offset 0 in the image are available at
> +depth 2 (i.e. by opening in @code{raw} format the backing file of the
> +backing file of @var{filename}) starting at offset 327680.  Data that
> +is compressed, encrypted, or otherwise not available in raw format will
> +cause an error if @code{human} format is in use.

In case of a hybrid file (part raw, part encrypted), does this command
exit on first error, or only after printing as much raw information was
available?  That is, even if we can't describe the entire allocation
map, it may still be useful to dump as much information as possible
before declaring that more data is inaccessible via raw mapping.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH v3 14/19] docs, qapi: document qemu-img map

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 30/07/2013 17:48, Eric Blake ha scritto:
> In case of a hybrid file (part raw, part encrypted), does this
> command exit on first error, or only after printing as much raw
> information was available?

On first error.  The reason is that usually everything will be
encrypted (in the common case where you have encryption but no backing
files), and it would be a waste of time to walk the whole image.

Paolo

> That is, even if we can't describe the entire allocation map, it
> may still be useful to dump as much information as possible before
> declaring that more data is inaccessible via raw mapping.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR9+GyAAoJEBvWZb6bTYbycHkQAJ4vGYkpxvCMRbJcgAL4UgTM
0gxYOeUjz2CsxkbTn+gYN6hYyuAreuIDWiuQ4nlS573CIXaZXUiCw4iuIZItgUuU
gD2l9/PMz2GdeJk7rP7XaybRh5FUsKjbxXcWMSz5lSvIw4QYD0a0xe+GUZCUJ+Yj
sbw4EWmWsp/fQftm0HIeU/UdZ2S+frMw7owsxRrbLVnf5tCLGAp+iHmMMu1Jtz97
f6z5ijvXia+q0JUF4ui1zosCybLNJ7yiG63TLdu/Pk1NymBMNAAIkLbzSRrR/xoc
un0IHG1aIrTldmaoJL0SfdkHWP2f7jzfNuhODptVWk8XnJIK+bpSbF1KjfJ6irrE
VjfxFbJN/V8f7tJf3Ck3SUHMzQSsHMTtG5IwD//au1ppb467j96OXAOUftxY4s8A
10C0e2jVPeTmbJxOkN3rQFwhYVhYre2sd0VfSbZqHw/FDkcpHqlMCsu6BqNy5iHI
WiTNMS9KP7hNGVD7JHOTnCUmeVfBQXCqL6QMr8Da5cJxbBIQ36XDep/Gr1Qd593P
BNs74MT80H4xpdwl+bBK8WHpzmD6+o0PlhpbxZq5abji8WitgvcPRM9v7ydq/d77
qGJ8JcfNmY7hlqU/Cwom3WP6Wdv+sayojZJ1aI4TXTuXVzGzlqck+KSspv7sTTwE
eRwSQdD1/81OpppvIy+R
=sVYH
-----END PGP SIGNATURE-----

[Qemu-devel] [PATCH v3 15/19] block: use bdrv_has_zero_init to return BDRV_BLOCK_ZERO

[Paolo Bonzini]

Alternatively, this could use a "discard zeroes data" flag returned
by bdrv_get_info.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 7cfbf71..fc1da56 100644
--- a/block.c
+++ b/block.c
@@ -2991,6 +2991,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
                                                      int nb_sectors, int *pnum)
 {
     int64_t n;
+    int64_t ret;
 
     if (sector_num >= bs->total_sectors) {
         *pnum = 0;
@@ -3007,7 +3008,12 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         return BDRV_BLOCK_DATA;
     }
 
-    return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
+    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
+    if (!(ret & BDRV_BLOCK_DATA) && bdrv_has_zero_init(bs)) {
+        ret |= BDRV_BLOCK_ZERO;
+    }
+
+    return ret;
 }
 
 /* Coroutine wrapper for bdrv_get_block_status() */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 15/19] block: use bdrv_has_zero_init to return BDRV_BLOCK_ZERO

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Alternatively, this could use a "discard zeroes data" flag returned
> by bdrv_get_info.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 7cfbf71..fc1da56 100644
> --- a/block.c
> +++ b/block.c
> @@ -2991,6 +2991,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int nb_sectors, int *pnum)
>  {
>      int64_t n;
> +    int64_t ret;
>  
>      if (sector_num >= bs->total_sectors) {
>          *pnum = 0;
> @@ -3007,7 +3008,12 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          return BDRV_BLOCK_DATA;
>      }
>  
> -    return bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> +    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);

I think here you should check for ret < 0 before modifying bits.

> +    if (!(ret & BDRV_BLOCK_DATA) && bdrv_has_zero_init(bs)) {
> +        ret |= BDRV_BLOCK_ZERO;
> +    }
> +
> +    return ret;
>  }

Kevin

[Qemu-devel] [PATCH v3 16/19] raw-posix: return get_block_status data and flags

[Paolo Bonzini]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index dbc65b0..d011cfd 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1089,7 +1089,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                             int nb_sectors, int *pnum)
 {
     off_t start, data, hole;
-    int ret;
+    int64_t ret;
 
     ret = fd_open(bs);
     if (ret < 0) {
@@ -1097,6 +1097,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
     }
 
     start = sector_num * BDRV_SECTOR_SIZE;
+    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
 
 #ifdef CONFIG_FIEMAP
 
@@ -1114,7 +1115,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
     if (ioctl(s->fd, FS_IOC_FIEMAP, &f) == -1) {
         /* Assume everything is allocated.  */
         *pnum = nb_sectors;
-        return 1;
+        return ret;
     }
 
     if (f.fm.fm_mapped_extents == 0) {
@@ -1141,7 +1142,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
 
         /* Most likely EINVAL.  Assume everything is allocated.  */
         *pnum = nb_sectors;
-        return 1;
+        return ret;
     }
 
     if (hole > start) {
@@ -1154,19 +1155,21 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
         }
     }
 #else
-    *pnum = nb_sectors;
-    return 1;
+    data = 0;
+    hole = start + nb_sectors * BDRV_SECTOR_SIZE;
 #endif
 
     if (data <= start) {
         /* On a data extent, compute sectors to the end of the extent.  */
         *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE);
-        return 1;
     } else {
         /* On a hole, compute sectors to the beginning of the next extent.  */
         *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
-        return 0;
+        ret &= ~BDRV_BLOCK_DATA;
+        ret |= BDRV_BLOCK_ZERO;
     }
+
+    return ret;
 }
 
 static coroutine_fn BlockDriverAIOCB *raw_aio_discard(BlockDriverState *bs,
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 17/19] raw-posix: detect XFS unwritten extents

[Paolo Bonzini]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index d011cfd..1b41ea3 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1128,6 +1128,9 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
     } else {
         data = f.fe.fe_logical;
         hole = f.fe.fe_logical + f.fe.fe_length;
+        if (f.fe.fe_flags & FIEMAP_EXTENT_UNWRITTEN) {
+            ret |= BDRV_BLOCK_ZERO;
+        }
     }
 
 #elif defined SEEK_HOLE && defined SEEK_DATA
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 18/19] block: add default get_block_status implementation for protocols

[Paolo Bonzini]

Protocols return raw data, so you can assume the offsets to pass
through unchanged.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index fc1da56..8738937 100644
--- a/block.c
+++ b/block.c
@@ -3005,7 +3005,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
 
     if (!bs->drv->bdrv_co_get_block_status) {
         *pnum = nb_sectors;
-        return BDRV_BLOCK_DATA;
+        ret = BDRV_BLOCK_DATA;
+        if (bs->drv->protocol_name) {
+            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+        }
+        return ret;
     }
 
     ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 18/19] block: add default get_block_status implementation for protocols

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Protocols return raw data, so you can assume the offsets to pass
> through unchanged.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Not really objecting, but is this useful?

In formats, BDRV_BLOCK_OFFSET_VALID means that the given offset is valid
for bs->file, which is something that protocols simply don't have. You
can do it like you do here, but actually this information is redundant.

Do you make use of it for anything?

Kevin

Re: [Qemu-devel] [PATCH v3 18/19] block: add default get_block_status implementation for protocols

[Paolo Bonzini]

----- Original Message -----
> From: "Kevin Wolf" <kwolf@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, stefanha@redhat.com, eblake@redhat.com, pl@kamp.de
> Sent: Wednesday, July 31, 2013 11:12:27 AM
> Subject: Re: [PATCH v3 18/19] block: add default get_block_status implementation for protocols
> 
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> > Protocols return raw data, so you can assume the offsets to pass
> > through unchanged.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Not really objecting, but is this useful?
> 
> In formats, BDRV_BLOCK_OFFSET_VALID means that the given offset is valid
> for bs->file, which is something that protocols simply don't have. You
> can do it like you do here, but actually this information is redundant.
> 
> Do you make use of it for anything?

It is used by format=raw.  I could do it in raw.c, but some protocols
are opened directly without the "raw" veneer on top.

Paolo

[Qemu-devel] [PATCH v3 19/19] block: look for zero blocks in bs->file

[Paolo Bonzini]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 8738937..1493f22 100644
--- a/block.c
+++ b/block.c
@@ -2991,7 +2991,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
                                                      int nb_sectors, int *pnum)
 {
     int64_t n;
-    int64_t ret;
+    int64_t ret, ret2;
 
     if (sector_num >= bs->total_sectors) {
         *pnum = 0;
@@ -3017,6 +3017,14 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         ret |= BDRV_BLOCK_ZERO;
     }
 
+    if (bs->file &&
+        (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
+        (ret & BDRV_BLOCK_OFFSET_VALID)) {
+        ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
+                                        *pnum, pnum);
+        ret |= (ret2 & BDRV_BLOCK_ZERO);
+    }
+
     return ret;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 19/19] block: look for zero blocks in bs->file

[Kevin Wolf]

Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 8738937..1493f22 100644
> --- a/block.c
> +++ b/block.c
> @@ -2991,7 +2991,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int nb_sectors, int *pnum)
>  {
>      int64_t n;
> -    int64_t ret;
> +    int64_t ret, ret2;
>  
>      if (sector_num >= bs->total_sectors) {
>          *pnum = 0;
> @@ -3017,6 +3017,14 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          ret |= BDRV_BLOCK_ZERO;
>      }
>  
> +    if (bs->file &&
> +        (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
> +        (ret & BDRV_BLOCK_OFFSET_VALID)) {
> +        ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> +                                        *pnum, pnum);

Here the error check is missing, too.

> +        ret |= (ret2 & BDRV_BLOCK_ZERO);
> +    }
> +
>      return ret;
>  }

Kevin

Re: [Qemu-devel] [PATCH v3 19/19] block: look for zero blocks in bs->file

[Paolo Bonzini]

----- Original Message -----
> From: "Kevin Wolf" <kwolf@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, stefanha@redhat.com, eblake@redhat.com, pl@kamp.de
> Sent: Wednesday, July 31, 2013 11:21:27 AM
> Subject: Re: [PATCH v3 19/19] block: look for zero blocks in bs->file
> 
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  block.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index 8738937..1493f22 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2991,7 +2991,7 @@ static int64_t coroutine_fn
> > bdrv_co_get_block_status(BlockDriverState *bs,
> >                                                       int nb_sectors, int
> >                                                       *pnum)
> >  {
> >      int64_t n;
> > -    int64_t ret;
> > +    int64_t ret, ret2;
> >  
> >      if (sector_num >= bs->total_sectors) {
> >          *pnum = 0;
> > @@ -3017,6 +3017,14 @@ static int64_t coroutine_fn
> > bdrv_co_get_block_status(BlockDriverState *bs,
> >          ret |= BDRV_BLOCK_ZERO;
> >      }
> >  
> > +    if (bs->file &&
> > +        (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
> > +        (ret & BDRV_BLOCK_OFFSET_VALID)) {
> > +        ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> > +                                        *pnum, pnum);
> 
> Here the error check is missing, too.

Right.  Here I can ignore errors, but I should still check ret2 >= 0 before
taking random bits out of it.

Paolo

> 
> > +        ret |= (ret2 & BDRV_BLOCK_ZERO);
> > +    }
> > +
> >      return ret;
> >  }
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH v3 00/19] Add qemu-img subcommand to dump file metadata

[Stefan Hajnoczi]

On Thu, Jul 25, 2013 at 04:22:58PM +0200, Paolo Bonzini wrote:
> This series adds a subcommand to "map" that can dump file metadata.
> Metadata that is dumped includes:
> 
> - whether blocks are allocated in bs->file and, if so, where
> 
> - whether blocks are zero
> 
> - whether data is read from bs or bs->backing_hd
> 
> Metadata is dumped for an entire chain of images.  One example usage is
> (for non-compressed, non-encrypted images) to transform the metadata
> into a Linux device-mapper setup, and make a qcow2 image available (for
> read only) as a block device.  Another possible usage is to determine
> the used areas of a file, and convert it in place to another format.
> Alternatively, the richer data can be used by block jobs to quickly
> determine if a block is zero without reading it.
> 
> The patches implement a new operation, bdrv_co_get_block_status, that
> supersedes bdrv_co_is_allocated.  The bdrv_is_allocated API is still
> available of course, and implemented on top of the new operation.
> 
> Patches 1-3 touch cow, which uses bdrv_co_is_allocated during its reads
> and writes.  I optimized it a bit because it was unbearably slow during
> testing.  With these patches (also tested with blkverify), booting of
> a cow guest image is not particularly slow.
> 
> Patch 4 fixes the bs->total_sectors file size cache, which is used in
> bdrv_co_get_block_status to clamp invalid input.
> 
> Patches 5 to 7 clean up the bdrv_is_allocated and bdrv_is_allocated_above
> implementation, eliminating some code duplication.
> 
> Patches 8 and 9 tweak bdrv_has_zero_init and its usage in qemu-img in
> a way that helps when implementing the new API.
> 
> Patches 10 to 12 implement bdrv_get_block_status and change the formats
> to report all the available information.
> 
> Patch 13 and 14 adds the "map" subcommand to qemu-img, and the relevant
> documentation.
> 
> Finally, patches 15 to 19 tweak the implementation to extract more
> information from protocols, and combine this information with format
> metadata whenever possible.
> 
> v2->v3:
>         new patch 4 to fix qemu-iotests regression
>         new patch 14 for documentation
> 
> Paolo Bonzini (19):
>   cow: make reads go at a decent speed
>   cow: make writes go at a less indecent speed
>   cow: do not call bdrv_co_is_allocated
>   block: update bs->total_sectors on writes
>   block: make bdrv_co_is_allocated static
>   block: remove bdrv_is_allocated_above/bdrv_co_is_allocated_above
>     distinction
>   block: expect errors from bdrv_co_is_allocated
>   qemu-img: always probe the input image for allocated sectors
>   block: make bdrv_has_zero_init return false for copy-on-write-images
>   block: introduce bdrv_get_block_status API
>   block: define get_block_status return value
>   block: return get_block_status data and flags for formats
>   qemu-img: add a "map" subcommand
>   docs, qapi: document qemu-img map
>   block: use bdrv_has_zero_init to return BDRV_BLOCK_ZERO
>   raw-posix: return get_block_status data and flags
>   raw-posix: detect XFS unwritten extents
>   block: add default get_block_status implementation for protocols
>   block: look for zero blocks in bs->file

Looking forward to v4 :).  It seems there are still some things to tidy
up but soon this can be merged.

Stefan