[Qemu-devel] [PATCH v2 0/4] qemu-img check: format allocation info

[Qemu-devel] [PATCH v2 0/4] qemu-img check: format allocation info

[Vladimir Sementsov-Ogievskiy]

Hi all.

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

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 (4):
  block: add bdrv_get_format_alloc_stat format interface
  qcow2: add .bdrv_get_format_alloc_stat
  qemu-img check: add format allocation info
  qemu-img check: improve dump_human_format_alloc_info

 block.c                   |  16 ++++++
 block/qcow2-refcount.c    | 144 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |   2 +
 block/qcow2.h             |   2 +
 include/block/block.h     |   3 +
 include/block/block_int.h |   2 +
 qapi/block-core.json      |  32 ++++++++++-
 qemu-img.c                |  57 +++++++++++++++++-
 8 files changed, 255 insertions(+), 3 deletions(-)

-- 
2.11.1

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

[Vladimir Sementsov-Ogievskiy]

The function should collect statistics, about allocted/unallocated by
top-level format driver space (in its .file) and allocation status
(allocated/hole/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      | 26 ++++++++++++++++++++++++++
 4 files changed, 47 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..365070b3eb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -139,6 +139,32 @@
            '*format-specific': 'ImageInfoSpecific' } }
 
 ##
+# @BlockFormatAllocInfo:
+#
+# Information about allocations, including metadata. All fields are in bytes.
+#
+# @alloc_alloc: allocated by format driver and allocated in underlying file
+#
+# @alloc_hole: allocated by format driver but actually is a hole in
+#              underlying file
+#
+# @alloc_overhead: allocated by format driver after end of underlying file
+#
+# @hole_alloc: not allocated by format driver but allocated in underlying file
+#
+# @hole_hole: not allocated by format driver hole in underlying file
+#
+# Since: 2.10
+#
+##
+{ 'struct': 'BlockFormatAllocInfo',
+  'data': {'alloc_alloc':    'uint64',
+           'alloc_hole':     'uint64',
+           'alloc_overhead': 'uint64',
+           'hole_alloc':     'uint64',
+           'hole_hole':      'uint64' } }
+
+##
 # @ImageCheck:
 #
 # Information about a QEMU image file check
-- 
2.11.1

[Qemu-devel] [PATCH 2/4] 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 | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c          |   2 +
 block/qcow2.h          |   2 +
 3 files changed, 148 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..2010dfa048 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2931,3 +2931,147 @@ 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 allocated = false, f_allocated = 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) {
+            ret = bdrv_is_allocated_above(bs->file->bs, NULL, sector,
+                                          file_sectors - sector, &f_num);
+            if (ret < 0) {
+                goto fail;
+            }
+            f_allocated = ret;
+        }
+
+        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;
+            }
+            allocated = refcount > 0;
+            num = s->cluster_size >> BDRV_SECTOR_BITS;
+        }
+
+        dif = MIN(f_num, MIN(num, file_sectors - sector));
+        if (allocated) {
+            if (f_allocated) {
+                bfai->alloc_alloc += dif;
+            } else {
+                bfai->alloc_hole += dif;
+            }
+        } else {
+            if (f_allocated) {
+                bfai->hole_alloc += dif;
+            } else {
+                bfai->hole_hole += dif;
+            }
+        }
+        f_num -= dif;
+        num -= dif;
+    }
+
+    assert(f_num == 0);
+
+    if (allocated) {
+        bfai->alloc_overhead += 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->alloc_overhead++;
+            }
+        }
+
+        refcount_block_offset = 0;
+    }
+
+    qemu_co_mutex_unlock(&s->lock);
+
+    bfai->alloc_alloc = bfai->alloc_alloc << BDRV_SECTOR_BITS;
+    bfai->alloc_hole = bfai->alloc_hole << BDRV_SECTOR_BITS;
+    bfai->alloc_overhead = bfai->alloc_overhead << BDRV_SECTOR_BITS;
+
+    bfai->hole_alloc = bfai->hole_alloc << BDRV_SECTOR_BITS;
+    bfai->hole_hole = bfai->hole_hole << 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 3/4] 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           | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 365070b3eb..3357b61811 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -203,6 +203,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
 #
 ##
@@ -211,7 +214,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..55f8c1776c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,6 +560,29 @@ static void dump_json_image_check(ImageCheck *check, bool quiet)
     QDECREF(str);
 }
 
+static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
+{
+    char *alloc_alloc = size_to_str(bfai->alloc_alloc);
+    char *alloc_hole = size_to_str(bfai->alloc_hole);
+    char *alloc_overhead = size_to_str(bfai->alloc_overhead);
+    char *hole_alloc = size_to_str(bfai->hole_alloc);
+    char *hole_hole = size_to_str(bfai->hole_hole);
+
+    qprintf(quiet,
+            "Format allocation info (including metadata):\n"
+            "               file-alloc   file-hole   after-eof\n"
+            "format-alloc   %10s  %10s  %10s\n"
+            "format-hole    %10s  %10s\n",
+            alloc_alloc, alloc_hole, alloc_overhead,
+            hole_alloc, hole_hole);
+
+    g_free(alloc_alloc);
+    g_free(alloc_hole);
+    g_free(alloc_overhead);
+    g_free(hole_alloc);
+    g_free(hole_hole);
+}
+
 static void dump_human_image_check(ImageCheck *check, bool quiet)
 {
     if (!(check->corruptions || check->leaks || check->check_errors)) {
@@ -601,6 +624,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 +638,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 +667,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] [PATCH 4/4] qemu-img check: improve dump_human_format_alloc_info

[Vladimir Sementsov-Ogievskiy]

Improve dump_human_format_alloc_info() by specifying format names.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qemu-img.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 55f8c1776c..3c03690a4f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,7 +560,8 @@ static void dump_json_image_check(ImageCheck *check, bool quiet)
     QDECREF(str);
 }
 
-static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
+static void dump_human_format_alloc_info(BlockDriverState *bs,
+                                         BlockFormatAllocInfo *bfai, bool quiet)
 {
     char *alloc_alloc = size_to_str(bfai->alloc_alloc);
     char *alloc_hole = size_to_str(bfai->alloc_hole);
@@ -568,13 +569,28 @@ static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
     char *hole_alloc = size_to_str(bfai->hole_alloc);
     char *hole_hole = size_to_str(bfai->hole_hole);
 
+    const char *format = bdrv_get_format_name(bs);
+    const char *f_format =
+        bs->file ? bdrv_get_format_name(bs->file->bs) : "file";
+    int format_len, cw;
+
+    if (format == NULL) {
+        format = "format";
+    }
+    if (f_format == NULL) {
+        f_format = "file";
+    }
+    format_len = strlen(format);
+    cw = MAX(10, strlen(f_format) + 6);
+
     qprintf(quiet,
             "Format allocation info (including metadata):\n"
-            "               file-alloc   file-hole   after-eof\n"
-            "format-alloc   %10s  %10s  %10s\n"
-            "format-hole    %10s  %10s\n",
-            alloc_alloc, alloc_hole, alloc_overhead,
-            hole_alloc, hole_hole);
+            "%*s         %*s-alloc  %*s-hole  %*s\n"
+            "%s-alloc   %*s  %*s  %*s\n"
+            "%s-hole    %*s  %*s\n",
+            format_len, "", cw - 6, f_format, cw - 5, f_format, cw, "after-eof",
+            format, cw, alloc_alloc, cw, alloc_hole, cw, alloc_overhead,
+            format, cw, hole_alloc, cw, hole_hole);
 
     g_free(alloc_alloc);
     g_free(alloc_hole);
@@ -583,7 +599,8 @@ static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
     g_free(hole_hole);
 }
 
-static void dump_human_image_check(ImageCheck *check, bool quiet)
+static void dump_human_image_check(BlockDriverState *bs, ImageCheck *check,
+                                   bool quiet)
 {
     if (!(check->corruptions || check->leaks || check->check_errors)) {
         qprintf(quiet, "No errors were found on the image.\n");
@@ -626,7 +643,7 @@ static void dump_human_image_check(ImageCheck *check, bool quiet)
     }
 
     if (check->has_format_alloc_info) {
-        dump_human_format_alloc_info(check->format_alloc_info, quiet);
+        dump_human_format_alloc_info(bs, check->format_alloc_info, quiet);
     }
 }
 
@@ -840,7 +857,7 @@ static int img_check(int argc, char **argv)
     if (!ret) {
         switch (output_format) {
         case OFORMAT_HUMAN:
-            dump_human_image_check(check, quiet);
+            dump_human_image_check(bs, check, quiet);
             break;
         case OFORMAT_JSON:
             dump_json_image_check(check, quiet);
-- 
2.11.1

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

[Eric Blake]

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

On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
> The function should collect statistics, about allocted/unallocated by
> top-level format driver space (in its .file) and allocation status
> (allocated/hole/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      | 26 ++++++++++++++++++++++++++
>  4 files changed, 47 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 50ba264143..7d720ae0c2 100644
> --- a/block.c

> +++ b/qapi/block-core.json
> @@ -139,6 +139,32 @@
>             '*format-specific': 'ImageInfoSpecific' } }
>  
>  ##
> +# @BlockFormatAllocInfo:
> +#
> +# Information about allocations, including metadata. All fields are in bytes.
> +#
> +# @alloc_alloc: allocated by format driver and allocated in underlying file
> +#

New structs should favor naming with '-' rather than '_'. So this should
be 'alloc-alloc', if we like the name.

So this statistic represents the portions of the format file that are in
use by the format, and which are backed by data in the underlying protocol.

> +# @alloc_hole: allocated by format driver but actually is a hole in
> +#              underlying file

The portions of the format file in use by the format, but where the
entire cluster is a hole in the underlying file (note that with cluster
size large enough, you can get a cluster that is part-data/part-hole in
the underlying file, it looks like you are counting those as data).

> +#
> +# @alloc_overhead: allocated by format driver after end of underlying file

The portions of the format file in use by the format, but where the
entire cluster is beyond the end of the underlying file (the effective
hole).  Do we really need to distinguish between hole within the
underlying file and hole beyond the end of the file? Or can this number
be combined with the previous?

> +#
> +# @hole_alloc: not allocated by format driver but allocated in underlying file

I'm not sure I like this name.  "Hole" in file-system terminology means
"reads-as-zero" - but here we are talking about portions of the format
layer that are unallocated (defer to backing file, and we can't
guarantee they read as zero unless there is no backing file).

This statistic represents the portion of the underlying file that has
been previously allocated to hold data, but which is now unused by the
format layer (in other words, leaked clusters that can be reclaimed, or
which can be reused instead of growing the underlying the file if new
clusters are allocated).

> +#
> +# @hole_hole: not allocated by format driver hole in underlying file

This statistic represents fragmentation - portions of the format layer
that are no longer in use, but which are also occupying no space in the
underlying file.  A refragmentation operation (if we ever implemented
one) could remove the address space used by these clusters, but would
not change the disk usage.

> +#
> +# Since: 2.10
> +#
> +##
> +{ 'struct': 'BlockFormatAllocInfo',
> +  'data': {'alloc_alloc':    'uint64',
> +           'alloc_hole':     'uint64',
> +           'alloc_overhead': 'uint64',
> +           'hole_alloc':     'uint64',
> +           'hole_hole':      'uint64' } }

The idea seems okay, but the naming needs to be fixed.  Also, I'm not
sure if we need all 5, or if 4 is enough; and I'm not sure if we have
the right names ("how does alloc-hole differ from hole-alloc?"), or if
we can come up with something more descriptive.  Particularly since
"hole-" is not a hole in the filesystem sense, so much as unused
clusters.  But I'm also not coming up with better names to suggest at
the moment.

-- 
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] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

30.05.2017 17:53, Eric Blake wrote:
> On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The function should collect statistics, about allocted/unallocated by
>> top-level format driver space (in its .file) and allocation status
>> (allocated/hole/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      | 26 ++++++++++++++++++++++++++
>>   4 files changed, 47 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 50ba264143..7d720ae0c2 100644
>> --- a/block.c
>> +++ b/qapi/block-core.json
>> @@ -139,6 +139,32 @@
>>              '*format-specific': 'ImageInfoSpecific' } }
>>   
>>   ##
>> +# @BlockFormatAllocInfo:
>> +#
>> +# Information about allocations, including metadata. All fields are in bytes.
>> +#
>> +# @alloc_alloc: allocated by format driver and allocated in underlying file
>> +#
> New structs should favor naming with '-' rather than '_'. So this should
> be 'alloc-alloc', if we like the name.
>
> So this statistic represents the portions of the format file that are in
> use by the format, and which are backed by data in the underlying protocol.
>
>> +# @alloc_hole: allocated by format driver but actually is a hole in
>> +#              underlying file
> The portions of the format file in use by the format, but where the
> entire cluster is a hole in the underlying file (note that with cluster
> size large enough, you can get a cluster that is part-data/part-hole in
> the underlying file, it looks like you are counting those as data).

No, I account on sector boundary on bs->file. So it is not cluster-aligned.

>
>> +#
>> +# @alloc_overhead: allocated by format driver after end of underlying file
> The portions of the format file in use by the format, but where the
> entire cluster is beyond the end of the underlying file (the effective
> hole).  Do we really need to distinguish between hole within the
> underlying file and hole beyond the end of the file? Or can this number
> be combined with the previous?

alloc_alloc + alloc_hole + hole_alloc + hole_hole = size of bs->file, I 
think this is good and clear.

these 4 stats describes bs->file usage in details (not on cluster 
boundary, but on sector, based on bs->file block status), alloc_overhead 
is separate.

>
>> +#
>> +# @hole_alloc: not allocated by format driver but allocated in underlying file
> I'm not sure I like this name.  "Hole" in file-system terminology means
> "reads-as-zero" - but here we are talking about portions of the format
> layer that are unallocated (defer to backing file, and we can't
> guarantee they read as zero unless there is no backing file).
>
> This statistic represents the portion of the underlying file that has
> been previously allocated to hold data, but which is now unused by the
> format layer (in other words, leaked clusters that can be reclaimed, or
> which can be reused instead of growing the underlying the file if new
> clusters are allocated).
>
>> +#
>> +# @hole_hole: not allocated by format driver hole in underlying file
> This statistic represents fragmentation - portions of the format layer
> that are no longer in use, but which are also occupying no space in the
> underlying file.  A refragmentation operation (if we ever implemented
> one) could remove the address space used by these clusters, but would
> not change the disk usage.
>
>> +#
>> +# Since: 2.10
>> +#
>> +##
>> +{ 'struct': 'BlockFormatAllocInfo',
>> +  'data': {'alloc_alloc':    'uint64',
>> +           'alloc_hole':     'uint64',
>> +           'alloc_overhead': 'uint64',
>> +           'hole_alloc':     'uint64',
>> +           'hole_hole':      'uint64' } }
> The idea seems okay, but the naming needs to be fixed.  Also, I'm not
> sure if we need all 5, or if 4 is enough; and I'm not sure if we have
> the right names ("how does alloc-hole differ from hole-alloc?"), or if
> we can come up with something more descriptive.  Particularly since
> "hole-" is not a hole in the filesystem sense, so much as unused
> clusters.  But I'm also not coming up with better names to suggest at
> the moment.
>
yes, naming is a problem. Proposed has the advantage of short names, and 
easy to use, if you have read documentation) But without reading docs, 
it is hard to distinguish, what do they mean..


-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 05/30/2017 10:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.05.2017 17:53, Eric Blake wrote:
>> On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about allocted/unallocated by
>>> top-level format driver space (in its .file) and allocation status
>>> (allocated/hole/after eof) of corresponding areas in this .file.
>>>

>>> +# @alloc_hole: allocated by format driver but actually is a hole in
>>> +#              underlying file
>> The portions of the format file in use by the format, but where the
>> entire cluster is a hole in the underlying file (note that with cluster
>> size large enough, you can get a cluster that is part-data/part-hole in
>> the underlying file, it looks like you are counting those as data).
> 
> No, I account on sector boundary on bs->file. So it is not cluster-aligned.

Okay, worth knowing (and therefore worth including in the documentation).

> 
>>
>>> +#
>>> +# @alloc_overhead: allocated by format driver after end of
>>> underlying file
>> The portions of the format file in use by the format, but where the
>> entire cluster is beyond the end of the underlying file (the effective
>> hole).  Do we really need to distinguish between hole within the
>> underlying file and hole beyond the end of the file? Or can this number
>> be combined with the previous?
> 
> alloc_alloc + alloc_hole + hole_alloc + hole_hole = size of bs->file, I
> think this is good and clear.

That relationship is worth including in the documentation.

> 
> these 4 stats describes bs->file usage in details (not on cluster
> boundary, but on sector, based on bs->file block status), alloc_overhead
> is separate.

I'd lean more towards alloc-overrun than alloc-overhead (we have
metadata overhead no matter what, but overrun does a nice job of
explaining offsets that are important to the format but which are not
present in the underlying protocol).


-- 
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] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

30.05.2017 17:53, Eric Blake wrote:
> On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The function should collect statistics, about allocted/unallocated by
>> top-level format driver space (in its .file) and allocation status
>> (allocated/hole/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      | 26 ++++++++++++++++++++++++++
>>   4 files changed, 47 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 50ba264143..7d720ae0c2 100644
>> --- a/block.c
>> +++ b/qapi/block-core.json
>> @@ -139,6 +139,32 @@
>>              '*format-specific': 'ImageInfoSpecific' } }
>>   
>>   ##
>> +# @BlockFormatAllocInfo:
>> +#
>> +# Information about allocations, including metadata. All fields are in bytes.

s/All fields are in bytes./All fields are in bytes and aligned by sector 
(512 bytes)./

- ok? to emphasize that there is nothing about clusters... Or may be 
better to write that they are aligned by byte.

>> +#
>> +# @alloc_alloc: allocated by format driver and allocated in underlying file
>> +#
> New structs should favor naming with '-' rather than '_'. So this should
> be 'alloc-alloc', if we like the name.
>
> So this statistic represents the portions of the format file that are in
> use by the format, and which are backed by data in the underlying protocol.
>
>> +# @alloc_hole: allocated by format driver but actually is a hole in
>> +#              underlying file
> The portions of the format file in use by the format, but where the
> entire cluster is a hole in the underlying file (note that with cluster
> size large enough, you can get a cluster that is part-data/part-hole in
> the underlying file, it looks like you are counting those as data).
>
>> +#
>> +# @alloc_overhead: allocated by format driver after end of underlying file
> The portions of the format file in use by the format, but where the
> entire cluster is beyond the end of the underlying file (the effective
> hole).  Do we really need to distinguish between hole within the
> underlying file and hole beyond the end of the file? Or can this number
> be combined with the previous?
>
>> +#
>> +# @hole_alloc: not allocated by format driver but allocated in underlying file
> I'm not sure I like this name.  "Hole" in file-system terminology means
> "reads-as-zero" - but here we are talking about portions of the format
> layer that are unallocated (defer to backing file, and we can't
> guarantee they read as zero unless there is no backing file).
>
> This statistic represents the portion of the underlying file that has
> been previously allocated to hold data, but which is now unused by the
> format layer (in other words, leaked clusters that can be reclaimed, or
> which can be reused instead of growing the underlying the file if new
> clusters are allocated).
>
>> +#
>> +# @hole_hole: not allocated by format driver hole in underlying file
> This statistic represents fragmentation - portions of the format layer
> that are no longer in use, but which are also occupying no space in the
> underlying file.  A refragmentation operation (if we ever implemented
> one) could remove the address space used by these clusters, but would
> not change the disk usage.
>
>> +#
>> +# Since: 2.10
>> +#
>> +##
>> +{ 'struct': 'BlockFormatAllocInfo',
>> +  'data': {'alloc_alloc':    'uint64',
>> +           'alloc_hole':     'uint64',
>> +           'alloc_overhead': 'uint64',
>> +           'hole_alloc':     'uint64',
>> +           'hole_hole':      'uint64' } }
> The idea seems okay, but the naming needs to be fixed.  Also, I'm not
> sure if we need all 5, or if 4 is enough; and I'm not sure if we have
> the right names ("how does alloc-hole differ from hole-alloc?"), or if
> we can come up with something more descriptive.  Particularly since
> "hole-" is not a hole in the filesystem sense, so much as unused
> clusters.  But I'm also not coming up with better names to suggest at
> the moment.
>
how about:

used-allocated
used-discarded
used-overrun

unused-allocated
unused-discarded


also, do you mention that your detailed wordings should be included into 
block-core.json or you just clarify things?


-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 06/02/2017 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.05.2017 17:53, Eric Blake wrote:
>> On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about allocted/unallocated by
>>> top-level format driver space (in its .file) and allocation status
>>> (allocated/hole/after eof) of corresponding areas in this .file.
>>>

>>> +# @BlockFormatAllocInfo:
>>> +#
>>> +# Information about allocations, including metadata. All fields are
>>> in bytes.
> 
> s/All fields are in bytes./All fields are in bytes and aligned by sector
> (512 bytes)./

I wouldn't even promise sector alignment. We probably happen to have
sector alignment (especially for qcow2, since the smallest cluster size
permitted is sector aligned), but I see no inherent reason why we can't
support sub-sector values if we are reporting in bytes.

> 
> - ok? to emphasize that there is nothing about clusters... Or may be
> better to write that they are aligned by byte.

I think "All fields are in bytes" is sufficient.


>>> +{ 'struct': 'BlockFormatAllocInfo',
>>> +  'data': {'alloc_alloc':    'uint64',
>>> +           'alloc_hole':     'uint64',
>>> +           'alloc_overhead': 'uint64',
>>> +           'hole_alloc':     'uint64',
>>> +           'hole_hole':      'uint64' } }
>> The idea seems okay, but the naming needs to be fixed.  Also, I'm not
>> sure if we need all 5, or if 4 is enough; and I'm not sure if we have
>> the right names ("how does alloc-hole differ from hole-alloc?"), or if
>> we can come up with something more descriptive.  Particularly since
>> "hole-" is not a hole in the filesystem sense, so much as unused
>> clusters.  But I'm also not coming up with better names to suggest at
>> the moment.
>>
> how about:
> 
> used-allocated
> used-discarded
> used-overrun
> 
> unused-allocated
> unused-discarded

Those work for me.

> 
> 
> also, do you mention that your detailed wordings should be included into
> block-core.json or you just clarify things?

Good documentation is worth the effort. I don't know if you want all of
my details in block-core.json, but giving a better overview of how each
statistic is possible does make it easier to visualize what the
statistic is actually counting.

-- 
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 --]