[Qemu-devel] [PATCH 1/4] block: add image fragmentation statistics to qemu-img

[Qemu-devel] [PATCH 1/4] block: add image fragmentation statistics to qemu-img

[Dong Xu Wang]

From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>

Discussion can be found at:
http://patchwork.ozlabs.org/patch/128730/

This patch add image fragmentation statistics while using qemu-img info.

Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
---
 block.c     |   13 +++++++++++++
 block.h     |    7 +++++++
 block_int.h |    1 +
 qemu-img.c  |    9 +++++++++
 4 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 52ffe14..947607b 100644
--- a/block.c
+++ b/block.c
@@ -2588,6 +2588,19 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return drv->bdrv_get_info(bs, bdi);
 }
 
+int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_get_fragment) {
+        return -ENOTSUP;
+    }
+    memset(bfi, 0, sizeof(*bfi));
+    return drv->bdrv_get_fragment(bs, bfi);
+}
+
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
diff --git a/block.h b/block.h
index 48d0bf3..d76d386 100644
--- a/block.h
+++ b/block.h
@@ -17,6 +17,12 @@ typedef struct BlockDriverInfo {
     int64_t vm_state_offset;
 } BlockDriverInfo;
 
+typedef struct BlockFragInfo {
+    uint64_t allocated_clusters;
+    uint64_t total_clusters;
+    uint64_t fragmented_clusters;
+} BlockFragInfo;
+
 typedef struct QEMUSnapshotInfo {
     char id_str[128]; /* unique snapshot id */
     /* the following fields are informative. They are not needed for
@@ -290,6 +296,7 @@ const char *bdrv_get_device_name(BlockDriverState *bs);
 int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
                           const uint8_t *buf, int nb_sectors);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
+int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi);
 
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
diff --git a/block_int.h b/block_int.h
index b460c36..339a5ac 100644
--- a/block_int.h
+++ b/block_int.h
@@ -179,6 +179,7 @@ struct BlockDriver {
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
                                   const char *snapshot_name);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
+    int (*bdrv_get_fragment)(BlockDriverState *bs, BlockFragInfo *bdi);
 
     int (*bdrv_save_vmstate)(BlockDriverState *bs, const uint8_t *buf,
                              int64_t pos, int size);
diff --git a/qemu-img.c b/qemu-img.c
index 8df3564..17731a9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1075,6 +1075,7 @@ static int img_info(int argc, char **argv)
     char backing_filename[1024];
     char backing_filename2[1024];
     BlockDriverInfo bdi;
+    BlockFragInfo bfi;
 
     fmt = NULL;
     for(;;) {
@@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv)
             printf("cluster_size: %d\n", bdi.cluster_size);
         }
     }
+    if (bdrv_get_fragment(bs, &bfi) >= 0) {
+        if (bfi.total_clusters != 0 && bfi.allocated_clusters != 0) {
+            printf("%lld/%lld = %0.2f%% allocated, %0.2f%% fragmented\n",
+                bfi.allocated_clusters, bfi.total_clusters,
+                bfi.allocated_clusters * 100.0 / bfi.total_clusters,
+                bfi.fragmented_clusters * 100.0 / bfi.allocated_clusters);
+        }
+    }
     bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
     if (backing_filename[0] != '\0') {
         path_combine(backing_filename2, sizeof(backing_filename2),
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/4] block: image fragmentation statistics for qed

[Dong Xu Wang]

From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>

Add fragmentation statistics for qed file format.

Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
---
 block/qed.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index a041d31..eb4dd90 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1444,6 +1444,46 @@ static int bdrv_qed_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
+static int bdrv_qed_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi)
+{
+    BDRVQEDState *s = bs->opaque;
+    uint32_t cluster_size = s->header.cluster_size;
+    uint64_t table_noffsets = s->header.table_size * cluster_size / sizeof(uint64_t);
+    uint64_t i, j;
+    uint64_t l2_offset;
+    int ret = 0;
+    uint64_t last_offset = 0;
+    uint64_t size = s->header.table_size * cluster_size;
+    uint64_t *table = qemu_blockalign(s->bs, size);
+
+    for (i = 0; i < table_noffsets; i++) {
+        l2_offset = s->l1_table->offsets[i];
+        if (l2_offset == 0) {
+            continue;
+        }
+        ret = bdrv_pread(bs->file, l2_offset, table, size);
+        if (ret < 0) {
+            qemu_vfree(table);
+            return ret;
+        }
+        for (j = 0; j < size/sizeof(uint64_t); j++) {
+            uint64_t *offset = (uint64_t *)(table + j);
+            if (*offset < cluster_size) {
+                continue;
+            }
+            bfi->allocated_clusters++;
+            if (last_offset && (last_offset + cluster_size) != *offset) {
+                bfi->fragmented_clusters++;
+            }
+            last_offset = *offset;
+        }
+    }
+    bfi->total_clusters = (s->header.image_size + s->header.cluster_size - 1) /
+                        s->header.cluster_size;
+    qemu_vfree(table);
+    return ret;
+}
+
 static int bdrv_qed_change_backing_file(BlockDriverState *bs,
                                         const char *backing_file,
                                         const char *backing_fmt)
@@ -1569,6 +1609,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_get_info            = bdrv_qed_get_info,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_check               = bdrv_qed_check,
+    .bdrv_get_fragment        = bdrv_qed_get_fragment,
 };
 
 static void bdrv_qed_init(void)
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/4 v2 RESEND] block: add dirty flag status to qemu-img

[Dong Xu Wang]

From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>

Some block drivers can verify their image files are clean or not. So we can show
it while using "qemu-img info.

Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
---
 block.c     |   14 ++++++++++++++
 block.h     |    2 ++
 block_int.h |    1 +
 qemu-img.c  |    3 +++
 4 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 947607b..17e9ba8 100644
--- a/block.c
+++ b/block.c
@@ -193,6 +193,20 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
     qemu_co_queue_next(&bs->throttled_reqs);
 }
 
+/* check if the image was cleanly shut down */
+bool bdrv_not_cleanly_down(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+
+    if (!drv) {
+        return 0;
+    }
+    if (!drv->bdrv_not_cleanly_down) {
+        return 0;
+    }
+    return drv->bdrv_not_cleanly_down(bs);
+}
+
 /* check if the path starts with "<protocol>:" */
 static int path_has_protocol(const char *path)
 {
diff --git a/block.h b/block.h
index d76d386..00dc2a5 100644
--- a/block.h
+++ b/block.h
@@ -110,6 +110,8 @@ void bdrv_io_limits_enable(BlockDriverState *bs);
 void bdrv_io_limits_disable(BlockDriverState *bs);
 bool bdrv_io_limits_enabled(BlockDriverState *bs);
 
+bool bdrv_not_cleanly_down(BlockDriverState *bs);
+
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
 BlockDriver *bdrv_find_protocol(const char *filename);
diff --git a/block_int.h b/block_int.h
index 339a5ac..e28787c 100644
--- a/block_int.h
+++ b/block_int.h
@@ -114,6 +114,7 @@ struct BlockDriver {
     int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
     int (*bdrv_make_empty)(BlockDriverState *bs);
+    bool (*bdrv_not_cleanly_down)(BlockDriverState *bs);
     /* aio */
     BlockDriverAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
diff --git a/qemu-img.c b/qemu-img.c
index 17731a9..c84527b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1122,6 +1122,9 @@ static int img_info(int argc, char **argv)
     if (bdrv_is_encrypted(bs)) {
         printf("encrypted: yes\n");
     }
+    if (bdrv_not_cleanly_down(bs)) {
+        printf("cleanly shut down: no\n");
+    }
     if (bdrv_get_info(bs, &bdi) >= 0) {
         if (bdi.cluster_size != 0) {
             printf("cluster_size: %d\n", bdi.cluster_size);
-- 
1.7.5.4

[Qemu-devel] [PATCH 4/4 v2 RESEND] block: track dirty flag status in qed

[Dong Xu Wang]

From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>

qed driver use QED_F_NEED_CHECK to mark if the image is clean.

Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
---
 block/qed.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index eb4dd90..d45d5c5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1563,6 +1563,12 @@ static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result)
     return qed_check(s, result, false);
 }
 
+static bool bdrv_qed_not_cleanly_down(BlockDriverState *bs)
+{
+    BDRVQEDState *s = bs->opaque;
+    return s->header.features & QED_F_NEED_CHECK;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1610,6 +1616,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_check               = bdrv_qed_check,
     .bdrv_get_fragment        = bdrv_qed_get_fragment,
+    .bdrv_not_cleanly_down    = bdrv_qed_not_cleanly_down,
 };
 
 static void bdrv_qed_init(void)
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 2/4] block: image fragmentation statistics for qed

[Stefan Hajnoczi]

On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote:
> +    for (i = 0; i < table_noffsets; i++) {
> +        l2_offset = s->l1_table->offsets[i];
> +        if (l2_offset == 0) {

if (qed_offset_is_unalloc_cluster(l2_offset)) {

> +            continue;
> +        }
> +        ret = bdrv_pread(bs->file, l2_offset, table, size);
> +        if (ret < 0) {
> +            qemu_vfree(table);
> +            return ret;
> +        }
> +        for (j = 0; j < size/sizeof(uint64_t); j++) {
> +            uint64_t *offset = (uint64_t *)(table + j);

No need to cast, table is already uint64_t*.

Since you don't write to the offset, I would just do:

uint64_t offset = table[j];

> +            if (*offset < cluster_size) {
> +                continue;
> +            }

if (!qed_check_cluster_offset(s, offset)) {
    continue;
}

We will skip unallocated clusters and zero clusters.

Stefan

Re: [Qemu-devel] [PATCH 1/4] block: add image fragmentation statistics to qemu-img

[Stefan Hajnoczi]

On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote:
> From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>
> Discussion can be found at:
> http://patchwork.ozlabs.org/patch/128730/
>
> This patch add image fragmentation statistics while using qemu-img info.
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>  block.c     |   13 +++++++++++++
>  block.h     |    7 +++++++
>  block_int.h |    1 +
>  qemu-img.c  |    9 +++++++++
>  4 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/block.c b/block.c
> index 52ffe14..947607b 100644
> --- a/block.c
> +++ b/block.c
> @@ -2588,6 +2588,19 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>     return drv->bdrv_get_info(bs, bdi);
>  }
>
> +int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi)

bdrv_get_fraginfo() makes it clearer that this function gets a summary
of fragmentation information.  bdrv_get_fragment() makes me think it
gets one specific "fragment".

> +{
> +    BlockDriver *drv = bs->drv;
> +    if (!drv) {
> +        return -ENOMEDIUM;
> +    }
> +    if (!drv->bdrv_get_fragment) {
> +        return -ENOTSUP;
> +    }
> +    memset(bfi, 0, sizeof(*bfi));
> +    return drv->bdrv_get_fragment(bs, bfi);

For now this .drv_get_fraginfo() interface makes sense but if we merge
the QCOW2<->QED in-place conversion patch series in the future it will
be possible to implement this in a generic way because image formats
will expose their guest -> host mapping.

> @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv)
>             printf("cluster_size: %d\n", bdi.cluster_size);
>         }
>     }
> +    if (bdrv_get_fragment(bs, &bfi) >= 0) {

I think we need a separate sub-command for fragmentation info:

qemu-img fraginfo <image-file>

Utilities that invoke qemu-img info want it to be fast.  Reading all
metadata from a large image can take several seconds.  Since many
qemu-img info users don't need to see the fragmentation information,
it makes sense to put it in a new sub-command.

> +        if (bfi.total_clusters != 0 && bfi.allocated_clusters != 0) {
> +            printf("%lld/%lld = %0.2f%% allocated, %0.2f%% fragmented\n",

Please use the PRId64 macro instead of %lld because some compilers
warn when uint64_t is not typedefed to long long int.

Stefan

Re: [Qemu-devel] [PATCH 1/4] block: add image fragmentation statistics to qemu-img

[Kevin Wolf]

Am 12.03.2012 14:07, schrieb Stefan Hajnoczi:
> On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote:
>> From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>
>> Discussion can be found at:
>> http://patchwork.ozlabs.org/patch/128730/
>>
>> This patch add image fragmentation statistics while using qemu-img info.
>>
>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>> ---
>>  block.c     |   13 +++++++++++++
>>  block.h     |    7 +++++++
>>  block_int.h |    1 +
>>  qemu-img.c  |    9 +++++++++
>>  4 files changed, 30 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 52ffe14..947607b 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2588,6 +2588,19 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>>     return drv->bdrv_get_info(bs, bdi);
>>  }
>>
>> +int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi)
> 
> bdrv_get_fraginfo() makes it clearer that this function gets a summary
> of fragmentation information.  bdrv_get_fragment() makes me think it
> gets one specific "fragment".
> 
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    if (!drv) {
>> +        return -ENOMEDIUM;
>> +    }
>> +    if (!drv->bdrv_get_fragment) {
>> +        return -ENOTSUP;
>> +    }
>> +    memset(bfi, 0, sizeof(*bfi));
>> +    return drv->bdrv_get_fragment(bs, bfi);
> 
> For now this .drv_get_fraginfo() interface makes sense but if we merge
> the QCOW2<->QED in-place conversion patch series in the future it will
> be possible to implement this in a generic way because image formats
> will expose their guest -> host mapping.

We can probably resurrect Devin's patches for this part even now instead
of introducing an interface that we don't really want.

> 
>> @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv)
>>             printf("cluster_size: %d\n", bdi.cluster_size);
>>         }
>>     }
>> +    if (bdrv_get_fragment(bs, &bfi) >= 0) {
> 
> I think we need a separate sub-command for fragmentation info:
> 
> qemu-img fraginfo <image-file>
> 
> Utilities that invoke qemu-img info want it to be fast.  Reading all
> metadata from a large image can take several seconds.  Since many
> qemu-img info users don't need to see the fragmentation information,
> it makes sense to put it in a new sub-command.

Yes. If we wanted to merge it into an existing qemu-img subcommand, I
think check would be the one, as it scans the whole image already today
and fragmentation is something that could be added fairly easily.

Kevin

Re: [Qemu-devel] [PATCH 1/4] block: add image fragmentation statistics to qemu-img

[Stefan Hajnoczi]

On Mon, Mar 12, 2012 at 1:14 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 12.03.2012 14:07, schrieb Stefan Hajnoczi:
>> On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote:
>>> @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv)
>>>             printf("cluster_size: %d\n", bdi.cluster_size);
>>>         }
>>>     }
>>> +    if (bdrv_get_fragment(bs, &bfi) >= 0) {
>>
>> I think we need a separate sub-command for fragmentation info:
>>
>> qemu-img fraginfo <image-file>
>>
>> Utilities that invoke qemu-img info want it to be fast.  Reading all
>> metadata from a large image can take several seconds.  Since many
>> qemu-img info users don't need to see the fragmentation information,
>> it makes sense to put it in a new sub-command.
>
> Yes. If we wanted to merge it into an existing qemu-img subcommand, I
> think check would be the one, as it scans the whole image already today
> and fragmentation is something that could be added fairly easily.

In that case we might not even need a separate interface/struct.  This
would just be part of check.

Does that sound good?

Stefan

Re: [Qemu-devel] [PATCH 1/4] block: add image fragmentation statistics to qemu-img

[Kevin Wolf]

Am 12.03.2012 14:26, schrieb Stefan Hajnoczi:
> On Mon, Mar 12, 2012 at 1:14 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 12.03.2012 14:07, schrieb Stefan Hajnoczi:
>>> On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote:
>>>> @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv)
>>>>             printf("cluster_size: %d\n", bdi.cluster_size);
>>>>         }
>>>>     }
>>>> +    if (bdrv_get_fragment(bs, &bfi) >= 0) {
>>>
>>> I think we need a separate sub-command for fragmentation info:
>>>
>>> qemu-img fraginfo <image-file>
>>>
>>> Utilities that invoke qemu-img info want it to be fast.  Reading all
>>> metadata from a large image can take several seconds.  Since many
>>> qemu-img info users don't need to see the fragmentation information,
>>> it makes sense to put it in a new sub-command.
>>
>> Yes. If we wanted to merge it into an existing qemu-img subcommand, I
>> think check would be the one, as it scans the whole image already today
>> and fragmentation is something that could be added fairly easily.
> 
> In that case we might not even need a separate interface/struct.  This
> would just be part of check.
> 
> Does that sound good?

Sure, that would be the only way to take advantage of the scan that
bdrv_check already performs.

Kevin

Re: [Qemu-devel] [PATCH 3/4 v2 RESEND] block: add dirty flag status to qemu-img

[Stefan Hajnoczi]

On Wed, Mar 07, 2012 at 05:22:58PM +0800, Dong Xu Wang wrote:
> From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> 
> Some block drivers can verify their image files are clean or not. So we can show
> it while using "qemu-img info.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>  block.c     |   14 ++++++++++++++
>  block.h     |    2 ++
>  block_int.h |    1 +
>  qemu-img.c  |    3 +++
>  4 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 947607b..17e9ba8 100644
> --- a/block.c
> +++ b/block.c
> @@ -193,6 +193,20 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
>      qemu_co_queue_next(&bs->throttled_reqs);
>  }
>  
> +/* check if the image was cleanly shut down */
> +bool bdrv_not_cleanly_down(BlockDriverState *bs)

The name is a little cryptic to me and I suggest avoiding 'not' in
function names because it easily leads to double-negatives (!not_foo()).

How about:

bool bdrv_was_shutdown_cleanly()

if (!bdrv_was_shutdown_cleanly(bs)) {
    printf(...);
}

This patch and the QED patch look fine otherwise.

Stefan

Re: [Qemu-devel] [PATCH 3/4 v2 RESEND] block: add dirty flag status to qemu-img

[Kevin Wolf]

Am 12.03.2012 19:18, schrieb Stefan Hajnoczi:
> On Wed, Mar 07, 2012 at 05:22:58PM +0800, Dong Xu Wang wrote:
>> From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>
>> Some block drivers can verify their image files are clean or not. So we can show
>> it while using "qemu-img info.
>>
>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>> ---
>>  block.c     |   14 ++++++++++++++
>>  block.h     |    2 ++
>>  block_int.h |    1 +
>>  qemu-img.c  |    3 +++
>>  4 files changed, 20 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 947607b..17e9ba8 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -193,6 +193,20 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
>>      qemu_co_queue_next(&bs->throttled_reqs);
>>  }
>>  
>> +/* check if the image was cleanly shut down */
>> +bool bdrv_not_cleanly_down(BlockDriverState *bs)
> 
> The name is a little cryptic to me and I suggest avoiding 'not' in
> function names because it easily leads to double-negatives (!not_foo()).
> 
> How about:
> 
> bool bdrv_was_shutdown_cleanly()
> 
> if (!bdrv_was_shutdown_cleanly(bs)) {
>     printf(...);
> }
> 
> This patch and the QED patch look fine otherwise.

Should we rather add a new field to BlockDriverInfo and use the existing
bdrv_get_info() function?

Kevin

Re: [Qemu-devel] [PATCH 3/4 v2 RESEND] block: add dirty flag status to qemu-img

[Stefan Hajnoczi]

On Tue, Mar 13, 2012 at 10:03:51AM +0100, Kevin Wolf wrote:
> Am 12.03.2012 19:18, schrieb Stefan Hajnoczi:
> > On Wed, Mar 07, 2012 at 05:22:58PM +0800, Dong Xu Wang wrote:
> >> From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> >>
> >> Some block drivers can verify their image files are clean or not. So we can show
> >> it while using "qemu-img info.
> >>
> >> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> >> ---
> >>  block.c     |   14 ++++++++++++++
> >>  block.h     |    2 ++
> >>  block_int.h |    1 +
> >>  qemu-img.c  |    3 +++
> >>  4 files changed, 20 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 947607b..17e9ba8 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -193,6 +193,20 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
> >>      qemu_co_queue_next(&bs->throttled_reqs);
> >>  }
> >>  
> >> +/* check if the image was cleanly shut down */
> >> +bool bdrv_not_cleanly_down(BlockDriverState *bs)
> > 
> > The name is a little cryptic to me and I suggest avoiding 'not' in
> > function names because it easily leads to double-negatives (!not_foo()).
> > 
> > How about:
> > 
> > bool bdrv_was_shutdown_cleanly()
> > 
> > if (!bdrv_was_shutdown_cleanly(bs)) {
> >     printf(...);
> > }
> > 
> > This patch and the QED patch look fine otherwise.
> 
> Should we rather add a new field to BlockDriverInfo and use the existing
> bdrv_get_info() function?

Yeah, that sounds good.  In that case it's best to make the "clean"
value false and the "dirty" value true, so that block drivers that don't
care about this feature automatically report "clean".

struct BlockDriverInfo {
    bool is_dirty;
}

Stefan

Re: [Qemu-devel] [PATCH 3/4 v2 RESEND] block: add dirty flag status to qemu-img

[Kevin Wolf]

Am 13.03.2012 10:33, schrieb Stefan Hajnoczi:
> On Tue, Mar 13, 2012 at 10:03:51AM +0100, Kevin Wolf wrote:
>> Am 12.03.2012 19:18, schrieb Stefan Hajnoczi:
>>> On Wed, Mar 07, 2012 at 05:22:58PM +0800, Dong Xu Wang wrote:
>>>> From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>>>
>>>> Some block drivers can verify their image files are clean or not. So we can show
>>>> it while using "qemu-img info.
>>>>
>>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>>> ---
>>>>  block.c     |   14 ++++++++++++++
>>>>  block.h     |    2 ++
>>>>  block_int.h |    1 +
>>>>  qemu-img.c  |    3 +++
>>>>  4 files changed, 20 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 947607b..17e9ba8 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -193,6 +193,20 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
>>>>      qemu_co_queue_next(&bs->throttled_reqs);
>>>>  }
>>>>  
>>>> +/* check if the image was cleanly shut down */
>>>> +bool bdrv_not_cleanly_down(BlockDriverState *bs)
>>>
>>> The name is a little cryptic to me and I suggest avoiding 'not' in
>>> function names because it easily leads to double-negatives (!not_foo()).
>>>
>>> How about:
>>>
>>> bool bdrv_was_shutdown_cleanly()
>>>
>>> if (!bdrv_was_shutdown_cleanly(bs)) {
>>>     printf(...);
>>> }
>>>
>>> This patch and the QED patch look fine otherwise.
>>
>> Should we rather add a new field to BlockDriverInfo and use the existing
>> bdrv_get_info() function?
> 
> Yeah, that sounds good.  In that case it's best to make the "clean"
> value false and the "dirty" value true, so that block drivers that don't
> care about this feature automatically report "clean".
> 
> struct BlockDriverInfo {
>     bool is_dirty;
> }

Yes, I agree.

Kevin