[Qemu-devel] [PATCH v3 0/3] qemu-img check: format allocation info

[Qemu-devel] [PATCH v3 0/3] qemu-img check: format allocation info

[Vladimir Sementsov-Ogievskiy]

Hi all.

See 01 patch for the doc.

v3: - improve docs
    - rename fields
    - add 'zero' type of underlying file portions status. It as these
      areas cannot be presented as 'discarded', but they are not
      occupying real space, so we don't want to present them as
      'allocated data' too.
    - remove last patch. It is not needed after introducing new naming
      for the fields

v2: fix build error, gcc things that some variables may be used
    uninitialized (actually they didn't).

v1: 

These series is a replacement for "qemu-img check: unallocated size"
series.

There was a question, should we account allocated clusters in qcow2 but
actually holes in underalying file as allocated or not. Instead of
hiding this information in one-number statistic I've decided to print
the whole information, 5 numbers:

For allocated by top-level format driver (qcow2 for ex.) clusters, 3
numbers: number of bytes, which are:
 - allocated in underlying file
 - holes in underlying file
 - after end of underlying file

To account other areas of underlying file, 2 more numbers of bytes:
 - unallocated by top-level driver but allocated in underlying file
 - unallocated by top-level driver and holes in underlying file

Vladimir Sementsov-Ogievskiy (3):
  block: add bdrv_get_format_alloc_stat format interface
  qcow2: add .bdrv_get_format_alloc_stat
  qemu-img check: add format allocation info

 block.c                   |  16 +++++
 block/qcow2-refcount.c    | 152 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |   2 +
 block/qcow2.h             |   2 +
 include/block/block.h     |   3 +
 include/block/block_int.h |   2 +
 qapi/block-core.json      |  61 ++++++++++++++++++-
 qemu-img.c                |  42 +++++++++++++
 8 files changed, 279 insertions(+), 1 deletion(-)

-- 
2.11.1

[Qemu-devel] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

The function should collect statistics, about used/unused by top-level
format driver space (in its .file) and allocation status
(data/zero/discarded/after-eof) of corresponding areas in this .file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                   | 16 ++++++++++++++
 include/block/block.h     |  3 +++
 include/block/block_int.h |  2 ++
 qapi/block-core.json      | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+)

diff --git a/block.c b/block.c
index 50ba264143..7d720ae0c2 100644
--- a/block.c
+++ b/block.c
@@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
 }
 
 /**
+ * Collect format allocation info. See BlockFormatAllocInfo definition in
+ * qapi/block-core.json.
+ */
+int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo *bfai)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (drv->bdrv_get_format_alloc_stat) {
+        return drv->bdrv_get_format_alloc_stat(bs, bfai);
+    }
+    return -ENOTSUP;
+}
+
+/**
  * Return number of sectors on success, -errno on error.
  */
 int64_t bdrv_nb_sectors(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e92d8..646376a772 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,6 +335,9 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
+int bdrv_get_format_alloc_stat(BlockDriverState *bs,
+                               BlockFormatAllocInfo *bfai);
+
 /* The units of offset and total_work_size may be chosen arbitrarily by the
  * block driver; total_work_size may change during the course of the amendment
  * operation */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..458c715e99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -208,6 +208,8 @@ struct BlockDriver {
     int64_t (*bdrv_getlength)(BlockDriverState *bs);
     bool has_variable_length;
     int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
+    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
+                                      BlockFormatAllocInfo *bfai);
 
     int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
         uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ea0b3e8b13..fd7b52bd69 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -139,6 +139,61 @@
            '*format-specific': 'ImageInfoSpecific' } }
 
 ##
+# @BlockFormatAllocInfo:
+#
+#
+# Allocation relations between format file and underlying protocol file.
+# All fields are in bytes.
+#
+# There are two types of the format file portions: 'used' and 'unused'. It's up
+# to the format how to interpret these types. For now the only format supporting
+# the feature is Qcow2 and for this case 'used' are clusters with positive
+# refcount and unused a clusters with zero refcount. Described portions include
+# all format file allocations, not only virtual disk data (metadata, internal
+# snapshots, etc. are included).
+#
+# For the underlying file there are native block-status types of the portions:
+#  - data: allocated data
+#  - zero: read-as-zero holes
+#  - discarded: not allocated
+# 4th additional type is 'overrun', which is for the format file portions beyond
+# the end of the underlying file.
+#
+# So, the fields are:
+#
+# @used-data: used by the format file and backed by data in the underlying file
+#
+# @used-zero: used by the format file and backed by a hole in the underlying
+#             file
+#
+# @used-discarded: used by the format file but actually unallocated in the
+#                  underlying file
+#
+# @used-overrun: used by the format file beyond the end of the underlying file
+#
+# @unused-data: allocated data in the underlying file not used by the format
+#
+# @unused-zero: holes in the underlying file not used by the format file
+#
+# @unused-discarded: unallocated areas in the underlying file not used by the
+#                    format file
+#
+# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
+#       length of the underlying file.
+#
+# Since: 2.10
+#
+##
+{ 'struct': 'BlockFormatAllocInfo',
+  'data': {'used-data':        'uint64',
+           'used-zero':        'uint64',
+           'used-discarded':   'uint64',
+           'used-overrun':     'uint64',
+           'unused-data':      'uint64',
+           'unused-zero':      'uint64',
+           'unused-discarded': 'uint64' } }
+
+##
 # @ImageCheck:
 #
 # Information about a QEMU image file check
-- 
2.11.1

[Qemu-devel] [PATCH v3 2/3] qcow2: add .bdrv_get_format_alloc_stat

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_get_format_alloc_stat interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-refcount.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c          |   2 +
 block/qcow2.h          |   2 +
 3 files changed, 156 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..fd6027f0c2 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2931,3 +2931,155 @@ done:
     qemu_vfree(new_refblock);
     return ret;
 }
+
+typedef struct FormatAllocStatCo {
+    BlockDriverState *bs;
+    BlockFormatAllocInfo *bfai;
+    int64_t ret;
+} FormatAllocStatCo;
+
+static void coroutine_fn qcow2_get_format_alloc_stat_entry(void *opaque)
+{
+    int ret = 0;
+    FormatAllocStatCo *nbco = opaque;
+    BlockDriverState *bs = nbco->bs;
+    BDRVQcow2State *s = bs->opaque;
+    BlockFormatAllocInfo *bfai = nbco->bfai;
+    int64_t cluster, file_sectors, sector;
+    int refcount_block_offset;
+    uint32_t i;
+    bool used = false, f_data = false, f_zero = false;
+    int dif, num = 0, f_num = 0;
+
+    memset(bfai, 0, sizeof(*bfai));
+
+    file_sectors = bdrv_nb_sectors(bs->file->bs);
+    if (file_sectors < 0) {
+        nbco->ret = file_sectors;
+        return;
+    }
+
+    qemu_co_mutex_lock(&s->lock);
+
+    for (sector = 0; sector < file_sectors; sector += dif) {
+        if (f_num == 0) {
+            BlockDriverState *file;
+            ret = bdrv_get_block_status(bs->file->bs, sector,
+                                        file_sectors - sector, &f_num, &file);
+            if (ret < 0) {
+                goto fail;
+            }
+            f_data = ret & BDRV_BLOCK_DATA;
+            f_zero = ret & BDRV_BLOCK_ZERO;
+        }
+
+        if (num == 0) {
+            uint64_t refcount;
+            assert(((sector << BDRV_SECTOR_BITS) & (s->cluster_size - 1)) == 0);
+            ret = qcow2_get_refcount(
+                bs, (sector << BDRV_SECTOR_BITS) >> s->cluster_bits, &refcount);
+            if (ret < 0) {
+                goto fail;
+            }
+            used = refcount > 0;
+            num = s->cluster_size >> BDRV_SECTOR_BITS;
+        }
+
+        dif = MIN(f_num, MIN(num, file_sectors - sector));
+        if (used) {
+            if (f_data) {
+                bfai->used_data += dif;
+            } else if (f_zero) {
+                bfai->used_zero += dif;
+            } else {
+                bfai->used_discarded += dif;
+            }
+        } else {
+            if (f_data) {
+                bfai->unused_data += dif;
+            } else if (f_zero) {
+                bfai->unused_zero += dif;
+            } else {
+                bfai->unused_discarded += dif;
+            }
+        }
+        f_num -= dif;
+        num -= dif;
+    }
+
+    assert(f_num == 0);
+
+    if (used) {
+        bfai->used_overrun += num;
+    }
+
+    cluster = size_to_clusters(s, sector << BDRV_SECTOR_BITS);
+    refcount_block_offset = cluster & (s->refcount_block_size - 1);
+    for (i = cluster >> s->refcount_block_bits;
+         i <= s->max_refcount_table_index; i++)
+    {
+        int j;
+
+        if (!(s->refcount_table[i] & REFT_OFFSET_MASK)) {
+            refcount_block_offset = 0;
+            continue;
+        }
+
+        for (j = refcount_block_offset; j < s->refcount_block_size; j++) {
+            uint64_t refcount;
+            cluster = (i << s->refcount_block_bits) + j;
+
+            ret = qcow2_get_refcount(bs, cluster, &refcount);
+            if (ret < 0) {
+                goto fail;
+            }
+            if (refcount > 0) {
+                bfai->used_overrun++;
+            }
+        }
+
+        refcount_block_offset = 0;
+    }
+
+    qemu_co_mutex_unlock(&s->lock);
+
+    bfai->used_data = bfai->used_data << BDRV_SECTOR_BITS;
+    bfai->used_zero = bfai->used_zero << BDRV_SECTOR_BITS;
+    bfai->used_discarded = bfai->used_discarded << BDRV_SECTOR_BITS;
+    bfai->used_overrun = bfai->used_overrun << BDRV_SECTOR_BITS;
+
+    bfai->unused_data = bfai->unused_data << BDRV_SECTOR_BITS;
+    bfai->unused_zero = bfai->unused_zero << BDRV_SECTOR_BITS;
+    bfai->unused_discarded = bfai->unused_discarded << BDRV_SECTOR_BITS;
+
+    nbco->ret = 0;
+    return;
+
+fail:
+    nbco->ret = ret;
+    qemu_co_mutex_unlock(&s->lock);
+}
+
+/* qcow2_get_format_alloc_stat()
+ * Fills @bfai struct. In case of failure @bfai content is unpredicted.
+ */
+int qcow2_get_format_alloc_stat(BlockDriverState *bs,
+                                BlockFormatAllocInfo *bfai)
+{
+    FormatAllocStatCo nbco = {
+        .bs = bs,
+        .bfai = bfai,
+        .ret = -EINPROGRESS
+    };
+
+    if (qemu_in_coroutine()) {
+        qcow2_get_format_alloc_stat_entry(&nbco);
+    } else {
+        Coroutine *co =
+            qemu_coroutine_create(qcow2_get_format_alloc_stat_entry, &nbco);
+        qemu_coroutine_enter(co);
+        BDRV_POLL_WHILE(bs, nbco.ret == -EINPROGRESS);
+    }
+
+    return nbco.ret;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index a8d61f0981..0e26b548fd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3469,6 +3469,8 @@ BlockDriver bdrv_qcow2 = {
 
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+    .bdrv_get_format_alloc_stat = qcow2_get_format_alloc_stat,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 1801dc30dc..16d31a8624 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -598,4 +598,6 @@ int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
     void **table);
 void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
 
+int qcow2_get_format_alloc_stat(BlockDriverState *bs,
+                                BlockFormatAllocInfo *bfai);
 #endif
-- 
2.11.1

[Qemu-devel] [PATCH v3 3/3] qemu-img check: add format allocation info

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json |  6 +++++-
 qemu-img.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index fd7b52bd69..b9b9952541 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -232,6 +232,9 @@
 #                       field is present if the driver for the image format
 #                       supports it
 #
+# @format-alloc-info: Format-allocation information, see
+#                     BlockFormatAllocInfo description. (Since: 2.10)
+#
 # Since: 1.4
 #
 ##
@@ -240,7 +243,8 @@
            '*image-end-offset': 'int', '*corruptions': 'int', '*leaks': 'int',
            '*corruptions-fixed': 'int', '*leaks-fixed': 'int',
            '*total-clusters': 'int', '*allocated-clusters': 'int',
-           '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
+           '*fragmented-clusters': 'int', '*compressed-clusters': 'int',
+           '*format-alloc-info': 'BlockFormatAllocInfo' } }
 
 ##
 # @MapEntry:
diff --git a/qemu-img.c b/qemu-img.c
index b506839ef0..b3adf9a1a2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,6 +560,35 @@ static void dump_json_image_check(ImageCheck *check, bool quiet)
     QDECREF(str);
 }
 
+static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
+{
+    char *used_data = size_to_str(bfai->used_data);
+    char *used_zero = size_to_str(bfai->used_zero);
+    char *used_discarded = size_to_str(bfai->used_discarded);
+    char *used_overrun = size_to_str(bfai->used_overrun);
+
+    char *unused_data = size_to_str(bfai->unused_data);
+    char *unused_zero = size_to_str(bfai->unused_zero);
+    char *unused_discarded = size_to_str(bfai->unused_discarded);
+
+    qprintf(quiet,
+            "Format allocation info (including metadata):\n"
+            "               data        zero   discarded   after-eof\n"
+            "used     %10s  %10s  %10s  %10s\n"
+            "unused   %10s  %10s  %10s\n",
+            used_data, used_zero, used_discarded, used_overrun,
+            unused_data, unused_zero, unused_discarded);
+
+    g_free(used_data);
+    g_free(used_zero);
+    g_free(used_discarded);
+    g_free(used_overrun);
+
+    g_free(unused_data);
+    g_free(unused_zero);
+    g_free(unused_discarded);
+}
+
 static void dump_human_image_check(ImageCheck *check, bool quiet)
 {
     if (!(check->corruptions || check->leaks || check->check_errors)) {
@@ -601,6 +630,10 @@ static void dump_human_image_check(ImageCheck *check, bool quiet)
         qprintf(quiet,
                 "Image end offset: %" PRId64 "\n", check->image_end_offset);
     }
+
+    if (check->has_format_alloc_info) {
+        dump_human_format_alloc_info(check->format_alloc_info, quiet);
+    }
 }
 
 static int collect_image_check(BlockDriverState *bs,
@@ -611,6 +644,7 @@ static int collect_image_check(BlockDriverState *bs,
 {
     int ret;
     BdrvCheckResult result;
+    BlockFormatAllocInfo *bfai = g_new0(BlockFormatAllocInfo, 1);
 
     ret = bdrv_check(bs, &result, fix);
     if (ret < 0) {
@@ -639,6 +673,14 @@ static int collect_image_check(BlockDriverState *bs,
     check->compressed_clusters      = result.bfi.compressed_clusters;
     check->has_compressed_clusters  = result.bfi.compressed_clusters != 0;
 
+    ret = bdrv_get_format_alloc_stat(bs, bfai);
+    if (ret < 0) {
+        qapi_free_BlockFormatAllocInfo(bfai);
+    } else {
+        check->has_format_alloc_info = true;
+        check->format_alloc_info = bfai;
+    }
+
     return 0;
 }
 
-- 
2.11.1

[Qemu-devel] ping Re: [PATCH v3 0/3] qemu-img check: format allocation info

[Vladimir Sementsov-Ogievskiy]

ping

06.06.2017 19:26, Vladimir Sementsov-Ogievskiy wrote:
> Hi all.
>
> See 01 patch for the doc.
>
> v3: - improve docs
>      - rename fields
>      - add 'zero' type of underlying file portions status. It as these
>        areas cannot be presented as 'discarded', but they are not
>        occupying real space, so we don't want to present them as
>        'allocated data' too.
>      - remove last patch. It is not needed after introducing new naming
>        for the fields
>
> v2: fix build error, gcc things that some variables may be used
>      uninitialized (actually they didn't).
>
> v1:
>
> These series is a replacement for "qemu-img check: unallocated size"
> series.
>
> There was a question, should we account allocated clusters in qcow2 but
> actually holes in underalying file as allocated or not. Instead of
> hiding this information in one-number statistic I've decided to print
> the whole information, 5 numbers:
>
> For allocated by top-level format driver (qcow2 for ex.) clusters, 3
> numbers: number of bytes, which are:
>   - allocated in underlying file
>   - holes in underlying file
>   - after end of underlying file
>
> To account other areas of underlying file, 2 more numbers of bytes:
>   - unallocated by top-level driver but allocated in underlying file
>   - unallocated by top-level driver and holes in underlying file
>
> Vladimir Sementsov-Ogievskiy (3):
>    block: add bdrv_get_format_alloc_stat format interface
>    qcow2: add .bdrv_get_format_alloc_stat
>    qemu-img check: add format allocation info
>
>   block.c                   |  16 +++++
>   block/qcow2-refcount.c    | 152 ++++++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c             |   2 +
>   block/qcow2.h             |   2 +
>   include/block/block.h     |   3 +
>   include/block/block_int.h |   2 +
>   qapi/block-core.json      |  61 ++++++++++++++++++-
>   qemu-img.c                |  42 +++++++++++++
>   8 files changed, 279 insertions(+), 1 deletion(-)
>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[John Snow]

On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> The function should collect statistics, about used/unused by top-level
> format driver space (in its .file) and allocation status
> (data/zero/discarded/after-eof) of corresponding areas in this .file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                   | 16 ++++++++++++++
>  include/block/block.h     |  3 +++
>  include/block/block_int.h |  2 ++
>  qapi/block-core.json      | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 50ba264143..7d720ae0c2 100644
> --- a/block.c
> +++ b/block.c
> @@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
>  }
>  
>  /**
> + * Collect format allocation info. See BlockFormatAllocInfo definition in
> + * qapi/block-core.json.
> + */
> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo *bfai)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (!drv) {
> +        return -ENOMEDIUM;
> +    }
> +    if (drv->bdrv_get_format_alloc_stat) {
> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
> +    }
> +    return -ENOTSUP;
> +}
> +
> +/**
>   * Return number of sectors on success, -errno on error.
>   */
>  int64_t bdrv_nb_sectors(BlockDriverState *bs)
> diff --git a/include/block/block.h b/include/block/block.h
> index 9b355e92d8..646376a772 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -335,6 +335,9 @@ typedef enum {
>  
>  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  
> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
> +                               BlockFormatAllocInfo *bfai);
> +
>  /* The units of offset and total_work_size may be chosen arbitrarily by the
>   * block driver; total_work_size may change during the course of the amendment
>   * operation */
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8d3724cce6..458c715e99 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -208,6 +208,8 @@ struct BlockDriver {
>      int64_t (*bdrv_getlength)(BlockDriverState *bs);
>      bool has_variable_length;
>      int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
> +                                      BlockFormatAllocInfo *bfai);
>  
>      int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>          uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ea0b3e8b13..fd7b52bd69 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -139,6 +139,61 @@
>             '*format-specific': 'ImageInfoSpecific' } }
>  
>  ##
> +# @BlockFormatAllocInfo:
> +#

I apologize in advance, but I don't understand this patch very well. Let
me ask some questions to get patch review rolling again, since you've
been waiting a bit.

> +#
> +# Allocation relations between format file and underlying protocol file.
> +# All fields are in bytes.
> +#

The format file in this case would be ... what, the virtual file
represented by the qcow2? and the underlying protocol file is the raw
file that is the qcow2 itself?

> +# There are two types of the format file portions: 'used' and 'unused'. It's up
> +# to the format how to interpret these types. For now the only format supporting
> +# the feature is Qcow2 and for this case 'used' are clusters with positive
> +# refcount and unused a clusters with zero refcount. Described portions include
> +# all format file allocations, not only virtual disk data (metadata, internal
> +# snapshots, etc. are included).

I guess the semantic differentiation between "used" and "unused" is left
to the individual fields, below.

> +#
> +# For the underlying file there are native block-status types of the portions:
> +#  - data: allocated data
> +#  - zero: read-as-zero holes
> +#  - discarded: not allocated
> +# 4th additional type is 'overrun', which is for the format file portions beyond
> +# the end of the underlying file.
> +#
> +# So, the fields are:
> +#
> +# @used-data: used by the format file and backed by data in the underlying file
> +#

I assume this is "defined and addressable data".

> +# @used-zero: used by the format file and backed by a hole in the underlying
> +#             file
> +#

By a hole? Can you give me an example? Do you mean like a filesystem
hole ala falloc()?

> +# @used-discarded: used by the format file but actually unallocated in the
> +#                  underlying file
> +#

In what case do we have used data that is discarded/undefined, but not
zero? Shouldn't discarded data be zero...?

> +# @used-overrun: used by the format file beyond the end of the underlying file
> +#

When does this occur?

> +# @unused-data: allocated data in the underlying file not used by the format
> +#

I assume this is an allocation gap in qcow2. Unused, but non-zero. Right?

> +# @unused-zero: holes in the underlying file not used by the format file
> +#

I assume this is similar to the above -- Unused, but zero.

> +# @unused-discarded: unallocated areas in the underlying file not used by the
> +#                    format file
> +#

Again I am unsure of what discarded but non-zero might mean.

> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
> +#       length of the underlying file.
> +#
> +# Since: 2.10
> +#
> +##
> +{ 'struct': 'BlockFormatAllocInfo',
> +  'data': {'used-data':        'uint64',
> +           'used-zero':        'uint64',
> +           'used-discarded':   'uint64',
> +           'used-overrun':     'uint64',
> +           'unused-data':      'uint64',
> +           'unused-zero':      'uint64',
> +           'unused-discarded': 'uint64' } }
> +
> +##
>  # @ImageCheck:
>  #
>  # Information about a QEMU image file check
> 

Sorry for the dumb questions.

--John

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

27.06.2017 02:19, John Snow wrote:
>
> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>> The function should collect statistics, about used/unused by top-level
>> format driver space (in its .file) and allocation status
>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c                   | 16 ++++++++++++++
>>   include/block/block.h     |  3 +++
>>   include/block/block_int.h |  2 ++
>>   qapi/block-core.json      | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 76 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 50ba264143..7d720ae0c2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
>>   }
>>   
>>   /**
>> + * Collect format allocation info. See BlockFormatAllocInfo definition in
>> + * qapi/block-core.json.
>> + */
>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo *bfai)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    if (!drv) {
>> +        return -ENOMEDIUM;
>> +    }
>> +    if (drv->bdrv_get_format_alloc_stat) {
>> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
>> +    }
>> +    return -ENOTSUP;
>> +}
>> +
>> +/**
>>    * Return number of sectors on success, -errno on error.
>>    */
>>   int64_t bdrv_nb_sectors(BlockDriverState *bs)
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 9b355e92d8..646376a772 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -335,6 +335,9 @@ typedef enum {
>>   
>>   int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>>   
>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>> +                               BlockFormatAllocInfo *bfai);
>> +
>>   /* The units of offset and total_work_size may be chosen arbitrarily by the
>>    * block driver; total_work_size may change during the course of the amendment
>>    * operation */
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 8d3724cce6..458c715e99 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -208,6 +208,8 @@ struct BlockDriver {
>>       int64_t (*bdrv_getlength)(BlockDriverState *bs);
>>       bool has_variable_length;
>>       int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
>> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
>> +                                      BlockFormatAllocInfo *bfai);
>>   
>>       int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>>           uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ea0b3e8b13..fd7b52bd69 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -139,6 +139,61 @@
>>              '*format-specific': 'ImageInfoSpecific' } }
>>   
>>   ##
>> +# @BlockFormatAllocInfo:
>> +#
> I apologize in advance, but I don't understand this patch very well. Let
> me ask some questions to get patch review rolling again, since you've
> been waiting a bit.
>
>> +#
>> +# Allocation relations between format file and underlying protocol file.
>> +# All fields are in bytes.
>> +#
> The format file in this case would be ... what, the virtual file
> represented by the qcow2? and the underlying protocol file is the raw
> file that is the qcow2 itself?

yes

>
>> +# There are two types of the format file portions: 'used' and 'unused'. It's up
>> +# to the format how to interpret these types. For now the only format supporting
>> +# the feature is Qcow2 and for this case 'used' are clusters with positive
>> +# refcount and unused a clusters with zero refcount. Described portions include
>> +# all format file allocations, not only virtual disk data (metadata, internal
>> +# snapshots, etc. are included).
> I guess the semantic differentiation between "used" and "unused" is left
> to the individual fields, below.

hmm, I don't understand. differentiation is up to the format, and for 
qcow2 it is described above

>
>> +#
>> +# For the underlying file there are native block-status types of the portions:
>> +#  - data: allocated data
>> +#  - zero: read-as-zero holes
>> +#  - discarded: not allocated
>> +# 4th additional type is 'overrun', which is for the format file portions beyond
>> +# the end of the underlying file.
>> +#
>> +# So, the fields are:
>> +#
>> +# @used-data: used by the format file and backed by data in the underlying file
>> +#
> I assume this is "defined and addressable data".
>
>> +# @used-zero: used by the format file and backed by a hole in the underlying
>> +#             file
>> +#
> By a hole? Can you give me an example? Do you mean like a filesystem
> hole ala falloc()?

-zero, -data and -discarded are the block status of corresponding area 
in underlying file.

so, if underlying file is raw, yes, it should be a filesystem hole.

example:
-------------------------
# ./qemu-img create -f qcow2 x 1G
Formatting 'x', fmt=qcow2 size=1073741824 encryption=off 
cluster_size=65536 lazy_refcounts=off refcount_bits=16
# ./qemu-img check x
No errors were found on the image.
Image end offset: 262144
Format allocation info (including metadata):
                data        zero   discarded   after-eof
used        192 KiB         0 B         0 B    63.5 KiB
unused          0 B         0 B         0 B
# ./qemu-io -c 'write 0 100M' x
wrote 104857600/104857600 bytes at offset 0
100 MiB, 1 ops; 0.7448 sec (134.263 MiB/sec and 1.3426 ops/sec)
# ./qemu-img check x
No errors were found on the image.
1600/16384 = 9.77% allocated, 0.00% fragmented, 0.00% compressed clusters
Image end offset: 105185280
Format allocation info (including metadata):
                data        zero   discarded   after-eof
used        100 MiB      60 KiB         0 B         0 B
unused          0 B         0 B         0 B
# ./qemu-io -c 'discard 0 1M' x
discard 1048576/1048576 bytes at offset 0
1 MiB, 1 ops; 0.0002 sec (3.970 GiB/sec and 4065.0407 ops/sec)
# ./qemu-img check x
No errors were found on the image.
1584/16384 = 9.67% allocated, 0.00% fragmented, 0.00% compressed clusters
Image end offset: 105185280
Format allocation info (including metadata):
                data        zero   discarded   after-eof
used       99.3 MiB      60 KiB         0 B         0 B
unused          0 B       1 MiB         0 B
-------------------------

- hmm, 60 KiB, don't know what is it. some preallocation may be..


>
>> +# @used-discarded: used by the format file but actually unallocated in the
>> +#                  underlying file
>> +#
> In what case do we have used data that is discarded/undefined, but not
> zero? Shouldn't discarded data be zero...?

may be discarded is bad name.. this if for unallocated block status of 
underlying file.

>
>> +# @used-overrun: used by the format file beyond the end of the underlying file
>> +#
> When does this occur?

I think it shoud be some kind of corruption.

>
>> +# @unused-data: allocated data in the underlying file not used by the format
>> +#
> I assume this is an allocation gap in qcow2. Unused, but non-zero. Right?

or it may be some kind of error or due to underlying fs doesn't maintain 
holes.

>
>> +# @unused-zero: holes in the underlying file not used by the format file
>> +#
> I assume this is similar to the above -- Unused, but zero.

Unused and underlying block status is ZERO. It is a "good" case for 
unused areas.

>
>> +# @unused-discarded: unallocated areas in the underlying file not used by the
>> +#                    format file
>> +#
> Again I am unsure of what discarded but non-zero might mean.

looks like for raw format discarded is impossible, but to make a generic 
tool, let's consider block status = unallocated too.

>
>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
>> +#       length of the underlying file.
>> +#
>> +# Since: 2.10
>> +#
>> +##
>> +{ 'struct': 'BlockFormatAllocInfo',
>> +  'data': {'used-data':        'uint64',
>> +           'used-zero':        'uint64',
>> +           'used-discarded':   'uint64',
>> +           'used-overrun':     'uint64',
>> +           'unused-data':      'uint64',
>> +           'unused-zero':      'uint64',
>> +           'unused-discarded': 'uint64' } }
>> +
>> +##
>>   # @ImageCheck:
>>   #
>>   # Information about a QEMU image file check
>>
> Sorry for the dumb questions.

Don't worry)

>
> --John


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[John Snow]

On 06/28/2017 11:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.06.2017 02:19, John Snow wrote:
>>
>> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about used/unused by top-level
>>> format driver space (in its .file) and allocation status
>>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block.c                   | 16 ++++++++++++++
>>>   include/block/block.h     |  3 +++
>>>   include/block/block_int.h |  2 ++
>>>   qapi/block-core.json      | 55
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 76 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 50ba264143..7d720ae0c2 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3407,6 +3407,22 @@ int64_t
>>> bdrv_get_allocated_file_size(BlockDriverState *bs)
>>>   }
>>>     /**
>>> + * Collect format allocation info. See BlockFormatAllocInfo
>>> definition in
>>> + * qapi/block-core.json.
>>> + */
>>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>>> BlockFormatAllocInfo *bfai)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    if (!drv) {
>>> +        return -ENOMEDIUM;
>>> +    }
>>> +    if (drv->bdrv_get_format_alloc_stat) {
>>> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
>>> +    }
>>> +    return -ENOTSUP;
>>> +}
>>> +
>>> +/**
>>>    * Return number of sectors on success, -errno on error.
>>>    */
>>>   int64_t bdrv_nb_sectors(BlockDriverState *bs)
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 9b355e92d8..646376a772 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -335,6 +335,9 @@ typedef enum {
>>>     int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res,
>>> BdrvCheckMode fix);
>>>   +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>>> +                               BlockFormatAllocInfo *bfai);
>>> +
>>>   /* The units of offset and total_work_size may be chosen
>>> arbitrarily by the
>>>    * block driver; total_work_size may change during the course of
>>> the amendment
>>>    * operation */
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 8d3724cce6..458c715e99 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -208,6 +208,8 @@ struct BlockDriver {
>>>       int64_t (*bdrv_getlength)(BlockDriverState *bs);
>>>       bool has_variable_length;
>>>       int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
>>> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
>>> +                                      BlockFormatAllocInfo *bfai);
>>>         int coroutine_fn
>>> (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>>>           uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index ea0b3e8b13..fd7b52bd69 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -139,6 +139,61 @@
>>>              '*format-specific': 'ImageInfoSpecific' } }
>>>     ##
>>> +# @BlockFormatAllocInfo:
>>> +#
>> I apologize in advance, but I don't understand this patch very well. Let
>> me ask some questions to get patch review rolling again, since you've
>> been waiting a bit.
>>
>>> +#
>>> +# Allocation relations between format file and underlying protocol
>>> file.
>>> +# All fields are in bytes.
>>> +#
>> The format file in this case would be ... what, the virtual file
>> represented by the qcow2? and the underlying protocol file is the raw
>> file that is the qcow2 itself?
> 
> yes
> 
>>
>>> +# There are two types of the format file portions: 'used' and
>>> 'unused'. It's up
>>> +# to the format how to interpret these types. For now the only
>>> format supporting
>>> +# the feature is Qcow2 and for this case 'used' are clusters with
>>> positive
>>> +# refcount and unused a clusters with zero refcount. Described
>>> portions include
>>> +# all format file allocations, not only virtual disk data (metadata,
>>> internal
>>> +# snapshots, etc. are included).
>> I guess the semantic differentiation between "used" and "unused" is left
>> to the individual fields, below.
> 
> hmm, I don't understand. differentiation is up to the format, and for
> qcow2 it is described above
> 
>>
>>> +#
>>> +# For the underlying file there are native block-status types of the
>>> portions:
>>> +#  - data: allocated data
>>> +#  - zero: read-as-zero holes
>>> +#  - discarded: not allocated
>>> +# 4th additional type is 'overrun', which is for the format file
>>> portions beyond
>>> +# the end of the underlying file.
>>> +#
>>> +# So, the fields are:
>>> +#
>>> +# @used-data: used by the format file and backed by data in the
>>> underlying file
>>> +#
>> I assume this is "defined and addressable data".
>>
>>> +# @used-zero: used by the format file and backed by a hole in the
>>> underlying
>>> +#             file
>>> +#
>> By a hole? Can you give me an example? Do you mean like a filesystem
>> hole ala falloc()?
> 
> -zero, -data and -discarded are the block status of corresponding area
> in underlying file.
> 
> so, if underlying file is raw, yes, it should be a filesystem hole.
> 
> example:
> -------------------------
> # ./qemu-img create -f qcow2 x 1G
> Formatting 'x', fmt=qcow2 size=1073741824 encryption=off
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> # ./qemu-img check x
> No errors were found on the image.
> Image end offset: 262144
> Format allocation info (including metadata):
>                data        zero   discarded   after-eof
> used        192 KiB         0 B         0 B    63.5 KiB
> unused          0 B         0 B         0 B

OK, we create a 196624 byte file -- 3 clusters and a little bit of extra.

0: header
1: reftable
2: refcount block #0, accounting for clusters 0x0 - 0x7fff
3: l1_table, only partially allocated, and all zeroes

So we've got 16 bytes defined for this l1 table, leaving most of a
cluster defined but after EOF. I suppose your after-EOF counter there is
probably rounding a bit to the nearest 512.

So we've got three used clusters, and 99% of one cluster that's after
EOF. Shouldn't data here be 192.5 in this case?

Or Data: 192KiB; Zero 512 b?

I guess the ".5" is just truncated or rounded.

> # ./qemu-io -c 'write 0 100M' x
> wrote 104857600/104857600 bytes at offset 0
> 100 MiB, 1 ops; 0.7448 sec (134.263 MiB/sec and 1.3426 ops/sec)
> # ./qemu-img check x
> No errors were found on the image.
> 1600/16384 = 9.77% allocated, 0.00% fragmented, 0.00% compressed clusters
> Image end offset: 105185280
> Format allocation info (including metadata):
>                data        zero   discarded   after-eof
> used        100 MiB      60 KiB         0 B         0 B
> unused          0 B         0 B         0 B

Hmm, okay;

now the image is 105185280 bytes; 102720 KiB; 1,605 clusters.
100MiB + 320KiB. Again, it doesn't entirely look like your summaries
line up. Did we lose 256KiB to a rounding error under "100MiB" ?

>From what I can now tell, the map looks like:

== File Map ==

0x000000000 - 0x00004ffff [Metadata] (5 clusters)
0x000050000 - 0x00644ffff [Data] (1600 clusters)

1600 clusters at 64KiB each gives us 102400KiB / 100MiB of data.
Then we've got five clusters of metadata (320KiB).

Cluster 0: Header data. Data only occupies the first 512 bytes or so.
Data: 512b
Zeroes: 63.5KiB

Cluster 1: Reftable. Data only occupies the first 8 bytes.
Data: 512b
Zeroes: 63.5KiB

Cluster 2: Refcount Block #0. There are 0xC8A bytes, 3210/2 1605
refcounts. Makes sense. That's 7 sectors of data.
Data: 3.5KiB
Zeroes: 60.5KiB

Cluster 3: L1 table. One entry for L2 table. Takes 8 bytes.
Data: 512B
Zeroes: 63.5KiB

Cluster 4: L2 table. 1,600 entries. Takes 5120 bytes, about 10 sectors.
Data: 5KiB
Zeroes: 59KiB

Then clusters 5-1604 contain our data contiguously, the ascii byte 0xcd.

> # ./qemu-io -c 'discard 0 1M' x
> discard 1048576/1048576 bytes at offset 0
> 1 MiB, 1 ops; 0.0002 sec (3.970 GiB/sec and 4065.0407 ops/sec)
> # ./qemu-img check x
> No errors were found on the image.
> 1584/16384 = 9.67% allocated, 0.00% fragmented, 0.00% compressed clusters
> Image end offset: 105185280
> Format allocation info (including metadata):
>                data        zero   discarded   after-eof
> used       99.3 MiB      60 KiB         0 B         0 B
> unused          0 B       1 MiB         0 B
> -------------------------
> 
> - hmm, 60 KiB, don't know what is it. some preallocation may be..
> 

x doesn't lose any filesize, but we have 1584 allocated clusters. We
lost 16, corresponding to the discarded 1M.

Map is now:

0x000000000 - 0x00004ffff [Metadata] (5 clusters)
0x000050000 - 0x00014ffff [Vacant] (16 clusters)
0x000150000 - 0x00644ffff [Data] (1584 clusters)

OK.

0: Header. no change.
Data: 512b
Zeroes: 63.5KiB

1: Reftable. No change.
Data: 512b
Zeroes: 63.5KiB

Cluster 2: Almost the same.... ref[5] (i.e. the sixth) through ref[20]
have been decremented, but everything else remains at refcount of 01.
Still takes up the same amount of space at the sector granularity level.
Data: 3.5KiB
Zeroes: 60.5KiB

Cluster 3: L1 table. No change.
Data: 512B
Zeroes: 63.5KiB

Cluster 4: L2 table
Here, the first 16 data clusters have been modified to zero cluster
pointers: 0x0000000000000001, everything else remains defined as it was.
Data: 5KiB
Zeroes: 59KiB

Clusters 5-20 inclusive: non-zero data now discarded and considered
unused. 1MiB. makes sense.

Clusters 21-end: non-zero, used data. 1584 clusters; 101376KiB; 99MiB

My Tallies:

Metadata: 10KiB
Metadata Zeroes: 310KiB
Undefined Data: 1MiB
Data: 99MiB

Your Tallies:
'used-data': 99.3MiB (101683.2KiB)
'used-zeroes': 60KiB
'unused-data': 1MiB

Subtracting out the 99MiB of data surely accounted for correctly here;
you are counting about 0.3MiB + 60KiB of used data for presumably the
metadata regions; ~367.2KiB.

Looks like your counts are something like:
metadata: 260KiB (0.25MiB ... ~0.3 with rounding, OK)
metadata-zeroes: 60KiB

So it's probably just counting what is and isn't zeroes a little less
aggressively than I am doing. To what extent or how, I don't know. Maybe
it depends on the underlying filesystem:

jhuston@probe ~> qemu-img map -f raw X
Offset          Length          Mapped to       File
0               0x31000         0               X
0x40000         0x10000         0x40000         X
0x150000        0x6300000       0x150000        X

Looks like a hole from 0x31000 to 0x40000, 60KiB in the metadata region,
so that's probably it.

Then there's a hole from 0x50000 to 0x150000, 1MiB, so that's unused data.

Hey, interesting, the discarded data that would be read as zeroes is
still defined by the QCOW2 schema so that's "unused-zeroes" whereas the
zero space in the metadata is only counted as such because of the sparse
gap, so that's used-zero. OK, I think I'm starting to get what these
numbers mean.

> 
>>
>>> +# @used-discarded: used by the format file but actually unallocated
>>> in the
>>> +#                  underlying file
>>> +#
>> In what case do we have used data that is discarded/undefined, but not
>> zero? Shouldn't discarded data be zero...?
> 
> may be discarded is bad name.. this if for unallocated block status of
> underlying file.
> 

Unallocated in what sense, exactly? Do you have an example for qcow2?
I'm sorry that i still don't quite follow :\

>>
>>> +# @used-overrun: used by the format file beyond the end of the
>>> underlying file
>>> +#
>> When does this occur?
> 
> I think it shoud be some kind of corruption.
> 

Alright, let me see if I have this straight...

used-data: Normal data. We are standing on terra-firma.
used-zero: Data that is defined to be zeroes in some way.

(Does not necessarily include data clusters if they were not actually
zeroed out, I think. May not include regions that ARE zero, even if they
are literally zero, because the driver may not especially recognize them
as such. Anything marked as zero will DEFINITELY be zero, though. Yes?)

used-discarded: I'm not actually sure in this case.

used-overrun: Data that is defined to exist, but appears to fall outside
of or beyond EOF. Appears to happen with qcow2 metadata before any
writes occur.

unused-data: Normal data, but not in-use by the schema anywhere. Leaked
clusters and the like, effectively.

unused-zero: Similar to the above, but definitely zeroes.

unused-discarded: Not really sure.

>>
>>> +# @unused-data: allocated data in the underlying file not used by
>>> the format
>>> +#
>> I assume this is an allocation gap in qcow2. Unused, but non-zero. Right?
> 
> or it may be some kind of error or due to underlying fs doesn't maintain
> holes.
> 
>>
>>> +# @unused-zero: holes in the underlying file not used by the format
>>> file
>>> +#
>> I assume this is similar to the above -- Unused, but zero.
> 
> Unused and underlying block status is ZERO. It is a "good" case for
> unused areas.
> 
>>
>>> +# @unused-discarded: unallocated areas in the underlying file not
>>> used by the
>>> +#                    format file
>>> +#
>> Again I am unsure of what discarded but non-zero might mean.
> 
> looks like for raw format discarded is impossible, but to make a generic
> tool, let's consider block status = unallocated too.
> 
>>
>>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal
>>> to the
>>> +#       length of the underlying file.
>>> +#
>>> +# Since: 2.10
>>> +#
>>> +##
>>> +{ 'struct': 'BlockFormatAllocInfo',
>>> +  'data': {'used-data':        'uint64',
>>> +           'used-zero':        'uint64',
>>> +           'used-discarded':   'uint64',
>>> +           'used-overrun':     'uint64',
>>> +           'unused-data':      'uint64',
>>> +           'unused-zero':      'uint64',
>>> +           'unused-discarded': 'uint64' } }
>>> +
>>> +##
>>>   # @ImageCheck:
>>>   #
>>>   # Information about a QEMU image file check
>>>
>> Sorry for the dumb questions.
> 
> Don't worry)
> 
>>
>> --John
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

29.06.2017 03:15, John Snow wrote:
>
> On 06/28/2017 11:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 27.06.2017 02:19, John Snow wrote:
>>> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> The function should collect statistics, about used/unused by top-level
>>>> format driver space (in its .file) and allocation status
>>>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block.c                   | 16 ++++++++++++++
>>>>    include/block/block.h     |  3 +++
>>>>    include/block/block_int.h |  2 ++
>>>>    qapi/block-core.json      | 55
>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 76 insertions(+)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 50ba264143..7d720ae0c2 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -3407,6 +3407,22 @@ int64_t
>>>> bdrv_get_allocated_file_size(BlockDriverState *bs)
>>>>    }
>>>>      /**
>>>> + * Collect format allocation info. See BlockFormatAllocInfo
>>>> definition in
>>>> + * qapi/block-core.json.
>>>> + */
>>>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>>>> BlockFormatAllocInfo *bfai)
>>>> +{
>>>> +    BlockDriver *drv = bs->drv;
>>>> +    if (!drv) {
>>>> +        return -ENOMEDIUM;
>>>> +    }
>>>> +    if (drv->bdrv_get_format_alloc_stat) {
>>>> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
>>>> +    }
>>>> +    return -ENOTSUP;
>>>> +}
>>>> +
>>>> +/**
>>>>     * Return number of sectors on success, -errno on error.
>>>>     */
>>>>    int64_t bdrv_nb_sectors(BlockDriverState *bs)
>>>> diff --git a/include/block/block.h b/include/block/block.h
>>>> index 9b355e92d8..646376a772 100644
>>>> --- a/include/block/block.h
>>>> +++ b/include/block/block.h
>>>> @@ -335,6 +335,9 @@ typedef enum {
>>>>      int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res,
>>>> BdrvCheckMode fix);
>>>>    +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>>>> +                               BlockFormatAllocInfo *bfai);
>>>> +
>>>>    /* The units of offset and total_work_size may be chosen
>>>> arbitrarily by the
>>>>     * block driver; total_work_size may change during the course of
>>>> the amendment
>>>>     * operation */
>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>> index 8d3724cce6..458c715e99 100644
>>>> --- a/include/block/block_int.h
>>>> +++ b/include/block/block_int.h
>>>> @@ -208,6 +208,8 @@ struct BlockDriver {
>>>>        int64_t (*bdrv_getlength)(BlockDriverState *bs);
>>>>        bool has_variable_length;
>>>>        int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
>>>> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
>>>> +                                      BlockFormatAllocInfo *bfai);
>>>>          int coroutine_fn
>>>> (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>>>>            uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index ea0b3e8b13..fd7b52bd69 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -139,6 +139,61 @@
>>>>               '*format-specific': 'ImageInfoSpecific' } }
>>>>      ##
>>>> +# @BlockFormatAllocInfo:
>>>> +#
>>> I apologize in advance, but I don't understand this patch very well. Let
>>> me ask some questions to get patch review rolling again, since you've
>>> been waiting a bit.
>>>
>>>> +#
>>>> +# Allocation relations between format file and underlying protocol
>>>> file.
>>>> +# All fields are in bytes.
>>>> +#
>>> The format file in this case would be ... what, the virtual file
>>> represented by the qcow2? and the underlying protocol file is the raw
>>> file that is the qcow2 itself?
>> yes
>>
>>>> +# There are two types of the format file portions: 'used' and
>>>> 'unused'. It's up
>>>> +# to the format how to interpret these types. For now the only
>>>> format supporting
>>>> +# the feature is Qcow2 and for this case 'used' are clusters with
>>>> positive
>>>> +# refcount and unused a clusters with zero refcount. Described
>>>> portions include
>>>> +# all format file allocations, not only virtual disk data (metadata,
>>>> internal
>>>> +# snapshots, etc. are included).
>>> I guess the semantic differentiation between "used" and "unused" is left
>>> to the individual fields, below.
>> hmm, I don't understand. differentiation is up to the format, and for
>> qcow2 it is described above
>>
>>>> +#
>>>> +# For the underlying file there are native block-status types of the
>>>> portions:
>>>> +#  - data: allocated data
>>>> +#  - zero: read-as-zero holes
>>>> +#  - discarded: not allocated
>>>> +# 4th additional type is 'overrun', which is for the format file
>>>> portions beyond
>>>> +# the end of the underlying file.
>>>> +#
>>>> +# So, the fields are:
>>>> +#
>>>> +# @used-data: used by the format file and backed by data in the
>>>> underlying file
>>>> +#
>>> I assume this is "defined and addressable data".
>>>
>>>> +# @used-zero: used by the format file and backed by a hole in the
>>>> underlying
>>>> +#             file
>>>> +#
>>> By a hole? Can you give me an example? Do you mean like a filesystem
>>> hole ala falloc()?
>> -zero, -data and -discarded are the block status of corresponding area
>> in underlying file.
>>
>> so, if underlying file is raw, yes, it should be a filesystem hole.
>>
>> example:
>> -------------------------
>> # ./qemu-img create -f qcow2 x 1G
>> Formatting 'x', fmt=qcow2 size=1073741824 encryption=off
>> cluster_size=65536 lazy_refcounts=off refcount_bits=16
>> # ./qemu-img check x
>> No errors were found on the image.
>> Image end offset: 262144
>> Format allocation info (including metadata):
>>                 data        zero   discarded   after-eof
>> used        192 KiB         0 B         0 B    63.5 KiB
>> unused          0 B         0 B         0 B
> OK, we create a 196624 byte file -- 3 clusters and a little bit of extra.
>
> 0: header
> 1: reftable
> 2: refcount block #0, accounting for clusters 0x0 - 0x7fff
> 3: l1_table, only partially allocated, and all zeroes
>
> So we've got 16 bytes defined for this l1 table, leaving most of a
> cluster defined but after EOF. I suppose your after-EOF counter there is
> probably rounding a bit to the nearest 512.
>
> So we've got three used clusters, and 99% of one cluster that's after
> EOF. Shouldn't data here be 192.5 in this case?
>
> Or Data: 192KiB; Zero 512 b?
>
> I guess the ".5" is just truncated or rounded.
>
>> # ./qemu-io -c 'write 0 100M' x
>> wrote 104857600/104857600 bytes at offset 0
>> 100 MiB, 1 ops; 0.7448 sec (134.263 MiB/sec and 1.3426 ops/sec)
>> # ./qemu-img check x
>> No errors were found on the image.
>> 1600/16384 = 9.77% allocated, 0.00% fragmented, 0.00% compressed clusters
>> Image end offset: 105185280
>> Format allocation info (including metadata):
>>                 data        zero   discarded   after-eof
>> used        100 MiB      60 KiB         0 B         0 B
>> unused          0 B         0 B         0 B
> Hmm, okay;
>
> now the image is 105185280 bytes; 102720 KiB; 1,605 clusters.
> 100MiB + 320KiB. Again, it doesn't entirely look like your summaries
> line up. Did we lose 256KiB to a rounding error under "100MiB" ?
>
>  From what I can now tell, the map looks like:
>
> == File Map ==
>
> 0x000000000 - 0x00004ffff [Metadata] (5 clusters)
> 0x000050000 - 0x00644ffff [Data] (1600 clusters)
>
> 1600 clusters at 64KiB each gives us 102400KiB / 100MiB of data.
> Then we've got five clusters of metadata (320KiB).
>
> Cluster 0: Header data. Data only occupies the first 512 bytes or so.
> Data: 512b
> Zeroes: 63.5KiB
>
> Cluster 1: Reftable. Data only occupies the first 8 bytes.
> Data: 512b
> Zeroes: 63.5KiB
>
> Cluster 2: Refcount Block #0. There are 0xC8A bytes, 3210/2 1605
> refcounts. Makes sense. That's 7 sectors of data.
> Data: 3.5KiB
> Zeroes: 60.5KiB
>
> Cluster 3: L1 table. One entry for L2 table. Takes 8 bytes.
> Data: 512B
> Zeroes: 63.5KiB
>
> Cluster 4: L2 table. 1,600 entries. Takes 5120 bytes, about 10 sectors.
> Data: 5KiB
> Zeroes: 59KiB
>
> Then clusters 5-1604 contain our data contiguously, the ascii byte 0xcd.
>
>> # ./qemu-io -c 'discard 0 1M' x
>> discard 1048576/1048576 bytes at offset 0
>> 1 MiB, 1 ops; 0.0002 sec (3.970 GiB/sec and 4065.0407 ops/sec)
>> # ./qemu-img check x
>> No errors were found on the image.
>> 1584/16384 = 9.67% allocated, 0.00% fragmented, 0.00% compressed clusters
>> Image end offset: 105185280
>> Format allocation info (including metadata):
>>                 data        zero   discarded   after-eof
>> used       99.3 MiB      60 KiB         0 B         0 B
>> unused          0 B       1 MiB         0 B
>> -------------------------
>>
>> - hmm, 60 KiB, don't know what is it. some preallocation may be..
>>
> x doesn't lose any filesize, but we have 1584 allocated clusters. We
> lost 16, corresponding to the discarded 1M.
>
> Map is now:
>
> 0x000000000 - 0x00004ffff [Metadata] (5 clusters)
> 0x000050000 - 0x00014ffff [Vacant] (16 clusters)
> 0x000150000 - 0x00644ffff [Data] (1584 clusters)
>
> OK.
>
> 0: Header. no change.
> Data: 512b
> Zeroes: 63.5KiB
>
> 1: Reftable. No change.
> Data: 512b
> Zeroes: 63.5KiB
>
> Cluster 2: Almost the same.... ref[5] (i.e. the sixth) through ref[20]
> have been decremented, but everything else remains at refcount of 01.
> Still takes up the same amount of space at the sector granularity level.
> Data: 3.5KiB
> Zeroes: 60.5KiB
>
> Cluster 3: L1 table. No change.
> Data: 512B
> Zeroes: 63.5KiB
>
> Cluster 4: L2 table
> Here, the first 16 data clusters have been modified to zero cluster
> pointers: 0x0000000000000001, everything else remains defined as it was.
> Data: 5KiB
> Zeroes: 59KiB
>
> Clusters 5-20 inclusive: non-zero data now discarded and considered
> unused. 1MiB. makes sense.
>
> Clusters 21-end: non-zero, used data. 1584 clusters; 101376KiB; 99MiB
>
> My Tallies:
>
> Metadata: 10KiB
> Metadata Zeroes: 310KiB
> Undefined Data: 1MiB
> Data: 99MiB
>
> Your Tallies:
> 'used-data': 99.3MiB (101683.2KiB)
> 'used-zeroes': 60KiB
> 'unused-data': 1MiB
>
> Subtracting out the 99MiB of data surely accounted for correctly here;
> you are counting about 0.3MiB + 60KiB of used data for presumably the
> metadata regions; ~367.2KiB.
>
> Looks like your counts are something like:
> metadata: 260KiB (0.25MiB ... ~0.3 with rounding, OK)
> metadata-zeroes: 60KiB
>
> So it's probably just counting what is and isn't zeroes a little less
> aggressively than I am doing. To what extent or how, I don't know. Maybe
> it depends on the underlying filesystem:
>
> jhuston@probe ~> qemu-img map -f raw X
> Offset          Length          Mapped to       File
> 0               0x31000         0               X
> 0x40000         0x10000         0x40000         X
> 0x150000        0x6300000       0x150000        X
>
> Looks like a hole from 0x31000 to 0x40000, 60KiB in the metadata region,
> so that's probably it.
>
> Then there's a hole from 0x50000 to 0x150000, 1MiB, so that's unused data.
>
> Hey, interesting, the discarded data that would be read as zeroes is
> still defined by the QCOW2 schema so that's "unused-zeroes" whereas the
> zero space in the metadata is only counted as such because of the sparse
> gap, so that's used-zero. OK, I think I'm starting to get what these
> numbers mean.
>
>>>> +# @used-discarded: used by the format file but actually unallocated
>>>> in the
>>>> +#                  underlying file
>>>> +#
>>> In what case do we have used data that is discarded/undefined, but not
>>> zero? Shouldn't discarded data be zero...?
>> may be discarded is bad name.. this if for unallocated block status of
>> underlying file.
>>
> Unallocated in what sense, exactly? Do you have an example for qcow2?
> I'm sorry that i still don't quite follow :\
>
>>>> +# @used-overrun: used by the format file beyond the end of the
>>>> underlying file
>>>> +#
>>> When does this occur?
>> I think it shoud be some kind of corruption.
>>
> Alright, let me see if I have this straight...
>
> used-data: Normal data. We are standing on terra-firma.
> used-zero: Data that is defined to be zeroes in some way.
>
> (Does not necessarily include data clusters if they were not actually
> zeroed out, I think. May not include regions that ARE zero, even if they
> are literally zero, because the driver may not especially recognize them
> as such. Anything marked as zero will DEFINITELY be zero, though. Yes?)
>
> used-discarded: I'm not actually sure in this case.
>
> used-overrun: Data that is defined to exist, but appears to fall outside
> of or beyond EOF. Appears to happen with qcow2 metadata before any
> writes occur.
>
> unused-data: Normal data, but not in-use by the schema anywhere. Leaked
> clusters and the like, effectively.
>
> unused-zero: Similar to the above, but definitely zeroes.
>
> unused-discarded: Not really sure.

yes, something like this. again, -data, -zero and -discarded are just 
corresponding to return value of bdrv_get_block_status(bs->file),

if status & BDRV_BLOCK_DATA
    it is  -data
else if status & BDRV_BLOCK_ZERO
    it is -zero (should be holes for raw)
else
    it is -discarded (impossible for raw)
end

>
>>>> +# @unused-data: allocated data in the underlying file not used by
>>>> the format
>>>> +#
>>> I assume this is an allocation gap in qcow2. Unused, but non-zero. Right?
>> or it may be some kind of error or due to underlying fs doesn't maintain
>> holes.
>>
>>>> +# @unused-zero: holes in the underlying file not used by the format
>>>> file
>>>> +#
>>> I assume this is similar to the above -- Unused, but zero.
>> Unused and underlying block status is ZERO. It is a "good" case for
>> unused areas.
>>
>>>> +# @unused-discarded: unallocated areas in the underlying file not
>>>> used by the
>>>> +#                    format file
>>>> +#
>>> Again I am unsure of what discarded but non-zero might mean.
>> looks like for raw format discarded is impossible, but to make a generic
>> tool, let's consider block status = unallocated too.
>>
>>>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal
>>>> to the
>>>> +#       length of the underlying file.
>>>> +#
>>>> +# Since: 2.10
>>>> +#
>>>> +##
>>>> +{ 'struct': 'BlockFormatAllocInfo',
>>>> +  'data': {'used-data':        'uint64',
>>>> +           'used-zero':        'uint64',
>>>> +           'used-discarded':   'uint64',
>>>> +           'used-overrun':     'uint64',
>>>> +           'unused-data':      'uint64',
>>>> +           'unused-zero':      'uint64',
>>>> +           'unused-discarded': 'uint64' } }
>>>> +
>>>> +##
>>>>    # @ImageCheck:
>>>>    #
>>>>    # Information about a QEMU image file check
>>>>
>>> Sorry for the dumb questions.
>> Don't worry)
>>
>>> --John
>>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[John Snow]

On 06/29/2017 02:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 29.06.2017 03:15, John Snow wrote:
>>
>> Alright, let me see if I have this straight...
>>
>> used-data: Normal data. We are standing on terra-firma.
>> used-zero: Data that is defined to be zeroes in some way.
>>
>> (Does not necessarily include data clusters if they were not actually
>> zeroed out, I think. May not include regions that ARE zero, even if they
>> are literally zero, because the driver may not especially recognize them
>> as such. Anything marked as zero will DEFINITELY be zero, though. Yes?)
>>
>> used-discarded: I'm not actually sure in this case.
>>
>> used-overrun: Data that is defined to exist, but appears to fall outside
>> of or beyond EOF. Appears to happen with qcow2 metadata before any
>> writes occur.
>>
>> unused-data: Normal data, but not in-use by the schema anywhere. Leaked
>> clusters and the like, effectively.
>>
>> unused-zero: Similar to the above, but definitely zeroes.
>>
>> unused-discarded: Not really sure.
> 
> yes, something like this. again, -data, -zero and -discarded are just
> corresponding to return value of bdrv_get_block_status(bs->file),
> 
> if status & BDRV_BLOCK_DATA
>    it is  -data
> else if status & BDRV_BLOCK_ZERO
>    it is -zero (should be holes for raw)
> else
>    it is -discarded (impossible for raw)
> end
> 

Understood, but I was not too clear on the exact nature of block status,
so I needed to clarify. I'm a lot clearer now, thanks.

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[John Snow]

On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> The function should collect statistics, about used/unused by top-level
> format driver space (in its .file) and allocation status
> (data/zero/discarded/after-eof) of corresponding areas in this .file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                   | 16 ++++++++++++++
>  include/block/block.h     |  3 +++
>  include/block/block_int.h |  2 ++
>  qapi/block-core.json      | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 50ba264143..7d720ae0c2 100644
> --- a/block.c
> +++ b/block.c
> @@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
>  }
>  
>  /**
> + * Collect format allocation info. See BlockFormatAllocInfo definition in
> + * qapi/block-core.json.
> + */
> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo *bfai)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (!drv) {
> +        return -ENOMEDIUM;
> +    }
> +    if (drv->bdrv_get_format_alloc_stat) {
> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
> +    }
> +    return -ENOTSUP;
> +}
> +
> +/**
>   * Return number of sectors on success, -errno on error.
>   */
>  int64_t bdrv_nb_sectors(BlockDriverState *bs)
> diff --git a/include/block/block.h b/include/block/block.h
> index 9b355e92d8..646376a772 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -335,6 +335,9 @@ typedef enum {
>  
>  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  
> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
> +                               BlockFormatAllocInfo *bfai);
> +
>  /* The units of offset and total_work_size may be chosen arbitrarily by the
>   * block driver; total_work_size may change during the course of the amendment
>   * operation */
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8d3724cce6..458c715e99 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -208,6 +208,8 @@ struct BlockDriver {
>      int64_t (*bdrv_getlength)(BlockDriverState *bs);
>      bool has_variable_length;
>      int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
> +                                      BlockFormatAllocInfo *bfai);
>  
>      int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>          uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ea0b3e8b13..fd7b52bd69 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -139,6 +139,61 @@
>             '*format-specific': 'ImageInfoSpecific' } }
>  
>  ##
> +# @BlockFormatAllocInfo:
> +#
> +#
> +# Allocation relations between format file and underlying protocol file.
> +# All fields are in bytes.
> +#

I guess this is a relation in the sense that the format differentiates
between used-unused and the protocol differentiates between
data-zero-trim which gives us the 2D matrix, showing a relation between
"two" files.

I find the use of the word "file" here a bit of a misdirection, though,
as qcow2 (or any other format) in this sense is not a file but rather a
schema used for interpreting data, and the file itself belongs solely to
the protocol.

I might suggest phrasing this as ...

"Allocation information of an underlying protocol file, as partitioned
by a format driver's utilization of said allocations."

Maybe that's too wordy. Eric?

> +# There are two types of the format file portions: 'used' and 'unused'. It's up
> +# to the format how to interpret these types. For now the only format supporting
"format file" seems misleading for similar reasons to my objection
above, which is that the format doesn't have a file, exactly. It's an
abstract schema.

"It's up to the format how to interpret these types" is also a bit too
vague to help inform readers what the types mean, IMO.

Here's an attempt:

"Allocations may be considered either used or unused by the format
driver interpreting those allocations. It is at the discretion of the
format driver (e.g. qcow2) which regions of its backing storage are
considered in-use or not."

I'm trying to clarify that it is not up to the qcow2 driver what "used"
or "unused" *means* but rather that only the qcow2 driver itself can
know which segments are used or unused, as these are semantic
distinctions that belong to the format driver layer. Does that make sense?

That's the spirit of my suggestion.

> +# the feature is Qcow2 and for this case 'used' are clusters with positive
> +# refcount and unused a clusters with zero refcount. Described portions include
> +# all format file allocations, not only virtual disk data (metadata, internal
> +# snapshots, etc. are included).
> +#

"For now, the only format driver supporting this feature is Qcow2 which
is a cluster based format. Clusters considered in-use by qcow2 are those
with a non-zero refcount in the format metadata. All other clusters, if
present, are considered unused."

(Your original description is actually pretty clear.)

I might add some further examples to illustrate the abstraction boundary
we're targeting:

"Examples of unused allocations for the Qcow2 format are leaked
clusters, pre-allocated clusters, and recently freed clusters."

> +# For the underlying file there are native block-status types of the portions:

How about "underlying protocol file" or "underlying storage protocol" or
something that uses the word "protocol" to make it very clear about when
we're talking about a format (qcow2) and when we're talking about the
storage/protocol file itself (raw posix)

> +#  - data: allocated data

ACK. My favorite kind of data. Easy to understand for idiots like me.

> +#  - zero: read-as-zero holes

This is not *necessarily* a hole, at the discretion of the protocol.
Depending on how we've backed the qcow2 we might not actually know how
the zeroes are stored, or if they are stored. All we know is that the
storage protocol here knows that this data happens to be zero.

I find the usage of "hole" here to be misleading, as it suggests
naturally either filesystem sparse allocations (which is correct,
incidentally) but also qcow2 holes, which doesn't have anything to do
with zeroes, necessarily.

> +#  - discarded: not allocated

This might be OK; I don't have a better suggestion. "not allocated" is
again protocol-dependent, but I can't think of a better way to phrase
this, actually...

> +# 4th additional type is 'overrun', which is for the format file portions beyond
> +# the end of the underlying file.
> +#
"Which is data referenced by the format driver located beyond EOF of the
protocol file."

The key thing I am trying to illustrate in the phrase is that the format
file specifies or alludes to the existence of data that is beyond the
EOF for the protocol file.

I think -- though I cannot prove -- that this is almost certainly a
special case of read-as-zero. If that is the case, perhaps we could
mention as much.

An example here would be really illustrative:

"For example, a partially allocated cluster at the end of a QCOW2 file,
where Qcow2 generally operates on complete clusters."

> +# So, the fields are:
> +#
> +# @used-data: used by the format file and backed by data in the underlying file
> +#
> +# @used-zero: used by the format file and backed by a hole in the underlying
> +#             file
> +#

Maybe "backed by zeroes in the underlying file; which may be a
filesystem hole for e.g. POSIX files."

> +# @used-discarded: used by the format file but actually unallocated in the
> +#                  underlying file
> +#

Which would almost certainly be an error, right? Mentioning as much
might be good.

> +# @used-overrun: used by the format file beyond the end of the underlying file
> +#

Which may or may not be an error, depending on how the protocol file
(for the format driver?) handles reads to areas out of bounds.

> +# @unused-data: allocated data in the underlying file not used by the format
> +#
> +# @unused-zero: holes in the underlying file not used by the format file
> +#
> +# @unused-discarded: unallocated areas in the underlying file not used by the
> +#                    format file
> +#
> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
> +#       length of the underlying file.
> +#
> +# Since: 2.10
> +#
> +##
> +{ 'struct': 'BlockFormatAllocInfo',
> +  'data': {'used-data':        'uint64',
> +           'used-zero':        'uint64',
> +           'used-discarded':   'uint64',
> +           'used-overrun':     'uint64',
> +           'unused-data':      'uint64',
> +           'unused-zero':      'uint64',
> +           'unused-discarded': 'uint64' } }
> +
> +##
>  # @ImageCheck:
>  #
>  # Information about a QEMU image file check
> 

All of my suggestions here are purely on phrasings. The mechanics of
this patch are now clear to me and I think it is useful information to
have in qemu.

Thanks for putting up with my questions!

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Eric Blake]

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

On 06/29/2017 07:27 PM, John Snow wrote:
> 
> 
> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>> The function should collect statistics, about used/unused by top-level
>> format driver space (in its .file) and allocation status
>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---

>> +#
>> +# Allocation relations between format file and underlying protocol file.
>> +# All fields are in bytes.
>> +#
> 
> I guess this is a relation in the sense that the format differentiates
> between used-unused and the protocol differentiates between
> data-zero-trim which gives us the 2D matrix, showing a relation between
> "two" files.
> 
> I find the use of the word "file" here a bit of a misdirection, though,
> as qcow2 (or any other format) in this sense is not a file but rather a
> schema used for interpreting data, and the file itself belongs solely to
> the protocol.
> 
> I might suggest phrasing this as ...
> 
> "Allocation information of an underlying protocol file, as partitioned
> by a format driver's utilization of said allocations."
> 
> Maybe that's too wordy. Eric?

No, that actually sounds quite reasonable! We're stating how much of the
protocol BDS is allocated, and subdividing those allocations by how much
the format layer is using the allocation (where the format layer usage
includes metadata not visible to the guest).  Our division of
information will let us spot both directions of mismatches:

- the format layer is not currently referencing anything from the
protocol layer, but the protocol layer doesn't have a hole (for example,
possible in qcow2 both after a guest trims clusters, or when you delete
an internal snapshot - and where we don't punch holes when freeing clusters)

- the format layer is currently spending bytes to reference arbitrary
data from the protocol layer, but the protocol layer has a hole, so the
format layer could use a more compact representation (for example, qcow2
has an allocated cluster and assumes it is arbitrary data, but the
underlying protocol layer has a hole, and bdrv_get_block_status returns
a combination of BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO thanks to its recursive
calls to both BDS)

> 
> Here's an attempt:
> 
> "Allocations may be considered either used or unused by the format
> driver interpreting those allocations. It is at the discretion of the
> format driver (e.g. qcow2) which regions of its backing storage are
> considered in-use or not."
> 
> I'm trying to clarify that it is not up to the qcow2 driver what "used"
> or "unused" *means* but rather that only the qcow2 driver itself can
> know which segments are used or unused, as these are semantic
> distinctions that belong to the format driver layer. Does that make sense?

Works for me.


> "Which is data referenced by the format driver located beyond EOF of the
> protocol file."
> 
> The key thing I am trying to illustrate in the phrase is that the format
> file specifies or alludes to the existence of data that is beyond the
> EOF for the protocol file.
> 
> I think -- though I cannot prove -- that this is almost certainly a
> special case of read-as-zero. If that is the case, perhaps we could
> mention as much.

Yes, all format drivers that allow use of clusters beyond EOF of the
underlying protocol treat that additional data as read-as-zero (at any
rate, bdrv_co_get_block_status() already has that interpretation,
because it adds BDRV_BLOCK_ZERO to the return value).

> 
> An example here would be really illustrative:
> 
> "For example, a partially allocated cluster at the end of a QCOW2 file,
> where Qcow2 generally operates on complete clusters."
> 
>> +# So, the fields are:
>> +#
>> +# @used-data: used by the format file and backed by data in the underlying file
>> +#
>> +# @used-zero: used by the format file and backed by a hole in the underlying
>> +#             file
>> +#
> 
> Maybe "backed by zeroes in the underlying file; which may be a
> filesystem hole for e.g. POSIX files."

"for e.g." is redundant use of "for" (I remember a mnemonic for e.g. as
"example given" [better than looking up the actual Latin abbreviation];
but idiomatically, it's easiest to pronounce "e.g." as "for example").


> 
>> +# @used-discarded: used by the format file but actually unallocated in the
>> +#                  underlying file
>> +#
> 
> Which would almost certainly be an error, right? Mentioning as much
> might be good.

Not necessarily an error, if it corresponds to a section that the guest
has done a TRIM operation on, but where we did not have the means to
punch a hole in the protocol layer.  The semantics of guest TRIM is that
the data can no longer be reliably read, making it weaker (and thus
easier to implement) than explicitly writing things to zero (although
writing zeroes can often be sped up by trimming, when you have
guarantees on what will be read later).

> 
>> +# @used-overrun: used by the format file beyond the end of the underlying file
>> +#
> 
> Which may or may not be an error, depending on how the protocol file
> (for the format driver?) handles reads to areas out of bounds.

Back to that read-as-zero semantics beyond EOF we mentioned above - as
long as the format driver is okay with reads-as-zero content at those
offsets, it is not an error.  But yeah, if someone truncated the
underlying protocol file behind the format layer's back, then it's a
data corruption issue (maybe only the metadata, but more likely also the
guest-visible data).

> 
>> +# @unused-data: allocated data in the underlying file not used by the format
>> +#
>> +# @unused-zero: holes in the underlying file not used by the format file
>> +#
>> +# @unused-discarded: unallocated areas in the underlying file not used by the
>> +#                    format file
>> +#
>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
>> +#       length of the underlying file.
>> +#
>> +# Since: 2.10
>> +#
>> +##
>> +{ 'struct': 'BlockFormatAllocInfo',
>> +  'data': {'used-data':        'uint64',
>> +           'used-zero':        'uint64',
>> +           'used-discarded':   'uint64',
>> +           'used-overrun':     'uint64',
>> +           'unused-data':      'uint64',
>> +           'unused-zero':      'uint64',
>> +           'unused-discarded': 'uint64' } }
>> +
>> +##
>>  # @ImageCheck:
>>  #
>>  # Information about a QEMU image file check
>>
> 
> All of my suggestions here are purely on phrasings. The mechanics of
> this patch are now clear to me and I think it is useful information to
> have in qemu.
> 
> Thanks for putting up with my questions!
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[John Snow]

On 06/29/2017 08:45 PM, Eric Blake wrote:
> On 06/29/2017 07:27 PM, John Snow wrote:
>>
>>
>> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about used/unused by top-level
>>> format driver space (in its .file) and allocation status
>>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
> 
>>> +#
>>> +# Allocation relations between format file and underlying protocol file.
>>> +# All fields are in bytes.
>>> +#
>>
>> I guess this is a relation in the sense that the format differentiates
>> between used-unused and the protocol differentiates between
>> data-zero-trim which gives us the 2D matrix, showing a relation between
>> "two" files.
>>
>> I find the use of the word "file" here a bit of a misdirection, though,
>> as qcow2 (or any other format) in this sense is not a file but rather a
>> schema used for interpreting data, and the file itself belongs solely to
>> the protocol.
>>
>> I might suggest phrasing this as ...
>>
>> "Allocation information of an underlying protocol file, as partitioned
>> by a format driver's utilization of said allocations."
>>
>> Maybe that's too wordy. Eric?
> 
> No, that actually sounds quite reasonable! We're stating how much of the
> protocol BDS is allocated, and subdividing those allocations by how much
> the format layer is using the allocation (where the format layer usage
> includes metadata not visible to the guest).  Our division of
> information will let us spot both directions of mismatches:
> 
> - the format layer is not currently referencing anything from the
> protocol layer, but the protocol layer doesn't have a hole (for example,
> possible in qcow2 both after a guest trims clusters, or when you delete
> an internal snapshot - and where we don't punch holes when freeing clusters)
> 
> - the format layer is currently spending bytes to reference arbitrary
> data from the protocol layer, but the protocol layer has a hole, so the
> format layer could use a more compact representation (for example, qcow2
> has an allocated cluster and assumes it is arbitrary data, but the
> underlying protocol layer has a hole, and bdrv_get_block_status returns
> a combination of BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO thanks to its recursive
> calls to both BDS)
> 
>>
>> Here's an attempt:
>>
>> "Allocations may be considered either used or unused by the format
>> driver interpreting those allocations. It is at the discretion of the
>> format driver (e.g. qcow2) which regions of its backing storage are
>> considered in-use or not."
>>
>> I'm trying to clarify that it is not up to the qcow2 driver what "used"
>> or "unused" *means* but rather that only the qcow2 driver itself can
>> know which segments are used or unused, as these are semantic
>> distinctions that belong to the format driver layer. Does that make sense?
> 
> Works for me.
> 
> 
>> "Which is data referenced by the format driver located beyond EOF of the
>> protocol file."
>>
>> The key thing I am trying to illustrate in the phrase is that the format
>> file specifies or alludes to the existence of data that is beyond the
>> EOF for the protocol file.
>>
>> I think -- though I cannot prove -- that this is almost certainly a
>> special case of read-as-zero. If that is the case, perhaps we could
>> mention as much.
> 
> Yes, all format drivers that allow use of clusters beyond EOF of the
> underlying protocol treat that additional data as read-as-zero (at any
> rate, bdrv_co_get_block_status() already has that interpretation,
> because it adds BDRV_BLOCK_ZERO to the return value).
> 
>>
>> An example here would be really illustrative:
>>
>> "For example, a partially allocated cluster at the end of a QCOW2 file,
>> where Qcow2 generally operates on complete clusters."
>>
>>> +# So, the fields are:
>>> +#
>>> +# @used-data: used by the format file and backed by data in the underlying file
>>> +#
>>> +# @used-zero: used by the format file and backed by a hole in the underlying
>>> +#             file
>>> +#
>>
>> Maybe "backed by zeroes in the underlying file; which may be a
>> filesystem hole for e.g. POSIX files."
> 
> "for e.g." is redundant use of "for" (I remember a mnemonic for e.g. as
> "example given" [better than looking up the actual Latin abbreviation];
> but idiomatically, it's easiest to pronounce "e.g." as "for example").
> 

I surrender.

> 
>>
>>> +# @used-discarded: used by the format file but actually unallocated in the
>>> +#                  underlying file
>>> +#
>>
>> Which would almost certainly be an error, right? Mentioning as much
>> might be good.
> 
> Not necessarily an error, if it corresponds to a section that the guest
> has done a TRIM operation on, but where we did not have the means to
> punch a hole in the protocol layer.  The semantics of guest TRIM is that
> the data can no longer be reliably read, making it weaker (and thus
> easier to implement) than explicitly writing things to zero (although
> writing zeroes can often be sped up by trimming, when you have
> guarantees on what will be read later).
> 

But the data is considered "used" -- if it cannot be reliably read but
the format driver expects to be able to read it, is this not inherently
a category that signifies a semantic error?

Data that is considered "Discarded" by the protocol (we succeeded in
passing on the TRIM request from the guest through to the protocol)
should almost certainly no longer be considered "used" by the format...?

>>
>>> +# @used-overrun: used by the format file beyond the end of the underlying file
>>> +#
>>
>> Which may or may not be an error, depending on how the protocol file
>> (for the format driver?) handles reads to areas out of bounds.
> 
> Back to that read-as-zero semantics beyond EOF we mentioned above - as
> long as the format driver is okay with reads-as-zero content at those
> offsets, it is not an error.  But yeah, if someone truncated the
> underlying protocol file behind the format layer's back, then it's a
> data corruption issue (maybe only the metadata, but more likely also the
> guest-visible data).
> 
>>
>>> +# @unused-data: allocated data in the underlying file not used by the format
>>> +#
>>> +# @unused-zero: holes in the underlying file not used by the format file
>>> +#
>>> +# @unused-discarded: unallocated areas in the underlying file not used by the
>>> +#                    format file
>>> +#
>>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
>>> +#       length of the underlying file.
>>> +#
>>> +# Since: 2.10
>>> +#
>>> +##
>>> +{ 'struct': 'BlockFormatAllocInfo',
>>> +  'data': {'used-data':        'uint64',
>>> +           'used-zero':        'uint64',
>>> +           'used-discarded':   'uint64',
>>> +           'used-overrun':     'uint64',
>>> +           'unused-data':      'uint64',
>>> +           'unused-zero':      'uint64',
>>> +           'unused-discarded': 'uint64' } }
>>> +
>>> +##
>>>  # @ImageCheck:
>>>  #
>>>  # Information about a QEMU image file check
>>>
>>
>> All of my suggestions here are purely on phrasings. The mechanics of
>> this patch are now clear to me and I think it is useful information to
>> have in qemu.
>>
>> Thanks for putting up with my questions!
>>
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Eric Blake]

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

On 06/29/2017 07:54 PM, John Snow wrote:


>> Not necessarily an error, if it corresponds to a section that the guest
>> has done a TRIM operation on, but where we did not have the means to
>> punch a hole in the protocol layer.  The semantics of guest TRIM is that
>> the data can no longer be reliably read, making it weaker (and thus
>> easier to implement) than explicitly writing things to zero (although
>> writing zeroes can often be sped up by trimming, when you have
>> guarantees on what will be read later).
>>
> 
> But the data is considered "used" -- if it cannot be reliably read but
> the format driver expects to be able to read it, is this not inherently
> a category that signifies a semantic error?

It's a semantic error if the region belongs to the metadata portion of
the format driver, but not necessarily a problem if it belongs to the
guest-visible contents.  You're right that a guest probably shouldn't
read from areas that it trimmed, but since it is possible to do that on
bare metal block devices, it is reasonable that a format driver can
emulate what happens when a guest reads from trimmed (but
unknown-content) clusters.  You're also right that qcow2 probably can't
get into this situation (qcow2 v3 files always turned guest-trimmed
clusters into read-as-zero; and qcow2 v2 files discard the cluster
treating it as unused rather than used, thereby deferring to the backing
image).

It also corresponds somewhat to the behavior you get when putting a
qcow2 format on top of a block device (creating qcow2 as a POSIX file
gives you some nice assurances from the filesystem that clusters you
haven't touched read as zero; but not so with a brand-new qcow2 file on
a block device, where reads can be random until the first write)

> Data that is considered "Discarded" by the protocol (we succeeded in
> passing on the TRIM request from the guest through to the protocol)
> should almost certainly no longer be considered "used" by the format...?

The qcow2 format doesn't have a way to specifically represent a cluster
as discarded in this manner, but other protocols might.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

30.06.2017 03:27, John Snow wrote:
>
> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>> The function should collect statistics, about used/unused by top-level
>> format driver space (in its .file) and allocation status
>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c                   | 16 ++++++++++++++
>>   include/block/block.h     |  3 +++
>>   include/block/block_int.h |  2 ++
>>   qapi/block-core.json      | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 76 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 50ba264143..7d720ae0c2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
>>   }
>>   
>>   /**
>> + * Collect format allocation info. See BlockFormatAllocInfo definition in
>> + * qapi/block-core.json.
>> + */
>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo *bfai)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    if (!drv) {
>> +        return -ENOMEDIUM;
>> +    }
>> +    if (drv->bdrv_get_format_alloc_stat) {
>> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
>> +    }
>> +    return -ENOTSUP;
>> +}
>> +
>> +/**
>>    * Return number of sectors on success, -errno on error.
>>    */
>>   int64_t bdrv_nb_sectors(BlockDriverState *bs)
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 9b355e92d8..646376a772 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -335,6 +335,9 @@ typedef enum {
>>   
>>   int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>>   
>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>> +                               BlockFormatAllocInfo *bfai);
>> +
>>   /* The units of offset and total_work_size may be chosen arbitrarily by the
>>    * block driver; total_work_size may change during the course of the amendment
>>    * operation */
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 8d3724cce6..458c715e99 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -208,6 +208,8 @@ struct BlockDriver {
>>       int64_t (*bdrv_getlength)(BlockDriverState *bs);
>>       bool has_variable_length;
>>       int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
>> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
>> +                                      BlockFormatAllocInfo *bfai);
>>   
>>       int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>>           uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ea0b3e8b13..fd7b52bd69 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -139,6 +139,61 @@
>>              '*format-specific': 'ImageInfoSpecific' } }
>>   
>>   ##
>> +# @BlockFormatAllocInfo:
>> +#
>> +#
>> +# Allocation relations between format file and underlying protocol file.
>> +# All fields are in bytes.
>> +#
> I guess this is a relation in the sense that the format differentiates
> between used-unused and the protocol differentiates between
> data-zero-trim which gives us the 2D matrix, showing a relation between
> "two" files.
>
> I find the use of the word "file" here a bit of a misdirection, though,
> as qcow2 (or any other format) in this sense is not a file but rather a
> schema used for interpreting data, and the file itself belongs solely to
> the protocol.
>
> I might suggest phrasing this as ...
>
> "Allocation information of an underlying protocol file, as partitioned
> by a format driver's utilization of said allocations."
>
> Maybe that's too wordy. Eric?
>
>> +# There are two types of the format file portions: 'used' and 'unused'. It's up
>> +# to the format how to interpret these types. For now the only format supporting
> "format file" seems misleading for similar reasons to my objection
> above, which is that the format doesn't have a file, exactly. It's an
> abstract schema.
>
> "It's up to the format how to interpret these types" is also a bit too
> vague to help inform readers what the types mean, IMO.
>
> Here's an attempt:
>
> "Allocations may be considered either used or unused by the format
> driver interpreting those allocations. It is at the discretion of the
> format driver (e.g. qcow2) which regions of its backing storage are
> considered in-use or not."

So we are saying about "allocations". But unallocated data may be 
used/unused too, so,
can we call unallocated areas "allocations"?

>
> I'm trying to clarify that it is not up to the qcow2 driver what "used"
> or "unused" *means* but rather that only the qcow2 driver itself can
> know which segments are used or unused, as these are semantic
> distinctions that belong to the format driver layer. Does that make sense?
>
> That's the spirit of my suggestion.
>
>> +# the feature is Qcow2 and for this case 'used' are clusters with positive
>> +# refcount and unused a clusters with zero refcount. Described portions include
>> +# all format file allocations, not only virtual disk data (metadata, internal
>> +# snapshots, etc. are included).
>> +#
> "For now, the only format driver supporting this feature is Qcow2 which
> is a cluster based format. Clusters considered in-use by qcow2 are those
> with a non-zero refcount in the format metadata. All other clusters, if
> present, are considered unused."
>
> (Your original description is actually pretty clear.)
>
> I might add some further examples to illustrate the abstraction boundary
> we're targeting:
>
> "Examples of unused allocations for the Qcow2 format are leaked
> clusters, pre-allocated clusters, and recently freed clusters."
>
>> +# For the underlying file there are native block-status types of the portions:
> How about "underlying protocol file" or "underlying storage protocol" or
> something that uses the word "protocol" to make it very clear about when
> we're talking about a format (qcow2) and when we're talking about the
> storage/protocol file itself (raw posix)
>
>> +#  - data: allocated data
> ACK. My favorite kind of data. Easy to understand for idiots like me.
>
>> +#  - zero: read-as-zero holes
> This is not *necessarily* a hole, at the discretion of the protocol.
> Depending on how we've backed the qcow2 we might not actually know how
> the zeroes are stored, or if they are stored. All we know is that the
> storage protocol here knows that this data happens to be zero.
>
> I find the usage of "hole" here to be misleading, as it suggests
> naturally either filesystem sparse allocations (which is correct,
> incidentally) but also qcow2 holes, which doesn't have anything to do
> with zeroes, necessarily.
>
>> +#  - discarded: not allocated
> This might be OK; I don't have a better suggestion. "not allocated" is
> again protocol-dependent, but I can't think of a better way to phrase
> this, actually...
>
>> +# 4th additional type is 'overrun', which is for the format file portions beyond
>> +# the end of the underlying file.
>> +#
> "Which is data referenced by the format driver located beyond EOF of the
> protocol file."
>
> The key thing I am trying to illustrate in the phrase is that the format
> file specifies or alludes to the existence of data that is beyond the
> EOF for the protocol file.
>
> I think -- though I cannot prove -- that this is almost certainly a
> special case of read-as-zero. If that is the case, perhaps we could
> mention as much.
>
> An example here would be really illustrative:
>
> "For example, a partially allocated cluster at the end of a QCOW2 file,
> where Qcow2 generally operates on complete clusters."
>
>> +# So, the fields are:
>> +#
>> +# @used-data: used by the format file and backed by data in the underlying file
>> +#
>> +# @used-zero: used by the format file and backed by a hole in the underlying
>> +#             file
>> +#
> Maybe "backed by zeroes in the underlying file; which may be a
> filesystem hole for e.g. POSIX files."
>
>> +# @used-discarded: used by the format file but actually unallocated in the
>> +#                  underlying file
>> +#
> Which would almost certainly be an error, right? Mentioning as much
> might be good.
>
>> +# @used-overrun: used by the format file beyond the end of the underlying file
>> +#
> Which may or may not be an error, depending on how the protocol file
> (for the format driver?) handles reads to areas out of bounds.
>
>> +# @unused-data: allocated data in the underlying file not used by the format
>> +#
>> +# @unused-zero: holes in the underlying file not used by the format file
>> +#
>> +# @unused-discarded: unallocated areas in the underlying file not used by the
>> +#                    format file
>> +#
>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
>> +#       length of the underlying file.
>> +#
>> +# Since: 2.10
>> +#
>> +##
>> +{ 'struct': 'BlockFormatAllocInfo',
>> +  'data': {'used-data':        'uint64',
>> +           'used-zero':        'uint64',
>> +           'used-discarded':   'uint64',
>> +           'used-overrun':     'uint64',
>> +           'unused-data':      'uint64',
>> +           'unused-zero':      'uint64',
>> +           'unused-discarded': 'uint64' } }
>> +
>> +##
>>   # @ImageCheck:
>>   #
>>   # Information about a QEMU image file check
>>
> All of my suggestions here are purely on phrasings. The mechanics of
> this patch are now clear to me and I think it is useful information to
> have in qemu.
>
> Thanks for putting up with my questions!


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

12.07.2017 18:18, Vladimir Sementsov-Ogievskiy wrote:
> 30.06.2017 03:27, John Snow wrote:
>>
>> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about used/unused by top-level
>>> format driver space (in its .file) and allocation status
>>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block.c                   | 16 ++++++++++++++
>>>   include/block/block.h     |  3 +++
>>>   include/block/block_int.h |  2 ++
>>>   qapi/block-core.json      | 55 
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 76 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 50ba264143..7d720ae0c2 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3407,6 +3407,22 @@ int64_t 
>>> bdrv_get_allocated_file_size(BlockDriverState *bs)
>>>   }
>>>     /**
>>> + * Collect format allocation info. See BlockFormatAllocInfo 
>>> definition in
>>> + * qapi/block-core.json.
>>> + */
>>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, 
>>> BlockFormatAllocInfo *bfai)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    if (!drv) {
>>> +        return -ENOMEDIUM;
>>> +    }
>>> +    if (drv->bdrv_get_format_alloc_stat) {
>>> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
>>> +    }
>>> +    return -ENOTSUP;
>>> +}
>>> +
>>> +/**
>>>    * Return number of sectors on success, -errno on error.
>>>    */
>>>   int64_t bdrv_nb_sectors(BlockDriverState *bs)
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 9b355e92d8..646376a772 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -335,6 +335,9 @@ typedef enum {
>>>     int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, 
>>> BdrvCheckMode fix);
>>>   +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>>> +                               BlockFormatAllocInfo *bfai);
>>> +
>>>   /* The units of offset and total_work_size may be chosen 
>>> arbitrarily by the
>>>    * block driver; total_work_size may change during the course of 
>>> the amendment
>>>    * operation */
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 8d3724cce6..458c715e99 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -208,6 +208,8 @@ struct BlockDriver {
>>>       int64_t (*bdrv_getlength)(BlockDriverState *bs);
>>>       bool has_variable_length;
>>>       int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
>>> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
>>> +                                      BlockFormatAllocInfo *bfai);
>>>         int coroutine_fn 
>>> (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>>>           uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index ea0b3e8b13..fd7b52bd69 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -139,6 +139,61 @@
>>>              '*format-specific': 'ImageInfoSpecific' } }
>>>     ##
>>> +# @BlockFormatAllocInfo:
>>> +#
>>> +#
>>> +# Allocation relations between format file and underlying protocol 
>>> file.
>>> +# All fields are in bytes.
>>> +#
>> I guess this is a relation in the sense that the format differentiates
>> between used-unused and the protocol differentiates between
>> data-zero-trim which gives us the 2D matrix, showing a relation between
>> "two" files.
>>
>> I find the use of the word "file" here a bit of a misdirection, though,
>> as qcow2 (or any other format) in this sense is not a file but rather a
>> schema used for interpreting data, and the file itself belongs solely to
>> the protocol.
>>
>> I might suggest phrasing this as ...
>>
>> "Allocation information of an underlying protocol file, as partitioned
>> by a format driver's utilization of said allocations."
>>
>> Maybe that's too wordy. Eric?
>>
>>> +# There are two types of the format file portions: 'used' and 
>>> 'unused'. It's up
>>> +# to the format how to interpret these types. For now the only 
>>> format supporting
>> "format file" seems misleading for similar reasons to my objection
>> above, which is that the format doesn't have a file, exactly. It's an
>> abstract schema.
>>
>> "It's up to the format how to interpret these types" is also a bit too
>> vague to help inform readers what the types mean, IMO.
>>
>> Here's an attempt:
>>
>> "Allocations may be considered either used or unused by the format
>> driver interpreting those allocations. It is at the discretion of the
>> format driver (e.g. qcow2) which regions of its backing storage are
>> considered in-use or not."
>
> So we are saying about "allocations". But unallocated data may be 
> used/unused too, so,
> can we call unallocated areas "allocations"?


John?

Note: we can  add this with x- prefix or something like this if this 
simplifies things (and adjust doc later).


>
>>
>> I'm trying to clarify that it is not up to the qcow2 driver what "used"
>> or "unused" *means* but rather that only the qcow2 driver itself can
>> know which segments are used or unused, as these are semantic
>> distinctions that belong to the format driver layer. Does that make 
>> sense?
>>
>> That's the spirit of my suggestion.
>>
>>> +# the feature is Qcow2 and for this case 'used' are clusters with 
>>> positive
>>> +# refcount and unused a clusters with zero refcount. Described 
>>> portions include
>>> +# all format file allocations, not only virtual disk data 
>>> (metadata, internal
>>> +# snapshots, etc. are included).
>>> +#
>> "For now, the only format driver supporting this feature is Qcow2 which
>> is a cluster based format. Clusters considered in-use by qcow2 are those
>> with a non-zero refcount in the format metadata. All other clusters, if
>> present, are considered unused."
>>
>> (Your original description is actually pretty clear.)
>>
>> I might add some further examples to illustrate the abstraction boundary
>> we're targeting:
>>
>> "Examples of unused allocations for the Qcow2 format are leaked
>> clusters, pre-allocated clusters, and recently freed clusters."
>>
>>> +# For the underlying file there are native block-status types of 
>>> the portions:
>> How about "underlying protocol file" or "underlying storage protocol" or
>> something that uses the word "protocol" to make it very clear about when
>> we're talking about a format (qcow2) and when we're talking about the
>> storage/protocol file itself (raw posix)
>>
>>> +#  - data: allocated data
>> ACK. My favorite kind of data. Easy to understand for idiots like me.
>>
>>> +#  - zero: read-as-zero holes
>> This is not *necessarily* a hole, at the discretion of the protocol.
>> Depending on how we've backed the qcow2 we might not actually know how
>> the zeroes are stored, or if they are stored. All we know is that the
>> storage protocol here knows that this data happens to be zero.
>>
>> I find the usage of "hole" here to be misleading, as it suggests
>> naturally either filesystem sparse allocations (which is correct,
>> incidentally) but also qcow2 holes, which doesn't have anything to do
>> with zeroes, necessarily.
>>
>>> +#  - discarded: not allocated
>> This might be OK; I don't have a better suggestion. "not allocated" is
>> again protocol-dependent, but I can't think of a better way to phrase
>> this, actually...
>>
>>> +# 4th additional type is 'overrun', which is for the format file 
>>> portions beyond
>>> +# the end of the underlying file.
>>> +#
>> "Which is data referenced by the format driver located beyond EOF of the
>> protocol file."
>>
>> The key thing I am trying to illustrate in the phrase is that the format
>> file specifies or alludes to the existence of data that is beyond the
>> EOF for the protocol file.
>>
>> I think -- though I cannot prove -- that this is almost certainly a
>> special case of read-as-zero. If that is the case, perhaps we could
>> mention as much.
>>
>> An example here would be really illustrative:
>>
>> "For example, a partially allocated cluster at the end of a QCOW2 file,
>> where Qcow2 generally operates on complete clusters."
>>
>>> +# So, the fields are:
>>> +#
>>> +# @used-data: used by the format file and backed by data in the 
>>> underlying file
>>> +#
>>> +# @used-zero: used by the format file and backed by a hole in the 
>>> underlying
>>> +#             file
>>> +#
>> Maybe "backed by zeroes in the underlying file; which may be a
>> filesystem hole for e.g. POSIX files."
>>
>>> +# @used-discarded: used by the format file but actually unallocated 
>>> in the
>>> +#                  underlying file
>>> +#
>> Which would almost certainly be an error, right? Mentioning as much
>> might be good.
>>
>>> +# @used-overrun: used by the format file beyond the end of the 
>>> underlying file
>>> +#
>> Which may or may not be an error, depending on how the protocol file
>> (for the format driver?) handles reads to areas out of bounds.
>>
>>> +# @unused-data: allocated data in the underlying file not used by 
>>> the format
>>> +#
>>> +# @unused-zero: holes in the underlying file not used by the format 
>>> file
>>> +#
>>> +# @unused-discarded: unallocated areas in the underlying file not 
>>> used by the
>>> +#                    format file
>>> +#
>>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is 
>>> equal to the
>>> +#       length of the underlying file.
>>> +#
>>> +# Since: 2.10
>>> +#
>>> +##
>>> +{ 'struct': 'BlockFormatAllocInfo',
>>> +  'data': {'used-data':        'uint64',
>>> +           'used-zero':        'uint64',
>>> +           'used-discarded':   'uint64',
>>> +           'used-overrun':     'uint64',
>>> +           'unused-data':      'uint64',
>>> +           'unused-zero':      'uint64',
>>> +           'unused-discarded': 'uint64' } }
>>> +
>>> +##
>>>   # @ImageCheck:
>>>   #
>>>   # Information about a QEMU image file check
>>>
>> All of my suggestions here are purely on phrasings. The mechanics of
>> this patch are now clear to me and I think it is useful information to
>> have in qemu.
>>
>> Thanks for putting up with my questions!
>
>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[John Snow]

On 07/12/2017 11:18 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.06.2017 03:27, John Snow wrote:
>>
>> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about used/unused by top-level
>>> format driver space (in its .file) and allocation status
>>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block.c                   | 16 ++++++++++++++
>>>   include/block/block.h     |  3 +++
>>>   include/block/block_int.h |  2 ++
>>>   qapi/block-core.json      | 55 
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 76 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 50ba264143..7d720ae0c2 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3407,6 +3407,22 @@ int64_t 
>>> bdrv_get_allocated_file_size(BlockDriverState *bs)
>>>   }
>>>   /**
>>> + * Collect format allocation info. See BlockFormatAllocInfo 
>>> definition in
>>> + * qapi/block-core.json.
>>> + */
>>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, 
>>> BlockFormatAllocInfo *bfai)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    if (!drv) {
>>> +        return -ENOMEDIUM;
>>> +    }
>>> +    if (drv->bdrv_get_format_alloc_stat) {
>>> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
>>> +    }
>>> +    return -ENOTSUP;
>>> +}
>>> +
>>> +/**
>>>    * Return number of sectors on success, -errno on error.
>>>    */
>>>   int64_t bdrv_nb_sectors(BlockDriverState *bs)
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 9b355e92d8..646376a772 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -335,6 +335,9 @@ typedef enum {
>>>   int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, 
>>> BdrvCheckMode fix);
>>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>>> +                               BlockFormatAllocInfo *bfai);
>>> +
>>>   /* The units of offset and total_work_size may be chosen 
>>> arbitrarily by the
>>>    * block driver; total_work_size may change during the course of 
>>> the amendment
>>>    * operation */
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 8d3724cce6..458c715e99 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -208,6 +208,8 @@ struct BlockDriver {
>>>       int64_t (*bdrv_getlength)(BlockDriverState *bs);
>>>       bool has_variable_length;
>>>       int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
>>> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
>>> +                                      BlockFormatAllocInfo *bfai);
>>>       int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState 
>>> *bs,
>>>           uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index ea0b3e8b13..fd7b52bd69 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -139,6 +139,61 @@
>>>              '*format-specific': 'ImageInfoSpecific' } }
>>>   ##
>>> +# @BlockFormatAllocInfo:
>>> +#
>>> +#
>>> +# Allocation relations between format file and underlying protocol 
>>> file.
>>> +# All fields are in bytes.
>>> +#
>> I guess this is a relation in the sense that the format differentiates
>> between used-unused and the protocol differentiates between
>> data-zero-trim which gives us the 2D matrix, showing a relation between
>> "two" files.
>>
>> I find the use of the word "file" here a bit of a misdirection, though,
>> as qcow2 (or any other format) in this sense is not a file but rather a
>> schema used for interpreting data, and the file itself belongs solely to
>> the protocol.
>>
>> I might suggest phrasing this as ...
>>
>> "Allocation information of an underlying protocol file, as partitioned
>> by a format driver's utilization of said allocations."
>>
>> Maybe that's too wordy. Eric?
>>
>>> +# There are two types of the format file portions: 'used' and 
>>> 'unused'. It's up
>>> +# to the format how to interpret these types. For now the only 
>>> format supporting
>> "format file" seems misleading for similar reasons to my objection
>> above, which is that the format doesn't have a file, exactly. It's an
>> abstract schema.
>>
>> "It's up to the format how to interpret these types" is also a bit too
>> vague to help inform readers what the types mean, IMO.
>>
>> Here's an attempt:
>>
>> "Allocations may be considered either used or unused by the format
>> driver interpreting those allocations. It is at the discretion of the
>> format driver (e.g. qcow2) which regions of its backing storage are
>> considered in-use or not."
> 
> So we are saying about "allocations". But unallocated data may be 
> used/unused too, so,
> can we call unallocated areas "allocations"?
> 

You're right, "allocation" may imply something that is extant, but these 
are more like regions ...

Uh;

"Regions" may be considered either used or unused?

or

"Regions of the underlying protocol file may be considered used or 
unused by the format driver interpreting these regions."

Something along these lines, perhaps?

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

28.07.2017 00:23, John Snow wrote:
>
>
> On 07/12/2017 11:18 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 30.06.2017 03:27, John Snow wrote:
>>>
>>> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> The function should collect statistics, about used/unused by top-level
>>>> format driver space (in its .file) and allocation status
>>>> (data/zero/discarded/after-eof) of corresponding areas in this .file.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   block.c                   | 16 ++++++++++++++
>>>>   include/block/block.h     |  3 +++
>>>>   include/block/block_int.h |  2 ++
>>>>   qapi/block-core.json      | 55 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>   4 files changed, 76 insertions(+)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 50ba264143..7d720ae0c2 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -3407,6 +3407,22 @@ int64_t 
>>>> bdrv_get_allocated_file_size(BlockDriverState *bs)
>>>>   }
>>>>   /**
>>>> + * Collect format allocation info. See BlockFormatAllocInfo 
>>>> definition in
>>>> + * qapi/block-core.json.
>>>> + */
>>>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, 
>>>> BlockFormatAllocInfo *bfai)
>>>> +{
>>>> +    BlockDriver *drv = bs->drv;
>>>> +    if (!drv) {
>>>> +        return -ENOMEDIUM;
>>>> +    }
>>>> +    if (drv->bdrv_get_format_alloc_stat) {
>>>> +        return drv->bdrv_get_format_alloc_stat(bs, bfai);
>>>> +    }
>>>> +    return -ENOTSUP;
>>>> +}
>>>> +
>>>> +/**
>>>>    * Return number of sectors on success, -errno on error.
>>>>    */
>>>>   int64_t bdrv_nb_sectors(BlockDriverState *bs)
>>>> diff --git a/include/block/block.h b/include/block/block.h
>>>> index 9b355e92d8..646376a772 100644
>>>> --- a/include/block/block.h
>>>> +++ b/include/block/block.h
>>>> @@ -335,6 +335,9 @@ typedef enum {
>>>>   int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, 
>>>> BdrvCheckMode fix);
>>>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs,
>>>> +                               BlockFormatAllocInfo *bfai);
>>>> +
>>>>   /* The units of offset and total_work_size may be chosen 
>>>> arbitrarily by the
>>>>    * block driver; total_work_size may change during the course of 
>>>> the amendment
>>>>    * operation */
>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>> index 8d3724cce6..458c715e99 100644
>>>> --- a/include/block/block_int.h
>>>> +++ b/include/block/block_int.h
>>>> @@ -208,6 +208,8 @@ struct BlockDriver {
>>>>       int64_t (*bdrv_getlength)(BlockDriverState *bs);
>>>>       bool has_variable_length;
>>>>       int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
>>>> +    int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
>>>> +                                      BlockFormatAllocInfo *bfai);
>>>>       int coroutine_fn 
>>>> (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
>>>>           uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index ea0b3e8b13..fd7b52bd69 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -139,6 +139,61 @@
>>>>              '*format-specific': 'ImageInfoSpecific' } }
>>>>   ##
>>>> +# @BlockFormatAllocInfo:
>>>> +#
>>>> +#
>>>> +# Allocation relations between format file and underlying protocol 
>>>> file.
>>>> +# All fields are in bytes.
>>>> +#
>>> I guess this is a relation in the sense that the format differentiates
>>> between used-unused and the protocol differentiates between
>>> data-zero-trim which gives us the 2D matrix, showing a relation between
>>> "two" files.
>>>
>>> I find the use of the word "file" here a bit of a misdirection, though,
>>> as qcow2 (or any other format) in this sense is not a file but rather a
>>> schema used for interpreting data, and the file itself belongs 
>>> solely to
>>> the protocol.
>>>
>>> I might suggest phrasing this as ...
>>>
>>> "Allocation information of an underlying protocol file, as partitioned
>>> by a format driver's utilization of said allocations."
>>>
>>> Maybe that's too wordy. Eric?
>>>
>>>> +# There are two types of the format file portions: 'used' and 
>>>> 'unused'. It's up
>>>> +# to the format how to interpret these types. For now the only 
>>>> format supporting
>>> "format file" seems misleading for similar reasons to my objection
>>> above, which is that the format doesn't have a file, exactly. It's an
>>> abstract schema.
>>>
>>> "It's up to the format how to interpret these types" is also a bit too
>>> vague to help inform readers what the types mean, IMO.
>>>
>>> Here's an attempt:
>>>
>>> "Allocations may be considered either used or unused by the format
>>> driver interpreting those allocations. It is at the discretion of the
>>> format driver (e.g. qcow2) which regions of its backing storage are
>>> considered in-use or not."
>>
>> So we are saying about "allocations". But unallocated data may be 
>> used/unused too, so,
>> can we call unallocated areas "allocations"?
>>
>
> You're right, "allocation" may imply something that is extant, but 
> these are more like regions ...
>
> Uh;
>
> "Regions" may be considered either used or unused?
>
> or
>
> "Regions of the underlying protocol file may be considered used or 
> unused by the format driver interpreting these regions."
>
> Something along these lines, perhaps?

This is OK I think.



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Eric Blake]

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

On 07/27/2017 04:23 PM, John Snow wrote:

>>>
>>> "Allocations may be considered either used or unused by the format
>>> driver interpreting those allocations. It is at the discretion of the
>>> format driver (e.g. qcow2) which regions of its backing storage are
>>> considered in-use or not."
>>
>> So we are saying about "allocations". But unallocated data may be
>> used/unused too, so,
>> can we call unallocated areas "allocations"?
>>
> 
> You're right, "allocation" may imply something that is extant, but these
> are more like regions ...

Even 'extent' might have a connotation of something reserved; other
related words like 'block' are bad (a block of a block device?).

> 
> Uh;
> 
> "Regions" may be considered either used or unused?
> 
> or
> 
> "Regions of the underlying protocol file may be considered used or
> unused by the format driver interpreting these regions."

So this is one of the better variations I've seen, and works for me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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