Re: [Qemu-block] [Qemu-devel] [PATCH] block/qcow2-bitmap: fix use of uninitialized pointer

[Eric Blake]

On 09/22/2017 09:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> Without initialization to zero dirty_bitmap field may be not zero
> for a bitmap which should not be stored and
> qcow2_store_persistent_dirty_bitmaps will erroneously call
> store_bitmap for it which leads to SYGSEGV on bdrv_dirty_bitmap_name.

s/SYG/SIG/

Introduced in commit 5f72826e, therefore it impacts 2.10, so:

CC: qemu-sta...@nongnu.org

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/qcow2-bitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 13/34] hw/ide: remove old i386 dependency

[John Snow]

On 09/22/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
> and remove a duplicated include
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: John Snow 

Re: [Qemu-block] [PATCH 03/34] block: remove "qemu/osdep.h" from header file

[Peter Maydell]

On 22 September 2017 at 16:39, Philippe Mathieu-Daudé  wrote:
> applied using ./scripts/clean-includes
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block/dmg.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/block/dmg.h b/block/dmg.h
> index b592d6fa8b..2ecf239ba5 100644
> --- a/block/dmg.h
> +++ b/block/dmg.h
> @@ -26,7 +26,6 @@
>  #ifndef BLOCK_DMG_H
>  #define BLOCK_DMG_H
>
> -#include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "block/block_int.h"
>  #include 
> --

Reviewed-by: Peter Maydell 

thanks
-- PMM

[Qemu-block] [PATCH 13/34] hw/ide: remove old i386 dependency

[Philippe Mathieu-Daudé]

and remove a duplicated include

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/ahci.c   | 1 -
 hw/ide/cmd646.c | 1 -
 hw/ide/core.c   | 3 +--
 hw/ide/ich.c| 1 -
 hw/ide/isa.c| 1 -
 hw/ide/microdrive.c | 1 -
 hw/ide/pci.c| 1 -
 hw/ide/piix.c   | 2 +-
 hw/ide/via.c| 1 -
 9 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 32d1296a64..2634f8db3d 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -24,7 +24,6 @@
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "hw/pci/msi.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 
 #include "qemu/error-report.h"
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 86b2a8f504..65aff518ec 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -24,7 +24,6 @@
  */
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "sysemu/block-backend.h"
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d63eb4a72e..6438efa8f4 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -24,17 +24,16 @@
  */
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/blockdev.h"
 #include "sysemu/dma.h"
 #include "hw/block/block.h"
 #include "sysemu/block-backend.h"
 #include "qemu/cutils.h"
-#include "qemu/error-report.h"
 
 #include "hw/ide/internal.h"
 #include "trace.h"
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 9472a60cab..48513c7e9d 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -63,7 +63,6 @@
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "hw/pci/msi.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "sysemu/block-backend.h"
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 40213d662c..9fb24fc92b 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -24,7 +24,6 @@
  */
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "hw/i386/pc.h"
 #include "hw/isa/isa.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/dma.h"
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index 17917c0b30..fde4d4645e 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -24,7 +24,6 @@
  */
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "hw/i386/pc.h"
 #include "hw/pcmcia.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/dma.h"
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index f2dcc0ed77..34237c92fc 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -24,7 +24,6 @@
  */
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "sysemu/block-backend.h"
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index dfb21f65fa..a3afe1fd29 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -25,11 +25,11 @@
 
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/blockdev.h"
 #include "sysemu/dma.h"
 
 #include "hw/ide/pci.h"
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 35c3059325..117ac4d95e 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -25,7 +25,6 @@
  */
 #include "qemu/osdep.h"
 #include "hw/hw.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "sysemu/block-backend.h"
-- 
2.14.1

[Qemu-block] [PATCH 03/34] block: remove "qemu/osdep.h" from header file

[Philippe Mathieu-Daudé]

applied using ./scripts/clean-includes

Signed-off-by: Philippe Mathieu-Daudé 
---
 block/dmg.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/dmg.h b/block/dmg.h
index b592d6fa8b..2ecf239ba5 100644
--- a/block/dmg.h
+++ b/block/dmg.h
@@ -26,7 +26,6 @@
 #ifndef BLOCK_DMG_H
 #define BLOCK_DMG_H
 
-#include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "block/block_int.h"
 #include 
-- 
2.14.1

[Qemu-block] [PATCH] block/qcow2-bitmap: fix use of uninitialized pointer

[Vladimir Sementsov-Ogievskiy]

Without initialization to zero dirty_bitmap field may be not zero
for a bitmap which should not be stored and
qcow2_store_persistent_dirty_bitmaps will erroneously call
store_bitmap for it which leads to SYGSEGV on bdrv_dirty_bitmap_name.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index e8d3bdbd6e..14f41d0427 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -602,7 +602,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState 
*bs, uint64_t offset,
 goto fail;
 }
 
-bm = g_new(Qcow2Bitmap, 1);
+bm = g_new0(Qcow2Bitmap, 1);
 bm->table.offset = e->bitmap_table_offset;
 bm->table.size = e->bitmap_table_size;
 bm->flags = e->flags;
-- 
2.11.1

Re: [Qemu-block] [PATCH v2 1/6] qemu-io: Drop write permissions before read-only reopen

[Fam Zheng]

On Fri, 09/22 14:55, Kevin Wolf wrote:
> qemu-io provides a 'reopen' command that allows switching from writable
> to read-only access. We need to make sure that we don't try to keep
> write permissions to a BlockBackend that becomes read-only, otherwise
> things are going to fail.
> 
> This requires a bdrv_drain() call because otherwise in-flight AIO
> write requests could issue new internal requests while the permission
> has already gone away, which would cause assertion failures. Draining
> the queue doesn't break AIO requests in any new way, bdrv_reopen() would
> drain it anyway only a few lines later.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-io-cmds.c | 12 
>  tests/qemu-iotests/187.out |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 2811a89099..3727fb43f3 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -2010,6 +2010,18 @@ static int reopen_f(BlockBackend *blk, int argc, char 
> **argv)
>  return 0;
>  }
>  
> +if (!(flags & BDRV_O_RDWR)) {
> +uint64_t orig_perm, orig_shared_perm;
> +
> +bdrv_drain(bs);
> +
> +blk_get_perm(blk, _perm, _shared_perm);
> +blk_set_perm(blk,
> + orig_perm & ~(BLK_PERM_WRITE | 
> BLK_PERM_WRITE_UNCHANGED),
> + orig_shared_perm,
> + _abort);
> +}
> +
>  qopts = qemu_opts_find(_opts, NULL);
>  opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
>  qemu_opts_reset(_opts);
> diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
> index 68fb944cd5..30b987f71f 100644
> --- a/tests/qemu-iotests/187.out
> +++ b/tests/qemu-iotests/187.out
> @@ -12,7 +12,7 @@ Start from read-write
>  
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -write failed: Operation not permitted
> +Block node is read-only
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  *** done
> -- 
> 2.13.5
> 

Reviewed-by: Fam Zheng 

[Qemu-block] [PATCH v2 1/6] qemu-io: Drop write permissions before read-only reopen

[Kevin Wolf]

qemu-io provides a 'reopen' command that allows switching from writable
to read-only access. We need to make sure that we don't try to keep
write permissions to a BlockBackend that becomes read-only, otherwise
things are going to fail.

This requires a bdrv_drain() call because otherwise in-flight AIO
write requests could issue new internal requests while the permission
has already gone away, which would cause assertion failures. Draining
the queue doesn't break AIO requests in any new way, bdrv_reopen() would
drain it anyway only a few lines later.

Signed-off-by: Kevin Wolf 
---
 qemu-io-cmds.c | 12 
 tests/qemu-iotests/187.out |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2811a89099..3727fb43f3 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2010,6 +2010,18 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 return 0;
 }
 
+if (!(flags & BDRV_O_RDWR)) {
+uint64_t orig_perm, orig_shared_perm;
+
+bdrv_drain(bs);
+
+blk_get_perm(blk, _perm, _shared_perm);
+blk_set_perm(blk,
+ orig_perm & ~(BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED),
+ orig_shared_perm,
+ _abort);
+}
+
 qopts = qemu_opts_find(_opts, NULL);
 opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
 qemu_opts_reset(_opts);
diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
index 68fb944cd5..30b987f71f 100644
--- a/tests/qemu-iotests/187.out
+++ b/tests/qemu-iotests/187.out
@@ -12,7 +12,7 @@ Start from read-write
 
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-write failed: Operation not permitted
+Block node is read-only
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.13.5

Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Fam Zheng]

On Fri, 09/22 13:03, Kevin Wolf wrote:
> Am 22.09.2017 um 04:30 hat Fam Zheng geschrieben:
> > On Thu, 09/21 18:39, Manos Pitsidianakis wrote:
> > > On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
> > > > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> > > It might imply to someone that there's an assert(drv->bdrv_co_drain_begin 
> > > &&
> > > drv->bdrv_co_drain_end) somewhere unless you state they don't have to be
> > > implemented at the same time. How about we be completely explicit:
> > > 
> > >  bdrv_co_drain_begin is called if implemented in the beggining of a
> > >  drain operation to drain and stop any internal sources of requests in
> > >  the driver.
> > >  bdrv_co_drain_end is called if implemented at the end of the drain.
> > > 
> > >  They should be used by the driver to e.g. manage scheduled I/O
> > >  requests, or toggle an internal state. After the end of the drain new
> > >  requests will continue normally.
> > > 
> > > I hope this is easier for a reader to understand!
> > 
> > I don't like the inconsistent semantics of when the drained section
> > ends, if we allow drivers to implement bdrv_co_drain_begin but omit
> > bdrv_co_drained_end.  Currently the point where the section ends is,
> > as said in the comment, when next I/O callback is invoked. Now we are
> > adding the explicit ".bdrv_co_drain_end" into the fomular, if we still
> > keep the previous convention, the interface contract is just mixed of
> > two things for no good reason. I don't think it's technically
> > necessary.
> 
> We don't keep the convention with the next I/O callback. We just allow
> drivers to omit an empty implementation of either callback, which seems
> to be a very sensible default to me.
> 

OK, I'm fine with this.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/2] Truncate the tail of the image file in qcow2 shrinking

[Pavel Butsykin]

On 22.09.2017 12:50, Daniel P. Berrange wrote:

On Fri, Sep 22, 2017 at 12:39:24PM +0300, Pavel Butsykin wrote:

Now after shrinking the qcow2 image, at the end of the image file, there might
be a tail that probably will never be used. Although it will not bring any
tangible benefit, we can cut the tail if it is. Yes, it will not free up disk
space, but if the blocks were be allocated sequentially and the image is not
heavily fragmented then the virtual size of the image file will be commensurate
with the real size. It also doesn't look like a great plus.. Well, at least we
can discuss it.


If the block backend has discard support enabled, can't we get the tail
to be discarded rather than merely truncated ?



It has already been implemented. (see 
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04581.html)

Sorry, I just forgot to mention that this patch rebased on Max's block
branch (https://github.com/XanClic/qemu/commits/block). Actually the
truncation will always be done on the already discarded area. It can
be useful only if the block backend doesn't support discard or a file
system doesn't support sparse files.


Regards,
Daniel

Re: [Qemu-block] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Kevin Wolf]

Am 22.09.2017 um 04:30 hat Fam Zheng geschrieben:
> On Thu, 09/21 18:39, Manos Pitsidianakis wrote:
> > On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
> > > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> > It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&
> > drv->bdrv_co_drain_end) somewhere unless you state they don't have to be
> > implemented at the same time. How about we be completely explicit:
> > 
> >  bdrv_co_drain_begin is called if implemented in the beggining of a
> >  drain operation to drain and stop any internal sources of requests in
> >  the driver.
> >  bdrv_co_drain_end is called if implemented at the end of the drain.
> > 
> >  They should be used by the driver to e.g. manage scheduled I/O
> >  requests, or toggle an internal state. After the end of the drain new
> >  requests will continue normally.
> > 
> > I hope this is easier for a reader to understand!
> 
> I don't like the inconsistent semantics of when the drained section
> ends, if we allow drivers to implement bdrv_co_drain_begin but omit
> bdrv_co_drained_end.  Currently the point where the section ends is,
> as said in the comment, when next I/O callback is invoked. Now we are
> adding the explicit ".bdrv_co_drain_end" into the fomular, if we still
> keep the previous convention, the interface contract is just mixed of
> two things for no good reason. I don't think it's technically
> necessary.

We don't keep the convention with the next I/O callback. We just allow
drivers to omit an empty implementation of either callback, which seems
to be a very sensible default to me.

> Let's just add the assert:
> 
> assert(!!drv->bdrv_co_drain_begin == !!bdrv_co_drain_end);
> 
> in bdrv_drain_invoke, add a comment here

I'm not in favour of this, but if we do want to have it, wouldn't
bdrv_register() be a much better place for the assertion?

> then add an empty .bdrv_co_drain_end in qed.

If you need empty functions anywhere, it's a sign that we have a bad
default behaviour.

Kevin

Re: [Qemu-block] [PATCH 1/6] qemu-io: Reset qemuio_blk permissions before each command

[Kevin Wolf]

Am 21.09.2017 um 15:53 hat Kevin Wolf geschrieben:
> Am 15.09.2017 um 12:10 hat Kevin Wolf geschrieben:
> > qemu-io provides a 'reopen' command that allows switching from writable
> > to read-only access. We need to make sure that we don't try to keep
> > write permissions to a BlockBackend that becomes read-only, otherwise
> > things are going to fail.
> > 
> > command() already makes sure to request any additional permissions that
> > a qemu-io command requires, so just resetting the permissions to values
> > that are safe for read-only images is enough to fix this.
> > 
> > As a side effect, this makes the output of qemu-iotests case 187 more
> > consistent.
> > 
> > Signed-off-by: Kevin Wolf 
> 
> This seems to break qemu-iotests 077 and 081 for raw. I'll look into
> it tomorrow.

The problem seems to be related to 'aio_write': We already reset the
permissions while the request is still in flight and might still start
new internal requests that aren't allowed any more. We would have to
drain blk around resetting the permissions, but this would obviously
defeat the purpose of the aio_* commands.

I tried creating a separate temporary BB, but it doesn't seem to be that
straightforward (still crashes during 'aio_flush'). It would also
provide the wrong semantics, HMP 'qemu-io' commands are supposed to be
executed on the exact BlockBackend that was given.

So unless someone has an idea how to salvage this patch, I'm afraid I'll
just have to drop it. The original state isn't really correct either, but
it errs on the side of having more permissions than necessary, so
failure is less likely.

Kevin

Re: [Qemu-block] [PATCH v2 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks

[Stefan Hajnoczi]

On Thu, Sep 21, 2017 at 04:17:07PM +0300, Manos Pitsidianakis wrote:
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Manos Pitsidianakis 
> ---
>  block/throttle.c | 18 ++
>  1 file changed, 18 insertions(+)

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/2] Truncate the tail of the image file in qcow2 shrinking

[Daniel P. Berrange]

On Fri, Sep 22, 2017 at 12:39:24PM +0300, Pavel Butsykin wrote:
> Now after shrinking the qcow2 image, at the end of the image file, there might
> be a tail that probably will never be used. Although it will not bring any
> tangible benefit, we can cut the tail if it is. Yes, it will not free up disk
> space, but if the blocks were be allocated sequentially and the image is not
> heavily fragmented then the virtual size of the image file will be 
> commensurate
> with the real size. It also doesn't look like a great plus.. Well, at least we
> can discuss it.

If the block backend has discard support enabled, can't we get the tail
to be discarded rather than merely truncated ?

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-block] [PATCH v2 2/2] qcow2: truncate the tail of the image file after shrinking the image

[Pavel Butsykin]

Now after shrinking the image, at the end of the image file, there might be a
tail that probably will never be used. So we can find the last used cluster and
cut the tail.

Signed-off-by: Pavel Butsykin 
---
 block/qcow2-refcount.c | 22 ++
 block/qcow2.c  | 23 +++
 block/qcow2.h  |  1 +
 3 files changed, 46 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 88d5a3f1ad..aa3fd6cf17 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -3181,3 +3181,25 @@ out:
 g_free(reftable_tmp);
 return ret;
 }
+
+int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size)
+{
+BDRVQcow2State *s = bs->opaque;
+int64_t i;
+
+for (i = size_to_clusters(s, size) - 1; i >= 0; i--) {
+uint64_t refcount;
+int ret = qcow2_get_refcount(bs, i, );
+if (ret < 0) {
+fprintf(stderr, "Can't get refcount for cluster %" PRId64 ": %s\n",
+i, strerror(-ret));
+return ret;
+}
+if (refcount > 0) {
+return i;
+}
+}
+qcow2_signal_corruption(bs, true, -1, -1,
+"There are no references in the refcount table.");
+return -EIO;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 8a4311d338..8dfb5393a7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3106,6 +3106,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset,
 new_l1_size = size_to_l1(s, offset);
 
 if (offset < old_length) {
+int64_t last_cluster, old_file_size;
 if (prealloc != PREALLOC_MODE_OFF) {
 error_setg(errp,
"Preallocation can't be used for shrinking an image");
@@ -3134,6 +3135,28 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset,
  "Failed to discard unused refblocks");
 return ret;
 }
+
+old_file_size = bdrv_getlength(bs->file->bs);
+if (old_file_size < 0) {
+error_setg_errno(errp, -old_file_size,
+ "Failed to inquire current file length");
+return old_file_size;
+}
+last_cluster = qcow2_get_last_cluster(bs, old_file_size);
+if (last_cluster < 0) {
+error_setg_errno(errp, -last_cluster,
+ "Failed to find the last cluster");
+return last_cluster;
+}
+if ((last_cluster + 1) * s->cluster_size < old_file_size) {
+ret = bdrv_truncate(bs->file, (last_cluster + 1) * s->cluster_size,
+PREALLOC_MODE_OFF, NULL);
+if (ret < 0) {
+error_setg_errno(errp, -ret,
+ "Failed to truncate the tail of the image");
+return ret;
+}
+}
 } else {
 ret = qcow2_grow_l1_table(bs, new_l1_size, true);
 if (ret < 0) {
diff --git a/block/qcow2.h b/block/qcow2.h
index 5a289a81e2..782a206ecb 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -597,6 +597,7 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int 
refcount_order,
 BlockDriverAmendStatusCB *status_cb,
 void *cb_opaque, Error **errp);
 int qcow2_shrink_reftable(BlockDriverState *bs);
+int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
 
 /* qcow2-cluster.c functions */
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
-- 
2.14.1

[Qemu-block] [PATCH v2 0/2] Truncate the tail of the image file in qcow2 shrinking

[Pavel Butsykin]

Now after shrinking the qcow2 image, at the end of the image file, there might
be a tail that probably will never be used. Although it will not bring any
tangible benefit, we can cut the tail if it is. Yes, it will not free up disk
space, but if the blocks were be allocated sequentially and the image is not
heavily fragmented then the virtual size of the image file will be commensurate
with the real size. It also doesn't look like a great plus.. Well, at least we
can discuss it.

Changes from v1:
- rewrite qcow2_get_last_cluster() function according to Max's comments. (2)

Pavel Butsykin (2):
  qcow2: fix return error code in qcow2_truncate()
  qcow2: truncate the tail of the image file after shrinking the image

 block/qcow2-refcount.c | 22 ++
 block/qcow2.c  | 27 +--
 block/qcow2.h  |  1 +
 3 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.14.1

[Qemu-block] [PATCH v2 1/2] qcow2: fix return error code in qcow2_truncate()

[Pavel Butsykin]

Signed-off-by: Pavel Butsykin 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2174a84d1f..8a4311d338 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3166,7 +3166,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset,
 if (old_file_size < 0) {
 error_setg_errno(errp, -old_file_size,
  "Failed to inquire current file length");
-return ret;
+return old_file_size;
 }
 
 nb_new_data_clusters = DIV_ROUND_UP(offset - old_length,
@@ -3195,7 +3195,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset,
 if (allocation_start < 0) {
 error_setg_errno(errp, -allocation_start,
  "Failed to resize refcount structures");
-return -allocation_start;
+return allocation_start;
 }
 
 clusters_allocated = qcow2_alloc_clusters_at(bs, allocation_start,
-- 
2.14.1