Commit Graph

3515 Commits

Author SHA1 Message Date
Kevin Wolf
d736f119da block: Allow graph changes in subtree drained section
We need to remember how many of the drain sections in which a node is
were recursive (i.e. subtree drain rather than node drain), so that they
can be correctly applied when children are added or removed during the
drained section.

With this change, it is safe to modify the graph even inside a
bdrv_subtree_drained_begin/end() section.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-12-22 15:05:32 +01:00
Kevin Wolf
b016558590 block: Add bdrv_subtree_drained_begin/end()
bdrv_drained_begin() waits for the completion of requests in the whole
subtree, but it only actually keeps its immediate bs parameter quiesced
until bdrv_drained_end().

Add a version that keeps the whole subtree drained. As of this commit,
graph changes cannot be allowed during a subtree drained section, but
this will be fixed soon.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-12-22 15:05:32 +01:00
Kevin Wolf
0152bf400f block: Don't notify parents in drain call chain
This is in preparation for subtree drains, i.e. drained sections that
affect not only a single node, but recursively all child nodes, too.

Calling the parent callbacks for drain is pointless when we just came
from that parent node recursively and leads to multiple increases of
bs->quiesce_counter in a single drain call. Don't do it.

In order for this to work correctly, the parent callback must be called
for every bdrv_drain_begin/end() call, not only for the outermost one:

If we have a node N with two parents A and B, recursive draining of A
should cause the quiesce_counter of B to increase because its child N is
drained independently of B. If now B is recursively drained, too, A must
increase its quiesce_counter because N is drained independently of A
only now, even if N is going from quiesce_counter 1 to 2.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-12-22 15:05:32 +01:00
Kevin Wolf
0f11516894 block: Nested drain_end must still call callbacks
bdrv_do_drained_begin() restricts the call of parent callbacks and
aio_disable_external() to the outermost drain section, but the block
driver callbacks are always called. bdrv_do_drained_end() must match
this behaviour, otherwise nodes stay drained even if begin/end calls
were balanced.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-12-22 15:05:32 +01:00
Kevin Wolf
8119334918 block: Don't block_job_pause_all() in bdrv_drain_all()
Block jobs are already paused using the BdrvChildRole drain callbacks,
so we don't need an additional block_job_pause_all() call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-12-22 15:05:32 +01:00
Kevin Wolf
7b6a3d3553 block: Make bdrv_drain() driver callbacks non-recursive
bdrv_drained_begin() doesn't increase bs->quiesce_counter recursively
and also doesn't notify other parent nodes of children, which both means
that the child nodes are not actually drained, and bdrv_drained_begin()
is providing useful functionality only on a single node.

To keep things consistent, we also shouldn't call the block driver
callbacks recursively.

A proper recursive drain version that provides an actually working
drained section for child nodes will be introduced later.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
2017-12-22 15:05:31 +01:00
Kevin Wolf
9a7e86c804 block: Assert drain_all is only called from main AioContext
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
2017-12-22 15:05:31 +01:00
Fam Zheng
8e77e0bceb block: Remove unused bdrv_requests_pending
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-12-22 15:05:31 +01:00
Edgar Kaziakhmedov
546a7dc40e qcow2: get rid of qcow2_backing_read1 routine
Since bdrv_co_preadv does all neccessary checks including
reading after the end of the backing file, avoid duplication
of verification before bdrv_co_preadv call.

Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-12-22 15:03:41 +01:00
Kevin Wolf
60369b86c4 block: Unify order in drain functions
Drain requests are propagated to child nodes, parent nodes and directly
to the AioContext. The order in which this happened was different
between all combinations of drain/drain_all and begin/end.

The correct order is to keep children only drained when their parents
are also drained. This means that at the start of a drained section, the
AioContext needs to be drained first, the parents second and only then
the children. The correct order for the end of a drained section is the
opposite.

This patch changes the three other functions to follow the example of
bdrv_drained_begin(), which is the only one that got it right.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-12-22 15:03:41 +01:00
Kevin Wolf
5280aa32e1 block: Don't wait for requests in bdrv_drain*_end()
The device is drained, so there is no point in waiting for requests at
the end of the drained section. Remove the bdrv_drain_recurse() calls
there.

The bdrv_drain_recurse() calls were introduced in commit 481cad48e5
in order to call the .bdrv_co_drain_end() driver callback. This is now
done by a separate bdrv_drain_invoke() call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-12-22 15:03:41 +01:00
Kevin Wolf
99c05de918 block: bdrv_drain_recurse(): Remove unused begin parameter
Now that the bdrv_drain_invoke() calls are pulled up to the callers of
bdrv_drain_recurse(), the 'begin' parameter isn't needed any more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-12-22 15:03:41 +01:00
Kevin Wolf
2da9b7d456 block: Call .drain_begin only once in bdrv_drain_all_begin()
bdrv_drain_all_begin() used to call the .bdrv_co_drain_begin() driver
callback inside its polling loop. This means that how many times it got
called for each node depended on long it had to poll the event loop.

This is obviously not right and results in nodes that stay drained even
after bdrv_drain_all_end(), which calls .bdrv_co_drain_begin() once per
node.

Fix bdrv_drain_all_begin() to call the callback only once, too.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-12-22 15:03:41 +01:00
Kevin Wolf
db0289b9b2 block: Make bdrv_drain_invoke() recursive
This change separates bdrv_drain_invoke(), which calls the BlockDriver
drain callbacks, from bdrv_drain_recurse(). Instead, the function
performs its own recursion now.

One reason for this is that bdrv_drain_recurse() can be called multiple
times by bdrv_drain_all_begin(), but the callbacks may only be called
once. The separation is necessary to fix this bug.

The other reason is that we intend to go to a model where we call all
driver callbacks first, and only then start polling. This is not fully
achieved yet with this patch, as bdrv_drain_invoke() contains a
BDRV_POLL_WHILE() loop for the block driver callbacks, which can still
call callbacks for any unrelated event. It's a step in this direction
anyway.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-12-22 15:03:41 +01:00
Peter Lieven
e38bc23454 block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task
hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in
some cases and handle it gracefully. This is the case for misaligned UNMAPs
and WRITESAME10/16 calls without UNMAP. In this case a failure in the
logs can be quite misleading.

While we are at it improve the logging to reveal which operation failed
at what LBA.

Signed-off-by: Peter Lieven <pl@kamp.de>
Message-Id: <1512733868-9009-3-git-send-email-pl@kamp.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-12-21 09:30:32 +01:00
Peter Lieven
aef172ffdc block/iscsi: dont leave allocmap in an invalid state on UNMAP failure
we forgot to set the allocmap to invalid if an UNMAP call fails.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
Message-Id: <1512733868-9009-2-git-send-email-pl@kamp.de>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2017-12-21 09:30:31 +01:00
Peter Maydell
f1faf2d59c Pull request
v2:
  * Fixed incorrect virtio_blk_data_plane_create() local_err refactoring in
    "hw/block: Use errp directly rather than local_err" that broke virtio-blk
    over virtio-mmio [Peter]
 -----BEGIN PGP SIGNATURE-----
 
 iQEcBAABAgAGBQJaOSteAAoJEJykq7OBq3PIllkH/RkxTY6JIe9K8PRVsaAX2fRN
 edO/3E09KTQe9eHEixoMKOIyeKi3RPdipcktXIbdLIDEY4z4vELmQslTrxK/q+8J
 pccdwu+7tEXr14ciYSnq0m6ksvU5JHlJGyAJEvbCmLHE3dPJszABwT1XLLCb1C8s
 hSOr3nR/O2U3LHlq/FuvEUK8fohgKlECtE94V/DUWyC774iMw+9OdvTA0VQWYnN6
 B0gpYSn4AXmdt5HmpgCa+5rZrT2DjdwhtR9X+iOItPoXJPP81toUxvshLbTgdL54
 fSodd12Tbn2Pxr/osD1kwzM9z6oYX8Ay8YZTabODiFo20fhZKZ2wLpL4rrsNnBk=
 =Qcx2
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging

Pull request

v2:
 * Fixed incorrect virtio_blk_data_plane_create() local_err refactoring in
   "hw/block: Use errp directly rather than local_err" that broke virtio-blk
   over virtio-mmio [Peter]

# gpg: Signature made Tue 19 Dec 2017 15:08:14 GMT
# gpg:                using RSA key 0x9CA4ABB381AB73C8
# gpg: Good signature from "Stefan Hajnoczi <stefanha@redhat.com>"
# gpg:                 aka "Stefan Hajnoczi <stefanha@gmail.com>"
# Primary key fingerprint: 8695 A8BF D3F9 7CDA AC35  775A 9CA4 ABB3 81AB 73C8

* remotes/stefanha/tags/block-pull-request: (23 commits)
  qemu-iotests: add 203 savevm with IOThreads test
  iothread: fix iothread_stop() race condition
  iotests: add VM.add_object()
  blockdev: add x-blockdev-set-iothread force boolean
  docs: mark nested AioContext locking as a legacy API
  block: avoid recursive AioContext acquire in bdrv_inactivate_all()
  virtio-blk: reject configs with logical block size > physical block size
  virtio-blk: make queue size configurable
  qemu-iotests: add 202 external snapshots IOThread test
  blockdev: add x-blockdev-set-iothread testing command
  iothread: add iothread_by_id() API
  block: drop unused BlockDirtyBitmapState->aio_context field
  block: don't keep AioContext acquired after internal_snapshot_prepare()
  block: don't keep AioContext acquired after blockdev_backup_prepare()
  block: don't keep AioContext acquired after drive_backup_prepare()
  block: don't keep AioContext acquired after external_snapshot_prepare()
  blockdev: hold AioContext for bdrv_unref() in external_snapshot_clean()
  qdev: drop unused #include "sysemu/iothread.h"
  dev-storage: Fix the unusual function name
  hw/block: Use errp directly rather than local_err
  ...

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

# Conflicts:
#	hw/core/qdev-properties-system.c
2017-12-20 11:30:55 +00:00
Peter Maydell
03c1c09d56 -----BEGIN PGP SIGNATURE-----
iQIcBAABAgAGBQJaOC2xAAoJEL2+eyfA3jBXeOcP/RUB2PDP6xC+gEtQ/pNTuT8a
 SY+2ZnxeCl/n6grW7Tf4IXwDZCTIM4+fthODJe3gmF0u8RkaEIm4Km4lf9Bc2gal
 2ZmZoXk00va4FHlv7CVMQV/kcl3BudUqA7bwlGm8HQAEFkYaYtSrpRIlCrvohQIX
 hb3hPme1Ia/GGz0W+QTli6bKxBdgXXO20ZOt03UkVRLBO5LN/U9VhHh2Uu1+QOaX
 bpuqDnfRmkZ5iKGSMkNLT81PAOSQelHz3eej8UhdQz2F1KU2RTWaZ6E3Y/Ky6qah
 OL5vmV4BJaOlX+c+Vkg97TxChccAJPb1TRXiUl1Ypo8YPOBnuJ0CSBbadI2CnFu4
 hAbGvzs77mOq1B3zY1gNMzVpK0R/nSPmXi63tY02VdedSbJ2s7dVyP49oV3Cko89
 8XGdUOD6fn5goaA+GwMPs6iQQZBSRCqX7L3tawIcHHqi09BG6D/MNqzcTV8DjJ9Z
 6UY1nGmPgO44fQmpkt+NJwYbEw8oENYEZZxsPupaFVqmIyPyjbhwV0ISLAGZtOWA
 lb4hETJ3a0dFhu6FawZyw2sDpTaSu+7AgIrBrmFFUUciYqWJr9xfAsvbFSqNARX2
 Msihq9T6oXfefO+d56C1AIJmIz+LL5KtAUfNaD9qLgHTcyMvfdPQJl5lLyXs7/NK
 RL6MRSe3KL5GQsga1nK7
 =8vnb
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/cody/tags/block-pull-request' into staging

# gpg: Signature made Mon 18 Dec 2017 21:05:53 GMT
# gpg:                using RSA key 0xBDBE7B27C0DE3057
# gpg: Good signature from "Jeffrey Cody <jcody@redhat.com>"
# gpg:                 aka "Jeffrey Cody <jeff@codyprime.org>"
# gpg:                 aka "Jeffrey Cody <codyprime@gmail.com>"
# Primary key fingerprint: 9957 4B4D 3474 90E7 9D98  D624 BDBE 7B27 C0DE 3057

* remotes/cody/tags/block-pull-request:
  block/curl: fix minor memory leaks
  block/curl: check error return of curl_global_init()
  block/sheepdog: code beautification
  block/sheepdog: remove spurious NULL check
  blockjob: kick jobs on set-speed
  backup: use copy_bitmap in incremental backup
  backup: simplify non-dirty bits progress processing
  backup: init copy_bitmap from sync_bitmap for incremental
  backup: move from done_bitmap to copy_bitmap
  hbitmap: add next_zero function

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2017-12-19 17:44:42 +00:00
Stefan Hajnoczi
78f1d3d6a6 coroutine: simplify co_aio_sleep_ns() prototype
The AioContext pointer argument to co_aio_sleep_ns() is only used for
the sleep timer.  It does not affect where the caller coroutine is
resumed.

Due to changes to coroutine and AIO APIs it is now possible to drop the
AioContext pointer argument.  This is safe to do since no caller has
specific requirements for which AioContext the timer must run in.

This patch drops the AioContext pointer argument and renames the
function to simplify the API.

Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20171109102652.6360-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2017-12-19 09:25:27 +00:00
Jeff Cody
996922de45 block/curl: fix minor memory leaks
Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 15:44:39 -05:00
Jeff Cody
2d25964d18 block/curl: check error return of curl_global_init()
If curl_global_init() fails, per the documentation no other curl
functions may be called, so make sure to check the return value.

Also, some minor changes to the initialization latch variable 'inited':

- Make it static in the file, for clarity
- Change the name for clarity
- Make it a bool

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 15:42:07 -05:00
Jeff Cody
d507c5f682 block/sheepdog: code beautification
No functional changes, just whitespace manipulation.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 15:41:44 -05:00
Jeff Cody
ac90dad94b block/sheepdog: remove spurious NULL check
'tag' is already checked in the lines immediately preceding this check,
and set to non-NULL if NULL.  No need to check again, it hasn't changed.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 15:41:17 -05:00
Vladimir Sementsov-Ogievskiy
53f1c8794f backup: use copy_bitmap in incremental backup
We can use copy_bitmap instead of sync_bitmap. copy_bitmap is
initialized from sync_bitmap and it is more informative: we will not try
to process data, that is already in progress (by write notifier).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: 20171012135313.227864-6-vsementsov@virtuozzo.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 10:54:13 -05:00
Vladimir Sementsov-Ogievskiy
085bd08e6f backup: simplify non-dirty bits progress processing
Set fake progress for non-dirty clusters in copy_bitmap initialization,
to. It simplifies code and allows further refactoring.

This patch changes user's view of backup progress, but formally it
doesn't changed: progress hops are just moved to the beginning.

Actually it's just a point of view: when do we actually skip clusters?
We can say in the very beginning, that we skip these clusters and do
not think about them later.

Of course, if go through disk sequentially, it's logical to say, that
we skip clusters between copied portions to the left and to the right
of them. But even now copying progress is not sequential because of
write notifiers. Future patches will introduce new backup architecture
which will do copying in several coroutines in parallel, so it will
make no sense to publish fake progress by parts in parallel with
other copying requests.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: 20171012135313.227864-5-vsementsov@virtuozzo.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 10:54:13 -05:00
Vladimir Sementsov-Ogievskiy
8cc6dc6215 backup: init copy_bitmap from sync_bitmap for incremental
We should not copy non-dirty clusters in write notifiers. So,
initialize copy_bitmap from sync_bitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20171012135313.227864-4-vsementsov@virtuozzo.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 10:54:13 -05:00
Vladimir Sementsov-Ogievskiy
a193b0f0a8 backup: move from done_bitmap to copy_bitmap
Use HBitmap copy_bitmap instead of done_bitmap. This is needed to
improve incremental backup in following patches and to unify backup
loop for full/incremental modes in future patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20171012135313.227864-3-vsementsov@virtuozzo.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 10:54:13 -05:00
Vladimir Sementsov-Ogievskiy
56207df55e hbitmap: add next_zero function
The function searches for next zero bit.
Also add interface for BdrvDirtyBitmap and unit test.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20171012135313.227864-2-vsementsov@virtuozzo.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
2017-12-18 10:54:13 -05:00
Philippe Mathieu-Daudé
7d98febd67 block: remove "qemu/osdep.h" from header file
applied using ./scripts/clean-includes

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
2017-12-18 17:07:02 +03:00
Peter Lieven
f1a7ff770f block/nfs: fix nfs_client_open for filesize greater than 1TB
DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE) was overflowing ret (int) if
st.st_size is greater than 1TB.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
Message-id: 1511798407-31129-1-git-send-email-pl@kamp.de
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-29 15:28:15 +01:00
Paolo Bonzini
5bf1d5a73a blockjob: remove clock argument from block_job_sleep_ns
All callers are using QEMU_CLOCK_REALTIME, and it will not be possible to
support more than one clock when block_job_sleep_ns switches to a single
timer stored in the BlockJob struct.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Tested-By: Jeff Cody <jcody@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-11-29 15:11:02 +01:00
Kevin Wolf
02d213009d block: Expect graph changes in bdrv_parent_drained_begin/end
The .drained_begin/end callbacks can (directly or indirectly via
aio_poll()) cause block nodes to be removed or the current BdrvChild to
point to a different child node.

Use QLIST_FOREACH_SAFE() to make sure we don't access invalid
BlockDriverStates or accidentally continue iterating the parents of the
new child node instead of the node we actually came from.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-11-29 14:22:03 +01:00
Kevin Wolf
70a5afedd6 block: Error out on load_vm with active dirty bitmaps
Loading a snapshot invalidates the bitmap. Just marking all blocks dirty
is not a useful response in practice, instead the user needs to be aware
that we switch to a completely different state. If they are okay with
losing the dirty bitmap, they can just explicitly delete it.

This effectively reverts commit 04dec3c3ae.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
2017-11-21 14:48:23 +01:00
Kevin Wolf
2b624fe079 block: Add errp to bdrv_all_goto_snapshot()
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
2017-11-21 14:48:22 +01:00
Kevin Wolf
0b62bcbc61 block: Add errp to bdrv_snapshot_goto()
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
2017-11-21 14:48:22 +01:00
Kevin Wolf
1f4ad7d3b8 block: Don't request I/O permission with BDRV_O_NO_IO
'qemu-img info' makes sense even when BLK_PERM_CONSISTENT_READ cannot be
granted because of a block job in a running qemu process. It already
sets BDRV_O_NO_IO to indicate that it doesn't access the guest visible
data at all.

Check the BDRV_O_NO_IO flags in blk_new_open(), so that I/O related
permissions are not unnecessarily requested and 'qemu-img info' can work
even if BLK_PERM_CONSISTENT_READ cannot be granted.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
2017-11-21 14:48:22 +01:00
Peter Maydell
2e02083438 Block layer patches for 2.11.0-rc2
-----BEGIN PGP SIGNATURE-----
 
 iQIcBAABAgAGBQJaDyNMAAoJEH8JsnLIjy/WP+oP/1G0r1b9PBtrEc+B9QG0sMh1
 qivbMRmNUFhuzYW8VBzb2Cpsl/lpe/VlAumf75RUACOh4ZfxkOhaYTMB5x9FHvoa
 5w35UC382zuXTMfeF4JufF5rVJdB+bB9THJOJ7cuaDQSHZNPo8uMmHrk4J5p5uuU
 IrOZLfqgQaWGdobGe6FPWy67z2PRWrzEI/Ogr8zwpa95GJhX3fY1CmGvt2MUBG+j
 cwdYcuX9pawFfjasCkfwYMzY7Uc9wvjIJYSe+WvrDrpjInISgxMOvXylzdWkpsGS
 rHnsLgRXnUE4BmOavzq+uvlo14hlrmdVn5UYlUkbJcx+Bkp7MZGuNjX6doimCdWg
 PpbQB3TIi7ce4v9cYWZSrzDDo6E/d1Evi3xMG0ZouU0ff1DPFIilm8lxb3xYia+b
 ItTLZ7yXWLijGhr4jTeGVhn0+zENiczyiQsL1sDdj83VnpAOGPNZTwtnNGug8ATJ
 VELwA7jlVEL47HYovQoUIFs0NA5xCbOQm5avS5gk44PB/dNkYGKdHhwgQUV/R36A
 lRNj7Vh7hwHSyHO/gKi3sqXTbNG/fTDSy1Nu1xc0pAROIKpc6AanP0mFn2DlZS35
 kyZbxMbuGUsDQ2j0pcGuP60O7+2CY4jlXRCXo6vaHzP3xDfOTx2L9UYVfk/dwNJu
 uYbnbXWv06igjm3Y/2Vv
 =ClZj
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging

Block layer patches for 2.11.0-rc2

# gpg: Signature made Fri 17 Nov 2017 17:58:36 GMT
# gpg:                using RSA key 0x7F09B272C88F2FD6
# gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>"
# Primary key fingerprint: DC3D EB15 9A9A F95D 3D74  56FE 7F09 B272 C88F 2FD6

* remotes/kevin/tags/for-upstream: (25 commits)
  iotests: Make 087 pass without AIO enabled
  block: Make bdrv_next() keep strong references
  qcow2: Fix overly broad madvise()
  qcow2: Refuse to get unaligned offsets from cache
  qcow2: Add bounds check to get_refblock_offset()
  block: Guard against NULL bs->drv
  qcow2: Unaligned zero cluster in handle_alloc()
  qcow2: check_errors are fatal
  qcow2: reject unaligned offsets in write compressed
  iotests: Add test for failing qemu-img commit
  tests: Add check-qobject for equality tests
  iotests: Add test for non-string option reopening
  block: qobject_is_equal() in bdrv_reopen_prepare()
  qapi: Add qobject_is_equal()
  qapi/qlist: Add qlist_append_null() macro
  qapi/qnull: Add own header
  qcow2: fix image corruption on commit with persistent bitmap
  iotests: test clearing unknown autoclear_features by qcow2
  block: Fix permissions in image activation
  qcow2: fix image corruption after committing qcow2 image into base
  ...

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2017-11-17 19:08:07 +00:00
Max Reitz
5e003f17ec block: Make bdrv_next() keep strong references
On one hand, it is a good idea for bdrv_next() to return a strong
reference because ideally nearly every pointer should be refcounted.
This fixes intermittent failure of iotest 194.

On the other, it is absolutely necessary for bdrv_next() itself to keep
a strong reference to both the BB (in its first phase) and the BDS (at
least in the second phase) because when called the next time, it will
dereference those objects to get a link to the next one.  Therefore, it
needs these objects to stay around until then.  Just storing the pointer
to the next in the iterator is not really viable because that pointer
might become invalid as well.

Both arguments taken together means we should probably just invoke
bdrv_ref() and blk_ref() in bdrv_next().  This means we have to assert
that bdrv_next() is always called from the main loop, but that was
probably necessary already before this patch and judging from the
callers, it also looks to actually be the case.

Keeping these strong references means however that callers need to give
them up if they decide to abort the iteration early.  They can do so
through the new bdrv_next_cleanup() function.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171110172545.32609-1-mreitz@redhat.com
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:31 +01:00
Max Reitz
08546bcfb2 qcow2: Fix overly broad madvise()
@mem_size and @offset are both size_t, thus subtracting them from one
another will just return a big size_t if mem_size < offset -- even more
obvious here because the result is stored in another size_t.

Checking that result to be positive is therefore not sufficient to
exclude the case that offset > mem_size.  Thus, we currently sometimes
issue an madvise() over a very large address range.

This is triggered by iotest 163, but with -m64, this does not result in
tangible problems.  But with -m32, this test produces three segfaults,
all of which are fixed by this patch.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171114184127.24238-1-mreitz@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:31 +01:00
Max Reitz
4efb1f7c61 qcow2: Refuse to get unaligned offsets from cache
Instead of using an assertion, it is better to emit a corruption event
here.  Checking all offsets for correct alignment can be tedious and it
is easily possible to forget to do so.  qcow2_cache_do_get() is a
function every L2 and refblock access has to go through, so this is a
good central point to add such a check.

And for good measure, let us also add an assertion that the offset is
non-zero.  Making this a corruption event is not feasible, because a
zero offset usually means something special (such as the cluster is
unused), so all callers should be checking this anyway.  If they do not,
it is their fault, hence the assertion here.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171110203111.7666-6-mreitz@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:31 +01:00
Max Reitz
23482f8a60 qcow2: Add bounds check to get_refblock_offset()
Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728661
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171110203111.7666-5-mreitz@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:31 +01:00
Max Reitz
d470ad42ac block: Guard against NULL bs->drv
We currently do not guard everywhere against a NULL bs->drv where we
should be doing so.  Most of the places fixed here just do not care
about that case at all.

Some care implicitly, e.g. through a prior function call to
bdrv_getlength() which would always fail for an ejected BDS.  Add an
assert there to make it more obvious.

Other places seem to care, but do so insufficiently: Freeing clusters in
a qcow2 image is an error-free operation, but it may leave the image in
an unusable state anyway.  Giving qcow2_free_clusters() an error code is
not really viable, it is much easier to note that bs->drv may be NULL
even after a successful driver call.  This concerns bdrv_co_flush(), and
the way the check is added to bdrv_co_pdiscard() (in every iteration
instead of only once).

Finally, some places employ at least an assert(bs->drv); somewhere, that
may be reasonable (such as in the reopen code), but in
bdrv_has_zero_init(), it is definitely not.  Returning 0 there in case
of an ejected BDS saves us much headache instead.

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728660
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171110203111.7666-4-mreitz@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:31 +01:00
Max Reitz
93bbaf03ff qcow2: Unaligned zero cluster in handle_alloc()
We should check whether the cluster offset we are about to use is
actually valid; that is, whether it is aligned to cluster boundaries.

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728643
Buglink: https://bugs.launchpad.net/qemu/+bug/1728657
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171110203111.7666-3-mreitz@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:30 +01:00
Max Reitz
791fff504c qcow2: check_errors are fatal
When trying to repair a dirty image, qcow2_check() may apparently
succeed (no really fatal error occurred that would prevent the check
from continuing), but if check_errors in the result object is non-zero,
we cannot trust the image to be usable.

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728639
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20171110203111.7666-2-mreitz@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:30 +01:00
Anton Nefedov
3e3b838ffe qcow2: reject unaligned offsets in write compressed
Misaligned compressed write is not supported.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Message-id: 1510654613-47868-2-git-send-email-anton.nefedov@virtuozzo.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
2017-11-17 18:21:30 +01:00
Eric Blake
4096974e18 qcow2: fix image corruption on commit with persistent bitmap
If an image contains persistent bitmaps, we cannot use the
fast path of bdrv_make_empty() to clear the image during
qemu-img commit, because that will lose the clusters related
to the bitmaps.

Also leave a comment in qcow2_read_extensions to remind future
feature additions to think about fast-path removal, since we
just barely fixed the same bug for LUKS encryption.

It's a pain that qemu-img has not yet been taught to manipulate,
or even at a very minimum display, information about persistent
bitmaps; instead, we have to use QMP commands.  It's also a
pain that only qeury-block and x-debug-block-dirty-bitmap-sha256
will allow bitmap introspection; but the former requires the
node to be hooked to a block device, and the latter is experimental.

Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-11-17 18:21:01 +01:00
Eric Blake
08ace1d753 nbd: Don't crash when server reports NBD_CMD_READ failure
If a server fails a read, for example with EIO, but the connection
is still live, then we would crash trying to print a non-existent
error message in nbd_client_co_preadv().  For consistency, also
change the error printout in nbd_read_reply_entry(), although that
instance does not crash.  Bug introduced in commit f140e300.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20171112013936.5942-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
2017-11-17 08:02:45 -06:00
Daniel P. Berrange
f06033295b qcow2: fix image corruption after committing qcow2 image into base
After committing the qcow2 image contents into the base image, qemu-img
will call bdrv_make_empty to drop the payload in the layered image.

When this is done for qcow2 images, it blows away the LUKS encryption
header, making the resulting image unusable. There are two codepaths
for emptying a qcow2 image, and the second (slower) codepath leaves
the LUKS header intact, so force use of that codepath.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-11-17 13:36:03 +01:00
Kevin Wolf
398e6ad014 block: Deprecate bdrv_set_read_only() and users
bdrv_set_read_only() is used by some block drivers to override the
read-only option given by the user. This is not how read-only images
generally work in QEMU: Instead of second guessing what the user really
meant (which currently includes making an image read-only even if the
user didn't only use the default, but explicitly said read-only=off), we
should error out if we can't provide what the user requested.

This adds deprecation warnings to all callers of bdrv_set_read_only() so
that the behaviour can be corrected after the usual deprecation period.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-11-17 13:35:59 +01:00
Daniel P. Berrange
f66afbe26f qcow2: don't permit changing encryption parameters
Currently if trying to change encryption parameters on a qcow2 image, qemu-img
will abort. We already explicitly check for attempt to change encrypt.format
but missed other parameters like encrypt.key-secret. Rather than list each
parameter, just blacklist changing of all parameters with a 'encrypt.' prefix.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2017-11-17 13:35:59 +01:00