aboutsummaryrefslogtreecommitdiff
path: root/block
AgeCommit message (Collapse)Author
2020-05-05block: Comment cleanupsEric Blake
It's been a while since we got rid of the sector-based bdrv_read and bdrv_write (commit 2e11d756); let's finish the job on a few remaining comments. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200428213807.776655-1-eblake@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-05-05qcow2: Tweak comment about bitmaps vs. resizeEric Blake
Our comment did not actually match the code. Rewrite the comment to be less sensitive to any future changes to qcow2-bitmap.c that might implement scenarios that we currently reject. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200428192648.749066-4-eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-05-05qcow2: Allow resize of images with internal snapshotsEric Blake
We originally refused to allow resize of images with internal snapshots because the v2 image format did not require the tracking of snapshot size, making it impossible to safely revert to a snapshot with a different size than the current view of the image. But the snapshot size tracking was rectified in v3, and our recent fixes to qemu-img amend (see 0a85af35) guarantee that we always have a valid snapshot size. Thus, we no longer need to artificially limit image resizes, but it does become one more thing that would prevent a downgrade back to v2. And now that we support different-sized snapshots, it's also easy to fix reverting to a snapshot to apply the new size. Upgrade iotest 61 to cover this (we previously had NO coverage of refusal to resize while snapshots exist). Note that the amend process can fail but still have effects: in particular, since we break things into upgrade, resize, downgrade, a failure during resize does not roll back changes made during upgrade, nor does failure in downgrade roll back a resize. But this situation is pre-existing even without this patch; and without journaling, the best we could do is minimize the chance of partial failure by collecting all changes prior to doing any writes - which adds a lot of complexity but could still fail with EIO. On the other hand, we are careful that even if we have partial modification but then fail, the image is left viable (that is, we are careful to sequence things so that after each successful cluster write, there may be transient leaked clusters but no corrupt metadata). And complicating the code to make it more transaction-like is not worth the effort: a user can always request multiple 'qemu-img amend' changing one thing each, if they need finer-grained control over detecting the first failure than what they get by letting qemu decide how to sequence multiple changes. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200428192648.749066-3-eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-05-05block: Add blk_new_with_bs() helperEric Blake
There are several callers that need to create a new block backend from an existing BDS; make the task slightly easier with a common helper routine. Suggested-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200424190903.522087-2-eblake@redhat.com> [mreitz: Set @ret only in error paths, see https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg01216.html] Signed-off-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200428192648.749066-2-eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-04-30qcow2: Forward ZERO_WRITE flag for full preallocationKevin Wolf
The BDRV_REQ_ZERO_WRITE is currently implemented in a way that first the image is possibly preallocated and then the zero flag is added to all clusters. This means that a copy-on-write operation may be needed when writing to these clusters, despite having used preallocation, negating one of the major benefits of preallocation. Instead, try to forward the BDRV_REQ_ZERO_WRITE to the protocol driver, and if the protocol driver can ensure that the new area reads as zeros, we can skip setting the zero flag in the qcow2 layer. Unfortunately, the same approach doesn't work for metadata preallocation, so we'll still set the zero flag there. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200424142701.67053-1-kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30block: truncate: Don't make backing file data visibleKevin Wolf
When extending the size of an image that has a backing file larger than its old size, make sure that the backing file data doesn't become visible in the guest, but the added area is properly zeroed out. Consider the following scenario where the overlay is shorter than its backing file: base.qcow2: AAAAAAAA overlay.qcow2: BBBB When resizing (extending) overlay.qcow2, the new blocks should not stay unallocated and make the additional As from base.qcow2 visible like before this patch, but zeros should be read. A similar case happens with the various variants of a commit job when an intermediate file is short (- for unallocated): base.qcow2: A-A-AAAA mid.qcow2: BB-B top.qcow2: C--C--C- After commit top.qcow2 to mid.qcow2, the following happens: mid.qcow2: CB-C00C0 (correct result) mid.qcow2: CB-C--C- (before this fix) Without the fix, blocks that previously read as zeros on top.qcow2 suddenly turn into A. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200424125448.63318-8-kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30file-posix: Support BDRV_REQ_ZERO_WRITE for truncateKevin Wolf
For regular files, we always get BDRV_REQ_ZERO_WRITE behaviour from the OS, so we can advertise the flag and just ignore it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200424125448.63318-7-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30raw-format: Support BDRV_REQ_ZERO_WRITE for truncateKevin Wolf
The raw format driver can simply forward the flag and let its bs->file child take care of actually providing the zeros. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200424125448.63318-6-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30qcow2: Support BDRV_REQ_ZERO_WRITE for truncateKevin Wolf
If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't undo any previous preallocation, but just adds the zero flag to all relevant L2 entries. If an external data file is in use, a write_zeroes request to the data file is made instead. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200424125448.63318-5-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30block-backend: Add flags to blk_truncate()Kevin Wolf
Now that node level interface bdrv_truncate() supports passing request flags to the block driver, expose this on the BlockBackend level, too. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200424125448.63318-4-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30block: Add flags to bdrv(_co)_truncate()Kevin Wolf
Now that block drivers can support flags for .bdrv_co_truncate, expose the parameter in the node level interfaces bdrv_co_truncate() and bdrv_truncate(). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200424125448.63318-3-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30block: Add flags to BlockDriver.bdrv_co_truncate()Kevin Wolf
This adds a new BdrvRequestFlags parameter to the .bdrv_co_truncate() driver callbacks, and a supported_truncate_flags field in BlockDriverState that allows drivers to advertise support for request flags in the context of truncate. For now, we always pass 0 and no drivers declare support for any flag. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200424125448.63318-2-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-30qapi: Only input visitors can actually failMarkus Armbruster
The previous few commits have made this more obvious, and removed the one exception. Time to clarify the documentation, and drop dead error checking. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20200424084338.26803-13-armbru@redhat.com>
2020-04-29block/file-posix: Fix check_cache_dropped() error handlingMarkus Armbruster
The Error ** argument must be NULL, &error_abort, &error_fatal, or a pointer to a variable containing NULL. Passing an argument of the latter kind twice without clearing it in between is wrong: if the first call sets an error, it no longer points to NULL for the second call. check_cache_dropped() calls error_setg() in a loop. It fails to break the loop in one instance. If a subsequent iteration error_setg()s again, it trips error_setv()'s assertion. Fix it to break the loop. Fixes: 31be8a2a97ecba7d31a82932286489cac318e9e9 Cc: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <20200422130719.28225-3-armbru@redhat.com>
2020-04-29various: Remove suspicious '\' character outside of #define in C codePhilippe Mathieu-Daudé
Fixes the following coccinelle warnings: $ spatch --sp-file --verbose-parsing ... \ scripts/coccinelle/remove_local_err.cocci ... SUSPICIOUS: a \ character appears outside of a #define at ./target/ppc/translate_init.inc.c:5213 SUSPICIOUS: a \ character appears outside of a #define at ./target/ppc/translate_init.inc.c:5261 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:166 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:167 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:169 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:170 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:171 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:172 SUSPICIOUS: a \ character appears outside of a #define at ./target/microblaze/cpu.c:173 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5787 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5789 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5800 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5801 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5802 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5804 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5805 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:5806 SUSPICIOUS: a \ character appears outside of a #define at ./target/i386/cpu.c:6329 SUSPICIOUS: a \ character appears outside of a #define at ./hw/sd/sdhci.c:1133 SUSPICIOUS: a \ character appears outside of a #define at ./hw/scsi/scsi-disk.c:3081 SUSPICIOUS: a \ character appears outside of a #define at ./hw/net/virtio-net.c:1529 SUSPICIOUS: a \ character appears outside of a #define at ./hw/riscv/sifive_u.c:468 SUSPICIOUS: a \ character appears outside of a #define at ./dump/dump.c:1895 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2209 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2215 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2221 SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2222 SUSPICIOUS: a \ character appears outside of a #define at ./block/replication.c:172 SUSPICIOUS: a \ character appears outside of a #define at ./block/replication.c:173 Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-Id: <20200412223619.11284-2-f4bug@amsat.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2020-04-20block/iscsi:fix heap-buffer-overflow in iscsi_aio_ioctl_cbChen Qun
There is an overflow, the source 'datain.data[2]' is 100 bytes, but the 'ss' is 252 bytes.This may cause a security issue because we can access a lot of unrelated memory data. The len for sbp copy data should take the minimum of mx_sb_len and sb_len_wr, not the maximum. If we use iscsi device for VM backend storage, ASAN show stack: READ of size 252 at 0xfffd149dcfc4 thread T0 #0 0xaaad433d0d34 in __asan_memcpy (aarch64-softmmu/qemu-system-aarch64+0x2cb0d34) #1 0xaaad45f9d6d0 in iscsi_aio_ioctl_cb /qemu/block/iscsi.c:996:9 #2 0xfffd1af0e2dc (/usr/lib64/iscsi/libiscsi.so.8+0xe2dc) #3 0xfffd1af0d174 (/usr/lib64/iscsi/libiscsi.so.8+0xd174) #4 0xfffd1af19fac (/usr/lib64/iscsi/libiscsi.so.8+0x19fac) #5 0xaaad45f9acc8 in iscsi_process_read /qemu/block/iscsi.c:403:5 #6 0xaaad4623733c in aio_dispatch_handler /qemu/util/aio-posix.c:467:9 #7 0xaaad4622f350 in aio_dispatch_handlers /qemu/util/aio-posix.c:510:20 #8 0xaaad4622f350 in aio_dispatch /qemu/util/aio-posix.c:520 #9 0xaaad46215944 in aio_ctx_dispatch /qemu/util/async.c:298:5 #10 0xfffd1bed12f4 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x512f4) #11 0xaaad46227de0 in glib_pollfds_poll /qemu/util/main-loop.c:219:9 #12 0xaaad46227de0 in os_host_main_loop_wait /qemu/util/main-loop.c:242 #13 0xaaad46227de0 in main_loop_wait /qemu/util/main-loop.c:518 #14 0xaaad43d9d60c in qemu_main_loop /qemu/softmmu/vl.c:1662:9 #15 0xaaad4607a5b0 in main /qemu/softmmu/main.c:49:5 #16 0xfffd1a460b9c in __libc_start_main (/lib64/libc.so.6+0x20b9c) #17 0xaaad43320740 in _start (aarch64-softmmu/qemu-system-aarch64+0x2c00740) 0xfffd149dcfc4 is located 0 bytes to the right of 100-byte region [0xfffd149dcf60,0xfffd149dcfc4) allocated by thread T0 here: #0 0xaaad433d1e70 in __interceptor_malloc (aarch64-softmmu/qemu-system-aarch64+0x2cb1e70) #1 0xfffd1af0e254 (/usr/lib64/iscsi/libiscsi.so.8+0xe254) #2 0xfffd1af0d174 (/usr/lib64/iscsi/libiscsi.so.8+0xd174) #3 0xfffd1af19fac (/usr/lib64/iscsi/libiscsi.so.8+0x19fac) #4 0xaaad45f9acc8 in iscsi_process_read /qemu/block/iscsi.c:403:5 #5 0xaaad4623733c in aio_dispatch_handler /qemu/util/aio-posix.c:467:9 #6 0xaaad4622f350 in aio_dispatch_handlers /qemu/util/aio-posix.c:510:20 #7 0xaaad4622f350 in aio_dispatch /qemu/util/aio-posix.c:520 #8 0xaaad46215944 in aio_ctx_dispatch /qemu/util/async.c:298:5 #9 0xfffd1bed12f4 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x512f4) #10 0xaaad46227de0 in glib_pollfds_poll /qemu/util/main-loop.c:219:9 #11 0xaaad46227de0 in os_host_main_loop_wait /qemu/util/main-loop.c:242 #12 0xaaad46227de0 in main_loop_wait /qemu/util/main-loop.c:518 #13 0xaaad43d9d60c in qemu_main_loop /qemu/softmmu/vl.c:1662:9 #14 0xaaad4607a5b0 in main /qemu/softmmu/main.c:49:5 #15 0xfffd1a460b9c in __libc_start_main (/lib64/libc.so.6+0x20b9c) #16 0xaaad43320740 in _start (aarch64-softmmu/qemu-system-aarch64+0x2c00740) Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-id: 20200418062602.10776-1-kuhn.chenqun@huawei.com Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2020-04-07Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into stagingPeter Maydell
Block layer patches: - Fix crashes and hangs related to iothreads, bdrv_drain and block jobs: - Fix some AIO context locking in jobs - Fix blk->in_flight during blk_wait_while_drained() - vpc: Don't round up already aligned BAT sizes # gpg: Signature made Tue 07 Apr 2020 15:25:24 BST # gpg: using RSA key 7F09B272C88F2FD6 # gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>" [full] # Primary key fingerprint: DC3D EB15 9A9A F95D 3D74 56FE 7F09 B272 C88F 2FD6 * remotes/kevin/tags/for-upstream: vpc: Don't round up already aligned BAT sizes block: Fix blk->in_flight during blk_wait_while_drained() block: Increase BB.in_flight for coroutine and sync interfaces block-backend: Reorder flush/pdiscard function definitions backup: don't acquire aio_context in backup_clean replication: assert we own context before job_cancel_sync job: take each job's lock individually in job_txn_apply Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2020-04-07vpc: Don't round up already aligned BAT sizesKevin Wolf
As reported on Launchpad, Azure apparently doesn't accept images for upload that are not both aligned to 1 MB blocks and have a BAT size that matches the image size exactly. As far as I can tell, there is no real reason why we create a BAT that is one entry longer than necessary for aligned image sizes, so change that. (Even though the condition is only mentioned as "should" in the spec and previous products accepted larger BATs - but we'll try to maintain compatibility with as many of Microsoft's ever-changing interpretations of the VHD spec as possible.) Fixes: https://bugs.launchpad.net/bugs/1870098 Reported-by: Tobias Witek Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200402093603.2369-1-kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-07block: Fix blk->in_flight during blk_wait_while_drained()Kevin Wolf
Waiting in blk_wait_while_drained() while blk->in_flight is increased for the current request is wrong because it will cause the drain operation to deadlock. This patch makes sure that blk_wait_while_drained() is called with blk->in_flight increased exactly once for the current request, and that it temporarily decreases the counter while it waits. Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200407121259.21350-4-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-07block: Increase BB.in_flight for coroutine and sync interfacesKevin Wolf
External callers of blk_co_*() and of the synchronous blk_*() functions don't currently increase the BlockBackend.in_flight counter, but calls from blk_aio_*() do, so there is an inconsistency whether the counter has been increased or not. This patch moves the actual operations to static functions that can later know they will always be called with in_flight increased exactly once, even for external callers using the blk_co_*() coroutine interfaces. If the public blk_co_*() interface is unused, remove it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200407121259.21350-3-kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-07block-backend: Reorder flush/pdiscard function definitionsKevin Wolf
Move all variants of the flush/pdiscard functions to a single place and put the blk_co_*() version first because it is called by all other variants (and will become static in the next patch). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200407121259.21350-2-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-07backup: don't acquire aio_context in backup_cleanStefan Reiter
All code-paths leading to backup_clean (via job_clean) have the job's context already acquired. The job's context is guaranteed to be the same as the one used by backup_top via backup_job_create. Since the previous logic effectively acquired the lock twice, this broke cleanup of backups for disks using IO threads, since the BDRV_POLL_WHILE in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock once, thus deadlocking with the IO thread. This is a partial revert of 0abf2581717a19. Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20200407115651.69472-4-s.reiter@proxmox.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-07replication: assert we own context before job_cancel_syncStefan Reiter
job_cancel_sync requires the job's lock to be held, all other callers already do this (replication_stop, drive_backup_abort, blockdev_backup_abort, job_cancel_sync_all, cancel_common). In this case we're in a BlockDriver handler, so we already have a lock, just assert that it is the same as the one used for the commit_job. Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> Message-Id: <20200407115651.69472-3-s.reiter@proxmox.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-04-07qcow2: Check request size in qcow2_co_pwritev_compressed_part()Alberto Garcia
When issuing a compressed write request the number of bytes must be a multiple of the cluster size or reach the end of the last cluster. With the current code such requests are allowed and we hit an assertion: $ qemu-img create -f qcow2 img.qcow2 1M $ qemu-io -c 'write -c 0 32k' img.qcow2 qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task: Assertion `bytes == s->cluster_size || (bytes < s->cluster_size && (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' failed. Aborted This patch fixes a regression introduced in 0d483dce38 Signed-off-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200406143401.26854-1-berto@igalia.com> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-04-07qcow2: Forbid discard in qcow2 v2 images with backing filesAlberto Garcia
A discard request deallocates the selected clusters so they read back as zeroes. This is done by clearing the cluster offset field and setting QCOW_OFLAG_ZERO in the L2 entry. This flag is however only supported when qcow_version >= 3. In older images the cluster is simply deallocated, exposing any possible stale data from the backing file. Since discard is an advisory operation it's safer to simply forbid it in this scenario. Note that we are adding this check to qcow2_co_pdiscard() and not to qcow2_cluster_discard() or discard_in_l2_slice() because the last two are also used by qcow2_snapshot_create() to discard the clusters used by the VM state. In this case there's no risk of exposing stale data to the guest and we really want that the clusters are always discarded. Signed-off-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200331114345.29993-1-berto@igalia.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-27qcow2: Remove unused fields from BDRVQcow2StateKevin Wolf
These fields were already removed in commit c3c10f72, but then commit b58deb34 revived them probably due to bad merge conflict resolution. They are still unused, so remove them again. Fixes: b58deb344ddff3b9d8b265bf73a65274767ee5f4 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200326170757.12344-1-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-03-27mirror: Wait only for in-flight operationsKevin Wolf
mirror_wait_for_free_in_flight_slot() just picks a random operation to wait for. However, a MirrorOp is already in s->ops_in_flight when mirror_co_read() waits for free slots, so if not enough slots are immediately available, an operation can end up waiting for itself, or two or more operations can wait for each other to complete, which results in a hang. Fix this by adding a flag to MirrorOp that tells us if the request is already in flight (and therefore occupies slots that it will later free), and picking only such operations for waiting. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1794692 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200326153628.4869-3-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-03-27Revert "mirror: Don't let an operation wait for itself"Kevin Wolf
This reverts commit 7e6c4ff792734e196c8ca82564c56b5e7c6288ca. The fix was incomplete as it only protected against requests waiting for themselves, but not against requests waiting for each other. We need a different solution. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200326153628.4869-2-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-03-27block/iscsi:use the flags in iscsi_open() prevent Clang warningChen Qun
Clang static code analyzer show warning: block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read flags &= ~BDRV_O_RDWR; ^ ~~~~~~~~~~~~ In iscsi_allocmap_init() only checks BDRV_O_NOCACHE, which is the same in both of flags and bs->open_flags. We can use the flags instead bs->open_flags to prevent Clang warning. Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200311032927.35092-1-kuhn.chenqun@huawei.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2020-03-26sheepdog: Consistently set bdrv_has_zero_init_truncateEric Blake
block_int.h claims that .bdrv_has_zero_init must return 0 if .bdrv_has_zero_init_truncate does likewise; but this is violated if only the former callback is provided if .bdrv_co_truncate also exists. When adding the latter callback, it was mistakenly added to only one of the three possible sheepdog instantiations. Fixes: 1dcaf527 Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200324174233.1622067-5-eblake@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-26qcow2: Avoid feature name extension on small cluster sizeEric Blake
As the feature name table can be quite large (over 9k if all 64 bits of all three feature fields have names; a mere 8 features leaves only 8 bytes for a backing file name in a 512-byte cluster), it is unwise to emit this optional header in images with small cluster sizes. Update iotest 036 to skip running on small cluster sizes; meanwhile, note that iotest 061 never passed on alternative cluster sizes (however, I limited this patch to tests with output affected by adding feature names, rather than auditing for other tests that are not robust to alternative cluster sizes). Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200324174233.1622067-4-eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-26qcow2: List autoclear bit names in headerEric Blake
The feature table is supposed to advertise the name of all feature bits that we support; however, we forgot to update the table for autoclear bits. While at it, move the table to read-only memory in code, and tweak the qcow2 spec to name the second autoclear bit. Update iotests that are affected by the longer header length. Fixes: 88ddffae Fixes: 93c24936 Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200324174233.1622067-3-eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-26qcow2: Comment typo fixesEric Blake
Various trivial typos noticed while working on this file. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200324174233.1622067-2-eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-26block: trickle down the fallback image creation function use to the block ↵Maxim Levitsky
drivers Instead of checking the .bdrv_co_create_opts to see if we need the fallback, just implement the .bdrv_co_create_opts in the drivers that need it. This way we don't break various places that need to know if the underlying protocol/format really supports image creation, and this way we still allow some drivers to not support image creation. Fixes: fd17146cd93d1704cd96d7c2757b325fc7aac6fd Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1816007 Note that technically this driver reverts the image creation fallback for the vxhs driver since I don't have a means to test it, and IMHO it is better to leave it not supported as it was prior to generic image creation patches. Also drop iscsi_create_opts which was left accidentally. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> Message-Id: <20200326011218.29230-3-mlevitsk@redhat.com> Reviewed-by: Denis V. Lunev <den@openvz.org> [mreitz: Fixed alignment, and moved bdrv_co_create_opts_simple() and bdrv_create_opts_simple from block.h into block_int.h] Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-26block: pass BlockDriver reference to the .bdrv_co_createMaxim Levitsky
This will allow the reuse of a single generic .bdrv_co_create implementation for several drivers. No functional changes. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> Message-Id: <20200326011218.29230-2-mlevitsk@redhat.com> Reviewed-by: Denis V. Lunev <den@openvz.org> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-26block/mirror: fix use after free of local_errVladimir Sementsov-Ogievskiy
local_err is used again in mirror_exit_common() after bdrv_set_backing_hd(), so we must zero it. Otherwise try to set non-NULL local_err will crash. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200324153630.11882-3-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-24block/qcow2: zero data_file child after freeVladimir Sementsov-Ogievskiy
data_file being NULL doesn't seem to be a correct state, but it's better than dead pointer and simpler to debug. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200316060631.30052-3-vsementsov@virtuozzo.com> Reviewed-by: John Snow <jsnow@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-24block: Avoid memleak on qcow2 image info failureEric Blake
If we fail to get bitmap info, we must not leak the encryption info. Fixes: b8968c875f403 Fixes: Coverity CID 1421894 Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200320183620.1112123-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Tested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Signed-off-by: Max Reitz <mreitz@redhat.com>
2020-03-19Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into ↵Peter Maydell
staging Pull request # gpg: Signature made Wed 18 Mar 2020 20:23:28 GMT # gpg: using RSA key F9B7ABDBBCACDF95BE76CBD07DEF8106AAFC390E # gpg: Good signature from "John Snow (John Huston) <jsnow@redhat.com>" [full] # Primary key fingerprint: FAEB 9711 A12C F475 812F 18F2 88A9 064D 1835 61EB # Subkey fingerprint: F9B7 ABDB BCAC DF95 BE76 CBD0 7DEF 8106 AAFC 390E * remotes/jnsnow/tags/bitmaps-pull-request: block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty nbd/server: use bdrv_dirty_bitmap_next_dirty_area nbd/server: introduce NBDExtentArray block/dirty-bitmap: improve _next_dirty_area API block/dirty-bitmap: add _next_dirty API block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t hbitmap: drop meta bitmaps as they are unused hbitmap: unpublish hbitmap_iter_skip_words hbitmap: move hbitmap_iter_next_word to hbitmap.c hbitmap: assert that we don't create bitmap larger than INT64_MAX build: Silence clang warning on older glib autoptr usage Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2020-03-18block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirtyVladimir Sementsov-Ogievskiy
store_bitmap_data() loop does bdrv_set_dirty_iter() on each iteration, which means that we actually don't need iterator itself and we can use simpler bitmap API. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-id: 20200205112041.6003-11-vsementsov@virtuozzo.com Signed-off-by: John Snow <jsnow@redhat.com>
2020-03-18block/dirty-bitmap: improve _next_dirty_area APIVladimir Sementsov-Ogievskiy
Firstly, _next_dirty_area is for scenarios when we may contiguously search for next dirty area inside some limited region, so it is more comfortable to specify "end" which should not be recalculated on each iteration. Secondly, let's add a possibility to limit resulting area size, not limiting searching area. This will be used in NBD code in further commit. (Note that now bdrv_dirty_bitmap_next_dirty_area is unused) Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-id: 20200205112041.6003-8-vsementsov@virtuozzo.com Signed-off-by: John Snow <jsnow@redhat.com>
2020-03-18block/dirty-bitmap: add _next_dirty APIVladimir Sementsov-Ogievskiy
We have bdrv_dirty_bitmap_next_zero, let's add corresponding bdrv_dirty_bitmap_next_dirty, which is more comfortable to use than bitmap iterators in some cases. For test modify test_hbitmap_next_zero_check_range to check both next_zero and next_dirty and add some new checks. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-id: 20200205112041.6003-7-vsementsov@virtuozzo.com Signed-off-by: John Snow <jsnow@redhat.com>
2020-03-18block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_tVladimir Sementsov-Ogievskiy
We are going to introduce bdrv_dirty_bitmap_next_dirty so that same variable may be used to store its return value and to be its parameter, so it would int64_t. Similarly, we are going to refactor hbitmap_next_dirty_area to use hbitmap_next_dirty together with hbitmap_next_zero, therefore we want hbitmap_next_zero parameter type to be int64_t too. So, for convenience update all parameters of *_next_zero and *_next_dirty_area to be int64_t. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-id: 20200205112041.6003-6-vsementsov@virtuozzo.com Signed-off-by: John Snow <jsnow@redhat.com>
2020-03-18Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2020-03-17' ↵Peter Maydell
into staging Error reporting patches for 2020-03-17 # gpg: Signature made Tue 17 Mar 2020 16:30:49 GMT # gpg: using RSA key 354BC8B3D7EB2A6B68674E5F3870B400EB918653 # gpg: issuer "armbru@redhat.com" # gpg: Good signature from "Markus Armbruster <armbru@redhat.com>" [full] # gpg: aka "Markus Armbruster <armbru@pond.sub.org>" [full] # Primary key fingerprint: 354B C8B3 D7EB 2A6B 6867 4E5F 3870 B400 EB91 8653 * remotes/armbru/tags/pull-error-2020-03-17: hw/sd/ssi-sd: fix error handling in ssi_sd_realize xen-block: Use one Error * variable instead of two hw/misc/ivshmem: Use one Error * variable instead of two Use &error_abort instead of separate assert() Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2020-03-17Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into stagingPeter Maydell
* Bugfixes all over the place * get/set_uint cleanups (Felipe) * Lock guard support (Stefan) * MemoryRegion ownership cleanup (Philippe) * AVX512 optimization for buffer_is_zero (Robert) # gpg: Signature made Tue 17 Mar 2020 15:01:54 GMT # gpg: using RSA key BFFBD25F78C7AE83 # gpg: Good signature from "Paolo Bonzini <bonzini@gnu.org>" [full] # gpg: aka "Paolo Bonzini <pbonzini@redhat.com>" [full] # Primary key fingerprint: 46F5 9FBD 57D6 12E7 BFD4 E2F7 7E15 100C CD36 69B1 # Subkey fingerprint: F133 3857 4B66 2389 866C 7682 BFFB D25F 78C7 AE83 * remotes/bonzini/tags/for-upstream: (62 commits) hw/arm: Let devices own the MemoryRegion they create hw/arm: Remove unnecessary memory_region_set_readonly() on ROM alias hw/ppc/ppc405: Use memory_region_init_rom() with read-only regions hw/arm/stm32: Use memory_region_init_rom() with read-only regions hw/char: Let devices own the MemoryRegion they create hw/riscv: Let devices own the MemoryRegion they create hw/dma: Let devices own the MemoryRegion they create hw/display: Let devices own the MemoryRegion they create hw/core: Let devices own the MemoryRegion they create scripts/cocci: Patch to let devices own their MemoryRegions scripts/cocci: Patch to remove unnecessary memory_region_set_readonly() scripts/cocci: Patch to detect potential use of memory_region_init_rom hw/sparc: Use memory_region_init_rom() with read-only regions hw/sh4: Use memory_region_init_rom() with read-only regions hw/riscv: Use memory_region_init_rom() with read-only regions hw/ppc: Use memory_region_init_rom() with read-only regions hw/pci-host: Use memory_region_init_rom() with read-only regions hw/net: Use memory_region_init_rom() with read-only regions hw/m68k: Use memory_region_init_rom() with read-only regions hw/display: Use memory_region_init_rom() with read-only regions ... Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2020-03-17Use &error_abort instead of separate assert()Markus Armbruster
Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20200313170517.22480-2-armbru@redhat.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Acked-by: Alexander Bulekov <alxndr@bu.edu> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> [Unused Error *variable deleted]
2020-03-16misc: Replace zero-length arrays with flexible array member (manual)Philippe Mathieu-Daudé
Description copied from Linux kernel commit from Gustavo A. R. Silva (see [3]): --v-- description start --v-- The current codebase makes use of the zero-length array language extension to the C90 standard, but the preferred mechanism to declare variable-length types such as these ones is a flexible array member [1], introduced in C99: struct foo { int stuff; struct boo array[]; }; By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being unadvertenly introduced [2] to the Linux codebase from now on. --^-- description end --^-- Do the similar housekeeping in the QEMU codebase (which uses C99 since commit 7be41675f7cb). All these instances of code were found with the help of the following command (then manual analysis, without modifying structures only having a single flexible array member, such QEDTable in block/qed.h): git grep -F '[0];' [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f [3] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1 Inspired-by: Gustavo A. R. Silva <gustavo@embeddedor.com> Reviewed-by: David Hildenbrand <david@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2020-03-16misc: Replace zero-length arrays with flexible array member (automatic)Philippe Mathieu-Daudé
Description copied from Linux kernel commit from Gustavo A. R. Silva (see [3]): --v-- description start --v-- The current codebase makes use of the zero-length array language extension to the C90 standard, but the preferred mechanism to declare variable-length types such as these ones is a flexible array member [1], introduced in C99: struct foo { int stuff; struct boo array[]; }; By making use of the mechanism above, we will get a compiler warning in case the flexible array does not occur last in the structure, which will help us prevent some kind of undefined behavior bugs from being unadvertenly introduced [2] to the Linux codebase from now on. --^-- description end --^-- Do the similar housekeeping in the QEMU codebase (which uses C99 since commit 7be41675f7cb). All these instances of code were found with the help of the following Coccinelle script: @@ identifier s, m, a; type t, T; @@ struct s { ... t m; - T a[0]; + T a[]; }; @@ identifier s, m, a; type t, T; @@ struct s { ... t m; - T a[0]; + T a[]; } QEMU_PACKED; [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f [3] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1 Inspired-by: Gustavo A. R. Silva <gustavo@embeddedor.com> Reviewed-by: David Hildenbrand <david@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2020-03-16block/io: fix bdrv_co_do_copy_on_readvVladimir Sementsov-Ogievskiy
Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end anyway. But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on part of original qiov, defined by qiov_offset and bytes. So we must not touch qiov behind qiov_offset+bytes bound. Fix it. Cc: qemu-stable@nongnu.org # v4.2 Fixes: 1143ec5ebf4 Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-id: 20200312081949.5350-1-vsementsov@virtuozzo.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2020-03-12Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into stagingPeter Maydell
Block layer patches: - Relax restrictions for blockdev-snapshot (allows libvirt to do live storage migration with blockdev-mirror) - luks: Delete created files when block_crypto_co_create_opts_luks fails - Fix memleaks in qmp_object_add # gpg: Signature made Wed 11 Mar 2020 15:38:59 GMT # gpg: using RSA key 7F09B272C88F2FD6 # gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>" [full] # Primary key fingerprint: DC3D EB15 9A9A F95D 3D74 56FE 7F09 B272 C88F 2FD6 * remotes/kevin/tags/for-upstream: qemu-iotests: adding LUKS cleanup for non-UTF8 secret error crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails block.c: adding bdrv_co_delete_file block: introducing 'bdrv_co_delete_file' interface tests/qemu-iotests: Fix socket_scm_helper build path qapi: Add '@allow-write-only-overlay' feature for 'blockdev-snapshot' iotests: Add iothread cases to 155 block: Fix cross-AioContext blockdev-snapshot iotests: Test mirror with temporarily disabled target backing file iotests: Fix run_job() with use_log=False block: Relax restrictions for blockdev-snapshot block: Make bdrv_get_cumulative_perm() public qom-qmp-cmds: fix two memleaks in qmp_object_add Signed-off-by: Peter Maydell <peter.maydell@linaro.org>