Patchwork [4/5] qemu-img: add compressed clusters to BlockFragInfo

login
register
mail settings
Submitter Stefan Hajnoczi
Date Feb. 5, 2013, 6:54 p.m.
Message ID <1360090451-26543-5-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/218322/
State New
Headers show

Comments

Stefan Hajnoczi - Feb. 5, 2013, 6:54 p.m.
Show how many clusters are compressed.  This can be used to monitor how
many compressed clusters remain and whether to recompress the image.

Suggested-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/block.h        | 1 +
 qemu-img.c                   | 5 +++--
 tests/qemu-iotests/026       | 6 +++---
 tests/qemu-iotests/036       | 2 +-
 tests/qemu-iotests/039       | 2 +-
 tests/qemu-iotests/044.out   | 1 +
 tests/qemu-iotests/common.rc | 6 +++---
 7 files changed, 13 insertions(+), 10 deletions(-)
Eric Blake - Feb. 5, 2013, 10:27 p.m.
On 02/05/2013 11:54 AM, Stefan Hajnoczi wrote:
> Show how many clusters are compressed.  This can be used to monitor how
> many compressed clusters remain and whether to recompress the image.
> 
> Suggested-by: Cole Robinson <crobinso@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

> +++ b/qemu-img.c
> @@ -471,10 +471,11 @@ static int img_check(int argc, char **argv)
>  
>      if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
>          printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
> -               "%0.2f%% fragmented\n",
> +               "%0.2f%% fragmented, %0.2f%% compressed\n",

That might be misleading output.  Stating 90% fragmented makes sense (90
percent of my allocations are disjoint), but stating 90% compressed has
two possible interpretations (of the unknown number of clusters that
were compressed, I got an impressive 90% compression ratio; vs. 90% of
the clusters are compressed, but no idea what compression ratio that
actually gave me).  Obviously, you meant the latter interpretation (we
aren't telling the percentage of saved disk space due to compression,
merely how much of the disk has been compressed).  Maybe "%0.2f%% of
clusters use compression" would more accurately express things, but it
ends up being longer.  Any other thoughts on avoiding an ambiguity?

> +++ b/tests/qemu-iotests/common.rc
> @@ -161,9 +161,9 @@ _cleanup_test_img()
>  
>  _check_test_img()
>  {
> -    $QEMU_IMG check -f $IMGFMT $TEST_IMG 2>&1 | \
> -        grep -v "fragmented$" | \
> -    	sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'
> +    $QEMU_IMG check "$@" -f $IMGFMT $TEST_IMG 2>&1 | \
> +        grep -v -e "compressed$" | \

Did you really intend to lose the filtering of 'fragmented$'?

> +        sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'

I think I've mentioned this on other pending patches that touch this
same function...
/me goes looking
https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg05364.html

'grep -v | sed' is slightly inefficient compared to:

sed -e '/compressed$/d' \
    -e 's/qemu-img:.../'

\: is not portable sed, so we should be fixing it up as long as we are
touching this.
Stefan Hajnoczi - Feb. 6, 2013, 10:09 a.m.
On Tue, Feb 05, 2013 at 03:27:10PM -0700, Eric Blake wrote:
> On 02/05/2013 11:54 AM, Stefan Hajnoczi wrote:
> > Show how many clusters are compressed.  This can be used to monitor how
> > many compressed clusters remain and whether to recompress the image.
> > 
> > Suggested-by: Cole Robinson <crobinso@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> 
> > +++ b/qemu-img.c
> > @@ -471,10 +471,11 @@ static int img_check(int argc, char **argv)
> >  
> >      if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
> >          printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
> > -               "%0.2f%% fragmented\n",
> > +               "%0.2f%% fragmented, %0.2f%% compressed\n",
> 
> That might be misleading output.  Stating 90% fragmented makes sense (90
> percent of my allocations are disjoint), but stating 90% compressed has
> two possible interpretations (of the unknown number of clusters that
> were compressed, I got an impressive 90% compression ratio; vs. 90% of
> the clusters are compressed, but no idea what compression ratio that
> actually gave me).  Obviously, you meant the latter interpretation (we
> aren't telling the percentage of saved disk space due to compression,
> merely how much of the disk has been compressed).  Maybe "%0.2f%% of
> clusters use compression" would more accurately express things, but it
> ends up being longer.  Any other thoughts on avoiding an ambiguity?

Let's change it to "%0.2f%% compressed clusters".  I think that
indicates we're talking about the percentage of clusters that are
compressed, not the compression ratio.

> > +++ b/tests/qemu-iotests/common.rc
> > @@ -161,9 +161,9 @@ _cleanup_test_img()
> >  
> >  _check_test_img()
> >  {
> > -    $QEMU_IMG check -f $IMGFMT $TEST_IMG 2>&1 | \
> > -        grep -v "fragmented$" | \
> > -    	sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'
> > +    $QEMU_IMG check "$@" -f $IMGFMT $TEST_IMG 2>&1 | \
> > +        grep -v -e "compressed$" | \
> 
> Did you really intend to lose the filtering of 'fragmented$'?

Yes.

The '$' is the hint - this command filters out the entire fragmentation
info line.  But this regex no longer matches once we added the
compressed percentage to the output.  We now match against 'compressed$'
at end-of-line.

I have replaced it with a longer regex that matches "allocated",
"fragmented", and "compressed".  This way it's clear we're trying to
filter out the entire fragmentation info line.

> > +        sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'
> 
> I think I've mentioned this on other pending patches that touch this
> same function...
> /me goes looking
> https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg05364.html
> 
> 'grep -v | sed' is slightly inefficient compared to:
> 
> sed -e '/compressed$/d' \
>     -e 's/qemu-img:.../'
> 
> \: is not portable sed, so we should be fixing it up as long as we are
> touching this.

I only converted the tab to 4 spaces on this line but I'm happy to
squash the grep and sed invocations into a single sed invocation.

Stefan
Kevin Wolf - Feb. 6, 2013, 11:43 a.m.
Am 05.02.2013 19:54, schrieb Stefan Hajnoczi:
> Show how many clusters are compressed.  This can be used to monitor how
> many compressed clusters remain and whether to recompress the image.
> 
> Suggested-by: Cole Robinson <crobinso@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/qemu-img.c b/qemu-img.c
> index 167d65f..a06456b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -471,10 +471,11 @@ static int img_check(int argc, char **argv)
>  
>      if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
>          printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
> -               "%0.2f%% fragmented\n",
> +               "%0.2f%% fragmented, %0.2f%% compressed\n",
>                 bfi->allocated_clusters, bfi->total_clusters,
>                 bfi->allocated_clusters * 100.0 / bfi->total_clusters,
> -               bfi->fragmented_clusters * 100.0 / bfi->allocated_clusters);
> +               bfi->fragmented_clusters * 100.0 / bfi->allocated_clusters,
> +               bfi->compressed_clusters * 100.0 / bfi->allocated_clusters);
>      }

Having the output in this if block assumes that all block drivers either
have both fragmentation and compression statistics or neither. Wouldn't
it make more sense to check compressed_clusters separately and not print
anything on non-qcow2 (and possibly even uncompressed qcow2) images?

Instead of getting a longer and longer output line for each new number
we add, maybe we could use the chance to introduce multiline output:

Total number of clusters:   1000
- allocated                  427 (42.7%)
- fragmented                  71 (7.1%)
- compressed                 120 (12.0%)

Or something like that. And if a line doesn't apply to the given image
(format), it's just left out.

Kevin
Eric Blake - Feb. 6, 2013, 1:40 p.m.
On 02/06/2013 04:43 AM, Kevin Wolf wrote:
> 
> Instead of getting a longer and longer output line for each new number
> we add, maybe we could use the chance to introduce multiline output:
> 
> Total number of clusters:   1000
> - allocated                  427 (42.7%)
> - fragmented                  71 (7.1%)
> - compressed                 120 (12.0%)

That's a bit nicer for scraping the output, but the ultimate niceness
would be providing an --output=json representation, where the JSON
objects are only output when present.

> 
> Or something like that. And if a line doesn't apply to the given image
> (format), it's just left out.
> 
> Kevin
> 
> 
>
Kevin Wolf - Feb. 6, 2013, 1:48 p.m.
Am 06.02.2013 14:40, schrieb Eric Blake:
> On 02/06/2013 04:43 AM, Kevin Wolf wrote:
>>
>> Instead of getting a longer and longer output line for each new number
>> we add, maybe we could use the chance to introduce multiline output:
>>
>> Total number of clusters:   1000
>> - allocated                  427 (42.7%)
>> - fragmented                  71 (7.1%)
>> - compressed                 120 (12.0%)
> 
> That's a bit nicer for scraping the output, but the ultimate niceness
> would be providing an --output=json representation, where the JSON
> objects are only output when present.

Er yes, but this was meant for humans, not for parsers... Don't scrape
output without at least telling us before, or you'll get to keep the pieces.

Kevin

Patch

diff --git a/include/block/block.h b/include/block/block.h
index 5c3b911..24db852 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -24,6 +24,7 @@  typedef struct BlockFragInfo {
     uint64_t allocated_clusters;
     uint64_t total_clusters;
     uint64_t fragmented_clusters;
+    uint64_t compressed_clusters;
 } BlockFragInfo;
 
 typedef struct QEMUSnapshotInfo {
diff --git a/qemu-img.c b/qemu-img.c
index 167d65f..a06456b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -471,10 +471,11 @@  static int img_check(int argc, char **argv)
 
     if (bfi->total_clusters != 0 && bfi->allocated_clusters != 0) {
         printf("%" PRId64 "/%" PRId64 "= %0.2f%% allocated, "
-               "%0.2f%% fragmented\n",
+               "%0.2f%% fragmented, %0.2f%% compressed\n",
                bfi->allocated_clusters, bfi->total_clusters,
                bfi->allocated_clusters * 100.0 / bfi->total_clusters,
-               bfi->fragmented_clusters * 100.0 / bfi->allocated_clusters);
+               bfi->fragmented_clusters * 100.0 / bfi->allocated_clusters,
+               bfi->compressed_clusters * 100.0 / bfi->allocated_clusters);
     }
 
     bdrv_delete(bs);
diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index 1602ccd..32c6d32 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -102,7 +102,7 @@  if [ "$event" == "l2_load" ]; then
     $QEMU_IO -c "read $vmstate 0 128k " $BLKDBG_TEST_IMG | _filter_qemu_io
 fi
 
-$QEMU_IMG check $TEST_IMG 2>&1 | grep -v "refcount=1 reference=0"
+_check_test_img | grep -v "refcount=1 reference=0"
 
 done
 done
@@ -147,7 +147,7 @@  echo
 echo "Event: $event; errno: $errno; imm: $imm; once: $once; write $vmstate"
 $QEMU_IO -c "write $vmstate 0 64M" $BLKDBG_TEST_IMG | _filter_qemu_io
 
-$QEMU_IMG check $TEST_IMG 2>&1 | grep -v "refcount=1 reference=0"
+_check_test_img | grep -v "refcount=1 reference=0"
 
 done
 done
@@ -186,7 +186,7 @@  echo
 echo "Event: $event; errno: $errno; imm: $imm; once: $once"
 $QEMU_IO -c "write -b 0 64k" $BLKDBG_TEST_IMG | _filter_qemu_io
 
-$QEMU_IMG check $TEST_IMG 2>&1 | grep -v "refcount=1 reference=0"
+_check_test_img | grep -v "refcount=1 reference=0"
 
 done
 done
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index 329533e..3c76d89 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -59,7 +59,7 @@  _make_test_img 64M
 echo
 echo === Repair image ===
 echo
-$QEMU_IMG check -r all $TEST_IMG
+_check_test_img -r all
 ./qcow2.py $TEST_IMG dump-header
 
 # success, all done
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index c5ae806..ae35175 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -86,7 +86,7 @@  $QEMU_IO -r -c "read -P 0x5a 0 512" $TEST_IMG | _filter_qemu_io
 echo
 echo "== Repairing the image file must succeed =="
 
-$QEMU_IMG check -r all $TEST_IMG
+_check_test_img -r all
 
 # The dirty bit must not be set
 ./qcow2.py $TEST_IMG dump-header | grep incompatible_features
diff --git a/tests/qemu-iotests/044.out b/tests/qemu-iotests/044.out
index 7a40071..875df0c 100644
--- a/tests/qemu-iotests/044.out
+++ b/tests/qemu-iotests/044.out
@@ -1,4 +1,5 @@ 
 No errors were found on the image.
+7292415/8391499= 86.90% allocated, 0.00% fragmented, 0.00% compressed
 .
 ----------------------------------------------------------------------
 Ran 1 tests
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index aef5f52..1c09e73 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -161,9 +161,9 @@  _cleanup_test_img()
 
 _check_test_img()
 {
-    $QEMU_IMG check -f $IMGFMT $TEST_IMG 2>&1 | \
-        grep -v "fragmented$" | \
-    	sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'
+    $QEMU_IMG check "$@" -f $IMGFMT $TEST_IMG 2>&1 | \
+        grep -v -e "compressed$" | \
+        sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'
 }
 
 _img_info()