| Message ID | 1360090451-26543-5-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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 > > >
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
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()
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(-)