diff mbox

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

Message ID 1331112179-12726-1-git-send-email-wdongxu@linux.vnet.ibm.com
State New
Headers show

Commit Message

Robert Wang March 7, 2012, 9:22 a.m. UTC 
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(-)

Comments

Stefan Hajnoczi March 12, 2012, 1:07 p.m. UTC | #1 
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
Kevin Wolf March 12, 2012, 1:14 p.m. UTC | #2 
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
Stefan Hajnoczi March 12, 2012, 1:26 p.m. UTC | #3 
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
Kevin Wolf March 12, 2012, 1:36 p.m. UTC | #4 
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
diff mbox

Patch

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),