Re: [Qemu-block] [Qemu-devel] [PATCH v3 10/16] block/qcow2: Lock s->lock in preallocate()

[Eric Blake]

On 05/26/2017 11:55 AM, Max Reitz wrote:
> preallocate() is and will be called only from places that do not lock

Maybe: "that do not otherwise need to lock"

> s->lock: Currently that is qcow2_create2(), as of a future patch it will
> be called from qcow2_truncate(), too.
> 
> It therefore makes sense to move locking that mutex into preallocate()
> itself.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2.c | 22 +++---
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 

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 v3 09/16] block/qcow2: Generalize preallocate()

[Eric Blake]

On 05/26/2017 11:55 AM, Max Reitz wrote:
> This patch adds two new parameters to the preallocate() function so we
> will be able to use it not just for preallocating a new image but also
> for preallocated image growth.
> 
> The offset parameter allows the caller to specify a virtual offset from
> which to start preallocating. For newly created images this is always 0,
> but for preallocating growth this will be the old image length.
> 
> The new_length parameter specifies the supposed new length of the image
> (basically the "end offset" for preallocation). During image truncation,
> bdrv_getlength() will return the old image length so we cannot rely on
> its return value then.

bdrv_getlength() is (currently) always sector-aligned (rounding up as
needed).

new_length is passed from qcow2_create2()'s total_size, which in turn
comes from qcow2_create()'s size, which can be user-supplied - but also
appears that we round up to ensure it is always sector-aligned.

Testing: 'qemu-img create -f qcow2 a.img 1' reports "size=1", but
'qemu-img info a.img' reports "virtual size: 512" - so we have a
secondary bug worth fixing later in that we are rounding AFTER what we
report to the user, but I'm not seeing a behavior change in this patch.

> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2.c | 17 -
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 

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 v3 04/16] qemu-img: Expose PreallocMode for resizing

[Eric Blake]

On 05/26/2017 11:55 AM, Max Reitz wrote:
> Add a --preallocation command line option to qemu-img resize which can
> be used to set the PreallocMode parameter of blk_truncate().
> 
> Signed-off-by: Max Reitz 
> ---
>  qemu-img.c| 33 ++---
>  qemu-img.texi |  7 ++-
>  2 files changed, 36 insertions(+), 4 deletions(-)

> @@ -3553,8 +3566,16 @@ static int img_resize(int argc, char **argv)
>  goto out;
>  }
>  
> +current_size = blk_getlength(blk);
> +if (current_size < 0) {
> +error_report("Failed to inquire current image length: %s",
> + strerror(-current_size));
> +ret = -1;
> +goto out;
> +}
> +
>  if (relative) {
> -total_size = blk_getlength(blk) + n * relative;
> +total_size = current_size + n * relative;

You snuck in a bug fix here (reporting failure, rather than using a
bogus total_size, if querying the size fails).  Please mention that in
the commit message.

> @@ -541,6 +541,11 @@ After using this command to grow a disk image, you must 
> use file system and
>  partitioning tools inside the VM to actually begin using the new space on the
>  device.
>  
> +When growing an image, the @code{--preallocation} option may be used to 
> specify
> +how the additional image area should be allocated on the host.  See the 
> format
> +description in the @code{NOTES} section which values are allowed.  Using this
> +option may result in more data being allocated than necessary.

Should we tone it down a bit by saying 'slightly more data'?  (We'd
rather over-estimate than fall short, but our over-estimation will
probably be < 1% off, and not something drastic like an order of
magnitude off).

With the improved commit message,
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] [PATCH] live-block-ops.txt: Rewrite and improve it

[Eric Blake]

On 05/30/2017 03:24 PM, Kashyap Chamarthy wrote:
> First, thanks for the quick feedback!
> 
> On Tue, May 30, 2017 at 02:24:40PM -0500, Eric Blake wrote:
>> On 05/30/2017 10:38 AM, Kashyap Chamarthy wrote:
>>> This edition documents all four operations:
> 

>>> +
>>> +(1) `Disk image backing chain notation`_
>>> +(2) `Brief overview of live block QMP primitives`_
>>> +(3) `Interacting with a QEMU instance`_
>>> +(4) `Example disk image chain`_
>>> +(5) `A note on points-in-time vs file names`_
>>> +(6) `Live block streaming -- `block-stream``_
>>> +(7) `QMP invocation for `block-stream``_
>>> +(8) `Live block commit -- `block-commit``_
>>> +(9) `QMP invocation for `block-commit``_
>>> +(10) `Live disk synchronization -- `drive-mirror`&`blockdev-mirror``_
>>> +(11) `QMP invocation for `drive-mirror``_
>>> +(12) `Notes on `blockdev-mirror``_
>>> +(13) `Live disk backup -- `drive-backup`&`blockdev-backup``_
>>> +(14) `QMP invocation for `drive-backup``_
>>> +(15) `Notes on `blockdev-backup``_

> Eric, do you have an opinion:  Now that I _do_ have the notes
> available[*], do you think it is worth spelling out the invocation for
> QMP `blockdev-{backup, mirror}`?  Or wait for later?

I liked how you compared drive-mirror and blockdev-mirror, but it's
probably worth an invocation example in sections 12/15 in addition to
"this is how the block version differs from the drive version".

-- 
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] [PATCH] live-block-ops.txt: Rewrite and improve it

[Kashyap Chamarthy]

First, thanks for the quick feedback!

On Tue, May 30, 2017 at 02:24:40PM -0500, Eric Blake wrote:
> On 05/30/2017 10:38 AM, Kashyap Chamarthy wrote:
> > This edition documents all four operations:

[...]

> s/occurance/occurrence/
> 
> > use raw JSON; for subsequent occurances, use 'qmp-shell', with an
> 
> s/occurances/occurrences/

Will address, both.  Sigh, I first ran it through GNU `aspell`, but
forgot to have to save the typo corrections.

> > Signed-off-by: Kashyap Chamarthy 
> > ---
> > * An HTML rendered version is here:
> >   https://kashyapc.fedorapeople.org/virt/qemu/live-block-operations.html
> 
> Helpful; thanks!
> 
> > diff --git a/docs/live-block-operations.rst b/docs/live-block-operations.rst
> > new file mode 100644
> > index 
> > ..19107776cd1edd20fac17ddda192d677199190d9
> > --- /dev/null
> > +++ b/docs/live-block-operations.rst
> > @@ -0,0 +1,757 @@
> > +
> > +Live Block Device Operations
> > +
> 
> Is it worth mentioning a copyright/license in this prologue?  It's not
> mandatory (you get the default of GPLv2+ based on the top-level if you
> don't do it), but being explicit never hurts.

Good point, I add the boilerplate copyright / license text.
 
> > +
> > +Table of Contents
> > +=
> > +
> > +(1) `Disk image backing chain notation`_
> > +(2) `Brief overview of live block QMP primitives`_
> > +(3) `Interacting with a QEMU instance`_
> > +(4) `Example disk image chain`_
> > +(5) `A note on points-in-time vs file names`_
> > +(6) `Live block streaming -- `block-stream``_
> > +(7) `QMP invocation for `block-stream``_
> > +(8) `Live block commit -- `block-commit``_
> > +(9) `QMP invocation for `block-commit``_
> > +(10) `Live disk synchronization -- `drive-mirror`&`blockdev-mirror``_
> > +(11) `QMP invocation for `drive-mirror``_
> > +(12) `Notes on `blockdev-mirror``_
> > +(13) `Live disk backup -- `drive-backup`&`blockdev-backup``_
> > +(14) `QMP invocation for `drive-backup``_
> > +(15) `Notes on `blockdev-backup``_
> > +
> 
> Does .rst have any mechanisms for ensuring the ToC doesn't get out of
> sync if we later change sections around?

Hmm, good question, I don't know such a mechanism exists yet, I think it
will get out of sync; will check.  But if we _do_ rearrange, it's
trivial to fix it.  But I see where you're coming from.

> > +
> > +.. _`Disk image backing chain notation`:
> > +
> 
> > +
> > +Arrow to be read as: Image [A] is the backing file of disk image [B].
> 
> Maybe: "The arrow can be read as:"

Yes, will fix.

> > +And live QEMU is currently writing to image [B], consequently, it is
> > +also referred to as the "active layer".
> > +
> > +There are two kinds of terminology that is common when referring to
> 
> s/that is common/that are common/

Likewise.

[...]

> > +- `block-stream`: Live copy data from overlay files into backing images.
> 
> Backwards.  This is a live copy of data from backing images into
> overlays (with the optional goal of removing the backing image from the
> chain).

Err, indeed.  Will fix.

> > +
> > +- `block-commit`: Live merge data from backing files into overlay
> > +  images.  Since QEMU 2.0, this includes "active `block-commit`" (i.e.
> > +  merge the current active layer into the base image).
> 
> Backwards.  This is live merge of data from overlay images into backing
> images (with the optional goal of removing the overlay image from the
> chain).

Yes, again, will fix.

[...]

> > +Interacting with a QEMU instance
> > +
> > +
> > +To show some example invocations of command-line, we shall use the
> 
> 'shall' is okay, but sounds rather formal; using 'will' sounds more typical.

Funny you mention, I actually _began_ with "will", but think my
subconscious self 'overwrote' it with "shall", as I was reading a book
("The Blind Watchmaker") which was using "shall" throughout.  Probably
that had an influence. :-)

> > +following invocation of QEMU, with a QMP server running over UNIX
> > +socket:
> > +
> > +.. code-block::
> > +
> > +$ ./x86_64-softmmu/qemu-system-x86_64 -display none -nodefconfig \
> > +-M q35 -nodefaults -m 512 \
> > +-blockdev 
> > node-name=node-A,driver=qcow2,file.driver=file,file.filename=./a.qcow2 \
> > +-device virtio-blk,drive=node-A,id=virtio0 \
> > +-monitor stdio -qmp unix:/tmp/qmp-sock,server,nowait
> 
> Nice use of -blockdev! 

I was conscious to use the latest upstream preferences, regardless
whether they're used by higher layers.

> Is it worth also giving file.node-name for a
> non-generated node name on the protocol layer?

Hmm, if I had to guess, you say the above for completeness' sake.
Because, IIRC, the 'node-name; at the protocol layer will come in handy
when doing things like IOPS throttling (for which

Do you have a preferred `-blockdev` invocation?

> > +
> > +The `-blockdev` command-line option, 

Re: [Qemu-block] [PATCH v2 4/4] qemu-iotests: Block migration test

[Jeff Cody]

On Tue, May 30, 2017 at 06:57:05PM +0200, Kevin Wolf wrote:
> Am 30.05.2017 um 17:52 hat Jeff Cody geschrieben:
> > On Tue, May 30, 2017 at 05:22:53PM +0200, Kevin Wolf wrote:
> > > Signed-off-by: Kevin Wolf 
> > > ---
> > >  tests/qemu-iotests/183 | 143 
> > > +
> > >  tests/qemu-iotests/183.out |  46 +++
> > >  tests/qemu-iotests/group   |   1 +
> > >  3 files changed, 190 insertions(+)
> > >  create mode 100755 tests/qemu-iotests/183
> > >  create mode 100644 tests/qemu-iotests/183.out
> > > 
> > > diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
> > > new file mode 100755
> > > index 000..5eda0e9
> > > --- /dev/null
> > > +++ b/tests/qemu-iotests/183
> > > @@ -0,0 +1,143 @@
> > > +#!/bin/bash
> > > +#
> > > +# Test old-style block migration (migrate -b)
> > > +#
> > > +# Copyright (C) 2017 Red Hat, Inc.
> > > +#
> > > +# This program is free software; you can redistribute it and/or modify
> > > +# it under the terms of the GNU General Public License as published by
> > > +# the Free Software Foundation; either version 2 of the License, or
> > > +# (at your option) any later version.
> > > +#
> > > +# This program is distributed in the hope that it will be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program.  If not, see .
> > > +#
> > > +
> > > +# creator
> > > +owner=kw...@redhat.com
> > > +
> > > +seq=`basename $0`
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +status=1 # failure is the default!
> > > +
> > > +MIG_SOCKET="${TEST_DIR}/migrate"
> > > +
> > > +_cleanup()
> > > +{
> > > +rm -f "${MIG_SOCKET}"
> > > +rm -f "${TEST_IMG}.dest"
> > > + _cleanup_test_img
> > > +_cleanup_qemu
> > > +}
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common.rc
> > > +. ./common.filter
> > > +. ./common.qemu
> > > +
> > > +_supported_fmt generic
> > 
> > Not sure to what extent we are really keeping the _supported_fmt up to date,
> > but this test will only work on qcow2 and raw, right?
> > 
> > I'd recommend changing this to:
> > 
> > + _supported_fmt qcow2 raw
> > 
> > (that can probably be done when applying, however).
> 
> Seems you are right, but I fail to see why the other formats shouldn't
> work? Are these just bugs or do I make any assumption in the test script
> that is invalid for other image formats?
>

Well, the following formats have live migration blockers:

vmdk, vhdx, vdi, vpc, qcow, vvfat

So that leaves qed, dmg, quorum.

Of those, qed is the only one that fails.  And I would guess it is a bug?

While waiting for  "status: active" from the query-migrate (in the section
where I proposed a timeout), the migration is hanging, and appears to never
get started.  If I redirect that output to a debug file, I just see this
over and over (I prettified the json output here):


{
  "return": {
"expected-downtime": 300,
"status": "active",
"disk": {
  "total": 67108864,
  "postcopy-requests": 0,
  "dirty-sync-count": 0,
  "page-size": 0,
  "remaining": 63963136,
  "mbps": 0,
  "transferred": 3145728,
  "duplicate": 0,
  "dirty-pages-rate": 0,
  "skipped": 0,
  "normal-bytes": 0,
  "normal": 0
},
"setup-time": 1,
"total-time": 4956,
"ram": {
  "total": 134750208,
  "postcopy-requests": 0,
  "dirty-sync-count": 1,
  "page-size": 4096,
  "remaining": 134750208,
  "mbps": 0,
  "transferred": 0,
  "duplicate": 0,
  "dirty-pages-rate": 0,
  "skipped": 0,
  "normal-bytes": 0,
  "normal": 0
}
  }
}

The only thing that advances is "total-time".

So I would make _supported_fmt be 'qcow2 raw qed dmg quorum', and the
failure of qed is an actual failure.

[...]

> > > +echo === Do block migration to destination ===
> > > +echo
> > > +
> > > +reply="$(_send_qemu_cmd $src \
> > > +"{ 'execute': 'migrate',
> > > +   'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \
> > > +'return\|error')"
> > > +echo "$reply"
> > > +if echo "$reply" | grep "compiled without old-style" > /dev/null; then
> > > +_notrun "migrate -b support not compiled in"
> > > +fi
> > > +
> > > +while _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" "return"  |
> > > +  grep '"status": "active"' > /dev/null
> > > +do
> > > +sleep 0.1
> > 
> > Would it make sense here to add a timeout?  It would be nice to be able to
> > reply on scripts always failing for git-bisect (rather than potentially
> > hanging at a spot).
> 
> What would you think about squashing this in:
> 
> --- 

Re: [Qemu-block] [PATCH v2 4/4] qemu-iotests: Block migration test

[Kevin Wolf]

Am 30.05.2017 um 17:52 hat Jeff Cody geschrieben:
> On Tue, May 30, 2017 at 05:22:53PM +0200, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf 
> > ---
> >  tests/qemu-iotests/183 | 143 
> > +
> >  tests/qemu-iotests/183.out |  46 +++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 190 insertions(+)
> >  create mode 100755 tests/qemu-iotests/183
> >  create mode 100644 tests/qemu-iotests/183.out
> > 
> > diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
> > new file mode 100755
> > index 000..5eda0e9
> > --- /dev/null
> > +++ b/tests/qemu-iotests/183
> > @@ -0,0 +1,143 @@
> > +#!/bin/bash
> > +#
> > +# Test old-style block migration (migrate -b)
> > +#
> > +# Copyright (C) 2017 Red Hat, Inc.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see .
> > +#
> > +
> > +# creator
> > +owner=kw...@redhat.com
> > +
> > +seq=`basename $0`
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +status=1   # failure is the default!
> > +
> > +MIG_SOCKET="${TEST_DIR}/migrate"
> > +
> > +_cleanup()
> > +{
> > +rm -f "${MIG_SOCKET}"
> > +rm -f "${TEST_IMG}.dest"
> > +   _cleanup_test_img
> > +_cleanup_qemu
> > +}
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +# get standard environment, filters and checks
> > +. ./common.rc
> > +. ./common.filter
> > +. ./common.qemu
> > +
> > +_supported_fmt generic
> 
> Not sure to what extent we are really keeping the _supported_fmt up to date,
> but this test will only work on qcow2 and raw, right?
> 
> I'd recommend changing this to:
> 
> + _supported_fmt qcow2 raw
> 
> (that can probably be done when applying, however).

Seems you are right, but I fail to see why the other formats shouldn't
work? Are these just bugs or do I make any assumption in the test script
that is invalid for other image formats?

> > +_supported_proto file
> > +_supported_os Linux
> > +
> > +size=64M
> > +_make_test_img $size
> > +TEST_IMG="${TEST_IMG}.dest" _make_test_img $size
> > +
> > +echo
> > +echo === Starting VMs ===
> > +echo
> > +
> > +qemu_comm_method="qmp"
> > +
> > +_launch_qemu \
> > +-drive file="${TEST_IMG}",cache=${CACHEMODE},driver=$IMGFMT,id=disk
> > +src=$QEMU_HANDLE
> > +_send_qemu_cmd $src "{ 'execute': 'qmp_capabilities' }" 'return'
> > +
> > +_launch_qemu \
> > +-drive 
> > file="${TEST_IMG}.dest",cache=${CACHEMODE},driver=$IMGFMT,id=disk \
> > +-incoming "unix:${MIG_SOCKET}"
> > +dest=$QEMU_HANDLE
> > +_send_qemu_cmd $dest "{ 'execute': 'qmp_capabilities' }" 'return'
> > +
> > +echo
> > +echo === Write something on the source ===
> > +echo
> > +
> > +_send_qemu_cmd $src \
> > +"{ 'execute': 'human-monitor-command',
> > +   'arguments': { 'command-line':
> > +  'qemu-io disk \"write -P 0x55 0 64k\"' } }" \
> > +'return'
> > +_send_qemu_cmd $src \
> > +"{ 'execute': 'human-monitor-command',
> > +   'arguments': { 'command-line':
> > +  'qemu-io disk \"read -P 0x55 0 64k\"' } }" \
> > +'return'
> > +
> > +echo
> > +echo === Do block migration to destination ===
> > +echo
> > +
> > +reply="$(_send_qemu_cmd $src \
> > +"{ 'execute': 'migrate',
> > +   'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \
> > +'return\|error')"
> > +echo "$reply"
> > +if echo "$reply" | grep "compiled without old-style" > /dev/null; then
> > +_notrun "migrate -b support not compiled in"
> > +fi
> > +
> > +while _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" "return"  |
> > +  grep '"status": "active"' > /dev/null
> > +do
> > +sleep 0.1
> 
> Would it make sense here to add a timeout?  It would be nice to be able to
> reply on scripts always failing for git-bisect (rather than potentially
> hanging at a spot).

What would you think about squashing this in:

--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -96,11 +96,8 @@ if echo "$reply" | grep "compiled without old-style" > 
/dev/null; then
 _notrun "migrate -b support not compiled in"
 fi
 
-while _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" "return"  |
-  grep '"status": "active"' > /dev/null
-do
-sleep 0.1
-done
+QEMU_COMM_TIMEOUT=0.1 qemu_cmd_repeat=50 silent=yes \
+_send_qemu_cmd $src "{ 'execute': 'query-migrate' }" '"status": 

Re: [Qemu-block] [PATCH v2 4/4] qemu-iotests: Block migration test

[Eric Blake]

On 05/30/2017 10:22 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/183 | 143 
> +
>  tests/qemu-iotests/183.out |  46 +++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 190 insertions(+)
>  create mode 100755 tests/qemu-iotests/183
>  create mode 100644 tests/qemu-iotests/183.out
> 

In addition to Jeff's review comments:

> +
> +_cleanup()
> +{
> +rm -f "${MIG_SOCKET}"
> +rm -f "${TEST_IMG}.dest"
> + _cleanup_test_img
> +_cleanup_qemu

Odd indentation.

> +echo
> +echo === Do block migration to destination ===
> +echo
> +
> +reply="$(_send_qemu_cmd $src \
> +"{ 'execute': 'migrate',
> +   'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \
> +'return\|error')"
> +echo "$reply"
> +if echo "$reply" | grep "compiled without old-style" > /dev/null; then
> +_notrun "migrate -b support not compiled in"
> +fi

Oh cool - I didn't realize that _notrun already existed as our way to
skip a test, even if we've already produced unexpected output (if we
ever want to play with exit status 77 in the future for skipped tests,
then _notrun would be a great place to centralize that).

> +
> +while _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" "return"  |
> +  grep '"status": "active"' > /dev/null
> +do
> +sleep 0.1

More interesting indentation (I would have probably stuck to 4 spaces on
both lines)

With the nits fixed,
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] [PATCH v2 4/4] qemu-iotests: Block migration test

[Jeff Cody]

On Tue, May 30, 2017 at 05:22:53PM +0200, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/183 | 143 
> +
>  tests/qemu-iotests/183.out |  46 +++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 190 insertions(+)
>  create mode 100755 tests/qemu-iotests/183
>  create mode 100644 tests/qemu-iotests/183.out
> 
> diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
> new file mode 100755
> index 000..5eda0e9
> --- /dev/null
> +++ b/tests/qemu-iotests/183
> @@ -0,0 +1,143 @@
> +#!/bin/bash
> +#
> +# Test old-style block migration (migrate -b)
> +#
> +# Copyright (C) 2017 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +# creator
> +owner=kw...@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +status=1 # failure is the default!
> +
> +MIG_SOCKET="${TEST_DIR}/migrate"
> +
> +_cleanup()
> +{
> +rm -f "${MIG_SOCKET}"
> +rm -f "${TEST_IMG}.dest"
> + _cleanup_test_img
> +_cleanup_qemu
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +_supported_fmt generic

Not sure to what extent we are really keeping the _supported_fmt up to date,
but this test will only work on qcow2 and raw, right?

I'd recommend changing this to:

+ _supported_fmt qcow2 raw

(that can probably be done when applying, however).


> +_supported_proto file
> +_supported_os Linux
> +
> +size=64M
> +_make_test_img $size
> +TEST_IMG="${TEST_IMG}.dest" _make_test_img $size
> +
> +echo
> +echo === Starting VMs ===
> +echo
> +
> +qemu_comm_method="qmp"
> +
> +_launch_qemu \
> +-drive file="${TEST_IMG}",cache=${CACHEMODE},driver=$IMGFMT,id=disk
> +src=$QEMU_HANDLE
> +_send_qemu_cmd $src "{ 'execute': 'qmp_capabilities' }" 'return'
> +
> +_launch_qemu \
> +-drive file="${TEST_IMG}.dest",cache=${CACHEMODE},driver=$IMGFMT,id=disk 
> \
> +-incoming "unix:${MIG_SOCKET}"
> +dest=$QEMU_HANDLE
> +_send_qemu_cmd $dest "{ 'execute': 'qmp_capabilities' }" 'return'
> +
> +echo
> +echo === Write something on the source ===
> +echo
> +
> +_send_qemu_cmd $src \
> +"{ 'execute': 'human-monitor-command',
> +   'arguments': { 'command-line':
> +  'qemu-io disk \"write -P 0x55 0 64k\"' } }" \
> +'return'
> +_send_qemu_cmd $src \
> +"{ 'execute': 'human-monitor-command',
> +   'arguments': { 'command-line':
> +  'qemu-io disk \"read -P 0x55 0 64k\"' } }" \
> +'return'
> +
> +echo
> +echo === Do block migration to destination ===
> +echo
> +
> +reply="$(_send_qemu_cmd $src \
> +"{ 'execute': 'migrate',
> +   'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \
> +'return\|error')"
> +echo "$reply"
> +if echo "$reply" | grep "compiled without old-style" > /dev/null; then
> +_notrun "migrate -b support not compiled in"
> +fi
> +
> +while _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" "return"  |
> +  grep '"status": "active"' > /dev/null
> +do
> +sleep 0.1

Would it make sense here to add a timeout?  It would be nice to be able to
reply on scripts always failing for git-bisect (rather than potentially
hanging at a spot).

> +done

Modulo the updated _supported_fmt line:

Reviewed-by: Jeff Cody 


> +_send_qemu_cmd $src "{ 'execute': 'query-status' }" "return"
> +
> +echo
> +echo === Do some I/O on the destination ===
> +echo
> +
> +# It is important that we use the BlockBackend of the guest device here 
> instead
> +# of the node name, which would create a new BlockBackend and not test 
> whether
> +# the guest has the necessary permissions to access the image now
> +silent=yes _send_qemu_cmd $dest "" "100 %"
> +_send_qemu_cmd $dest "{ 'execute': 'query-status' }" "return"
> +_send_qemu_cmd $dest \
> +"{ 'execute': 'human-monitor-command',
> +   'arguments': { 'command-line':
> +  'qemu-io disk \"read -P 0x55 0 64k\"' } }" \
> +'return'
> +_send_qemu_cmd $dest \
> +"{ 'execute': 'human-monitor-command',
> +   'arguments': { 'command-line':
> +  'qemu-io disk \"write -P 0x66 1M 64k\"' } }" \
> +'return'
> +
> +echo
> +echo === Shut down and 

Re: [Qemu-block] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Eric Blake]

On 05/30/2017 10:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.05.2017 17:53, Eric Blake wrote:
>> On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about allocted/unallocated by
>>> top-level format driver space (in its .file) and allocation status
>>> (allocated/hole/after eof) of corresponding areas in this .file.
>>>

>>> +# @alloc_hole: allocated by format driver but actually is a hole in
>>> +#  underlying file
>> The portions of the format file in use by the format, but where the
>> entire cluster is a hole in the underlying file (note that with cluster
>> size large enough, you can get a cluster that is part-data/part-hole in
>> the underlying file, it looks like you are counting those as data).
> 
> No, I account on sector boundary on bs->file. So it is not cluster-aligned.

Okay, worth knowing (and therefore worth including in the documentation).

> 
>>
>>> +#
>>> +# @alloc_overhead: allocated by format driver after end of
>>> underlying file
>> The portions of the format file in use by the format, but where the
>> entire cluster is beyond the end of the underlying file (the effective
>> hole).  Do we really need to distinguish between hole within the
>> underlying file and hole beyond the end of the file? Or can this number
>> be combined with the previous?
> 
> alloc_alloc + alloc_hole + hole_alloc + hole_hole = size of bs->file, I
> think this is good and clear.

That relationship is worth including in the documentation.

> 
> these 4 stats describes bs->file usage in details (not on cluster
> boundary, but on sector, based on bs->file block status), alloc_overhead
> is separate.

I'd lean more towards alloc-overrun than alloc-overhead (we have
metadata overhead no matter what, but overrun does a nice job of
explaining offsets that are important to the format but which are not
present in the underlying protocol).


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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] live-block-ops.txt: Rewrite and improve it

[Kashyap Chamarthy]

This edition documents all four operations:

  - `block-stream`
  - `block-commit`
  - `drive-mirror` (& `blockdev-mirror`)
  - `drive-backup` (& `blockdev-backup`)

Things considered while writing this document:

  - Use reStructuredText format.  It is gentler on the eye, and can be
trivially converted to different formats.  (Another reason: upstream
QEMU is considering to switch to Sphinx, which uses reStructuredText
as its markup language.)

  - Raw QMP JSON output vs. 'qmp-shell'.  I debated with myself whether
to only show raw QMP JSON output (as that is the canonical
representation), or use 'qmp-shell', which takes key-value pairs.  I
settled on the approach of: for the first occurance of a command,
use raw JSON; for subsequent occurances, use 'qmp-shell', with an
occasional exception.

  - Using 'node-name' vs. file path to refer to disks.  While we have
`blockdev-{mirror, backup}` as 'node-name'-alternatives for
`drive-{mirror, backup}`, the `block-{stream, commit}` commands
still operate on file names for parameters 'base' and 'top'.  So I
added a caveat at the beginning to that effect.

Refer this related thread that I started:
https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg06466.html
"[RFC] Making 'block-stream', and 'block-commit' accept node-name"

All commands showed in this document were tested while documenting.

Thanks: Eric Blake for the section: "A note on points-in-time vs file
names".  This useful bit was originally articulated by Eric in his
KVMForum 2015 presentation, so I included that specific bit in this
document.

Signed-off-by: Kashyap Chamarthy 
---
* An HTML rendered version is here:
  https://kashyapc.fedorapeople.org/virt/qemu/live-block-operations.html

* To keep the document relatively short, I didn't explicitly spell out
  the complete QMP output for `blockdev-mirror`, and `blockdev-backup`.
  But I have them locally available here:

  https://kashyapc.fedorapeople.org/virt/qemu/blockdev-backup-and-mirror.html

  So I can trivially update, if the reviewers ask for it.
---
 docs/live-block-operations.rst | 757 +
 docs/live-block-ops.txt|  72 
 2 files changed, 757 insertions(+), 72 deletions(-)
 create mode 100644 docs/live-block-operations.rst
 delete mode 100644 docs/live-block-ops.txt

diff --git a/docs/live-block-operations.rst b/docs/live-block-operations.rst
new file mode 100644
index 
..19107776cd1edd20fac17ddda192d677199190d9
--- /dev/null
+++ b/docs/live-block-operations.rst
@@ -0,0 +1,757 @@
+
+Live Block Device Operations
+
+
+QEMU Block Layer currently (as of QEMU 2.9) supports four major kinds of
+live block device jobs -- stream, commit, mirror, and backup.  These can
+be used to manipulate disk image chains to accomplish certain tasks,
+namely: live copy data from backing files into overlays; shorten long
+disk image chains by merging data from overlays into backing files; live
+synchronize data from a disk image chain (including current active disk)
+to another target image; point-in-time (and incremental) backups of a
+block device.  Below is a description of the said block (QMP)
+primitives, and some (non-exhaustive list of) examples to illustrate
+their use.
+
+NB: The file qapi/block-core.json in the QEMU source tree has the
+canonical QEMU API (QAPI) schema documentation for the QMP primitives
+discussed here.
+
+
+Table of Contents
+=
+
+(1) `Disk image backing chain notation`_
+(2) `Brief overview of live block QMP primitives`_
+(3) `Interacting with a QEMU instance`_
+(4) `Example disk image chain`_
+(5) `A note on points-in-time vs file names`_
+(6) `Live block streaming -- `block-stream``_
+(7) `QMP invocation for `block-stream``_
+(8) `Live block commit -- `block-commit``_
+(9) `QMP invocation for `block-commit``_
+(10) `Live disk synchronization -- `drive-mirror`&`blockdev-mirror``_
+(11) `QMP invocation for `drive-mirror``_
+(12) `Notes on `blockdev-mirror``_
+(13) `Live disk backup -- `drive-backup`&`blockdev-backup``_
+(14) `QMP invocation for `drive-backup``_
+(15) `Notes on `blockdev-backup``_
+
+
+.. _`Disk image backing chain notation`:
+
+Disk image backing chain notation
+-
+
+A simple disk image chain.  (This can be created live, using QMP
+`blockdev-snapshot-sync`, or offline, via `qemu-img`):
+
+.. code-block::
+
+   (Live QEMU)
+|
+.
+V
+
+[A] <- [B]
+
+(backing file)(overlay)
+
+Arrow to be read as: Image [A] is the backing file of disk image [B].
+And live QEMU is currently writing to image [B], consequently, it is
+also referred to as the "active layer".
+
+There are two kinds of terminology that is common when referring to
+files in 

[Qemu-block] [PATCH] live-block-ops.txt: Rewrite and improve it

[Kashyap Chamarthy]

This edition documents all four operations:

  - `block-stream`
  - `block-commit`
  - `drive-mirror` (& `blockdev-mirror`)
  - `drive-backup` (& `blockdev-backup`)

Things considered while writing this document:

  - Use reStructuredText format.  It is gentler on the eye, and can be
trivially converted to different formats.  (Another reason: upstream
QEMU is considering to switch to Sphinx, which uses reStructuredText
as its markup language.)

  - Raw QMP JSON output vs. 'qmp-shell'.  I debated with myself whether
to only show raw QMP JSON output (as that is the canonical
representation), or use 'qmp-shell', which takes key-value pairs.  I
settled on the approach of: for the first occurance of a command,
use raw JSON; for subsequent occurances, use 'qmp-shell', with an
occasional exception.

  - Using 'node-name' vs. file path to refer to disks.  While we have
`blockdev-{mirror, backup}` as 'node-name'-alternatives for
`drive-{mirror, backup}`, the `block-{stream, commit}` commands
still operate on file names for parameters 'base' and 'top'.  So I
added a caveat at the beginning to that effect.

Refer this related thread that I started:
https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg06466.html
"[RFC] Making 'block-stream', and 'block-commit' accept node-name"

All commands showed in this document were tested while documenting.

Thanks: Eric Blake for the section: "A note on points-in-time vs file
names".  This useful bit was originally articulated by Eric in his
KVMForum 2015 presentation, so I included that specific in this
document.

Signed-off-by: Kashyap Chamarthy 
---
* An HTML rendered version is here:
  https://kashyapc.fedorapeople.org/virt/qemu/live-block-operations.html

* To keep the document relatively short, I didn't explicitly spell out
  the complete QMP output for `blockdev-mirror`, and `blockdev-backup`.
  But I have them locally available here:

  https://kashyapc.fedorapeople.org/virt/qemu/blockdev-backup-and-mirror.html

  So I can trivially update, if the reviewers ask for it.
---
 docs/live-block-operations.rst | 757 +
 docs/live-block-ops.txt|  72 
 2 files changed, 757 insertions(+), 72 deletions(-)
 create mode 100644 docs/live-block-operations.rst
 delete mode 100644 docs/live-block-ops.txt

diff --git a/docs/live-block-operations.rst b/docs/live-block-operations.rst
new file mode 100644
index 000..1910777
--- /dev/null
+++ b/docs/live-block-operations.rst
@@ -0,0 +1,757 @@
+
+Live Block Device Operations
+
+
+QEMU Block Layer currently (as of QEMU 2.9) supports four major kinds of
+live block device jobs -- stream, commit, mirror, and backup.  These can
+be used to manipulate disk image chains to accomplish certain tasks,
+namely: live copy data from backing files into overlays; shorten long
+disk image chains by merging data from overlays into backing files; live
+synchronize data from a disk image chain (including current active disk)
+to another target image; point-in-time (and incremental) backups of a
+block device.  Below is a description of the said block (QMP)
+primitives, and some (non-exhaustive list of) examples to illustrate
+their use.
+
+NB: The file qapi/block-core.json in the QEMU source tree has the
+canonical QEMU API (QAPI) schema documentation for the QMP primitives
+discussed here.
+
+
+Table of Contents
+=
+
+(1) `Disk image backing chain notation`_
+(2) `Brief overview of live block QMP primitives`_
+(3) `Interacting with a QEMU instance`_
+(4) `Example disk image chain`_
+(5) `A note on points-in-time vs file names`_
+(6) `Live block streaming -- `block-stream``_
+(7) `QMP invocation for `block-stream``_
+(8) `Live block commit -- `block-commit``_
+(9) `QMP invocation for `block-commit``_
+(10) `Live disk synchronization -- `drive-mirror`&`blockdev-mirror``_
+(11) `QMP invocation for `drive-mirror``_
+(12) `Notes on `blockdev-mirror``_
+(13) `Live disk backup -- `drive-backup`&`blockdev-backup``_
+(14) `QMP invocation for `drive-backup``_
+(15) `Notes on `blockdev-backup``_
+
+
+.. _`Disk image backing chain notation`:
+
+Disk image backing chain notation
+-
+
+A simple disk image chain.  (This can be created live, using QMP
+`blockdev-snapshot-sync`, or offline, via `qemu-img`):
+
+.. code-block::
+
+   (Live QEMU)
+|
+.
+V
+
+[A] <- [B]
+
+(backing file)(overlay)
+
+Arrow to be read as: Image [A] is the backing file of disk image [B].
+And live QEMU is currently writing to image [B], consequently, it is
+also referred to as the "active layer".
+
+There are two kinds of terminology that is common when referring to
+files in a disk image backing chain:
+
+(1) Directional: 'base' and 'top'.  

Re: [Qemu-block] [PATCH v2 3/4] migration/block: Clean up BBs in block_save_complete()

[Jeff Cody]

On Tue, May 30, 2017 at 05:22:52PM +0200, Kevin Wolf wrote:
> We need to release any block migrations BlockBackends on the source
> before successfully completing the migration because otherwise
> inactivating the images will fail (inactivation only tolerates device
> BBs).
> 
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Eric Blake 
> ---
>  migration/block.c | 22 +-
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 13f90d3..deb4b0e 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -673,16 +673,14 @@ static int64_t get_remaining_dirty(void)
>  return dirty << BDRV_SECTOR_BITS;
>  }
>  
> -/* Called with iothread lock taken.  */
>  
> -static void block_migration_cleanup(void *opaque)
> +
> +/* Called with iothread lock taken.  */
> +static void block_migration_cleanup_bmds(void)
>  {
>  BlkMigDevState *bmds;
> -BlkMigBlock *blk;
>  AioContext *ctx;
>  
> -bdrv_drain_all();
> -
>  unset_dirty_tracking();
>  
>  while ((bmds = QSIMPLEQ_FIRST(_mig_state.bmds_list)) != NULL) {
> @@ -700,6 +698,16 @@ static void block_migration_cleanup(void *opaque)
>  g_free(bmds->aio_bitmap);
>  g_free(bmds);
>  }
> +}
> +
> +/* Called with iothread lock taken.  */
> +static void block_migration_cleanup(void *opaque)
> +{
> +BlkMigBlock *blk;
> +
> +bdrv_drain_all();
> +
> +block_migration_cleanup_bmds();
>  
>  blk_mig_lock();
>  while ((blk = QSIMPLEQ_FIRST(_mig_state.blk_list)) != NULL) {
> @@ -843,6 +851,10 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>  
>  qemu_put_be64(f, BLK_MIG_FLAG_EOS);
>  
> +/* Make sure that our BlockBackends are gone, so that the block driver
> + * nodes can be inactivated. */
> +block_migration_cleanup_bmds();
> +
>  return 0;
>  }
>  
> -- 
> 1.8.3.1
> 
>

Reviewed-by: Jeff Cody 

Re: [Qemu-block] Throttling groups vs filter nodes

[Kevin Wolf]

Am 30.05.2017 um 16:29 hat Alberto Garcia geschrieben:
> On Sat 27 May 2017 09:56:03 AM CEST, Stefan Hajnoczi wrote:
> > Throttling groups allow multiple drives to share the same throttling
> > state (i.e. budget) between them.  Manos is working on moving the
> > throttling code into a block filter driver so it is no longer
> > hardcoded into the I/O code path.
> 
> Now that we're discussing changes to the throttling code, I'll take the
> opportunity to describe a feature that I'd like to work on at some point
> in the future.
> 
> This is of course out of the scope of the work that Manos is going to
> do, but I'd like to discuss it briefly to see what people think of this
> idea and whether it's compatible with the changes that we are now
> proposing.
> 
> Currently each block device can have either zero or one set of I/O
> limits, and that set of limits can be shared among several drives using
> throttling groups.
> 
> I have the following use case that cannot be handled with the current
> code:
> 
>- The provider offers the user different storage types, and each one
>  of them can have a different storage backend and a different set of
>  I/O limits.
> 
>- Inside each storage type, the user can have several volumes, each
>  one with its own set of I/O limits.
> 
>- So the user could have drive A, B and C, each one of them with its
>  own separate limits, and in addition to that the combination of all
>  three would have a limit specific to the storage type.
> 
> I believe that this could be achieved by simply having two filter nodes
> per drive, one of them would be attached to a throttling object specific
> to that drive, and the other one would be attached to a throttling
> object that would be shared by all drives (i.e. it would be a 3-drive
> throttling group).
> 
> I think this should be possible with the -object approach that we are
> discussing.

Yes, that should be possible. In fact, I don't think it's out of scope
for Manos' work, but it should just natually fall out of it.

That you can put throttle filter nodes anywhere in the graph (and
therefore also stack multiple throttle filter nodes on top of each
other) is the whole point of doing this work, so I would consider this a
very basic requirement for any configuration interface that we could
come up wih.

Kevin

Re: [Qemu-block] [PATCH v2 2/4] migration: Inactivate images after .save_live_complete_precopy()

[Jeff Cody]

On Tue, May 30, 2017 at 05:22:51PM +0200, Kevin Wolf wrote:
> Block migration may still access the image during its
> .save_live_complete_precopy() implementation, so we should only
> inactivate the image afterwards.
> 
> Another reason for the change is that inactivating an image fails when
> there is still a non-device BlockBackend using it, which includes the
> BBs used by block migration. We want to give block migration a chance to
> release the BBs before trying to inactivate the image (this will be done
> in another patch).
> 
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Juan Quintela 
> Reviewed-by: Eric Blake 
> ---
>  migration/migration.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index ad29e53..77a05d1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1820,17 +1820,19 @@ static void migration_completion(MigrationState *s, 
> int current_active_state,
>  
>  if (!ret) {
>  ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> +if (ret >= 0) {
> +qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
> +qemu_savevm_state_complete_precopy(s->to_dst_file, false);
> +}
>  /*
>   * Don't mark the image with BDRV_O_INACTIVE flag if
>   * we will go into COLO stage later.
>   */
>  if (ret >= 0 && !migrate_colo_enabled()) {
>  ret = bdrv_inactivate_all();
> -}
> -if (ret >= 0) {
> -qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
> -qemu_savevm_state_complete_precopy(s->to_dst_file, false);
> -s->block_inactive = true;
> +if (ret >= 0) {
> +s->block_inactive = true;
> +}
>  }
>  }
>  qemu_mutex_unlock_iothread();
> -- 
> 1.8.3.1
> 
>

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

30.05.2017 17:53, Eric Blake wrote:

On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:

The function should collect statistics, about allocted/unallocated by
top-level format driver space (in its .file) and allocation status
(allocated/hole/after eof) of corresponding areas in this .file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c   | 16 
  include/block/block.h |  3 +++
  include/block/block_int.h |  2 ++
  qapi/block-core.json  | 26 ++
  4 files changed, 47 insertions(+)

diff --git a/block.c b/block.c
index 50ba264143..7d720ae0c2 100644
--- a/block.c
+++ b/qapi/block-core.json
@@ -139,6 +139,32 @@
 '*format-specific': 'ImageInfoSpecific' } }
  
  ##

+# @BlockFormatAllocInfo:
+#
+# Information about allocations, including metadata. All fields are in bytes.
+#
+# @alloc_alloc: allocated by format driver and allocated in underlying file
+#

New structs should favor naming with '-' rather than '_'. So this should
be 'alloc-alloc', if we like the name.

So this statistic represents the portions of the format file that are in
use by the format, and which are backed by data in the underlying protocol.


+# @alloc_hole: allocated by format driver but actually is a hole in
+#  underlying file

The portions of the format file in use by the format, but where the
entire cluster is a hole in the underlying file (note that with cluster
size large enough, you can get a cluster that is part-data/part-hole in
the underlying file, it looks like you are counting those as data).


No, I account on sector boundary on bs->file. So it is not cluster-aligned.




+#
+# @alloc_overhead: allocated by format driver after end of underlying file

The portions of the format file in use by the format, but where the
entire cluster is beyond the end of the underlying file (the effective
hole).  Do we really need to distinguish between hole within the
underlying file and hole beyond the end of the file? Or can this number
be combined with the previous?


alloc_alloc + alloc_hole + hole_alloc + hole_hole = size of bs->file, I 
think this is good and clear.


these 4 stats describes bs->file usage in details (not on cluster 
boundary, but on sector, based on bs->file block status), alloc_overhead 
is separate.





+#
+# @hole_alloc: not allocated by format driver but allocated in underlying file

I'm not sure I like this name.  "Hole" in file-system terminology means
"reads-as-zero" - but here we are talking about portions of the format
layer that are unallocated (defer to backing file, and we can't
guarantee they read as zero unless there is no backing file).

This statistic represents the portion of the underlying file that has
been previously allocated to hold data, but which is now unused by the
format layer (in other words, leaked clusters that can be reclaimed, or
which can be reused instead of growing the underlying the file if new
clusters are allocated).


+#
+# @hole_hole: not allocated by format driver hole in underlying file

This statistic represents fragmentation - portions of the format layer
that are no longer in use, but which are also occupying no space in the
underlying file.  A refragmentation operation (if we ever implemented
one) could remove the address space used by these clusters, but would
not change the disk usage.


+#
+# Since: 2.10
+#
+##
+{ 'struct': 'BlockFormatAllocInfo',
+  'data': {'alloc_alloc':'uint64',
+   'alloc_hole': 'uint64',
+   'alloc_overhead': 'uint64',
+   'hole_alloc': 'uint64',
+   'hole_hole':  'uint64' } }

The idea seems okay, but the naming needs to be fixed.  Also, I'm not
sure if we need all 5, or if 4 is enough; and I'm not sure if we have
the right names ("how does alloc-hole differ from hole-alloc?"), or if
we can come up with something more descriptive.  Particularly since
"hole-" is not a hole in the filesystem sense, so much as unused
clusters.  But I'm also not coming up with better names to suggest at
the moment.

yes, naming is a problem. Proposed has the advantage of short names, and 
easy to use, if you have read documentation) But without reading docs, 
it is hard to distinguish, what do they mean..



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 1/4] block: Fix anonymous BBs in blk_root_inactivate()

[Jeff Cody]

On Tue, May 30, 2017 at 05:22:50PM +0200, Kevin Wolf wrote:
> blk->name isn't an array, but a pointer that can be NULL. Checking for
> an anonymous BB must involve a NULL check first, otherwise we get
> crashes.
> 
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Juan Quintela 
> Reviewed-by: Eric Blake 
> ---
>  block/block-backend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index f3a6008..7d7f369 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -168,7 +168,7 @@ static int blk_root_inactivate(BdrvChild *child)
>   * this point because the VM is stopped) and unattached monitor-owned
>   * BlockBackends. If there is still any other user like a block job, then
>   * we simply can't inactivate the image. */
> -if (!blk->dev && !blk->name[0]) {
> +if (!blk->dev && !blk_name(blk)[0]) {
>  return -EPERM;
>  }
>  
> -- 
> 1.8.3.1
> 
>

Reviewed-by: Jeff Cody 

[Qemu-block] [PATCH v2 3/4] migration/block: Clean up BBs in block_save_complete()

[Kevin Wolf]

We need to release any block migrations BlockBackends on the source
before successfully completing the migration because otherwise
inactivating the images will fail (inactivation only tolerates device
BBs).

Signed-off-by: Kevin Wolf 
Reviewed-by: Fam Zheng 
Reviewed-by: Eric Blake 
---
 migration/block.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 13f90d3..deb4b0e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -673,16 +673,14 @@ static int64_t get_remaining_dirty(void)
 return dirty << BDRV_SECTOR_BITS;
 }
 
-/* Called with iothread lock taken.  */
 
-static void block_migration_cleanup(void *opaque)
+
+/* Called with iothread lock taken.  */
+static void block_migration_cleanup_bmds(void)
 {
 BlkMigDevState *bmds;
-BlkMigBlock *blk;
 AioContext *ctx;
 
-bdrv_drain_all();
-
 unset_dirty_tracking();
 
 while ((bmds = QSIMPLEQ_FIRST(_mig_state.bmds_list)) != NULL) {
@@ -700,6 +698,16 @@ static void block_migration_cleanup(void *opaque)
 g_free(bmds->aio_bitmap);
 g_free(bmds);
 }
+}
+
+/* Called with iothread lock taken.  */
+static void block_migration_cleanup(void *opaque)
+{
+BlkMigBlock *blk;
+
+bdrv_drain_all();
+
+block_migration_cleanup_bmds();
 
 blk_mig_lock();
 while ((blk = QSIMPLEQ_FIRST(_mig_state.blk_list)) != NULL) {
@@ -843,6 +851,10 @@ static int block_save_complete(QEMUFile *f, void *opaque)
 
 qemu_put_be64(f, BLK_MIG_FLAG_EOS);
 
+/* Make sure that our BlockBackends are gone, so that the block driver
+ * nodes can be inactivated. */
+block_migration_cleanup_bmds();
+
 return 0;
 }
 
-- 
1.8.3.1

[Qemu-block] [PATCH v2 0/4] Block migration (migrate -b) fixes

[Kevin Wolf]

v2:
- Detect when migrate -b is compiled out [Eric]
- Switched the whole test to QMP for this, HMP is a bit hard to
  deal with

Kevin Wolf (4):
  block: Fix anonymous BBs in blk_root_inactivate()
  migration: Inactivate images after .save_live_complete_precopy()
  migration/block: Clean up BBs in block_save_complete()
  qemu-iotests: Block migration test

 block/block-backend.c  |   2 +-
 migration/block.c  |  22 +--
 migration/migration.c  |  12 ++--
 tests/qemu-iotests/183 | 143 +
 tests/qemu-iotests/183.out |  46 +++
 tests/qemu-iotests/group   |   1 +
 6 files changed, 215 insertions(+), 11 deletions(-)
 create mode 100755 tests/qemu-iotests/183
 create mode 100644 tests/qemu-iotests/183.out

-- 
1.8.3.1

[Qemu-block] [PATCH v2 4/4] qemu-iotests: Block migration test

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/183 | 143 +
 tests/qemu-iotests/183.out |  46 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 190 insertions(+)
 create mode 100755 tests/qemu-iotests/183
 create mode 100644 tests/qemu-iotests/183.out

diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
new file mode 100755
index 000..5eda0e9
--- /dev/null
+++ b/tests/qemu-iotests/183
@@ -0,0 +1,143 @@
+#!/bin/bash
+#
+# Test old-style block migration (migrate -b)
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+MIG_SOCKET="${TEST_DIR}/migrate"
+
+_cleanup()
+{
+rm -f "${MIG_SOCKET}"
+rm -f "${TEST_IMG}.dest"
+   _cleanup_test_img
+_cleanup_qemu
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+size=64M
+_make_test_img $size
+TEST_IMG="${TEST_IMG}.dest" _make_test_img $size
+
+echo
+echo === Starting VMs ===
+echo
+
+qemu_comm_method="qmp"
+
+_launch_qemu \
+-drive file="${TEST_IMG}",cache=${CACHEMODE},driver=$IMGFMT,id=disk
+src=$QEMU_HANDLE
+_send_qemu_cmd $src "{ 'execute': 'qmp_capabilities' }" 'return'
+
+_launch_qemu \
+-drive file="${TEST_IMG}.dest",cache=${CACHEMODE},driver=$IMGFMT,id=disk \
+-incoming "unix:${MIG_SOCKET}"
+dest=$QEMU_HANDLE
+_send_qemu_cmd $dest "{ 'execute': 'qmp_capabilities' }" 'return'
+
+echo
+echo === Write something on the source ===
+echo
+
+_send_qemu_cmd $src \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io disk \"write -P 0x55 0 64k\"' } }" \
+'return'
+_send_qemu_cmd $src \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io disk \"read -P 0x55 0 64k\"' } }" \
+'return'
+
+echo
+echo === Do block migration to destination ===
+echo
+
+reply="$(_send_qemu_cmd $src \
+"{ 'execute': 'migrate',
+   'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \
+'return\|error')"
+echo "$reply"
+if echo "$reply" | grep "compiled without old-style" > /dev/null; then
+_notrun "migrate -b support not compiled in"
+fi
+
+while _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" "return"  |
+  grep '"status": "active"' > /dev/null
+do
+sleep 0.1
+done
+_send_qemu_cmd $src "{ 'execute': 'query-status' }" "return"
+
+echo
+echo === Do some I/O on the destination ===
+echo
+
+# It is important that we use the BlockBackend of the guest device here instead
+# of the node name, which would create a new BlockBackend and not test whether
+# the guest has the necessary permissions to access the image now
+silent=yes _send_qemu_cmd $dest "" "100 %"
+_send_qemu_cmd $dest "{ 'execute': 'query-status' }" "return"
+_send_qemu_cmd $dest \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io disk \"read -P 0x55 0 64k\"' } }" \
+'return'
+_send_qemu_cmd $dest \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io disk \"write -P 0x66 1M 64k\"' } }" \
+'return'
+
+echo
+echo === Shut down and check image ===
+echo
+
+_send_qemu_cmd $src '{"execute":"quit"}' 'return'
+_send_qemu_cmd $dest '{"execute":"quit"}' 'return'
+wait=1 _cleanup_qemu
+
+_check_test_img
+TEST_IMG="${TEST_IMG}.dest" _check_test_img
+
+$QEMU_IO -c 'write -P 0x66 1M 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG compare "$TEST_IMG.dest" "$TEST_IMG"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/183.out b/tests/qemu-iotests/183.out
new file mode 100644
index 000..6647488
--- /dev/null
+++ b/tests/qemu-iotests/183.out
@@ -0,0 +1,46 @@
+QA output created by 183
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.dest', fmt=IMGFMT size=67108864
+
+=== Starting VMs ===
+
+{"return": {}}
+{"return": {}}
+
+=== Write something on the source ===
+
+wrote 65536/65536 bytes at offset 0

[Qemu-block] [PATCH v2 2/4] migration: Inactivate images after .save_live_complete_precopy()

[Kevin Wolf]

Block migration may still access the image during its
.save_live_complete_precopy() implementation, so we should only
inactivate the image afterwards.

Another reason for the change is that inactivating an image fails when
there is still a non-device BlockBackend using it, which includes the
BBs used by block migration. We want to give block migration a chance to
release the BBs before trying to inactivate the image (this will be done
in another patch).

Signed-off-by: Kevin Wolf 
Reviewed-by: Fam Zheng 
Reviewed-by: Juan Quintela 
Reviewed-by: Eric Blake 
---
 migration/migration.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index ad29e53..77a05d1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1820,17 +1820,19 @@ static void migration_completion(MigrationState *s, int 
current_active_state,
 
 if (!ret) {
 ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+if (ret >= 0) {
+qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
+qemu_savevm_state_complete_precopy(s->to_dst_file, false);
+}
 /*
  * Don't mark the image with BDRV_O_INACTIVE flag if
  * we will go into COLO stage later.
  */
 if (ret >= 0 && !migrate_colo_enabled()) {
 ret = bdrv_inactivate_all();
-}
-if (ret >= 0) {
-qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
-qemu_savevm_state_complete_precopy(s->to_dst_file, false);
-s->block_inactive = true;
+if (ret >= 0) {
+s->block_inactive = true;
+}
 }
 }
 qemu_mutex_unlock_iothread();
-- 
1.8.3.1

[Qemu-block] [PATCH v2 1/4] block: Fix anonymous BBs in blk_root_inactivate()

[Kevin Wolf]

blk->name isn't an array, but a pointer that can be NULL. Checking for
an anonymous BB must involve a NULL check first, otherwise we get
crashes.

Signed-off-by: Kevin Wolf 
Reviewed-by: Fam Zheng 
Reviewed-by: Juan Quintela 
Reviewed-by: Eric Blake 
---
 block/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f3a6008..7d7f369 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -168,7 +168,7 @@ static int blk_root_inactivate(BdrvChild *child)
  * this point because the VM is stopped) and unattached monitor-owned
  * BlockBackends. If there is still any other user like a block job, then
  * we simply can't inactivate the image. */
-if (!blk->dev && !blk->name[0]) {
+if (!blk->dev && !blk_name(blk)[0]) {
 return -EPERM;
 }
 
-- 
1.8.3.1

Re: [Qemu-block] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Eric Blake]

On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
> The function should collect statistics, about allocted/unallocated by
> top-level format driver space (in its .file) and allocation status
> (allocated/hole/after eof) of corresponding areas in this .file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block.c   | 16 
>  include/block/block.h |  3 +++
>  include/block/block_int.h |  2 ++
>  qapi/block-core.json  | 26 ++
>  4 files changed, 47 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 50ba264143..7d720ae0c2 100644
> --- a/block.c

> +++ b/qapi/block-core.json
> @@ -139,6 +139,32 @@
> '*format-specific': 'ImageInfoSpecific' } }
>  
>  ##
> +# @BlockFormatAllocInfo:
> +#
> +# Information about allocations, including metadata. All fields are in bytes.
> +#
> +# @alloc_alloc: allocated by format driver and allocated in underlying file
> +#

New structs should favor naming with '-' rather than '_'. So this should
be 'alloc-alloc', if we like the name.

So this statistic represents the portions of the format file that are in
use by the format, and which are backed by data in the underlying protocol.

> +# @alloc_hole: allocated by format driver but actually is a hole in
> +#  underlying file

The portions of the format file in use by the format, but where the
entire cluster is a hole in the underlying file (note that with cluster
size large enough, you can get a cluster that is part-data/part-hole in
the underlying file, it looks like you are counting those as data).

> +#
> +# @alloc_overhead: allocated by format driver after end of underlying file

The portions of the format file in use by the format, but where the
entire cluster is beyond the end of the underlying file (the effective
hole).  Do we really need to distinguish between hole within the
underlying file and hole beyond the end of the file? Or can this number
be combined with the previous?

> +#
> +# @hole_alloc: not allocated by format driver but allocated in underlying 
> file

I'm not sure I like this name.  "Hole" in file-system terminology means
"reads-as-zero" - but here we are talking about portions of the format
layer that are unallocated (defer to backing file, and we can't
guarantee they read as zero unless there is no backing file).

This statistic represents the portion of the underlying file that has
been previously allocated to hold data, but which is now unused by the
format layer (in other words, leaked clusters that can be reclaimed, or
which can be reused instead of growing the underlying the file if new
clusters are allocated).

> +#
> +# @hole_hole: not allocated by format driver hole in underlying file

This statistic represents fragmentation - portions of the format layer
that are no longer in use, but which are also occupying no space in the
underlying file.  A refragmentation operation (if we ever implemented
one) could remove the address space used by these clusters, but would
not change the disk usage.

> +#
> +# Since: 2.10
> +#
> +##
> +{ 'struct': 'BlockFormatAllocInfo',
> +  'data': {'alloc_alloc':'uint64',
> +   'alloc_hole': 'uint64',
> +   'alloc_overhead': 'uint64',
> +   'hole_alloc': 'uint64',
> +   'hole_hole':  'uint64' } }

The idea seems okay, but the naming needs to be fixed.  Also, I'm not
sure if we need all 5, or if 4 is enough; and I'm not sure if we have
the right names ("how does alloc-hole differ from hole-alloc?"), or if
we can come up with something more descriptive.  Particularly since
"hole-" is not a hole in the filesystem sense, so much as unused
clusters.  But I'm also not coming up with better names to suggest at
the moment.

-- 
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] [RFC] Making 'block-stream', and 'block-commit' accept node-name

[Alberto Garcia]

On Mon 29 May 2017 09:03:22 PM CEST, Kashyap Chamarthy wrote:

> Observe the following ('qmp-shell', for brevity) invocation of the
> four major types (stream, commit, mirror, backup) of live block
> operations:
>
> (QEMU) block-stream device=node-D base=a.qcow2 job-id=job-block-stream
> (QEMU) block-commit device=node-D base=a.qcow2 top=b.qcow2 
> job-id=job-block-commit
> (QEMU) drive-mirror device=node-D target=e.qcow2 sync=full 
> job-id=job-drive-mirror
> (QEMU) drive-backup device=node-D sync=full target=e.qcow2 
> job-id=job-drive-backup
> (QEMU) blockdev-backup device=node-B target=node-E sync=full 
> job-id=job-blockdev-backup
> (QEMU) blockdev-mirror device=node-D target=node-E sync=full 
> job-id=job-blockdev-mirror
>
> While we have `blockdev-{mirror, backup}` as 'node-name'-alternatives
> for `drive-{mirror, backup}`, as the eagle-eyed will, the
> `block-stream` and `block-commit` commands still operate on file names
> for parameters 'base' and 'top'.

block-stream does accept a node name for the top and base images (see
554b614765090f47d and 312fe09cc8af86c).

block-commit hasn't changed, but it should be possible to follow a
similar approach.

Berto

Re: [Qemu-block] Throttling groups vs filter nodes

[Alberto Garcia]

On Sat 27 May 2017 09:56:03 AM CEST, Stefan Hajnoczi wrote:
> Throttling groups allow multiple drives to share the same throttling
> state (i.e. budget) between them.  Manos is working on moving the
> throttling code into a block filter driver so it is no longer
> hardcoded into the I/O code path.

Now that we're discussing changes to the throttling code, I'll take the
opportunity to describe a feature that I'd like to work on at some point
in the future.

This is of course out of the scope of the work that Manos is going to
do, but I'd like to discuss it briefly to see what people think of this
idea and whether it's compatible with the changes that we are now
proposing.

Currently each block device can have either zero or one set of I/O
limits, and that set of limits can be shared among several drives using
throttling groups.

I have the following use case that cannot be handled with the current
code:

   - The provider offers the user different storage types, and each one
 of them can have a different storage backend and a different set of
 I/O limits.

   - Inside each storage type, the user can have several volumes, each
 one with its own set of I/O limits.

   - So the user could have drive A, B and C, each one of them with its
 own separate limits, and in addition to that the combination of all
 three would have a limit specific to the storage type.

I believe that this could be achieved by simply having two filter nodes
per drive, one of them would be attached to a throttling object specific
to that drive, and the other one would be attached to a throttling
object that would be shared by all drives (i.e. it would be a 3-drive
throttling group).

I think this should be possible with the -object approach that we are
discussing.

Thoughts?

Berto

Re: [Qemu-block] [PATCH v3 0/4] block: fix 'savevm' hang with -object iothread

[Stefan Hajnoczi]

On Mon, May 22, 2017 at 02:57:00PM +0100, Stefan Hajnoczi wrote:
> v3:
>  * Add missing bdrv_drain_all_end() in error code paths [Kevin]
> v2:
>  * New patch to use bdrv_drain_all_begin/end() in savevm/loadvm [Kevin]
>(All other patches unchanged)
> 
> The 'savevm' command hangs when -object iothread is used.  See patches for
> details, but basically the vmstate read/write code didn't conform to the 
> latest
> block layer locking rules.
> 
> Stefan Hajnoczi (4):
>   block: count bdrv_co_rw_vmstate() requests
>   block: use BDRV_POLL_WHILE() in bdrv_rw_vmstate()
>   migration: avoid recursive AioContext locking in save_vmstate()
>   migration: use bdrv_drain_all_begin/end() instead bdrv_drain_all()
> 
>  block/io.c | 21 +
>  migration/savevm.c | 30 ++
>  2 files changed, 39 insertions(+), 12 deletions(-)

Ping


signature.asc
Description: PGP signature

[Qemu-block] [PATCH 3/4] qemu-img check: add format allocation info

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json |  6 +-
 qemu-img.c   | 36 
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 365070b3eb..3357b61811 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -203,6 +203,9 @@
 #   field is present if the driver for the image format
 #   supports it
 #
+# @format-alloc-info: Format-allocation information, see
+# BlockFormatAllocInfo description. (Since: 2.10)
+#
 # Since: 1.4
 #
 ##
@@ -211,7 +214,8 @@
'*image-end-offset': 'int', '*corruptions': 'int', '*leaks': 'int',
'*corruptions-fixed': 'int', '*leaks-fixed': 'int',
'*total-clusters': 'int', '*allocated-clusters': 'int',
-   '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
+   '*fragmented-clusters': 'int', '*compressed-clusters': 'int',
+   '*format-alloc-info': 'BlockFormatAllocInfo' } }
 
 ##
 # @MapEntry:
diff --git a/qemu-img.c b/qemu-img.c
index b506839ef0..55f8c1776c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,6 +560,29 @@ static void dump_json_image_check(ImageCheck *check, bool 
quiet)
 QDECREF(str);
 }
 
+static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool 
quiet)
+{
+char *alloc_alloc = size_to_str(bfai->alloc_alloc);
+char *alloc_hole = size_to_str(bfai->alloc_hole);
+char *alloc_overhead = size_to_str(bfai->alloc_overhead);
+char *hole_alloc = size_to_str(bfai->hole_alloc);
+char *hole_hole = size_to_str(bfai->hole_hole);
+
+qprintf(quiet,
+"Format allocation info (including metadata):\n"
+"   file-alloc   file-hole   after-eof\n"
+"format-alloc   %10s  %10s  %10s\n"
+"format-hole%10s  %10s\n",
+alloc_alloc, alloc_hole, alloc_overhead,
+hole_alloc, hole_hole);
+
+g_free(alloc_alloc);
+g_free(alloc_hole);
+g_free(alloc_overhead);
+g_free(hole_alloc);
+g_free(hole_hole);
+}
+
 static void dump_human_image_check(ImageCheck *check, bool quiet)
 {
 if (!(check->corruptions || check->leaks || check->check_errors)) {
@@ -601,6 +624,10 @@ static void dump_human_image_check(ImageCheck *check, bool 
quiet)
 qprintf(quiet,
 "Image end offset: %" PRId64 "\n", check->image_end_offset);
 }
+
+if (check->has_format_alloc_info) {
+dump_human_format_alloc_info(check->format_alloc_info, quiet);
+}
 }
 
 static int collect_image_check(BlockDriverState *bs,
@@ -611,6 +638,7 @@ static int collect_image_check(BlockDriverState *bs,
 {
 int ret;
 BdrvCheckResult result;
+BlockFormatAllocInfo *bfai = g_new0(BlockFormatAllocInfo, 1);
 
 ret = bdrv_check(bs, , fix);
 if (ret < 0) {
@@ -639,6 +667,14 @@ static int collect_image_check(BlockDriverState *bs,
 check->compressed_clusters  = result.bfi.compressed_clusters;
 check->has_compressed_clusters  = result.bfi.compressed_clusters != 0;
 
+ret = bdrv_get_format_alloc_stat(bs, bfai);
+if (ret < 0) {
+qapi_free_BlockFormatAllocInfo(bfai);
+} else {
+check->has_format_alloc_info = true;
+check->format_alloc_info = bfai;
+}
+
 return 0;
 }
 
-- 
2.11.1

[Qemu-block] [PATCH v2 0/4] qemu-img check: format allocation info

[Vladimir Sementsov-Ogievskiy]

Hi all.

v2: fix build error, gcc things that some variables may be used
uninitialized (actually they didn't).

These series is a replacement for "qemu-img check: unallocated size"
series.

There was a question, should we account allocated clusters in qcow2 but
actually holes in underalying file as allocated or not. Instead of
hiding this information in one-number statistic I've decided to print
the whole information, 5 numbers:

For allocated by top-level format driver (qcow2 for ex.) clusters, 3
numbers: number of bytes, which are:
 - allocated in underlying file
 - holes in underlying file
 - after end of underlying file

To account other areas of underlying file, 2 more numbers of bytes:
 - unallocated by top-level driver but allocated in underlying file
 - unallocated by top-level driver and holes in underlying file

Vladimir Sementsov-Ogievskiy (4):
  block: add bdrv_get_format_alloc_stat format interface
  qcow2: add .bdrv_get_format_alloc_stat
  qemu-img check: add format allocation info
  qemu-img check: improve dump_human_format_alloc_info

 block.c   |  16 ++
 block/qcow2-refcount.c| 144 ++
 block/qcow2.c |   2 +
 block/qcow2.h |   2 +
 include/block/block.h |   3 +
 include/block/block_int.h |   2 +
 qapi/block-core.json  |  32 ++-
 qemu-img.c|  57 +-
 8 files changed, 255 insertions(+), 3 deletions(-)

-- 
2.11.1

[Qemu-block] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

The function should collect statistics, about allocted/unallocated by
top-level format driver space (in its .file) and allocation status
(allocated/hole/after eof) of corresponding areas in this .file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c   | 16 
 include/block/block.h |  3 +++
 include/block/block_int.h |  2 ++
 qapi/block-core.json  | 26 ++
 4 files changed, 47 insertions(+)

diff --git a/block.c b/block.c
index 50ba264143..7d720ae0c2 100644
--- a/block.c
+++ b/block.c
@@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState 
*bs)
 }
 
 /**
+ * Collect format allocation info. See BlockFormatAllocInfo definition in
+ * qapi/block-core.json.
+ */
+int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo 
*bfai)
+{
+BlockDriver *drv = bs->drv;
+if (!drv) {
+return -ENOMEDIUM;
+}
+if (drv->bdrv_get_format_alloc_stat) {
+return drv->bdrv_get_format_alloc_stat(bs, bfai);
+}
+return -ENOTSUP;
+}
+
+/**
  * Return number of sectors on success, -errno on error.
  */
 int64_t bdrv_nb_sectors(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e92d8..646376a772 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,6 +335,9 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
+int bdrv_get_format_alloc_stat(BlockDriverState *bs,
+   BlockFormatAllocInfo *bfai);
+
 /* The units of offset and total_work_size may be chosen arbitrarily by the
  * block driver; total_work_size may change during the course of the amendment
  * operation */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..458c715e99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -208,6 +208,8 @@ struct BlockDriver {
 int64_t (*bdrv_getlength)(BlockDriverState *bs);
 bool has_variable_length;
 int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
+int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
+  BlockFormatAllocInfo *bfai);
 
 int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
 uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ea0b3e8b13..365070b3eb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -139,6 +139,32 @@
'*format-specific': 'ImageInfoSpecific' } }
 
 ##
+# @BlockFormatAllocInfo:
+#
+# Information about allocations, including metadata. All fields are in bytes.
+#
+# @alloc_alloc: allocated by format driver and allocated in underlying file
+#
+# @alloc_hole: allocated by format driver but actually is a hole in
+#  underlying file
+#
+# @alloc_overhead: allocated by format driver after end of underlying file
+#
+# @hole_alloc: not allocated by format driver but allocated in underlying file
+#
+# @hole_hole: not allocated by format driver hole in underlying file
+#
+# Since: 2.10
+#
+##
+{ 'struct': 'BlockFormatAllocInfo',
+  'data': {'alloc_alloc':'uint64',
+   'alloc_hole': 'uint64',
+   'alloc_overhead': 'uint64',
+   'hole_alloc': 'uint64',
+   'hole_hole':  'uint64' } }
+
+##
 # @ImageCheck:
 #
 # Information about a QEMU image file check
-- 
2.11.1

[Qemu-block] [PATCH 4/4] qemu-img check: improve dump_human_format_alloc_info

[Vladimir Sementsov-Ogievskiy]

Improve dump_human_format_alloc_info() by specifying format names.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-img.c | 35 ++-
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 55f8c1776c..3c03690a4f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,7 +560,8 @@ static void dump_json_image_check(ImageCheck *check, bool 
quiet)
 QDECREF(str);
 }
 
-static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool 
quiet)
+static void dump_human_format_alloc_info(BlockDriverState *bs,
+ BlockFormatAllocInfo *bfai, bool 
quiet)
 {
 char *alloc_alloc = size_to_str(bfai->alloc_alloc);
 char *alloc_hole = size_to_str(bfai->alloc_hole);
@@ -568,13 +569,28 @@ static void 
dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
 char *hole_alloc = size_to_str(bfai->hole_alloc);
 char *hole_hole = size_to_str(bfai->hole_hole);
 
+const char *format = bdrv_get_format_name(bs);
+const char *f_format =
+bs->file ? bdrv_get_format_name(bs->file->bs) : "file";
+int format_len, cw;
+
+if (format == NULL) {
+format = "format";
+}
+if (f_format == NULL) {
+f_format = "file";
+}
+format_len = strlen(format);
+cw = MAX(10, strlen(f_format) + 6);
+
 qprintf(quiet,
 "Format allocation info (including metadata):\n"
-"   file-alloc   file-hole   after-eof\n"
-"format-alloc   %10s  %10s  %10s\n"
-"format-hole%10s  %10s\n",
-alloc_alloc, alloc_hole, alloc_overhead,
-hole_alloc, hole_hole);
+"%*s %*s-alloc  %*s-hole  %*s\n"
+"%s-alloc   %*s  %*s  %*s\n"
+"%s-hole%*s  %*s\n",
+format_len, "", cw - 6, f_format, cw - 5, f_format, cw, 
"after-eof",
+format, cw, alloc_alloc, cw, alloc_hole, cw, alloc_overhead,
+format, cw, hole_alloc, cw, hole_hole);
 
 g_free(alloc_alloc);
 g_free(alloc_hole);
@@ -583,7 +599,8 @@ static void 
dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
 g_free(hole_hole);
 }
 
-static void dump_human_image_check(ImageCheck *check, bool quiet)
+static void dump_human_image_check(BlockDriverState *bs, ImageCheck *check,
+   bool quiet)
 {
 if (!(check->corruptions || check->leaks || check->check_errors)) {
 qprintf(quiet, "No errors were found on the image.\n");
@@ -626,7 +643,7 @@ static void dump_human_image_check(ImageCheck *check, bool 
quiet)
 }
 
 if (check->has_format_alloc_info) {
-dump_human_format_alloc_info(check->format_alloc_info, quiet);
+dump_human_format_alloc_info(bs, check->format_alloc_info, quiet);
 }
 }
 
@@ -840,7 +857,7 @@ static int img_check(int argc, char **argv)
 if (!ret) {
 switch (output_format) {
 case OFORMAT_HUMAN:
-dump_human_image_check(check, quiet);
+dump_human_image_check(bs, check, quiet);
 break;
 case OFORMAT_JSON:
 dump_json_image_check(check, quiet);
-- 
2.11.1

[Qemu-block] [PATCH 2/4] qcow2: add .bdrv_get_format_alloc_stat

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_get_format_alloc_stat interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-refcount.c | 144 +
 block/qcow2.c  |   2 +
 block/qcow2.h  |   2 +
 3 files changed, 148 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..2010dfa048 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2931,3 +2931,147 @@ done:
 qemu_vfree(new_refblock);
 return ret;
 }
+
+typedef struct FormatAllocStatCo {
+BlockDriverState *bs;
+BlockFormatAllocInfo *bfai;
+int64_t ret;
+} FormatAllocStatCo;
+
+static void coroutine_fn qcow2_get_format_alloc_stat_entry(void *opaque)
+{
+int ret = 0;
+FormatAllocStatCo *nbco = opaque;
+BlockDriverState *bs = nbco->bs;
+BDRVQcow2State *s = bs->opaque;
+BlockFormatAllocInfo *bfai = nbco->bfai;
+int64_t cluster, file_sectors, sector;
+int refcount_block_offset;
+uint32_t i;
+bool allocated = false, f_allocated = false;
+int dif, num = 0, f_num = 0;
+
+memset(bfai, 0, sizeof(*bfai));
+
+file_sectors = bdrv_nb_sectors(bs->file->bs);
+if (file_sectors < 0) {
+nbco->ret = file_sectors;
+return;
+}
+
+qemu_co_mutex_lock(>lock);
+
+for (sector = 0; sector < file_sectors; sector += dif) {
+if (f_num == 0) {
+ret = bdrv_is_allocated_above(bs->file->bs, NULL, sector,
+  file_sectors - sector, _num);
+if (ret < 0) {
+goto fail;
+}
+f_allocated = ret;
+}
+
+if (num == 0) {
+uint64_t refcount;
+assert(((sector << BDRV_SECTOR_BITS) & (s->cluster_size - 1)) == 
0);
+ret = qcow2_get_refcount(
+bs, (sector << BDRV_SECTOR_BITS) >> s->cluster_bits, 
);
+if (ret < 0) {
+goto fail;
+}
+allocated = refcount > 0;
+num = s->cluster_size >> BDRV_SECTOR_BITS;
+}
+
+dif = MIN(f_num, MIN(num, file_sectors - sector));
+if (allocated) {
+if (f_allocated) {
+bfai->alloc_alloc += dif;
+} else {
+bfai->alloc_hole += dif;
+}
+} else {
+if (f_allocated) {
+bfai->hole_alloc += dif;
+} else {
+bfai->hole_hole += dif;
+}
+}
+f_num -= dif;
+num -= dif;
+}
+
+assert(f_num == 0);
+
+if (allocated) {
+bfai->alloc_overhead += num;
+}
+
+cluster = size_to_clusters(s, sector << BDRV_SECTOR_BITS);
+refcount_block_offset = cluster & (s->refcount_block_size - 1);
+for (i = cluster >> s->refcount_block_bits;
+ i <= s->max_refcount_table_index; i++)
+{
+int j;
+
+if (!(s->refcount_table[i] & REFT_OFFSET_MASK)) {
+refcount_block_offset = 0;
+continue;
+}
+
+for (j = refcount_block_offset; j < s->refcount_block_size; j++) {
+uint64_t refcount;
+cluster = (i << s->refcount_block_bits) + j;
+
+ret = qcow2_get_refcount(bs, cluster, );
+if (ret < 0) {
+goto fail;
+}
+if (refcount > 0) {
+bfai->alloc_overhead++;
+}
+}
+
+refcount_block_offset = 0;
+}
+
+qemu_co_mutex_unlock(>lock);
+
+bfai->alloc_alloc = bfai->alloc_alloc << BDRV_SECTOR_BITS;
+bfai->alloc_hole = bfai->alloc_hole << BDRV_SECTOR_BITS;
+bfai->alloc_overhead = bfai->alloc_overhead << BDRV_SECTOR_BITS;
+
+bfai->hole_alloc = bfai->hole_alloc << BDRV_SECTOR_BITS;
+bfai->hole_hole = bfai->hole_hole << BDRV_SECTOR_BITS;
+
+nbco->ret = 0;
+return;
+
+fail:
+nbco->ret = ret;
+qemu_co_mutex_unlock(>lock);
+}
+
+/* qcow2_get_format_alloc_stat()
+ * Fills @bfai struct. In case of failure @bfai content is unpredicted.
+ */
+int qcow2_get_format_alloc_stat(BlockDriverState *bs,
+BlockFormatAllocInfo *bfai)
+{
+FormatAllocStatCo nbco = {
+.bs = bs,
+.bfai = bfai,
+.ret = -EINPROGRESS
+};
+
+if (qemu_in_coroutine()) {
+qcow2_get_format_alloc_stat_entry();
+} else {
+Coroutine *co =
+qemu_coroutine_create(qcow2_get_format_alloc_stat_entry, );
+qemu_coroutine_enter(co);
+BDRV_POLL_WHILE(bs, nbco.ret == -EINPROGRESS);
+}
+
+return nbco.ret;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index a8d61f0981..0e26b548fd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3469,6 +3469,8 @@ BlockDriver bdrv_qcow2 = {
 
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+.bdrv_get_format_alloc_stat = qcow2_get_format_alloc_stat,
 };
 
 static 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] qemu-img check: format allocation info

[no-reply]

Hi,

This series failed build test on s390x host. Please find the details below.

Message-id: 20170530103641.68891-1-vsement...@virtuozzo.com
Type: series
Subject: [Qemu-devel] [PATCH 0/4] qemu-img check: format allocation info

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  patchew/20170530085943.18141-1-lpro...@redhat.com -> 
patchew/20170530085943.18141-1-lpro...@redhat.com
 * [new tag] patchew/20170530103641.68891-1-vsement...@virtuozzo.com -> 
patchew/20170530103641.68891-1-vsement...@virtuozzo.com
Switched to a new branch 'test'
63da84a qemu-img check: improve dump_human_format_alloc_info
375560f qemu-img check: add format allocation info
d6dbd38 qcow2: add .bdrv_get_format_alloc_stat
382abf1 block: add bdrv_get_format_alloc_stat format interface

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=73926
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-d5sxs1zw/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
giflib-4.1.6-15.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
libidn-1.33-1.fc25.s390x
slang-2.3.0-7.fc25.s390x
libsemanage-2.5-8.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
python-fedora-0.8.0-2.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
wget-1.18-2.fc25.s390x
dhcp-client-4.3.5-1.fc25.s390x
parted-3.2-21.fc25.s390x
flex-2.6.0-3.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
perl-Pod-Simple-3.35-1.fc25.noarch
python2-simplejson-3.10.0-1.fc25.s390x
brltty-5.4-2.fc25.s390x
librados2-10.2.4-2.fc25.s390x
tcp_wrappers-7.6-83.fc25.s390x
libcephfs_jni1-10.2.4-2.fc25.s390x
nettle-devel-3.3-1.fc25.s390x
bzip2-devel-1.0.6-21.fc25.s390x
libuuid-2.28.2-2.fc25.s390x
pango-1.40.4-1.fc25.s390x
python3-dnf-1.1.10-6.fc25.noarch
cryptsetup-libs-1.7.4-1.fc25.s390x
texlive-kpathsea-doc-svn41139-33.fc25.1.noarch
netpbm-10.77.00-3.fc25.s390x
openssh-7.4p1-4.fc25.s390x
texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x
texlive-graphics-svn41015-33.fc25.1.noarch
texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch
texlive-mfware-svn40768-33.fc25.1.noarch
texlive-texlive-scripts-svn41433-33.fc25.1.noarch
texlive-euro-svn22191.1.1-33.fc25.1.noarch
texlive-etex-svn37057.0-33.fc25.1.noarch
texlive-iftex-svn29654.0.2-33.fc25.1.noarch
texlive-palatino-svn31835.0-33.fc25.1.noarch
texlive-texlive-docindex-svn41430-33.fc25.1.noarch
texlive-xunicode-svn30466.0.981-33.fc25.1.noarch
texlive-koma-script-svn41508-33.fc25.1.noarch
texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch
texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch
texlive-jknapltx-svn19440.0-33.fc25.1.noarch
netpbm-progs-10.77.00-3.fc25.s390x
texinfo-6.1-4.fc25.s390x
openssl-devel-1.0.2k-1.fc25.s390x
python2-sssdconfig-1.15.2-1.fc25.noarch
gdk-pixbuf2-2.36.6-1.fc25.s390x
mesa-libEGL-13.0.4-3.fc25.s390x
pcre-cpp-8.40-6.fc25.s390x
pcre-utf16-8.40-6.fc25.s390x
glusterfs-extra-xlators-3.10.1-1.fc25.s390x
mesa-libGL-devel-13.0.4-3.fc25.s390x
nss-devel-3.29.3-1.1.fc25.s390x
libaio-0.3.110-6.fc24.s390x
libfontenc-1.1.3-3.fc24.s390x
lzo-2.08-8.fc24.s390x
isl-0.14-5.fc24.s390x
libXau-1.0.8-6.fc24.s390x
linux-atm-libs-2.5.1-14.fc24.s390x
libXext-1.3.3-4.fc24.s390x
libXxf86vm-1.1.4-3.fc24.s390x
bison-3.0.4-4.fc24.s390x
perl-srpm-macros-1-20.fc25.noarch
gawk-4.1.3-8.fc25.s390x
libwayland-client-1.12.0-1.fc25.s390x
perl-Exporter-5.72-366.fc25.noarch
perl-version-0.99.17-1.fc25.s390x
fftw-libs-double-3.3.5-3.fc25.s390x
libssh2-1.8.0-1.fc25.s390x
ModemManager-glib-1.6.4-1.fc25.s390x
newt-python3-0.52.19-2.fc25.s390x
python-munch-2.0.4-3.fc25.noarch
python-bugzilla-1.2.2-4.fc25.noarch
libedit-3.1-16.20160618cvs.fc25.s390x
python-pycurl-7.43.0-4.fc25.s390x
createrepo_c-0.10.0-6.fc25.s390x
device-mapper-multipath-libs-0.4.9-83.fc25.s390x
yum-3.4.3-510.fc25.noarch
dhcp-common-4.3.5-1.fc25.noarch
dracut-config-rescue-044-78.fc25.s390x
teamd-1.26-1.fc25.s390x
mozjs17-17.0.0-16.fc25.s390x
libselinux-2.5-13.fc25.s390x

[Qemu-block] [PATCH 2/4] qcow2: add .bdrv_get_format_alloc_stat

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_get_format_alloc_stat interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-refcount.c | 144 +
 block/qcow2.c  |   2 +
 block/qcow2.h  |   2 +
 3 files changed, 148 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..e1c4f6c7e0 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2931,3 +2931,147 @@ done:
 qemu_vfree(new_refblock);
 return ret;
 }
+
+typedef struct FormatAllocStatCo {
+BlockDriverState *bs;
+BlockFormatAllocInfo *bfai;
+int64_t ret;
+} FormatAllocStatCo;
+
+static void coroutine_fn qcow2_get_format_alloc_stat_entry(void *opaque)
+{
+int ret = 0;
+FormatAllocStatCo *nbco = opaque;
+BlockDriverState *bs = nbco->bs;
+BDRVQcow2State *s = bs->opaque;
+BlockFormatAllocInfo *bfai = nbco->bfai;
+int64_t cluster, file_sectors, sector;
+int refcount_block_offset;
+uint32_t i;
+bool allocated, f_allocated;
+int dif, num = 0, f_num = 0;
+
+memset(bfai, 0, sizeof(*bfai));
+
+file_sectors = bdrv_nb_sectors(bs->file->bs);
+if (file_sectors < 0) {
+nbco->ret = file_sectors;
+return;
+}
+
+qemu_co_mutex_lock(>lock);
+
+for (sector = 0; sector < file_sectors; sector += dif) {
+if (f_num == 0) {
+ret = bdrv_is_allocated_above(bs->file->bs, NULL, sector,
+  file_sectors - sector, _num);
+if (ret < 0) {
+goto fail;
+}
+f_allocated = ret;
+}
+
+if (num == 0) {
+uint64_t refcount;
+assert(((sector << BDRV_SECTOR_BITS) & (s->cluster_size - 1)) == 
0);
+ret = qcow2_get_refcount(
+bs, (sector << BDRV_SECTOR_BITS) >> s->cluster_bits, 
);
+if (ret < 0) {
+goto fail;
+}
+allocated = refcount > 0;
+num = s->cluster_size >> BDRV_SECTOR_BITS;
+}
+
+dif = MIN(f_num, MIN(num, file_sectors - sector));
+if (allocated) {
+if (f_allocated) {
+bfai->alloc_alloc += dif;
+} else {
+bfai->alloc_hole += dif;
+}
+} else {
+if (f_allocated) {
+bfai->hole_alloc += dif;
+} else {
+bfai->hole_hole += dif;
+}
+}
+f_num -= dif;
+num -= dif;
+}
+
+assert(f_num == 0);
+
+if (allocated) {
+bfai->alloc_overhead += num;
+}
+
+cluster = size_to_clusters(s, sector << BDRV_SECTOR_BITS);
+refcount_block_offset = cluster & (s->refcount_block_size - 1);
+for (i = cluster >> s->refcount_block_bits;
+ i <= s->max_refcount_table_index; i++)
+{
+int j;
+
+if (!(s->refcount_table[i] & REFT_OFFSET_MASK)) {
+refcount_block_offset = 0;
+continue;
+}
+
+for (j = refcount_block_offset; j < s->refcount_block_size; j++) {
+uint64_t refcount;
+cluster = (i << s->refcount_block_bits) + j;
+
+ret = qcow2_get_refcount(bs, cluster, );
+if (ret < 0) {
+goto fail;
+}
+if (refcount > 0) {
+bfai->alloc_overhead++;
+}
+}
+
+refcount_block_offset = 0;
+}
+
+qemu_co_mutex_unlock(>lock);
+
+bfai->alloc_alloc = bfai->alloc_alloc << BDRV_SECTOR_BITS;
+bfai->alloc_hole = bfai->alloc_hole << BDRV_SECTOR_BITS;
+bfai->alloc_overhead = bfai->alloc_overhead << BDRV_SECTOR_BITS;
+
+bfai->hole_alloc = bfai->hole_alloc << BDRV_SECTOR_BITS;
+bfai->hole_hole = bfai->hole_hole << BDRV_SECTOR_BITS;
+
+nbco->ret = 0;
+return;
+
+fail:
+nbco->ret = ret;
+qemu_co_mutex_unlock(>lock);
+}
+
+/* qcow2_get_format_alloc_stat()
+ * Fills @bfai struct. In case of failure @bfai content is unpredicted.
+ */
+int qcow2_get_format_alloc_stat(BlockDriverState *bs,
+BlockFormatAllocInfo *bfai)
+{
+FormatAllocStatCo nbco = {
+.bs = bs,
+.bfai = bfai,
+.ret = -EINPROGRESS
+};
+
+if (qemu_in_coroutine()) {
+qcow2_get_format_alloc_stat_entry();
+} else {
+Coroutine *co =
+qemu_coroutine_create(qcow2_get_format_alloc_stat_entry, );
+qemu_coroutine_enter(co);
+BDRV_POLL_WHILE(bs, nbco.ret == -EINPROGRESS);
+}
+
+return nbco.ret;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index a8d61f0981..0e26b548fd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3469,6 +3469,8 @@ BlockDriver bdrv_qcow2 = {
 
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+.bdrv_get_format_alloc_stat = qcow2_get_format_alloc_stat,
 };
 
 static void 

[Qemu-block] [PATCH 0/4] qemu-img check: format allocation info

[Vladimir Sementsov-Ogievskiy]

Hi all.

These series is a replacement for "qemu-img check: unallocated size"
series.

There was a question, should we account allocated clusters in qcow2 but
actually holes in underalying file as allocated or not. Instead of
hiding this information in one-number statistic I've decided to print
the whole information, 5 numbers:

For allocated by top-level format driver (qcow2 for ex.) clusters, 3
numbers: number of bytes, which are:
 - allocated in underlying file
 - holes in underlying file
 - after end of underlying file

To account other areas of underlying file, 2 more numbers of bytes:
 - unallocated by top-level driver but allocated in underlying file
 - unallocated by top-level driver and holes in underlying file

Vladimir Sementsov-Ogievskiy (4):
  block: add bdrv_get_format_alloc_stat format interface
  qcow2: add .bdrv_get_format_alloc_stat
  qemu-img check: add format allocation info
  qemu-img check: improve dump_human_format_alloc_info

 block.c   |  16 ++
 block/qcow2-refcount.c| 144 ++
 block/qcow2.c |   2 +
 block/qcow2.h |   2 +
 include/block/block.h |   3 +
 include/block/block_int.h |   2 +
 qapi/block-core.json  |  32 ++-
 qemu-img.c|  57 +-
 8 files changed, 255 insertions(+), 3 deletions(-)

-- 
2.11.1

[Qemu-block] [PATCH 3/4] qemu-img check: add format allocation info

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json |  6 +-
 qemu-img.c   | 36 
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 365070b3eb..3357b61811 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -203,6 +203,9 @@
 #   field is present if the driver for the image format
 #   supports it
 #
+# @format-alloc-info: Format-allocation information, see
+# BlockFormatAllocInfo description. (Since: 2.10)
+#
 # Since: 1.4
 #
 ##
@@ -211,7 +214,8 @@
'*image-end-offset': 'int', '*corruptions': 'int', '*leaks': 'int',
'*corruptions-fixed': 'int', '*leaks-fixed': 'int',
'*total-clusters': 'int', '*allocated-clusters': 'int',
-   '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
+   '*fragmented-clusters': 'int', '*compressed-clusters': 'int',
+   '*format-alloc-info': 'BlockFormatAllocInfo' } }
 
 ##
 # @MapEntry:
diff --git a/qemu-img.c b/qemu-img.c
index b506839ef0..55f8c1776c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,6 +560,29 @@ static void dump_json_image_check(ImageCheck *check, bool 
quiet)
 QDECREF(str);
 }
 
+static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool 
quiet)
+{
+char *alloc_alloc = size_to_str(bfai->alloc_alloc);
+char *alloc_hole = size_to_str(bfai->alloc_hole);
+char *alloc_overhead = size_to_str(bfai->alloc_overhead);
+char *hole_alloc = size_to_str(bfai->hole_alloc);
+char *hole_hole = size_to_str(bfai->hole_hole);
+
+qprintf(quiet,
+"Format allocation info (including metadata):\n"
+"   file-alloc   file-hole   after-eof\n"
+"format-alloc   %10s  %10s  %10s\n"
+"format-hole%10s  %10s\n",
+alloc_alloc, alloc_hole, alloc_overhead,
+hole_alloc, hole_hole);
+
+g_free(alloc_alloc);
+g_free(alloc_hole);
+g_free(alloc_overhead);
+g_free(hole_alloc);
+g_free(hole_hole);
+}
+
 static void dump_human_image_check(ImageCheck *check, bool quiet)
 {
 if (!(check->corruptions || check->leaks || check->check_errors)) {
@@ -601,6 +624,10 @@ static void dump_human_image_check(ImageCheck *check, bool 
quiet)
 qprintf(quiet,
 "Image end offset: %" PRId64 "\n", check->image_end_offset);
 }
+
+if (check->has_format_alloc_info) {
+dump_human_format_alloc_info(check->format_alloc_info, quiet);
+}
 }
 
 static int collect_image_check(BlockDriverState *bs,
@@ -611,6 +638,7 @@ static int collect_image_check(BlockDriverState *bs,
 {
 int ret;
 BdrvCheckResult result;
+BlockFormatAllocInfo *bfai = g_new0(BlockFormatAllocInfo, 1);
 
 ret = bdrv_check(bs, , fix);
 if (ret < 0) {
@@ -639,6 +667,14 @@ static int collect_image_check(BlockDriverState *bs,
 check->compressed_clusters  = result.bfi.compressed_clusters;
 check->has_compressed_clusters  = result.bfi.compressed_clusters != 0;
 
+ret = bdrv_get_format_alloc_stat(bs, bfai);
+if (ret < 0) {
+qapi_free_BlockFormatAllocInfo(bfai);
+} else {
+check->has_format_alloc_info = true;
+check->format_alloc_info = bfai;
+}
+
 return 0;
 }
 
-- 
2.11.1

[Qemu-block] [PATCH 4/4] qemu-img check: improve dump_human_format_alloc_info

[Vladimir Sementsov-Ogievskiy]

Improve dump_human_format_alloc_info() by specifying format names.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-img.c | 35 ++-
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 55f8c1776c..3c03690a4f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,7 +560,8 @@ static void dump_json_image_check(ImageCheck *check, bool 
quiet)
 QDECREF(str);
 }
 
-static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool 
quiet)
+static void dump_human_format_alloc_info(BlockDriverState *bs,
+ BlockFormatAllocInfo *bfai, bool 
quiet)
 {
 char *alloc_alloc = size_to_str(bfai->alloc_alloc);
 char *alloc_hole = size_to_str(bfai->alloc_hole);
@@ -568,13 +569,28 @@ static void 
dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
 char *hole_alloc = size_to_str(bfai->hole_alloc);
 char *hole_hole = size_to_str(bfai->hole_hole);
 
+const char *format = bdrv_get_format_name(bs);
+const char *f_format =
+bs->file ? bdrv_get_format_name(bs->file->bs) : "file";
+int format_len, cw;
+
+if (format == NULL) {
+format = "format";
+}
+if (f_format == NULL) {
+f_format = "file";
+}
+format_len = strlen(format);
+cw = MAX(10, strlen(f_format) + 6);
+
 qprintf(quiet,
 "Format allocation info (including metadata):\n"
-"   file-alloc   file-hole   after-eof\n"
-"format-alloc   %10s  %10s  %10s\n"
-"format-hole%10s  %10s\n",
-alloc_alloc, alloc_hole, alloc_overhead,
-hole_alloc, hole_hole);
+"%*s %*s-alloc  %*s-hole  %*s\n"
+"%s-alloc   %*s  %*s  %*s\n"
+"%s-hole%*s  %*s\n",
+format_len, "", cw - 6, f_format, cw - 5, f_format, cw, 
"after-eof",
+format, cw, alloc_alloc, cw, alloc_hole, cw, alloc_overhead,
+format, cw, hole_alloc, cw, hole_hole);
 
 g_free(alloc_alloc);
 g_free(alloc_hole);
@@ -583,7 +599,8 @@ static void 
dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool quiet)
 g_free(hole_hole);
 }
 
-static void dump_human_image_check(ImageCheck *check, bool quiet)
+static void dump_human_image_check(BlockDriverState *bs, ImageCheck *check,
+   bool quiet)
 {
 if (!(check->corruptions || check->leaks || check->check_errors)) {
 qprintf(quiet, "No errors were found on the image.\n");
@@ -626,7 +643,7 @@ static void dump_human_image_check(ImageCheck *check, bool 
quiet)
 }
 
 if (check->has_format_alloc_info) {
-dump_human_format_alloc_info(check->format_alloc_info, quiet);
+dump_human_format_alloc_info(bs, check->format_alloc_info, quiet);
 }
 }
 
@@ -840,7 +857,7 @@ static int img_check(int argc, char **argv)
 if (!ret) {
 switch (output_format) {
 case OFORMAT_HUMAN:
-dump_human_image_check(check, quiet);
+dump_human_image_check(bs, check, quiet);
 break;
 case OFORMAT_JSON:
 dump_json_image_check(check, quiet);
-- 
2.11.1

[Qemu-block] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

The function should collect statistics, about allocted/unallocated by
top-level format driver space (in its .file) and allocation status
(allocated/hole/after eof) of corresponding areas in this .file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c   | 16 
 include/block/block.h |  3 +++
 include/block/block_int.h |  2 ++
 qapi/block-core.json  | 26 ++
 4 files changed, 47 insertions(+)

diff --git a/block.c b/block.c
index 50ba264143..7d720ae0c2 100644
--- a/block.c
+++ b/block.c
@@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState 
*bs)
 }
 
 /**
+ * Collect format allocation info. See BlockFormatAllocInfo definition in
+ * qapi/block-core.json.
+ */
+int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo 
*bfai)
+{
+BlockDriver *drv = bs->drv;
+if (!drv) {
+return -ENOMEDIUM;
+}
+if (drv->bdrv_get_format_alloc_stat) {
+return drv->bdrv_get_format_alloc_stat(bs, bfai);
+}
+return -ENOTSUP;
+}
+
+/**
  * Return number of sectors on success, -errno on error.
  */
 int64_t bdrv_nb_sectors(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e92d8..646376a772 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,6 +335,9 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
+int bdrv_get_format_alloc_stat(BlockDriverState *bs,
+   BlockFormatAllocInfo *bfai);
+
 /* The units of offset and total_work_size may be chosen arbitrarily by the
  * block driver; total_work_size may change during the course of the amendment
  * operation */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..458c715e99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -208,6 +208,8 @@ struct BlockDriver {
 int64_t (*bdrv_getlength)(BlockDriverState *bs);
 bool has_variable_length;
 int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
+int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
+  BlockFormatAllocInfo *bfai);
 
 int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
 uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ea0b3e8b13..365070b3eb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -139,6 +139,32 @@
'*format-specific': 'ImageInfoSpecific' } }
 
 ##
+# @BlockFormatAllocInfo:
+#
+# Information about allocations, including metadata. All fields are in bytes.
+#
+# @alloc_alloc: allocated by format driver and allocated in underlying file
+#
+# @alloc_hole: allocated by format driver but actually is a hole in
+#  underlying file
+#
+# @alloc_overhead: allocated by format driver after end of underlying file
+#
+# @hole_alloc: not allocated by format driver but allocated in underlying file
+#
+# @hole_hole: not allocated by format driver hole in underlying file
+#
+# Since: 2.10
+#
+##
+{ 'struct': 'BlockFormatAllocInfo',
+  'data': {'alloc_alloc':'uint64',
+   'alloc_hole': 'uint64',
+   'alloc_overhead': 'uint64',
+   'hole_alloc': 'uint64',
+   'hole_hole':  'uint64' } }
+
+##
 # @ImageCheck:
 #
 # Information about a QEMU image file check
-- 
2.11.1

Re: [Qemu-block] [PULL 00/15] Block layer patches

[Stefan Hajnoczi]

On Mon, May 29, 2017 at 05:06:39PM +0200, Kevin Wolf wrote:
> The following changes since commit 9964e96dccf7f7c936ee854a795415d19b60:
> 
>   Merge remote-tracking branch 'jasowang/tags/net-pull-request' into staging 
> (2017-05-23 15:01:31 +0100)
> 
> are available in the git repository at:
> 
> 
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 42a48128417b3bfade93d1a4721348cc480e9e50:
> 
>   Merge remote-tracking branch 'mreitz/tags/pull-block-2017-05-29-v3' into 
> queue-block (2017-05-29 16:34:27 +0200)
> 
> 
> 
> Block layer patches
> 
> 
> Alberto Garcia (2):
>   stream: fix crash in stream_start() when block_job_create() fails
>   qcow2: remove extra local_error variable
> 
> Daniel P. Berrange (4):
>   qemu-img: add support for --object with 'dd' command
>   qemu-img: fix --image-opts usage with dd command
>   qemu-img: introduce --target-image-opts for 'convert' command
>   qemu-img: copy *key-secret opts when opening newly created files
> 
> Eric Blake (1):
>   block: Tweak error message related to qemu-img amend
> 
> Fam Zheng (3):
>   iotests: 147: Don't test inet6 if not available
>   qemu-img: Fix documentation of convert
>   qemu-img: Fix leakage of options on error
> 
> Kevin Wolf (3):
>   qemu-iotests: Test streaming with missing job ID
>   mirror: Drop permissions on s->target on completion
>   Merge remote-tracking branch 'mreitz/tags/pull-block-2017-05-29-v3' 
> into queue-block
> 
> Max Reitz (2):
>   block: Fix backing paths for filenames with colons
>   block/file-*: *_parse_filename() and colons
> 
> Stephen Bates (1):
>   nvme: Add support for Controller Memory Buffers
> 
>  block.c|  50 +--
>  block/file-posix.c |  17 +-
>  block/file-win32.c |  12 +---
>  block/mirror.c |   7 ++-
>  block/qcow2-cluster.c  |   3 +-
>  block/qcow2.c  |   5 +-
>  block/stream.c |   2 +-
>  hw/block/nvme.c|  75 +--
>  hw/block/nvme.h|  73 ++
>  include/block/block_int.h  |   3 +
>  qemu-img-cmds.hx   |   4 +-
>  qemu-img.c | 148 
> +++--
>  qemu-img.texi  |  12 +++-
>  tests/qemu-iotests/030 |   4 ++
>  tests/qemu-iotests/030.out |   4 +-
>  tests/qemu-iotests/060.out |   2 +-
>  tests/qemu-iotests/147 |   7 +++
>  17 files changed, 351 insertions(+), 77 deletions(-)
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] Throttling groups vs filter nodes

[Kevin Wolf]

[ Cc: qemu-block - noticed only now that it's missing ]

Am 29.05.2017 um 22:57 hat Manos Pitsidianakis geschrieben:
> On Mon, May 29, 2017 at 05:05:17PM +0200, Alberto Garcia wrote:
> >On Sat 27 May 2017 09:56:03 AM CEST, Stefan Hajnoczi wrote:
> >>A quirk in the current implementation is that the throttling limits
> >>for the group are overwritten by each -drive throttling.group=group0.
> >>Limits for all but the last -drive in a group are ignored.
> >  - bps or iops != 0   -> set the I/O limits of a throttling group. The
> >  selected device is moved to that group if it
> >  wasn't there yet.
> >
> >  - bps and iops == 0  -> remove a device from a throttling group
> >  without touching that group's I/O limits.
> 
> These are very unintuitive.

I agree, this is not an interface to extend, but one to get rid of. (Of
course, we'll have to keep it around for a while because compatibility,
but we should try to offer something better.)

> However, even without considering backwards compatibility, I think
> that using -object notation (eg "object-add
> throttle-group,id=foo,iops=...) is intuitive in the case of groups,
> but not when you need individual limits for each device as the syntax
> would be too verbose.  Of course the old interface covers that.
> 
> In any case, is having multiple interfaces a problem or not? And, is
> using QOM straightforward implementation-wise?

We can have an interface for the throttling node that requires that you
specify either a throttle group object name or the limits, but never
both. If you specify the limits, this would just be a convenience
function that creates the right QOM object internally.

As for the implementation, QOM tends to be a bit heavy on boilerplate
code, but I think it's not too bad otherwise.

Kevin

Re: [Qemu-block] [PULL 00/12] Block patches

[Stefan Hajnoczi]

On Fri, May 26, 2017 at 03:23:52PM -0400, Jeff Cody wrote:
> The following changes since commit 9964e96dccf7f7c936ee854a795415d19b60:
> 
>   Merge remote-tracking branch 'jasowang/tags/net-pull-request' into staging 
> (2017-05-23 15:01:31 +0100)
> 
> are available in the git repository at:
> 
>   git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
> 
> for you to fetch changes up to 223a23c198787328ae75bc65d84edf5fde33c0b6:
> 
>   block/gluster: glfs_lseek() workaround (2017-05-24 16:44:46 -0400)
> 
> 
> Block patches
> 
> 
> Jeff Cody (1):
>   block/gluster: glfs_lseek() workaround
> 
> Paolo Bonzini (11):
>   blockjob: remove unnecessary check
>   blockjob: remove iostatus_reset callback
>   blockjob: introduce block_job_early_fail
>   blockjob: introduce block_job_pause/resume_all
>   blockjob: separate monitor and blockjob APIs
>   blockjob: move iostatus reset inside block_job_user_resume
>   blockjob: introduce block_job_cancel_async, check iostatus invariants
>   blockjob: group BlockJob transaction functions together
>   blockjob: strengthen a bit test-blockjob-txn
>   blockjob: reorganize block_job_completed_txn_abort
>   blockjob: use deferred_to_main_loop to indicate the coroutine has
> ended
> 
>  block/backup.c   |   2 +-
>  block/commit.c   |   2 +-
>  block/gluster.c  |  18 +-
>  block/io.c   |  19 +-
>  block/mirror.c   |   2 +-
>  blockdev.c   |   1 -
>  blockjob.c   | 750 
> ---
>  include/block/blockjob.h |  16 -
>  include/block/blockjob_int.h |  27 +-
>  tests/test-blockjob-txn.c|   7 +-
>  tests/test-blockjob.c|  10 +-
>  11 files changed, 463 insertions(+), 391 deletions(-)
> 
> -- 
> 2.9.3
> 
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH v7 19/20] qcow2: report encryption specific image information

[Daniel P. Berrange]

On Mon, May 29, 2017 at 11:53:01AM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > On Thu, May 25, 2017 at 02:52:30PM -0500, Eric Blake wrote:
> >> On 05/25/2017 11:38 AM, Daniel P. Berrange wrote:
> >> > Currently 'qemu-img info' reports a simple "encrypted: yes"
> >> > field. This is not very useful now that qcow2 can support
> >> > multiple encryption formats. Users want to know which format
> >> > is in use and some data related to it.
> >> > 
> >> > Wire up usage of the qcrypto_block_get_info() method so that
> >> > 'qemu-img info' can report about the encryption format
> >> > and parameters in use
> >> > 
> >> 
> >> > Signed-off-by: Daniel P. Berrange 
> >> > ---
> >> >  block/qcow2.c| 35 ++-
> >> >  qapi/block-core.json | 24 +++-
> >> >  2 files changed, 57 insertions(+), 2 deletions(-)
> >> > 
> >> > diff --git a/block/qcow2.c b/block/qcow2.c
> >> > index 38f9eb5..cb321a2 100644
> >> > --- a/block/qcow2.c
> >> > +++ b/block/qcow2.c
> >> > @@ -3196,8 +3196,17 @@ static int qcow2_get_info(BlockDriverState *bs, 
> >> > BlockDriverInfo *bdi)
> >> >  static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >> >  {
> >> >  BDRVQcow2State *s = bs->opaque;
> >> > -ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
> >> > +ImageInfoSpecific *spec_info;
> >> > +QCryptoBlockInfo *encrypt_info = NULL;
> >> >  
> >> > +if (s->crypto != NULL) {
> >> > +encrypt_info = qcrypto_block_get_info(s->crypto, NULL);
> >> 
> >> Worth using _abort instead of silently ignoring the error?  Is an
> >> error even possible in our output visitor [adding Markus for reference]?
> >
> > In fact the qcrypto_block_get_info() will never return an error
> > right now as implemented. So I guess we could even just remove
> > the Error **errp parameter from that call
> 
> Do you still need advice on QAPI from me here?

No need, thanks.

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 07/25] qcow2-refcount: rename inc_refcounts() and make it public

[Vladimir Sementsov-Ogievskiy]

This is needed for the following patch, which will introduce refcounts
checking for qcow2 bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/qcow2-refcount.c | 53 ++
 block/qcow2.h  |  4 
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..d7066c875b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1323,11 +1323,10 @@ static int realloc_refcount_array(BDRVQcow2State *s, 
void **array,
  *
  * Modifies the number of errors in res.
  */
-static int inc_refcounts(BlockDriverState *bs,
- BdrvCheckResult *res,
- void **refcount_table,
- int64_t *refcount_table_size,
- int64_t offset, int64_t size)
+int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
+ void **refcount_table,
+ int64_t *refcount_table_size,
+ int64_t offset, int64_t size)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t start, last, cluster_offset, k, refcount;
@@ -1420,8 +1419,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 nb_csectors = ((l2_entry >> s->csize_shift) &
s->csize_mask) + 1;
 l2_entry &= s->cluster_offset_mask;
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l2_entry & ~511, nb_csectors * 512);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   l2_entry & ~511, nb_csectors * 512);
 if (ret < 0) {
 goto fail;
 }
@@ -1454,8 +1454,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
BdrvCheckResult *res,
 }
 
 /* Mark cluster as used */
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   offset, s->cluster_size);
 if (ret < 0) {
 goto fail;
 }
@@ -1508,8 +1509,8 @@ static int check_refcounts_l1(BlockDriverState *bs,
 l1_size2 = l1_size * sizeof(uint64_t);
 
 /* Mark L1 table as used */
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l1_table_offset, l1_size2);
+ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
refcount_table_size,
+   l1_table_offset, l1_size2);
 if (ret < 0) {
 goto fail;
 }
@@ -1538,8 +1539,9 @@ static int check_refcounts_l1(BlockDriverState *bs,
 if (l2_offset) {
 /* Mark L2 table as used */
 l2_offset &= L1E_OFFSET_MASK;
-ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
-l2_offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, refcount_table_size,
+   l2_offset, s->cluster_size);
 if (ret < 0) {
 goto fail;
 }
@@ -1757,14 +1759,15 @@ static int check_refblocks(BlockDriverState *bs, 
BdrvCheckResult *res,
 }
 
 res->corruptions_fixed++;
-ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res,
+   refcount_table, nb_clusters,
+   offset, s->cluster_size);
 if (ret < 0) {
 return ret;
 }
 /* No need to check whether the refcount is now greater than 1:
  * This area was just allocated and zeroed, so it can only be
- * exactly 1 after inc_refcounts() */
+ * exactly 1 after qcow2_inc_refcounts_imrt() */
 continue;
 
 resize_fail:
@@ -1779,8 +1782,8 @@ resize_fail:
 }
 
 if (offset != 0) {
-ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
-offset, s->cluster_size);
+ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
nb_clusters,
+   offset, s->cluster_size);
 if (ret < 0) {
 return ret;
 }

[Qemu-block] [PATCH 08/25] qcow2: add bitmaps extension

[Vladimir Sementsov-Ogievskiy]

Add bitmap extension as specified in docs/specs/qcow2.txt.
For now, just mirror extension header into Qcow2 state and check
constraints. Also, calculate refcounts for qcow2 bitmaps, to not break
qemu-img check.

For now, disable image resize if it has bitmaps. It will be fixed later.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/Makefile.objs|   2 +-
 block/qcow2-bitmap.c   | 439 +
 block/qcow2-refcount.c |   6 +
 block/qcow2.c  | 124 +-
 block/qcow2.h  |  27 +++
 5 files changed, 592 insertions(+), 6 deletions(-)
 create mode 100644 block/qcow2-bitmap.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index ea955302c8..9efc6c49ea 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,5 +1,5 @@
 block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o 
dmg.o
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o qcow2-bitmap.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 00..b8e472b3e8
--- /dev/null
+++ b/block/qcow2-bitmap.c
@@ -0,0 +1,439 @@
+/*
+ * Bitmaps for the QCOW version 2 format
+ *
+ * Copyright (c) 2014-2017 Vladimir Sementsov-Ogievskiy
+ *
+ * This file is derived from qcow2-snapshot.c, original copyright:
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+
+#include "block/block_int.h"
+#include "block/qcow2.h"
+
+/* NOTICE: BME here means Bitmaps Extension and used as a namespace for
+ * _internal_ constants. Please do not use this _internal_ abbreviation for
+ * other needs and/or outside of this file. */
+
+/* Bitmap directory entry constraints */
+#define BME_MAX_TABLE_SIZE 0x800
+#define BME_MAX_PHYS_SIZE 0x2000 /* restrict BdrvDirtyBitmap size in RAM */
+#define BME_MAX_GRANULARITY_BITS 31
+#define BME_MIN_GRANULARITY_BITS 9
+#define BME_MAX_NAME_SIZE 1023
+
+/* Bitmap directory entry flags */
+#define BME_RESERVED_FLAGS 0xfffcU
+
+/* bits [1, 8] U [56, 63] are reserved */
+#define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
+#define BME_TABLE_ENTRY_OFFSET_MASK 0x00fffe00ULL
+#define BME_TABLE_ENTRY_FLAG_ALL_ONES (1ULL << 0)
+
+typedef struct QEMU_PACKED Qcow2BitmapDirEntry {
+/* header is 8 byte aligned */
+uint64_t bitmap_table_offset;
+
+uint32_t bitmap_table_size;
+uint32_t flags;
+
+uint8_t type;
+uint8_t granularity_bits;
+uint16_t name_size;
+uint32_t extra_data_size;
+/* extra data follows  */
+/* name follows  */
+} Qcow2BitmapDirEntry;
+
+typedef struct Qcow2BitmapTable {
+uint64_t offset;
+uint32_t size; /* number of 64bit entries */
+QSIMPLEQ_ENTRY(Qcow2BitmapTable) entry;
+} Qcow2BitmapTable;
+
+typedef struct Qcow2Bitmap {
+Qcow2BitmapTable table;
+uint32_t flags;
+uint8_t granularity_bits;
+char *name;
+
+QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
+} Qcow2Bitmap;
+typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
+
+typedef enum BitmapType {
+BT_DIRTY_TRACKING_BITMAP = 1
+} BitmapType;
+
+static int check_table_entry(uint64_t entry, int cluster_size)
+{
+uint64_t offset;
+
+if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
+return -EINVAL;
+}
+
+offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+if (offset != 0) {
+/* if offset specified, bit 0 is reserved */
+if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
+return -EINVAL;
+}
+
+

[Qemu-block] [PATCH 02/25] specs/qcow2: do not use wording 'bitmap header'

[Vladimir Sementsov-Ogievskiy]

A bitmap directory entry is sometimes called a 'bitmap header'. This
patch leaves only one name - 'bitmap directory entry'. The name 'bitmap
header' creates misunderstandings with 'qcow2 header' and 'qcow2 bitmap
header extension' (which is extension of qcow2 header)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
---
 docs/specs/qcow2.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index dda53dd2a3..8874e8c774 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -201,7 +201,7 @@ The fields of the bitmaps extension are:
 
   8 - 15:  bitmap_directory_size
Size of the bitmap directory in bytes. It is the cumulative
-   size of all (nb_bitmaps) bitmap headers.
+   size of all (nb_bitmaps) bitmap directory entries.
 
  16 - 23:  bitmap_directory_offset
Offset into the image file at which the bitmap directory
@@ -426,8 +426,7 @@ Each bitmap saved in the image is described in a bitmap 
directory entry. The
 bitmap directory is a contiguous area in the image file, whose starting offset
 and length are given by the header extension fields bitmap_directory_offset and
 bitmap_directory_size. The entries of the bitmap directory have variable
-length, depending on the lengths of the bitmap name and extra data. These
-entries are also called bitmap headers.
+length, depending on the lengths of the bitmap name and extra data.
 
 Structure of a bitmap directory entry:
 
-- 
2.11.1

[Qemu-block] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/dirty-bitmap.c | 28 
 block/io.c   |  8 
 blockdev.c   |  6 ++
 include/block/dirty-bitmap.h |  4 
 4 files changed, 46 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..733f19ca5e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
 int64_t size;   /* Size of the bitmap (Number of sectors) */
 bool disabled;  /* Bitmap is read-only */
 int active_iterators;   /* How many iterators are active */
+bool readonly;  /* Bitmap is read-only and may be changed only
+   by deserialize* functions */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
@@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t cur_sector, int64_t nr_sectors)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
 {
 assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 if (!out) {
 hbitmap_reset_all(bitmap->bitmap);
 } else {
@@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
 if (!bdrv_dirty_bitmap_enabled(bitmap)) {
 continue;
 }
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 }
@@ -540,3 +546,25 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
 return hbitmap_count(bitmap->meta);
 }
+
+bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
+{
+return bitmap->readonly;
+}
+
+void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
+{
+bitmap->readonly = true;
+}
+
+bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bm;
+QLIST_FOREACH(bm, >dirty_bitmaps, list) {
+if (bm->readonly) {
+return true;
+}
+}
+
+return false;
+}
diff --git a/block/io.c b/block/io.c
index fdd7485c22..0e28a1f595 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1349,6 +1349,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
 uint64_t bytes_remaining = bytes;
 int max_transfer;
 
+if (bdrv_has_readonly_bitmaps(bs)) {
+return -EPERM;
+}
+
 assert(is_power_of_2(align));
 assert((offset & (align - 1)) == 0);
 assert((bytes & (align - 1)) == 0);
@@ -2437,6 +2441,10 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, 
int64_t offset,
 return -ENOMEDIUM;
 }
 
+if (bdrv_has_readonly_bitmaps(bs)) {
+return -EPERM;
+}
+
 ret = bdrv_check_byte_request(bs, offset, count);
 if (ret < 0) {
 return ret;
diff --git a/blockdev.c b/blockdev.c
index c63f4e82c7..2b397abf66 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2033,6 +2033,9 @@ static void 
block_dirty_bitmap_clear_prepare(BlkActionState *common,
 } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
 error_setg(errp, "Cannot clear a disabled bitmap");
 return;
+} else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
+error_setg(errp, "Cannot clear a readonly bitmap");
+return;
 }
 
 bdrv_clear_dirty_bitmap(state->bitmap, >backup);
@@ -2813,6 +2816,9 @@ void qmp_block_dirty_bitmap_clear(const char *node, const 
char *name,
"Bitmap '%s' is currently disabled and cannot be cleared",
name);
 goto out;
+} else if (bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", 
name);
+goto out;
 }
 
 bdrv_clear_dirty_bitmap(bitmap, NULL);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1e17729ac2..c0c3ce9f85 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -75,4 +75,8 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap 
*bitmap,
 bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
+bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
+void 

[Qemu-block] [PATCH 17/25] qcow2: add .bdrv_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_can_store_new_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Reviewed-by: Max Reitz 
---
 block/qcow2-bitmap.c | 51 +++
 block/qcow2.c|  2 ++
 block/qcow2.h|  4 
 3 files changed, 57 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 4a8dae9961..a07fc0c712 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1303,3 +1303,54 @@ fail:
 
 bitmap_list_free(bm_list);
 }
+
+bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  uint32_t granularity,
+  Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+bool found;
+Qcow2BitmapList *bm_list;
+
+if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
+goto fail;
+}
+
+if (s->nb_bitmaps == 0) {
+return true;
+}
+
+if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
+error_setg(errp,
+   "Maximum number of persistent bitmaps is already reached");
+goto fail;
+}
+
+if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) >
+QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
+{
+error_setg(errp, "Not enough space in the bitmap directory");
+goto fail;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+goto fail;
+}
+
+found = find_bitmap_by_name(bm_list, name);
+bitmap_list_free(bm_list);
+if (found) {
+error_setg(errp, "Bitmap with the same name is already stored");
+goto fail;
+}
+
+return true;
+
+fail:
+error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
+  name, bdrv_get_device_or_node_name(bs));
+return false;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index b44fd64d12..80df8e040a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3605,6 +3605,8 @@ BlockDriver bdrv_qcow2 = {
 
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
+
+.bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index 4e71e9f7c1..c4700ac65d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -631,5 +631,9 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
   int64_t *refcount_table_size);
 bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  uint32_t granularity,
+  Error **errp);
 
 #endif
-- 
2.11.1

[Qemu-block] [PATCH 21/25] iotests: test qcow2 persistent dirty bitmap

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/165 | 105 +
 tests/qemu-iotests/165.out |   5 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 111 insertions(+)
 create mode 100755 tests/qemu-iotests/165
 create mode 100644 tests/qemu-iotests/165.out

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
new file mode 100755
index 00..74d7b79a0b
--- /dev/null
+++ b/tests/qemu-iotests/165
@@ -0,0 +1,105 @@
+#!/usr/bin/env python
+#
+# Tests for persistent dirty bitmaps.
+#
+# Copyright: Vladimir Sementsov-Ogievskiy 2015-2017
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import re
+import iotests
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+disk_size = 0x4000 # 1G
+
+# regions for qemu_io: (start, count) in bytes
+regions1 = ((0,0x10),
+(0x20, 0x10))
+
+regions2 = ((0x1000, 0x2),
+(0x3fff, 0x1))
+
+class TestPersistentDirtyBitmap(iotests.QMPTestCase):
+
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+
+def tearDown(self):
+os.remove(disk)
+
+def mkVm(self):
+return iotests.VM().add_drive(disk)
+
+def mkVmRo(self):
+return iotests.VM().add_drive(disk, opts='readonly=on')
+
+def getSha256(self):
+result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+return result['return']['sha256']
+
+def checkBitmap(self, sha256):
+result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+self.assert_qmp(result, 'return/sha256', sha256);
+
+def writeRegions(self, regions):
+for r in regions:
+self.vm.hmp_qemu_io('drive0',
+'write %d %d' % r)
+
+def qmpAddBitmap(self):
+self.vm.qmp('block-dirty-bitmap-add', node='drive0',
+name='bitmap0', persistent=True, autoload=True)
+
+def test_persistent(self):
+self.vm = self.mkVm()
+self.vm.launch()
+self.qmpAddBitmap()
+
+self.writeRegions(regions1)
+sha256 = self.getSha256()
+
+self.vm.shutdown()
+
+self.vm = self.mkVmRo()
+self.vm.launch()
+self.vm.shutdown()
+
+#catch 'Persistent bitmaps are lost' possible error
+log = self.vm.get_log()
+log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log)
+log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log)
+if log:
+print log
+
+self.vm = self.mkVm()
+self.vm.launch()
+
+self.checkBitmap(sha256)
+self.writeRegions(regions2)
+sha256 = self.getSha256()
+
+self.vm.shutdown()
+self.vm.launch()
+
+self.checkBitmap(sha256)
+
+self.vm.shutdown()
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
new file mode 100644
index 00..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/165.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5c8ea0f95c..1a446489c2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -163,6 +163,7 @@
 159 rw auto quick
 160 rw auto quick
 162 auto quick
+165 rw auto quick
 170 rw auto quick
 171 rw auto quick
 172 auto
-- 
2.11.1

[Qemu-block] [PATCH 20/25] qmp: add x-debug-block-dirty-bitmap-sha256

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c |  5 +
 blockdev.c   | 29 +
 include/block/dirty-bitmap.h |  2 ++
 include/qemu/hbitmap.h   |  8 
 qapi/block-core.json | 27 +++
 tests/Makefile.include   |  2 +-
 util/hbitmap.c   | 11 +++
 7 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index a3b8a1ea64..bf1abe4c3d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -616,3 +616,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState 
*bs,
 return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
 QLIST_NEXT(bitmap, list);
 }
+
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
+{
+return hbitmap_sha256(bitmap->bitmap, errp);
+}
diff --git a/blockdev.c b/blockdev.c
index d04432172a..e3dbef7427 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2857,6 +2857,35 @@ void qmp_block_dirty_bitmap_clear(const char *node, 
const char *name,
 aio_context_release(aio_context);
 }
 
+BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
+  const char *name,
+  Error **errp)
+{
+AioContext *aio_context;
+BdrvDirtyBitmap *bitmap;
+BlockDriverState *bs;
+BlockDirtyBitmapSha256 *ret = NULL;
+char *sha256;
+
+bitmap = block_dirty_bitmap_lookup(node, name, , _context, errp);
+if (!bitmap || !bs) {
+return NULL;
+}
+
+sha256 = bdrv_dirty_bitmap_sha256(bitmap, errp);
+if (sha256 == NULL) {
+goto out;
+}
+
+ret = g_new(BlockDirtyBitmapSha256, 1);
+ret->sha256 = sha256;
+
+out:
+aio_context_release(aio_context);
+
+return ret;
+}
+
 void hmp_drive_del(Monitor *mon, const QDict *qdict)
 {
 const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 2159daa95c..ef81c0b912 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -90,4 +90,6 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
 BdrvDirtyBitmap *bitmap);
 
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index b52304ac29..d3a74a21fc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -253,6 +253,14 @@ void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, 
uint64_t count,
 void hbitmap_deserialize_finish(HBitmap *hb);
 
 /**
+ * hbitmap_sha256:
+ * @bitmap: HBitmap to operate on.
+ *
+ * Returns SHA256 hash of the last level.
+ */
+char *hbitmap_sha256(const HBitmap *bitmap, Error **errp);
+
+/**
  * hbitmap_free:
  * @hb: HBitmap to operate on.
  *
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9134a317e5..3dd9f92e5a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1643,6 +1643,33 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @BlockDirtyBitmapSha256:
+#
+# SHA256 hash of dirty bitmap data
+#
+# @sha256: ASCII representation of SHA256 bitmap hash
+#
+# Since: 2.10
+##
+  { 'struct': 'BlockDirtyBitmapSha256',
+'data': {'sha256': 'str'} }
+
+##
+# @x-debug-block-dirty-bitmap-sha256:
+#
+# Get bitmap SHA256
+#
+# Returns: BlockDirtyBitmapSha256 on success
+#  If @node is not a valid block device, DeviceNotFound
+#  If @name is not found or if hashing has failed, GenericError with an
+#  explanation
+#
+# Since: 2.10
+##
+  { 'command': 'x-debug-block-dirty-bitmap-sha256',
+'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
+
+##
 # @blockdev-mirror:
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 75893838e5..38dbd2d9c8 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -552,7 +552,7 @@ tests/test-blockjob$(EXESUF): tests/test-blockjob.o 
$(test-block-obj-y) $(test-u
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o 
$(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
-tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
+tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) 
$(test-crypto-obj-y)
 tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o 
migration/page_cache.o $(test-util-obj-y)
 tests/test-cutils$(EXESUF): 

[Qemu-block] [PATCH 12/25] block: bdrv_close: release bitmaps after drv->bdrv_close

[Vladimir Sementsov-Ogievskiy]

Release bitmaps after 'if (bs->drv) { ... }' block. This will allow
format driver to save persistent bitmaps, which will appear in following
commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 50ba264143..96816658bd 100644
--- a/block.c
+++ b/block.c
@@ -2996,9 +2996,6 @@ static void bdrv_close(BlockDriverState *bs)
 bdrv_flush(bs);
 bdrv_drain(bs); /* in case flush left pending I/O */
 
-bdrv_release_named_dirty_bitmaps(bs);
-assert(QLIST_EMPTY(>dirty_bitmaps));
-
 if (bs->drv) {
 BdrvChild *child, *next;
 
@@ -3037,6 +3034,9 @@ static void bdrv_close(BlockDriverState *bs)
 bs->full_open_options = NULL;
 }
 
+bdrv_release_named_dirty_bitmaps(bs);
+assert(QLIST_EMPTY(>dirty_bitmaps));
+
 QLIST_FOREACH_SAFE(ban, >aio_notifiers, list, ban_next) {
 g_free(ban);
 }
-- 
2.11.1

[Qemu-block] [PATCH 24/25] qmp: block-dirty-bitmap-remove: remove persistent

[Vladimir Sementsov-Ogievskiy]

Remove persistent bitmap from the storage on block-dirty-bitmap-remove.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 10 ++
 qapi/block-core.json |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index e3dbef7427..7ec75524a8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2801,6 +2801,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
 AioContext *aio_context;
 BlockDriverState *bs;
 BdrvDirtyBitmap *bitmap;
+Error *local_err = NULL;
 
 bitmap = block_dirty_bitmap_lookup(node, name, , _context, errp);
 if (!bitmap || !bs) {
@@ -2813,6 +2814,15 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
name);
 goto out;
 }
+
+if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+bdrv_remove_persistent_dirty_bitmap(bs, name, _err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto out;
+}
+}
+
 bdrv_dirty_bitmap_make_anon(bitmap);
 bdrv_release_dirty_bitmap(bs, bitmap);
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3dd9f92e5a..37f35a1811 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1600,7 +1600,8 @@
 # @block-dirty-bitmap-remove:
 #
 # Stop write tracking and remove the dirty bitmap that was created
-# with block-dirty-bitmap-add.
+# with block-dirty-bitmap-add. If the bitmap is persistent, remove it from its
+# storage too.
 #
 # Returns: nothing on success
 #  If @node is not a valid block device or node, DeviceNotFound
-- 
2.11.1

[Qemu-block] [PATCH 23/25] qcow2: add .bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_remove_persistent_dirty_bitmap interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/qcow2-bitmap.c | 41 +
 block/qcow2.c|  1 +
 block/qcow2.h|  3 +++
 3 files changed, 45 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a07fc0c712..90c8415d4f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1174,6 +1174,47 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList 
*bm_list,
 return NULL;
 }
 
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  Error **errp)
+{
+int ret;
+BDRVQcow2State *s = bs->opaque;
+Qcow2Bitmap *bm;
+Qcow2BitmapList *bm_list;
+
+if (s->nb_bitmaps == 0) {
+/* Absence of the bitmap is not an error: see explanation above
+ * bdrv_remove_persistent_dirty_bitmap() definition. */
+return;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+return;
+}
+
+bm = find_bitmap_by_name(bm_list, name);
+if (bm == NULL) {
+goto fail;
+}
+
+QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
+
+ret = update_ext_header_and_dir(bs, bm_list);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to update bitmap extension");
+goto fail;
+}
+
+free_bitmap_clusters(bs, >table);
+
+fail:
+bitmap_free(bm);
+bitmap_list_free(bm_list);
+}
+
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
diff --git a/block/qcow2.c b/block/qcow2.c
index 80df8e040a..ba211ca9ff 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3607,6 +3607,7 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
 .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
+.bdrv_remove_persistent_dirty_bitmap = 
qcow2_remove_persistent_dirty_bitmap,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index c4700ac65d..40192e8b89 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -635,5 +635,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
   const char *name,
   uint32_t granularity,
   Error **errp);
+void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+  const char *name,
+  Error **errp);
 
 #endif
-- 
2.11.1

[Qemu-block] [PATCH 01/25] specs/qcow2: fix bitmap granularity qemu-specific note

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 docs/specs/qcow2.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0e91..dda53dd2a3 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -472,8 +472,7 @@ Structure of a bitmap directory entry:
  17:granularity_bits
 Granularity bits. Valid values: 0 - 63.
 
-Note: Qemu currently doesn't support granularity_bits
-greater than 31.
+Note: Qemu currently supports only values 9 - 31.
 
 Granularity is calculated as
 granularity = 1 << granularity_bits
-- 
2.11.1

[Qemu-block] [PATCH 06/25] block/dirty-bitmap: add deserialize_ones func

[Vladimir Sementsov-Ogievskiy]

Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
all-ones.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c |  7 +++
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h   | 15 +++
 util/hbitmap.c   | 17 +
 4 files changed, 42 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 186941cfc3..90af37287f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -499,6 +499,13 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap 
*bitmap,
 hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish);
 }
 
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+uint64_t start, uint64_t count,
+bool finish)
+{
+hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish);
+}
+
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
 {
 hbitmap_deserialize_finish(bitmap->bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 7cbe623ba7..1e17729ac2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -70,6 +70,9 @@ void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap 
*bitmap,
 void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
   uint64_t start, uint64_t count,
   bool finish);
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+uint64_t start, uint64_t count,
+bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 6b04391266..b52304ac29 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -229,6 +229,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t 
start, uint64_t count,
 bool finish);
 
 /**
+ * hbitmap_deserialize_ones
+ * @hb: HBitmap to operate on.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ * @finish: Whether to call hbitmap_deserialize_finish automatically.
+ *
+ * Fills the bitmap with ones.
+ *
+ * If @finish is false, caller must call hbitmap_serialize_finish before using
+ * the bitmap.
+ */
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+  bool finish);
+
+/**
  * hbitmap_deserialize_finish
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 0b38817505..0c1591a594 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -551,6 +551,23 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t 
start, uint64_t count,
 }
 }
 
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+  bool finish)
+{
+uint64_t el_count;
+unsigned long *first;
+
+if (!count) {
+return;
+}
+serialization_chunk(hb, start, count, , _count);
+
+memset(first, 0xff, el_count * sizeof(unsigned long));
+if (finish) {
+hbitmap_deserialize_finish(hb);
+}
+}
+
 void hbitmap_deserialize_finish(HBitmap *bitmap)
 {
 int64_t i, size, prev_size;
-- 
2.11.1

[Qemu-block] [PATCH 25/25] block: release persistent bitmaps on inactivate

[Vladimir Sementsov-Ogievskiy]

We should release them here to reload on invalidate cache.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c  |  4 
 block/dirty-bitmap.c | 29 +++--
 include/block/dirty-bitmap.h |  1 +
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index f7712c4136..0b21153b89 100644
--- a/block.c
+++ b/block.c
@@ -4091,6 +4091,10 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
 }
 }
 
+/* At this point persistent bitmaps should be already stored by the format
+ * driver */
+bdrv_release_persistent_dirty_bitmaps(bs);
+
 return 0;
 }
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 996f5fb6fc..9a07a44249 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -296,13 +296,18 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
 }
 }
 
-static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap,
-  bool only_named)
+static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
+{
+return !!bdrv_dirty_bitmap_name(bitmap);
+}
+
+static void bdrv_do_release_matching_dirty_bitmap(
+BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+bool (*cond)(BdrvDirtyBitmap *bitmap))
 {
 BdrvDirtyBitmap *bm, *next;
 QLIST_FOREACH_SAFE(bm, >dirty_bitmaps, list, next) {
-if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) {
+if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
 assert(!bm->active_iterators);
 assert(!bdrv_dirty_bitmap_frozen(bm));
 assert(!bm->meta);
@@ -323,7 +328,7 @@ static void 
bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
 
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
-bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false);
+bdrv_do_release_matching_dirty_bitmap(bs, bitmap, NULL);
 }
 
 /**
@@ -333,7 +338,19 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
  */
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
 {
-bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
+bdrv_do_release_matching_dirty_bitmap(bs, NULL, 
bdrv_dirty_bitmap_has_name);
+}
+
+/**
+ * Release all persistent dirty bitmaps attached to a BDS (for use in
+ * bdrv_inactivate_recurse()).
+ * There must not be any frozen bitmaps attached.
+ * This function does not remove persistent bitmaps from the storage.
+ */
+void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)
+{
+bdrv_do_release_matching_dirty_bitmap(bs, NULL,
+  bdrv_dirty_bitmap_get_persistance);
 }
 
 /**
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 69d2ebda9f..40faa37364 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -25,6 +25,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
+void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
  const char *name,
  Error **errp);
-- 
2.11.1

[Qemu-block] [PATCH 05/25] block: fix bdrv_dirty_bitmap_granularity signature

[Vladimir Sementsov-Ogievskiy]

Make getter signature const-correct. This allows other functions with
const dirty bitmap parameter use bdrv_dirty_bitmap_granularity().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Reviewed-by: Kevin Wolf 
---
 block/dirty-bitmap.c | 2 +-
 include/block/dirty-bitmap.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 519737c8d3..186941cfc3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ uint32_t 
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 return granularity;
 }
 
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
 return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9dea14ba03..7cbe623ba7 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -29,7 +29,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
 uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
-- 
2.11.1

[Qemu-block] [PATCH 16/25] block: add bdrv_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

This will be needed to check some restrictions before making bitmap
persistent in qmp-block-dirty-bitmap-add (this functionality will be
added by future patch)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block.c   | 22 ++
 include/block/block.h |  3 +++
 include/block/block_int.h |  5 +
 3 files changed, 30 insertions(+)

diff --git a/block.c b/block.c
index 96816658bd..f7712c4136 100644
--- a/block.c
+++ b/block.c
@@ -4889,3 +4889,25 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
BdrvChild *child, Error **errp)
 
 parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
+
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
+{
+BlockDriver *drv = bs->drv;
+
+if (!drv) {
+error_setg_errno(errp, ENOMEDIUM,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+if (!drv->bdrv_can_store_new_dirty_bitmap) {
+error_setg_errno(errp, ENOTSUP,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e92d8..ee320db909 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -629,4 +629,7 @@ void bdrv_add_child(BlockDriverState *parent, 
BlockDriverState *child,
 Error **errp);
 void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
 
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..70b73a98a8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -380,6 +380,11 @@ struct BlockDriver {
  uint64_t parent_perm, uint64_t parent_shared,
  uint64_t *nperm, uint64_t *nshared);
 
+bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
+const char *name,
+uint32_t granularity,
+Error **errp);
+
 QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.11.1

[Qemu-block] [PATCH 04/25] tests: add hbitmap iter test

[Vladimir Sementsov-Ogievskiy]

Test that hbitmap iter is resistant to bitmap resetting.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 tests/test-hbitmap.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 23773d2051..1acb353889 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -909,6 +909,22 @@ static void hbitmap_test_add(const char *testpath,
hbitmap_test_teardown);
 }
 
+static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
+const void *unused)
+{
+HBitmapIter hbi;
+
+hbitmap_test_init(data, L1 * 2, 0);
+hbitmap_set(data->hb, 0, data->size);
+
+hbitmap_iter_init(, data->hb, BITS_PER_LONG - 1);
+
+hbitmap_iter_next();
+
+hbitmap_reset_all(data->hb);
+hbitmap_iter_next();
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
@@ -966,6 +982,9 @@ int main(int argc, char **argv)
  test_hbitmap_serialize_part);
 hbitmap_test_add("/hbitmap/serialize/zeroes",
  test_hbitmap_serialize_zeroes);
+
+hbitmap_test_add("/hbitmap/iter/iter_and_reset",
+ test_hbitmap_iter_and_reset);
 g_test_run();
 
 return 0;
-- 
2.11.1

[Qemu-block] [PATCH 15/25] qcow2: add persistent dirty bitmaps support

[Vladimir Sementsov-Ogievskiy]

Store persistent dirty bitmaps in qcow2 image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/qcow2-bitmap.c | 475 +++
 block/qcow2.c|   9 +
 block/qcow2.h|   1 +
 3 files changed, 485 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 0d53075b2c..4a8dae9961 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -27,6 +27,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 
 #include "block/block_int.h"
 #include "block/qcow2.h"
@@ -42,6 +43,10 @@
 #define BME_MIN_GRANULARITY_BITS 9
 #define BME_MAX_NAME_SIZE 1023
 
+#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
+#error In the code bitmap table physical size assumed to fit into int
+#endif
+
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffcU
 #define BME_FLAG_IN_USE (1U << 0)
@@ -72,6 +77,8 @@ typedef struct Qcow2BitmapTable {
 uint32_t size; /* number of 64bit entries */
 QSIMPLEQ_ENTRY(Qcow2BitmapTable) entry;
 } Qcow2BitmapTable;
+typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
+Qcow2BitmapTableList;
 
 typedef struct Qcow2Bitmap {
 Qcow2BitmapTable table;
@@ -79,6 +86,8 @@ typedef struct Qcow2Bitmap {
 uint8_t granularity_bits;
 char *name;
 
+BdrvDirtyBitmap *dirty_bitmap;
+
 QSIMPLEQ_ENTRY(Qcow2Bitmap) entry;
 } Qcow2Bitmap;
 typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList;
@@ -104,6 +113,15 @@ static int update_header_sync(BlockDriverState *bs)
 return bdrv_flush(bs);
 }
 
+static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size)
+{
+size_t i;
+
+for (i = 0; i < size; ++i) {
+cpu_to_be64s(_table[i]);
+}
+}
+
 static int check_table_entry(uint64_t entry, int cluster_size)
 {
 uint64_t offset;
@@ -127,6 +145,70 @@ static int check_table_entry(uint64_t entry, int 
cluster_size)
 return 0;
 }
 
+static int check_constraints_on_bitmap(BlockDriverState *bs,
+   const char *name,
+   uint32_t granularity,
+   Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+int granularity_bits = ctz32(granularity);
+int64_t len = bdrv_getlength(bs);
+
+assert(granularity > 0);
+assert((granularity & (granularity - 1)) == 0);
+
+if (len < 0) {
+error_setg_errno(errp, -len, "Failed to get size of '%s'",
+ bdrv_get_device_or_node_name(bs));
+return len;
+}
+
+if (granularity_bits > BME_MAX_GRANULARITY_BITS) {
+error_setg(errp, "Granularity exceeds maximum (%llu bytes)",
+   1ULL << BME_MAX_GRANULARITY_BITS);
+return -EINVAL;
+}
+if (granularity_bits < BME_MIN_GRANULARITY_BITS) {
+error_setg(errp, "Granularity is under minimum (%llu bytes)",
+   1ULL << BME_MIN_GRANULARITY_BITS);
+return -EINVAL;
+}
+
+if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
+(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
+   granularity_bits))
+{
+error_setg(errp, "Too much space will be occupied by the bitmap. "
+   "Use larger granularity");
+return -EINVAL;
+}
+
+if (strlen(name) > BME_MAX_NAME_SIZE) {
+error_setg(errp, "Name length exceeds maximum (%u characters)",
+   BME_MAX_NAME_SIZE);
+return -EINVAL;
+}
+
+return 0;
+}
+
+static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
+   uint32_t bitmap_table_size)
+{
+BDRVQcow2State *s = bs->opaque;
+int i;
+
+for (i = 0; i < bitmap_table_size; ++i) {
+uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK;
+if (!addr) {
+continue;
+}
+
+qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
+bitmap_table[i] = 0;
+}
+}
+
 static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable *tb,
  uint64_t **bitmap_table)
 {
@@ -165,6 +247,28 @@ fail:
 return ret;
 }
 
+static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
+{
+int ret;
+uint64_t *bitmap_table;
+
+ret = bitmap_table_load(bs, tb, _table);
+if (ret < 0) {
+assert(bitmap_table == NULL);
+return ret;
+}
+
+clear_bitmap_table(bs, bitmap_table, tb->size);
+qcow2_free_clusters(bs, tb->offset, tb->size * sizeof(uint64_t),
+QCOW2_DISCARD_OTHER);
+g_free(bitmap_table);
+
+tb->offset = 0;
+tb->size = 0;
+
+return 0;
+}
+
 /* This function returns the number of disk sectors covered by a single qcow2
  * cluster of bitmap data. */
 static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State 

[Qemu-block] [PATCH 03/25] hbitmap: improve dirty iter

[Vladimir Sementsov-Ogievskiy]

Make dirty iter resistant to resetting bits in corresponding HBitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 include/qemu/hbitmap.h | 26 --
 util/hbitmap.c | 23 ++-
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 9239fe515e..6b04391266 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -256,10 +256,9 @@ void hbitmap_free(HBitmap *hb);
  * the lowest-numbered bit that is set in @hb, starting at @first.
  *
  * Concurrent setting of bits is acceptable, and will at worst cause the
- * iteration to miss some of those bits.  Resetting bits before the current
- * position of the iterator is also okay.  However, concurrent resetting of
- * bits can lead to unexpected behavior if the iterator has not yet reached
- * those bits.
+ * iteration to miss some of those bits.
+ *
+ * The concurrent resetting of bits is OK.
  */
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
 
@@ -298,24 +297,7 @@ void hbitmap_free_meta(HBitmap *hb);
  * Return the next bit that is set in @hbi's associated HBitmap,
  * or -1 if all remaining bits are zero.
  */
-static inline int64_t hbitmap_iter_next(HBitmapIter *hbi)
-{
-unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-int64_t item;
-
-if (cur == 0) {
-cur = hbitmap_iter_skip_words(hbi);
-if (cur == 0) {
-return -1;
-}
-}
-
-/* The next call will resume work from the next bit.  */
-hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
-item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
-
-return item << hbi->granularity;
-}
+int64_t hbitmap_iter_next(HBitmapIter *hbi);
 
 /**
  * hbitmap_iter_next_word:
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 35088e19c4..0b38817505 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 
 unsigned long cur;
 do {
-cur = hbi->cur[--i];
+i--;
 pos >>= BITS_PER_LEVEL;
+cur = hbi->cur[i] & hb->levels[i][pos];
 } while (cur == 0);
 
 /* Check for end of iteration.  We always use fewer than BITS_PER_LONG
@@ -139,6 +140,26 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 return cur;
 }
 
+int64_t hbitmap_iter_next(HBitmapIter *hbi)
+{
+unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1] &
+hbi->hb->levels[HBITMAP_LEVELS - 1][hbi->pos];
+int64_t item;
+
+if (cur == 0) {
+cur = hbitmap_iter_skip_words(hbi);
+if (cur == 0) {
+return -1;
+}
+}
+
+/* The next call will resume work from the next bit.  */
+hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
+item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
+
+return item << hbi->granularity;
+}
+
 void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
 {
 unsigned i, bit;
-- 
2.11.1

[Qemu-block] [PATCH 18/25] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 18 +-
 qapi/block-core.json |  8 +++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2b397abf66..12ee9a3eda 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1982,6 +1982,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 /* AIO context taken and released within qmp_block_dirty_bitmap_add */
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
+   action->has_persistent, action->persistent,
_err);
 
 if (!local_err) {
@@ -2730,10 +2731,12 @@ out:
 
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
+bool has_persistent, bool persistent,
 Error **errp)
 {
 AioContext *aio_context;
 BlockDriverState *bs;
+BdrvDirtyBitmap *bitmap;
 
 if (!name || name[0] == '\0') {
 error_setg(errp, "Bitmap name cannot be empty");
@@ -2759,7 +2762,20 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 granularity = bdrv_get_default_bitmap_granularity(bs);
 }
 
-bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+if (!has_persistent) {
+persistent = false;
+}
+
+if (persistent &&
+!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+{
+goto out;
+}
+
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+if (bitmap != NULL) {
+bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+}
 
  out:
 aio_context_release(aio_context);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ea0b3e8b13..5d9ed0e208 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1560,10 +1560,16 @@
 # @granularity: the bitmap granularity, default is 64k for
 #   block-dirty-bitmap-add
 #
+# @persistent: the bitmap is persistent, i.e. it will be saved to the
+#  corresponding block device image file on its close. For now only
+#  Qcow2 disks support persistent bitmaps. Default is false for
+#  block-dirty-bitmap-add. (Since: 2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
-  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
+'*persistent': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add:
-- 
2.11.1

[Qemu-block] [PATCH 11/25] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

Mirror AUTO flag from Qcow2 bitmap in BdrvDirtyBitmap. This will be
needed in future, to save this flag back to Qcow2 for persistent
bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 15 +++
 block/qcow2-bitmap.c |  2 ++
 include/block/dirty-bitmap.h |  3 +++
 3 files changed, 20 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 733f19ca5e..7da9f72725 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -46,6 +46,8 @@ struct BdrvDirtyBitmap {
 int active_iterators;   /* How many iterators are active */
 bool readonly;  /* Bitmap is read-only and may be changed only
by deserialize* functions */
+bool autoload;  /* For persistent bitmaps: bitmap must be
+   autoloaded on image opening */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -72,6 +74,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
 bitmap->name = NULL;
+bitmap->autoload = false;
 }
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
@@ -240,6 +243,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->name = NULL;
 successor->name = name;
 bitmap->successor = NULL;
+successor->autoload = bitmap->autoload;
+bitmap->autoload = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
 
 return successor;
@@ -568,3 +573,13 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
 
 return false;
 }
+
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
+{
+bitmap->autoload = autoload;
+}
+
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
+{
+return bitmap->autoload;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index cb596c2a85..9d2ea15e56 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -793,6 +793,8 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState 
*bs, Error **errp)
 if (bitmap == NULL) {
 goto fail;
 }
+
+bdrv_dirty_bitmap_set_autoload(bitmap, true);
 bm->flags |= BME_FLAG_IN_USE;
 created_dirty_bitmaps =
 g_slist_append(created_dirty_bitmaps, bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index c0c3ce9f85..0ad12e952e 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -79,4 +79,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap 
*bitmap);
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
+
 #endif
-- 
2.11.1

[Qemu-block] [PATCH 10/25] qcow2: autoloading dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
are loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive.

Extra data in bitmaps is not supported for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/qcow2-bitmap.c | 388 +++
 block/qcow2.c|  17 ++-
 block/qcow2.h|   2 +
 3 files changed, 405 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b8e472b3e8..cb596c2a85 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -44,6 +44,8 @@
 
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffcU
+#define BME_FLAG_IN_USE (1U << 0)
+#define BME_FLAG_AUTO   (1U << 1)
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff0001feULL
@@ -85,6 +87,23 @@ typedef enum BitmapType {
 BT_DIRTY_TRACKING_BITMAP = 1
 } BitmapType;
 
+static inline bool can_write(BlockDriverState *bs)
+{
+return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+}
+
+static int update_header_sync(BlockDriverState *bs)
+{
+int ret;
+
+ret = qcow2_update_header(bs);
+if (ret < 0) {
+return ret;
+}
+
+return bdrv_flush(bs);
+}
+
 static int check_table_entry(uint64_t entry, int cluster_size)
 {
 uint64_t offset;
@@ -146,6 +165,120 @@ fail:
 return ret;
 }
 
+/* This function returns the number of disk sectors covered by a single qcow2
+ * cluster of bitmap data. */
+static uint64_t sectors_covered_by_bitmap_cluster(const BDRVQcow2State *s,
+  const BdrvDirtyBitmap 
*bitmap)
+{
+uint32_t sector_granularity =
+bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+
+return (uint64_t)sector_granularity * (s->cluster_size << 3);
+}
+
+/* load_bitmap_data
+ * @bitmap_table entries must satisfy specification constraints.
+ * @bitmap must be cleared */
+static int load_bitmap_data(BlockDriverState *bs,
+const uint64_t *bitmap_table,
+uint32_t bitmap_table_size,
+BdrvDirtyBitmap *bitmap)
+{
+int ret = 0;
+BDRVQcow2State *s = bs->opaque;
+uint64_t sector, sbc;
+uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+uint8_t *buf = NULL;
+uint64_t i, tab_size =
+size_to_clusters(s,
+bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+if (tab_size != bitmap_table_size || tab_size > BME_MAX_TABLE_SIZE) {
+return -EINVAL;
+}
+
+buf = g_malloc(s->cluster_size);
+sbc = sectors_covered_by_bitmap_cluster(s, bitmap);
+for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
+uint64_t count = MIN(bm_size - sector, sbc);
+uint64_t entry = bitmap_table[i];
+uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
+
+assert(check_table_entry(entry, s->cluster_size) == 0);
+
+if (offset == 0) {
+if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
+bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, count,
+   false);
+} else {
+/* No need to deserialize zeros because the dirty bitmap is
+ * already cleared */
+}
+} else {
+ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
+if (ret < 0) {
+goto finish;
+}
+bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, count,
+   false);
+}
+}
+ret = 0;
+
+bdrv_dirty_bitmap_deserialize_finish(bitmap);
+
+finish:
+g_free(buf);
+
+return ret;
+}
+
+static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
+Qcow2Bitmap *bm, Error **errp)
+{
+int ret;
+uint64_t *bitmap_table = NULL;
+uint32_t granularity;
+BdrvDirtyBitmap *bitmap = NULL;
+
+if (bm->flags & BME_FLAG_IN_USE) {
+error_setg(errp, "Bitmap '%s' is in use", bm->name);
+goto fail;
+}
+
+ret = bitmap_table_load(bs, >table, _table);
+if (ret < 0) {
+error_setg_errno(errp, -ret,
+ "Could not read bitmap_table table from image for "
+ "bitmap '%s'", bm->name);
+goto fail;
+}
+
+granularity = 1U << bm->granularity_bits;
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+if (bitmap == NULL) {
+goto fail;
+}
+
+ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
+ bm->name);
+goto fail;
+}
+
+g_free(bitmap_table);

[Qemu-block] [PATCH 13/25] block: introduce persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
by format driver in .bdrv_close and .bdrv_inactivate. No format driver
supports it for now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/dirty-bitmap.c | 26 ++
 block/qcow2-bitmap.c |  1 +
 include/block/dirty-bitmap.h |  5 +
 3 files changed, 32 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7da9f72725..c9b809e4b9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,6 +48,7 @@ struct BdrvDirtyBitmap {
by deserialize* functions */
 bool autoload;  /* For persistent bitmaps: bitmap must be
autoloaded on image opening */
+bool persistent;/* bitmap must be saved to owner disk image */
 QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -74,6 +75,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
 g_free(bitmap->name);
 bitmap->name = NULL;
+bitmap->persistent = false;
 bitmap->autoload = false;
 }
 
@@ -243,6 +245,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->name = NULL;
 successor->name = name;
 bitmap->successor = NULL;
+successor->persistent = bitmap->persistent;
+bitmap->persistent = false;
 successor->autoload = bitmap->autoload;
 bitmap->autoload = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
@@ -583,3 +587,25 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
*bitmap)
 {
 return bitmap->autoload;
 }
+
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool 
persistent)
+{
+bitmap->persistent = persistent;
+}
+
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
+{
+return bitmap->persistent;
+}
+
+bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bm;
+QLIST_FOREACH(bm, >dirty_bitmaps, list) {
+if (bm->persistent && !bm->readonly) {
+return true;
+}
+}
+
+return false;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 9d2ea15e56..0d53075b2c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -794,6 +794,7 @@ bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState 
*bs, Error **errp)
 goto fail;
 }
 
+bdrv_dirty_bitmap_set_persistance(bitmap, true);
 bdrv_dirty_bitmap_set_autoload(bitmap, true);
 bm->flags |= BME_FLAG_IN_USE;
 created_dirty_bitmaps =
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 0ad12e952e..f811c02237 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -82,4 +82,9 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+bool persistent);
+bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
+
 #endif
-- 
2.11.1

[Qemu-block] [PATCH v19 00/25] qcow2: persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

Hi all!

There is a new update of qcow2-bitmap series - v19.

web: 
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=qcow2-bitmap-v19
git: https://src.openvz.org/scm/~vsementsov/qemu.git (tag qcow2-bitmap-v19)

v19:

rebased on master

05: move 'sign-off' over 'reviewed-by's
08: error_report -> error_setg in qcow2_truncate (because of rebase)
09: return EPERM in bdrv_aligned_pwritev and bdrv_co_pdiscard if there
are readonly bitmaps. EPERM is chosen because it is already used for
readonly image in bdrv_co_pdiscard.
Also handle readonly bitmap in block_dirty_bitmap_clear_prepare and
qmp_block_dirty_bitmap_clear
Max's r-b is not added
10: fix grammar in comment
add Max's r-b
12, 13, 15, 21: add Max's r-b
24: fix grammar in comment
25: fix grammar and wording in comment
also, I see contextual changes in inactiavate mechanism. Hope, they do not
affect these series.

v18:

rebased on master (sorry for v17)

08: contextual: qcow2_do_open is changed instead of qcow2_open
rename s/need_update_header/update_header/ in qcow2_do_open, to not do it 
in 10
save r-b's by Max and John
09: new patch
10: load_bitmap_data: do not clear bitmap parameter - it should be already 
cleared
(it actually created before single load_bitmap_data() call)
if some bitmaps are loaded, but we can't write the image (it is readonly
or inactive), so we can't mark bitmaps "in use" in the image, mark
corresponding BdrvDirtyBitmap read-only.
change error_setg to error_setg_errno for "Can't update bitmap directory"
no needs to rename s/need_update_header/update_header/ here, as it done in 
08
13: function bdrv_has_persistent_bitmaps becomes 
bdrv_has_changed_persistent_bitmaps,
to handle readonly field.
14: declaration moved to the bottom of .h, save r-b's
15: firstly check bdrv_has_changed_persistent_bitmaps and only then fail on 
!can_write, and then QSIMPLEQ_INIT(_tables)
skip readonly bitmaps in saving loop
18: remove '#optional', 2.9 -> 2.10, save r-b's
19: remove '#optional', 2.9 -> 2.10, save r-b's
20: 2.9 -> 2.10, save r-b's
21: add check of read-only image open, drop r-b's
24: add comment to qapi/block-core.json, that block-dirty-bitmap-add removes 
bitmap
from storage. r-b's by Max and John saved


v17:
08: add r-b's by Max and John
09: clear unknown autoclear features from BDRVQcow2State before calling
qcow2_load_autoloading_dirty_bitmaps(), and also do not extra update
header if it is updated by qcow2_load_autoloading_dirty_bitmaps().
11: new patch, splitted out from 16
12: rewrite commit message
14: changing bdrv_close moved to separate new patch 11
s/1/1ULL/ ; s/%u/%llu/ for two errors
16: s/No/Not/
add Max's r-b
24: new patch


v16:

mostly by Kevin's comments:
07: just moved here, as preparation for merging refcounts to 08
08: move "qcow2-bitmap: refcounts" staff to this patch, to not break qcow2-img 
check
drop r-b's
move necessary supporting static functions to this patch too (to not break 
compilation)
fprintf -> error_report
other small changes with error messages and comments
code style
for qcow2-bitmap.c:
  'include "exec/log.h"' was dropped
  s/1/(1U << 0) for BME_FLAG_IN_USE
  add BME_TABLE_ENTRY_FLAG_ALL_ONES to replace magic 1
  don't check 'cluster_size <= 0' in check_table_entry
old "[PATCH v15 08/25] block: introduce auto-loading bitmaps" was dropped
09: was "[PATCH v15 09/25] qcow2: add .bdrv_load_autoloading_dirty_bitmaps"
drop r-b's
some staff was moved to 08
update_header_sync - error on bdrv_flush fail
rename disk_sectors_in_bitmap_cluster to sectors_covered_by_bitmap_cluster
 and adjust comment.
 so, variable for storing this function result: s/dsc/sbc/
s/1/BME_TABLE_ENTRY_FLAG_ALL_ONES/
also, as Qcow2BitmapTable already introduced in 08,
   s/table_offset/table.offset/ and s/table_size/table.size, etc..
update_ext_header_and_dir_in_place: add comments, add additional
  update_header_sync, to reduce indeterminacy in case of error.
call qcow2_load_autoloading_dirty_bitmaps directly from qcow2_open
no .bdrv_load_autoloading_dirty_bitmaps
11: drop bdrv_store_persistent_dirty_bitmaps at all.
drop r-b's
13: rename patch, rewrite commit msg
drop r-b's
move bdrv_release_named_dirty_bitmaps in bdrv_close() after 
drv->bdrv_close()
Qcow2BitmapTable is already introduced, so no
  s/table_offset/table.offset/ and s/table_size/table.size, etc.. here
old 25 with check_constraints_on_bitmap() improvements merged here (Eric)
like in 09, s/dsc/sbc/ and 
s/disk_sectors_in_bitmap_cluster/sectors_covered_by_bitmap_cluster/
no .bdrv_store_persistent_dirty_bitmaps
call qcow2_store_persistent_dirty_bitmaps directly from qcow2_inactivate
15: corresponding part of 25 merged here. Add John's r-b, as 25 was also 
reviewed by John.
16: add John's r-b


v15:
13,14: add John's r-b
15: 

[Qemu-block] [PATCH 22/25] block/dirty-bitmap: add bdrv_remove_persistent_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Interface for removing persistent bitmap from its storage.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 18 ++
 include/block/block_int.h|  3 +++
 include/block/dirty-bitmap.h |  3 +++
 3 files changed, 24 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bf1abe4c3d..996f5fb6fc 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -329,12 +329,30 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
 /**
  * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()).
  * There must not be any frozen bitmaps attached.
+ * This function does not remove persistent bitmaps from the storage.
  */
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
 {
 bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
 }
 
+/**
+ * Remove persistent dirty bitmap from the storage if it exists.
+ * Absence of bitmap is not an error, because we have the following scenario:
+ * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
+ * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
+ * not fail.
+ * This function doesn't release corresponding BdrvDirtyBitmap.
+ */
+void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp)
+{
+if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
+bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+}
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
 assert(!bdrv_dirty_bitmap_frozen(bitmap));
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 70b73a98a8..dc63e9560e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -384,6 +384,9 @@ struct BlockDriver {
 const char *name,
 uint32_t granularity,
 Error **errp);
+void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+const char *name,
+Error **errp);
 
 QLIST_ENTRY(BlockDriver) list;
 };
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index ef81c0b912..69d2ebda9f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -25,6 +25,9 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
+void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
-- 
2.11.1

[Qemu-block] [PATCH 14/25] block/dirty-bitmap: add bdrv_dirty_bitmap_next()

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 block/dirty-bitmap.c | 7 +++
 include/block/dirty-bitmap.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c9b809e4b9..a3b8a1ea64 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -609,3 +609,10 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs)
 
 return false;
 }
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+BdrvDirtyBitmap *bitmap)
+{
+return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
+QLIST_NEXT(bitmap, list);
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index f811c02237..2159daa95c 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -87,4 +87,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap 
*bitmap,
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
+BdrvDirtyBitmap *bitmap);
+
 #endif
-- 
2.11.1

[Qemu-block] [PATCH 19/25] qmp: add autoload parameter to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Optional. Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
Reviewed-by: Max Reitz 
Reviewed-by: John Snow 
---
 blockdev.c   | 18 --
 qapi/block-core.json |  6 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 12ee9a3eda..d04432172a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1983,6 +1983,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
+   action->has_autoload, action->autoload,
_err);
 
 if (!local_err) {
@@ -2732,6 +2733,7 @@ out:
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
 bool has_persistent, bool persistent,
+bool has_autoload, bool autoload,
 Error **errp)
 {
 AioContext *aio_context;
@@ -2765,6 +2767,15 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 if (!has_persistent) {
 persistent = false;
 }
+if (!has_autoload) {
+autoload = false;
+}
+
+if (has_autoload && !persistent) {
+error_setg(errp, "Autoload flag must be used only for persistent "
+ "bitmaps");
+goto out;
+}
 
 if (persistent &&
 !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
@@ -2773,10 +2784,13 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 }
 
 bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
-if (bitmap != NULL) {
-bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+if (bitmap == NULL) {
+goto out;
 }
 
+bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
+
  out:
 aio_context_release(aio_context);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5d9ed0e208..9134a317e5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1565,11 +1565,15 @@
 #  Qcow2 disks support persistent bitmaps. Default is false for
 #  block-dirty-bitmap-add. (Since: 2.10)
 #
+# @autoload: the bitmap will be automatically loaded when the image it is 
stored
+#in is opened. This flag may only be specified for persistent
+#bitmaps. Default is false for block-dirty-bitmap-add. (Since: 
2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-'*persistent': 'bool' } }
+'*persistent': 'bool', '*autoload': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add:
-- 
2.11.1

Re: [Qemu-block] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

30.05.2017 10:31, Vladimir Sementsov-Ogievskiy wrote:

30.05.2017 09:50, Vladimir Sementsov-Ogievskiy wrote:

Thank you for this scenario. Hmm.

So, as I need guarantee that image and bitmap are unchanged, 
bdrv_set_dirty should return error and fail the whole write. Ok?


No, bad idea. As bdrv_set_dirty is called after write completed.


will just check for readonly bitmaps in all callers of set/reset/clear. 
They are few.






29.05.2017 21:35, Max Reitz wrote:

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/dirty-bitmap.c | 16 
  include/block/dirty-bitmap.h |  3 +++
  2 files changed, 19 insertions(+)

Revisiting this again after the whole series: So you never really make
sure that the read-only bitmaps are not written to (except for these
assertions). The idea is that you only set it for read-only BDS and
read-only BDS are never written to. But that assumption is not true,
generally, and can be broken e.g. using a commit job:

$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
Formatting 'top.qcow2', fmt=qcow2 size=67108864
 backing_file=backing.qcow2 encryption=off cluster_size=65536
 lazy_refcounts=off refcount_bits=16
$ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
"package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
{'execute': 'qmp_capabilities'}
{"return": {}}
{'execute': 'blockdev-add',
  'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
'file': {'driver': 'file', 'filename': 
'backing.qcow2'}}}

{"return": {}}
{'execute': 'block-dirty-bitmap-add',
  'arguments': {'node': 'backing-node', 'name': 'foo',
'persistent': true, 'autoload': true}}
{"return": {}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
{"return": {}}
{'execute': 'blockdev-add',
  'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
{"return": {}}
{'execute': 'human-monitor-command',
  'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
{"return": ""}
{'execute': 'block-commit',
  'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
{"return": {}}
qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
`!bdrv_dirty_bitmap_readonly(bitmap)' failed.
[1]10872 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -qmp
stdio

So there needs to be something else than just assertions to make sure
that read-only bitmaps are never written to.

Max







--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

On 30.05.2017 07:20, Markus Armbruster wrote:
> Thomas Huth  writes:
> 
>> We likely do not want to carry these legacy -drive options along forever.
>> Let's emit a deprecation warning for the -drive options that have a
>> replacement with the -device option, so that the (hopefully few) remaining
>> users are aware of this and can adapt their scripts / behaviour accordingly.
>>
>> Signed-off-by: Thomas Huth 
>> ---
[...]
>>  /* Change legacy command line options into QMP ones */
>>  static const struct {
>> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>> BlockInterfaceType block_default_type)
>if (qemu_opt_get(legacy_opts, "boot") != NULL) {
>fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
>"ignored. Future versions will reject this parameter. 
> Please "
>>  "update your scripts.\n");
> 
> Unrelated to this patch: this is ugly.  It's also almost three years
> old.  Can we bury the corpse already?

If you like to get rid of this now, feel free to send a patch ...
otherwise I'll make sure that it'll go away with QEMU v3.0 (it's on the
to-be-removed list on http://wiki.qemu.org/Features/LegacyRemoval already)

[...]
>> @@ -631,7 +633,8 @@ an untrusted format header.
>>  @item serial=@var{serial}
>>  This option specifies the serial number to assign to the device.
>>  @item addr=@var{addr}
>> -Specify the controller's PCI address (if=virtio only).
>> +Specify the controller's PCI address (if=virtio only). This parameter is
>> +deprecated, use the corresponding parameter of @code{-device} instead.
>>  @item werror=@var{action},rerror=@var{action}
>>  Specify which @var{action} to take on write and read errors. Valid actions 
>> are:
>>  "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
> 
> Reviewed-by: Markus Armbruster 

 Thanks!
  Thomas

Re: [Qemu-block] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

30.05.2017 09:50, Vladimir Sementsov-Ogievskiy wrote:

Thank you for this scenario. Hmm.

So, as I need guarantee that image and bitmap are unchanged, 
bdrv_set_dirty should return error and fail the whole write. Ok?


No, bad idea. As bdrv_set_dirty is called after write completed.



29.05.2017 21:35, Max Reitz wrote:

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/dirty-bitmap.c | 16 
  include/block/dirty-bitmap.h |  3 +++
  2 files changed, 19 insertions(+)

Revisiting this again after the whole series: So you never really make
sure that the read-only bitmaps are not written to (except for these
assertions). The idea is that you only set it for read-only BDS and
read-only BDS are never written to. But that assumption is not true,
generally, and can be broken e.g. using a commit job:

$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
Formatting 'top.qcow2', fmt=qcow2 size=67108864
 backing_file=backing.qcow2 encryption=off cluster_size=65536
 lazy_refcounts=off refcount_bits=16
$ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
"package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
{'execute': 'qmp_capabilities'}
{"return": {}}
{'execute': 'blockdev-add',
  'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
'file': {'driver': 'file', 'filename': 
'backing.qcow2'}}}

{"return": {}}
{'execute': 'block-dirty-bitmap-add',
  'arguments': {'node': 'backing-node', 'name': 'foo',
'persistent': true, 'autoload': true}}
{"return": {}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
{"return": {}}
{'execute': 'blockdev-add',
  'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
{"return": {}}
{'execute': 'human-monitor-command',
  'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
{"return": ""}
{'execute': 'block-commit',
  'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
{"return": {}}
qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
`!bdrv_dirty_bitmap_readonly(bitmap)' failed.
[1]10872 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -qmp
stdio

So there needs to be something else than just assertions to make sure
that read-only bitmaps are never written to.

Max





--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

Thank you for this scenario. Hmm.

So, as I need guarantee that image and bitmap are unchanged, 
bdrv_set_dirty should return error and fail the whole write. Ok?


29.05.2017 21:35, Max Reitz wrote:

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/dirty-bitmap.c | 16 
  include/block/dirty-bitmap.h |  3 +++
  2 files changed, 19 insertions(+)

Revisiting this again after the whole series: So you never really make
sure that the read-only bitmaps are not written to (except for these
assertions). The idea is that you only set it for read-only BDS and
read-only BDS are never written to. But that assumption is not true,
generally, and can be broken e.g. using a commit job:

$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
Formatting 'top.qcow2', fmt=qcow2 size=67108864
 backing_file=backing.qcow2 encryption=off cluster_size=65536
 lazy_refcounts=off refcount_bits=16
$ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
"package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
{'execute': 'qmp_capabilities'}
{"return": {}}
{'execute': 'blockdev-add',
  'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
'file': {'driver': 'file', 'filename': 'backing.qcow2'}}}
{"return": {}}
{'execute': 'block-dirty-bitmap-add',
  'arguments': {'node': 'backing-node', 'name': 'foo',
'persistent': true, 'autoload': true}}
{"return": {}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
{"return": {}}
{'execute': 'blockdev-add',
  'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
{"return": {}}
{'execute': 'human-monitor-command',
  'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
{"return": ""}
{'execute': 'block-commit',
  'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
{"return": {}}
qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
`!bdrv_dirty_bitmap_readonly(bitmap)' failed.
[1]10872 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -qmp
stdio

So there needs to be something else than just assertions to make sure
that read-only bitmaps are never written to.

Max



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 25/25] block: release persistent bitmaps on inactivate

[Vladimir Sementsov-Ogievskiy]

29.05.2017 20:54, Max Reitz wrote:

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:

We should release them here to reload on invalidate cache.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c  |  4 
  block/dirty-bitmap.c | 29 +++--
  include/block/dirty-bitmap.h |  1 +
  3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 795d36bb64..14896c65fa 100644
--- a/block.c
+++ b/block.c
@@ -4001,6 +4001,10 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
  if (setting_flag) {
  bs->open_flags |= BDRV_O_INACTIVE;
  }
+
+/* At this point persistent bitmaps should be stored by format driver */

s/by format driver/by the format driver/


+bdrv_release_persistent_dirty_bitmaps(bs);

Also, as far as I can see, this doesn't store the bitmaps but just
releases them (without storing them). I'm not sure whether that is
right, but it definitely contradicts the comment above.


I mean they should be _already_ stored.. They actually are stored in  
qcow2_inactivate.




Max


+
  return 0;
  }



--
Best regards,
Vladimir