The following changes since commit 4c8c1cc544dbd5e2564868e61c5037258e393832: Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.10-pull-request' into staging (2017-06-22 19:01:58 +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 1512008812410ca4054506a7c44343088abdd977: Merge remote-tracking branch 'mreitz/tags/pull-block-2017-06-23' into queue-block (2017-06-23 14:09:12 +0200) ---------------------------------------------------------------- Block layer patches ---------------------------------------------------------------- Alberto Garcia (9): throttle: Update throttle-groups.c documentation qcow2: Remove unused Error variable in do_perform_cow() qcow2: Use unsigned int for both members of Qcow2COWRegion qcow2: Make perform_cow() call do_perform_cow() twice qcow2: Split do_perform_cow() into _read(), _encrypt() and _write() qcow2: Allow reading both COW regions with only one request qcow2: Pass a QEMUIOVector to do_perform_cow_{read,write}() qcow2: Merge the writing of the COW regions with the guest data qcow2: Use offset_into_cluster() and offset_to_l2_index() Kevin Wolf (37): commit: Fix completion with extra reference qemu-iotests: Allow starting new qemu after cleanup qemu-iotests: Test exiting qemu with running job doc: Document generic -blockdev options doc: Document driver-specific -blockdev options qed: Use bottom half to resume waiting requests qed: Make qed_read_table() synchronous qed: Remove callback from qed_read_table() qed: Remove callback from qed_read_l2_table() qed: Remove callback from qed_find_cluster() qed: Make qed_read_backing_file() synchronous qed: Make qed_copy_from_backing_file() synchronous qed: Remove callback from qed_copy_from_backing_file() qed: Make qed_write_header() synchronous qed: Remove callback from qed_write_header() qed: Make qed_write_table() synchronous qed: Remove GenericCB qed: Remove callback from qed_write_table() qed: Make qed_aio_read_data() synchronous qed: Make qed_aio_write_main() synchronous qed: Inline qed_commit_l2_update() qed: Add return value to qed_aio_write_l1_update() qed: Add return value to qed_aio_write_l2_update() qed: Add return value to qed_aio_write_main() qed: Add return value to qed_aio_write_cow() qed: Add return value to qed_aio_write_inplace/alloc() qed: Add return value to qed_aio_read/write_data() qed: Remove ret argument from qed_aio_next_io() qed: Remove recursion in qed_aio_next_io() qed: Implement .bdrv_co_readv/writev qed: Use CoQueue for serialising allocations qed: Simplify request handling qed: Use a coroutine for need_check_timer qed: Add coroutine_fn to I/O path functions qed: Use bdrv_co_* for coroutine_fns block: Remove bdrv_aio_readv/writev/flush() Merge remote-tracking branch 'mreitz/tags/pull-block-2017-06-23' into queue-block Manos Pitsidianakis (1): block: change variable names in BlockDriverState Max Reitz (3): blkdebug: Catch bs->exact_filename overflow blkverify: Catch bs->exact_filename overflow block: Do not strcmp() with NULL uri->scheme Stefan Hajnoczi (10): 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() virtio-pci: use ioeventfd even when KVM is disabled migration: hold AioContext lock for loadvm qemu_fclose() qemu-iotests: 068: extract _qemu() function qemu-iotests: 068: use -drive/-device instead of -hda qemu-iotests: 068: test iothread mode qemu-img: don't shadow opts variable in img_dd() Stephen Bates (1): nvme: Add support for Read Data and Write Data in CMBs. sochin.jiang (1): fix: avoid an infinite loop or a dangling pointer problem in img_commit block/Makefile.objs | 2 +- block/blkdebug.c | 46 +-- block/blkreplay.c | 8 +- block/blkverify.c | 12 +- block/block-backend.c | 22 +- block/commit.c | 7 + block/file-posix.c | 34 +- block/io.c | 240 ++----------- block/iscsi.c | 20 +- block/mirror.c | 8 +- block/nbd-client.c | 8 +- block/nbd-client.h | 4 +- block/nbd.c | 6 +- block/nfs.c | 2 +- block/qcow2-cluster.c | 201 ++++++++--- block/qcow2.c | 94 +++-- block/qcow2.h | 11 +- block/qed-cluster.c | 124 +++---- block/qed-gencb.c | 33 -- block/qed-table.c | 261 +++++--------- block/qed.c | 779 ++++++++++++++++------------------------- block/qed.h | 54 +-- block/raw-format.c | 8 +- block/rbd.c | 4 +- block/sheepdog.c | 12 +- block/ssh.c | 2 +- block/throttle-groups.c | 2 +- block/trace-events | 3 - blockjob.c | 4 +- hw/block/nvme.c | 83 +++-- hw/block/nvme.h | 1 + hw/virtio/virtio-pci.c | 2 +- include/block/block.h | 16 +- include/block/block_int.h | 6 +- include/block/blockjob.h | 18 + include/sysemu/block-backend.h | 20 +- migration/savevm.c | 32 +- qemu-img.c | 29 +- qemu-io-cmds.c | 46 +-- qemu-options.hx | 221 ++++++++++-- tests/qemu-iotests/068 | 37 +- tests/qemu-iotests/068.out | 11 +- tests/qemu-iotests/185 | 206 +++++++++++ tests/qemu-iotests/185.out | 59 ++++ tests/qemu-iotests/common.qemu | 3 + tests/qemu-iotests/group | 1 + 46 files changed, 1477 insertions(+), 1325 deletions(-) delete mode 100644 block/qed-gencb.c create mode 100755 tests/qemu-iotests/185 create mode 100644 tests/qemu-iotests/185.out commit_complete() can't assume that after its block_job_completed() the job is actually immediately freed; someone else may still be holding references. In this case, the op blockers on the intermediate nodes make the graph reconfiguration in the completion code fail. Call block_job_remove_all_bdrv() manually so that we know for sure that any blockers on intermediate nodes are given up. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- block/commit.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/block/commit.c b/block/commit.c index XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ static void commit_complete(BlockJob *job, void *opaque) } g_free(s->backing_file_str); blk_unref(s->top); + + /* If there is more than one reference to the job (e.g. if called from + * block_job_finish_sync()), block_job_completed() won't free it and + * therefore the blockers on the intermediate nodes remain. This would + * cause bdrv_set_backing_hd() to fail. */ + block_job_remove_all_bdrv(job); + block_job_completed(&s->common, ret); g_free(data); -- 1.8.3.1 After _cleanup_qemu(), test cases should be able to start the next qemu process and call _cleanup_qemu() for that one as well. For this to work cleanly, we need to improve the cleanup so that the second invocation doesn't try to kill the qemu instances from the first invocation a second time (which would result in error messages). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/common.qemu | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/common.qemu +++ b/tests/qemu-iotests/common.qemu @@ -XXX,XX +XXX,XX @@ function _cleanup_qemu() rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}" eval "exec ${QEMU_IN[$i]}<&-" # close file descriptors eval "exec ${QEMU_OUT[$i]}<&-" + + unset QEMU_IN[$i] + unset QEMU_OUT[$i] done } -- 1.8.3.1 When qemu is exited, all running jobs should be cancelled successfully. This adds a test for this for all types of block jobs that currently exist in qemu. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- tests/qemu-iotests/185 | 206 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/185.out | 59 +++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 266 insertions(+) create mode 100755 tests/qemu-iotests/185 create mode 100644 tests/qemu-iotests/185.out diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/185 @@ -XXX,XX +XXX,XX @@ +#!/bin/bash +# +# Test exiting qemu while jobs are still running +# +# 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 <http://www.gnu.org/licenses/>. +# + +# creator +owner=kwolf@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 "${TEST_IMG}.mid" + rm -f "${TEST_IMG}.copy" + _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 qcow2 +_supported_proto file +_supported_os Linux + +size=64M +TEST_IMG="${TEST_IMG}.base" _make_test_img $size + +echo +echo === Starting VM === +echo + +qemu_comm_method="qmp" + +_launch_qemu \ + -drive file="${TEST_IMG}.base",cache=$CACHEMODE,driver=$IMGFMT,id=disk +h=$QEMU_HANDLE +_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return' + +echo +echo === Creating backing chain === +echo + +_send_qemu_cmd $h \ + "{ 'execute': 'blockdev-snapshot-sync', + 'arguments': { 'device': 'disk', + 'snapshot-file': '$TEST_IMG.mid', + 'format': '$IMGFMT', + 'mode': 'absolute-paths' } }" \ + "return" + +_send_qemu_cmd $h \ + "{ 'execute': 'human-monitor-command', + 'arguments': { 'command-line': + 'qemu-io disk \"write 0 4M\"' } }" \ + "return" + +_send_qemu_cmd $h \ + "{ 'execute': 'blockdev-snapshot-sync', + 'arguments': { 'device': 'disk', + 'snapshot-file': '$TEST_IMG', + 'format': '$IMGFMT', + 'mode': 'absolute-paths' } }" \ + "return" + +echo +echo === Start commit job and exit qemu === +echo + +# Note that the reference output intentionally includes the 'offset' field in +# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are +# predictable and any change in the offsets would hint at a bug in the job +# throttling code. +# +# In order to achieve these predictable offsets, all of the following tests +# use speed=65536. Each job will perform exactly one iteration before it has +# to sleep at least for a second, which is plenty of time for the 'quit' QMP +# command to be received (after receiving the command, the rest runs +# synchronously, so jobs can arbitrarily continue or complete). +# +# The buffer size for commit and streaming is 512k (waiting for 8 seconds after +# the first request), for active commit and mirror it's large enough to cover +# the full 4M, and for backup it's the qcow2 cluster size, which we know is +# 64k. As all of these are at least as large as the speed, we are sure that the +# offset doesn't advance after the first iteration before qemu exits. + +_send_qemu_cmd $h \ + "{ 'execute': 'block-commit', + 'arguments': { 'device': 'disk', + 'base':'$TEST_IMG.base', + 'top': '$TEST_IMG.mid', + 'speed': 65536 } }" \ + "return" + +_send_qemu_cmd $h "{ 'execute': 'quit' }" "return" +wait=1 _cleanup_qemu + +echo +echo === Start active commit job and exit qemu === +echo + +_launch_qemu \ + -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk +h=$QEMU_HANDLE +_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return' + +_send_qemu_cmd $h \ + "{ 'execute': 'block-commit', + 'arguments': { 'device': 'disk', + 'base':'$TEST_IMG.base', + 'speed': 65536 } }" \ + "return" + +_send_qemu_cmd $h "{ 'execute': 'quit' }" "return" +wait=1 _cleanup_qemu + +echo +echo === Start mirror job and exit qemu === +echo + +_launch_qemu \ + -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk +h=$QEMU_HANDLE +_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return' + +_send_qemu_cmd $h \ + "{ 'execute': 'drive-mirror', + 'arguments': { 'device': 'disk', + 'target': '$TEST_IMG.copy', + 'format': '$IMGFMT', + 'sync': 'full', + 'speed': 65536 } }" \ + "return" + +_send_qemu_cmd $h "{ 'execute': 'quit' }" "return" +wait=1 _cleanup_qemu + +echo +echo === Start backup job and exit qemu === +echo + +_launch_qemu \ + -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk +h=$QEMU_HANDLE +_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return' + +_send_qemu_cmd $h \ + "{ 'execute': 'drive-backup', + 'arguments': { 'device': 'disk', + 'target': '$TEST_IMG.copy', + 'format': '$IMGFMT', + 'sync': 'full', + 'speed': 65536 } }" \ + "return" + +_send_qemu_cmd $h "{ 'execute': 'quit' }" "return" +wait=1 _cleanup_qemu + +echo +echo === Start streaming job and exit qemu === +echo + +_launch_qemu \ + -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk +h=$QEMU_HANDLE +_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return' + +_send_qemu_cmd $h \ + "{ 'execute': 'block-stream', + 'arguments': { 'device': 'disk', + 'speed': 65536 } }" \ + "return" + +_send_qemu_cmd $h "{ 'execute': 'quit' }" "return" +wait=1 _cleanup_qemu + +_check_test_img + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/185.out @@ -XXX,XX +XXX,XX @@ +QA output created by 185 +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 + +=== Starting VM === + +{"return": {}} + +=== Creating backing chain === + +Formatting 'TEST_DIR/t.qcow2.mid', fmt=qcow2 size=67108864 backing_file=TEST_DIR/t.qcow2.base backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 +{"return": {}} +wrote 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +{"return": ""} +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 backing_file=TEST_DIR/t.qcow2.mid backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 +{"return": {}} + +=== Start commit job and exit qemu === + +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "commit"}} + +=== Start active commit job and exit qemu === + +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "commit"}} + +=== Start mirror job and exit qemu === + +{"return": {}} +Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}} + +=== Start backup job and exit qemu === + +{"return": {}} +Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 65536, "speed": 65536, "type": "backup"}} + +=== Start streaming job and exit qemu === + +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "stream"}} +No errors were found on the image. +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -XXX,XX +XXX,XX @@ 181 rw auto migration 182 rw auto quick 183 rw auto migration +185 rw auto -- 1.8.3.1 From: Stefan Hajnoczi <stefanha@redhat.com> Calling aio_poll() directly may have been fine previously, but this is the future, man! The difference between an aio_poll() loop and BDRV_POLL_WHILE() is that BDRV_POLL_WHILE() releases the AioContext around aio_poll(). This allows the IOThread to run fd handlers or BHs to complete the request. Failure to release the AioContext causes deadlocks. Using BDRV_POLL_WHILE() partially fixes a 'savevm' hang with -object iothread. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/io.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/block/io.c b/block/io.c index XXXXXXX..XXXXXXX 100644 --- a/block/io.c +++ b/block/io.c @@ -XXX,XX +XXX,XX @@ bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos, Coroutine *co = qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data); bdrv_coroutine_enter(bs, co); - while (data.ret == -EINPROGRESS) { - aio_poll(bdrv_get_aio_context(bs), true); - } + BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS); return data.ret; } } -- 1.8.3.1 From: Stefan Hajnoczi <stefanha@redhat.com> AioContext was designed to allow nested acquire/release calls. It uses a recursive mutex so callers don't need to worry about nesting...or so we thought. BDRV_POLL_WHILE() is used to wait for block I/O requests. It releases the AioContext temporarily around aio_poll(). This gives IOThreads a chance to acquire the AioContext to process I/O completions. It turns out that recursive locking and BDRV_POLL_WHILE() don't mix. BDRV_POLL_WHILE() only releases the AioContext once, so the IOThread will not be able to acquire the AioContext if it was acquired multiple times. Instead of trying to release AioContext n times in BDRV_POLL_WHILE(), this patch simply avoids nested locking in save_vmstate(). It's the simplest fix and we should step back to consider the big picture with all the recent changes to block layer threading. This patch is the final fix to solve 'savevm' hanging with -object iothread. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- migration/savevm.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index XXXXXXX..XXXXXXX 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -XXX,XX +XXX,XX @@ int save_snapshot(const char *name, Error **errp) goto the_end; } + /* The bdrv_all_create_snapshot() call that follows acquires the AioContext + * for itself. BDRV_POLL_WHILE() does not support nested locking because + * it only releases the lock once. Therefore synchronous I/O will deadlock + * unless we release the AioContext before bdrv_all_create_snapshot(). + */ + aio_context_release(aio_context); + aio_context = NULL; + ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs); if (ret < 0) { error_setg(errp, "Error while creating snapshot on '%s'", @@ -XXX,XX +XXX,XX @@ int save_snapshot(const char *name, Error **errp) ret = 0; the_end: - aio_context_release(aio_context); + if (aio_context) { + aio_context_release(aio_context); + } if (saved_vm_running) { vm_start(); } -- 1.8.3.1 From: Stefan Hajnoczi <stefanha@redhat.com> blk/bdrv_drain_all() only takes effect for a single instant and then resumes block jobs, guest devices, and other external clients like the NBD server. This can be handy when performing a synchronous drain before terminating the program, for example. Monitor commands usually need to quiesce I/O across an entire code region so blk/bdrv_drain_all() is not suitable. They must use bdrv_drain_all_begin/end() to mark the region. This prevents new I/O requests from slipping in or worse - block jobs completing and modifying the graph. I audited other blk/bdrv_drain_all() callers but did not find anything that needs a similar fix. This patch fixes the savevm/loadvm commands. Although I haven't encountered a read world issue this makes the code safer. Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- migration/savevm.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index XXXXXXX..XXXXXXX 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -XXX,XX +XXX,XX @@ int save_snapshot(const char *name, Error **errp) } vm_stop(RUN_STATE_SAVE_VM); + bdrv_drain_all_begin(); + aio_context_acquire(aio_context); memset(sn, 0, sizeof(*sn)); @@ -XXX,XX +XXX,XX @@ int save_snapshot(const char *name, Error **errp) if (aio_context) { aio_context_release(aio_context); } + + bdrv_drain_all_end(); + if (saved_vm_running) { vm_start(); } @@ -XXX,XX +XXX,XX @@ int load_snapshot(const char *name, Error **errp) } /* Flush all IO requests so they don't interfere with the new state. */ - bdrv_drain_all(); + bdrv_drain_all_begin(); ret = bdrv_all_goto_snapshot(name, &bs); if (ret < 0) { error_setg(errp, "Error %d while activating snapshot '%s' on '%s'", ret, name, bdrv_get_device_name(bs)); - return ret; + goto err_drain; } /* restore the VM state */ f = qemu_fopen_bdrv(bs_vm_state, 0); if (!f) { error_setg(errp, "Could not open VM state file"); - return -EINVAL; + ret = -EINVAL; + goto err_drain; } qemu_system_reset(SHUTDOWN_CAUSE_NONE); @@ -XXX,XX +XXX,XX @@ int load_snapshot(const char *name, Error **errp) ret = qemu_loadvm_state(f); aio_context_release(aio_context); + bdrv_drain_all_end(); + migration_incoming_state_destroy(); if (ret < 0) { error_setg(errp, "Error %d while loading VM state", ret); @@ -XXX,XX +XXX,XX @@ int load_snapshot(const char *name, Error **errp) } return 0; + +err_drain: + bdrv_drain_all_end(); + return ret; } void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev) -- 1.8.3.1 This adds documentation for the -blockdev options that apply to all nodes independent of the block driver used. All options that are shared by -blockdev and -drive are now explained in the section for -blockdev. The documentation of -drive mentions that all -blockdev options are accepted as well. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- qemu-options.hx | 108 +++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 79 insertions(+), 29 deletions(-) diff --git a/qemu-options.hx b/qemu-options.hx index XXXXXXX..XXXXXXX 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -XXX,XX +XXX,XX @@ DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev, " [,read-only=on|off][,detect-zeroes=on|off|unmap]\n" " [,driver specific parameters...]\n" " configure a block backend\n", QEMU_ARCH_ALL) +STEXI +@item -blockdev @var{option}[,@var{option}[,@var{option}[,...]]] +@findex -blockdev + +Define a new block driver node. + +@table @option +@item Valid options for any block driver node: + +@table @code +@item driver +Specifies the block driver to use for the given node. +@item node-name +This defines the name of the block driver node by which it will be referenced +later. The name must be unique, i.e. it must not match the name of a different +block driver node, or (if you use @option{-drive} as well) the ID of a drive. + +If no node name is specified, it is automatically generated. The generated node +name is not intended to be predictable and changes between QEMU invocations. +For the top level, an explicit node name must be specified. +@item read-only +Open the node read-only. Guest write attempts will fail. +@item cache.direct +The host page cache can be avoided with @option{cache.direct=on}. This will +attempt to do disk IO directly to the guest's memory. QEMU may still perform an +internal copy of the data. +@item cache.no-flush +In case you don't care about data integrity over host failures, you can use +@option{cache.no-flush=on}. This option tells QEMU that it never needs to write +any data to the disk but can instead keep things in cache. If anything goes +wrong, like your host losing power, the disk storage getting disconnected +accidentally, etc. your image will most probably be rendered unusable. +@item discard=@var{discard} +@var{discard} is one of "ignore" (or "off") or "unmap" (or "on") and controls +whether @code{discard} (also known as @code{trim} or @code{unmap}) requests are +ignored or passed to the filesystem. Some machine types may not support +discard requests. +@item detect-zeroes=@var{detect-zeroes} +@var{detect-zeroes} is "off", "on" or "unmap" and enables the automatic +conversion of plain zero writes by the OS to driver specific optimized +zero write commands. You may even choose "unmap" if @var{discard} is set +to "unmap" to allow a zero write to be converted to an @code{unmap} operation. +@end table + +@end table + +ETEXI DEF("drive", HAS_ARG, QEMU_OPTION_drive, "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n" @@ -XXX,XX +XXX,XX @@ STEXI @item -drive @var{option}[,@var{option}[,@var{option}[,...]]] @findex -drive -Define a new drive. Valid options are: +Define a new drive. This includes creating a block driver node (the backend) as +well as a guest device, and is mostly a shortcut for defining the corresponding +@option{-blockdev} and @option{-device} options. + +@option{-drive} accepts all options that are accepted by @option{-blockdev}. In +addition, it knows the following options: @table @option @item file=@var{file} @@ -XXX,XX +XXX,XX @@ These options have the same definition as they have in @option{-hdachs}. @var{snapshot} is "on" or "off" and controls snapshot mode for the given drive (see @option{-snapshot}). @item cache=@var{cache} -@var{cache} is "none", "writeback", "unsafe", "directsync" or "writethrough" and controls how the host cache is used to access block data. +@var{cache} is "none", "writeback", "unsafe", "directsync" or "writethrough" +and controls how the host cache is used to access block data. This is a +shortcut that sets the @option{cache.direct} and @option{cache.no-flush} +options (as in @option{-blockdev}), and additionally @option{cache.writeback}, +which provides a default for the @option{write-cache} option of block guest +devices (as in @option{-device}). The modes correspond to the following +settings: + +@c Our texi2pod.pl script doesn't support @multitable, so fall back to using +@c plain ASCII art (well, UTF-8 art really). This looks okay both in the manpage +@c and the HTML output. +@example +@ │ cache.writeback cache.direct cache.no-flush +─────────────┼───────────────────────────────────────────────── +writeback │ on off off +none │ on on off +writethrough │ off off off +directsync │ off on off +unsafe │ on off on +@end example + +The default mode is @option{cache=writeback}. + @item aio=@var{aio} @var{aio} is "threads", or "native" and selects between pthread based disk I/O and native Linux AIO. -@item discard=@var{discard} -@var{discard} is one of "ignore" (or "off") or "unmap" (or "on") and controls whether @dfn{discard} (also known as @dfn{trim} or @dfn{unmap}) requests are ignored or passed to the filesystem. Some machine types may not support discard requests. @item format=@var{format} Specify which disk @var{format} will be used rather than detecting the format. Can be used to specify format=raw to avoid interpreting @@ -XXX,XX +XXX,XX @@ Specify which @var{action} to take on write and read errors. Valid actions are: "report" (report the error to the guest), "enospc" (pause QEMU only if the host disk is full; report the error to the guest otherwise). The default setting is @option{werror=enospc} and @option{rerror=report}. -@item readonly -Open drive @option{file} as read-only. Guest write attempts will fail. @item copy-on-read=@var{copy-on-read} @var{copy-on-read} is "on" or "off" and enables whether to copy read backing file sectors into the image file. -@item detect-zeroes=@var{detect-zeroes} -@var{detect-zeroes} is "off", "on" or "unmap" and enables the automatic -conversion of plain zero writes by the OS to driver specific optimized -zero write commands. You may even choose "unmap" if @var{discard} is set -to "unmap" to allow a zero write to be converted to an UNMAP operation. @item bps=@var{b},bps_rd=@var{r},bps_wr=@var{w} Specify bandwidth throttling limits in bytes per second, either for all request types or for reads or writes only. Small values can lead to timeouts or hangs @@ -XXX,XX +XXX,XX @@ prevent guests from circumventing throttling limits by using many small disks instead of a single larger disk. @end table -By default, the @option{cache=writeback} mode is used. It will report data +By default, the @option{cache.writeback=on} mode is used. It will report data writes as completed as soon as the data is present in the host page cache. This is safe as long as your guest OS makes sure to correctly flush disk caches where needed. If your guest OS does not handle volatile disk write caches correctly and your host crashes or loses power, then the guest may experience data corruption. -For such guests, you should consider using @option{cache=writethrough}. This +For such guests, you should consider using @option{cache.writeback=off}. This means that the host page cache will be used to read and write data, but write notification will be sent to the guest only after QEMU has made sure to flush each write to the disk. Be aware that this has a major impact on performance. -The host page cache can be avoided entirely with @option{cache=none}. This will -attempt to do disk IO directly to the guest's memory. QEMU may still perform -an internal copy of the data. Note that this is considered a writeback mode and -the guest OS must handle the disk write cache correctly in order to avoid data -corruption on host crashes. - -The host page cache can be avoided while only sending write notifications to -the guest when the data has been flushed to the disk using -@option{cache=directsync}. - -In case you don't care about data integrity over host failures, use -@option{cache=unsafe}. This option tells QEMU that it never needs to write any -data to the disk but can instead keep things in cache. If anything goes wrong, -like your host losing power, the disk storage getting disconnected accidentally, -etc. your image will most probably be rendered unusable. When using -the @option{-snapshot} option, unsafe caching is always used. +When using the @option{-snapshot} option, unsafe caching is always used. Copy-on-read avoids accessing the same backing file sectors repeatedly and is useful when the backing file is over a slow network. By default copy-on-read -- 1.8.3.1 This documents the driver-specific options for the raw, qcow2 and file block drivers for the man page. For everything else, we refer to the QAPI documentation. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> --- qemu-options.hx | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) diff --git a/qemu-options.hx b/qemu-options.hx index XXXXXXX..XXXXXXX 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -XXX,XX +XXX,XX @@ STEXI @item -blockdev @var{option}[,@var{option}[,@var{option}[,...]]] @findex -blockdev -Define a new block driver node. +Define a new block driver node. Some of the options apply to all block drivers, +other options are only accepted for a specific block driver. See below for a +list of generic options and options for the most common block drivers. + +Options that expect a reference to another node (e.g. @code{file}) can be +given in two ways. Either you specify the node name of an already existing node +(file=@var{node-name}), or you define a new node inline, adding options +for the referenced node after a dot (file.filename=@var{path},file.aio=native). + +A block driver node created with @option{-blockdev} can be used for a guest +device by specifying its node name for the @code{drive} property in a +@option{-device} argument that defines a block device. @table @option @item Valid options for any block driver node: @@ -XXX,XX +XXX,XX @@ zero write commands. You may even choose "unmap" if @var{discard} is set to "unmap" to allow a zero write to be converted to an @code{unmap} operation. @end table +@item Driver-specific options for @code{file} + +This is the protocol-level block driver for accessing regular files. + +@table @code +@item filename +The path to the image file in the local filesystem +@item aio +Specifies the AIO backend (threads/native, default: threads) +@end table +Example: +@example +-blockdev driver=file,node-name=disk,filename=disk.img +@end example + +@item Driver-specific options for @code{raw} + +This is the image format block driver for raw images. It is usually +stacked on top of a protocol level block driver such as @code{file}. + +@table @code +@item file +Reference to or definition of the data source block driver node +(e.g. a @code{file} driver node) +@end table +Example 1: +@example +-blockdev driver=file,node-name=disk_file,filename=disk.img +-blockdev driver=raw,node-name=disk,file=disk_file +@end example +Example 2: +@example +-blockdev driver=raw,node-name=disk,file.driver=file,file.filename=disk.img +@end example + +@item Driver-specific options for @code{qcow2} + +This is the image format block driver for qcow2 images. It is usually +stacked on top of a protocol level block driver such as @code{file}. + +@table @code +@item file +Reference to or definition of the data source block driver node +(e.g. a @code{file} driver node) + +@item backing +Reference to or definition of the backing file block device (default is taken +from the image file). It is allowed to pass an empty string here in order to +disable the default backing file. + +@item lazy-refcounts +Whether to enable the lazy refcounts feature (on/off; default is taken from the +image file) + +@item cache-size +The maximum total size of the L2 table and refcount block caches in bytes +(default: 1048576 bytes or 8 clusters, whichever is larger) + +@item l2-cache-size +The maximum size of the L2 table cache in bytes +(default: 4/5 of the total cache size) + +@item refcount-cache-size +The maximum size of the refcount block cache in bytes +(default: 1/5 of the total cache size) + +@item cache-clean-interval +Clean unused entries in the L2 and refcount caches. The interval is in seconds. +The default value is 0 and it disables this feature. + +@item pass-discard-request +Whether discard requests to the qcow2 device should be forwarded to the data +source (on/off; default: on if discard=unmap is specified, off otherwise) + +@item pass-discard-snapshot +Whether discard requests for the data source should be issued when a snapshot +operation (e.g. deleting a snapshot) frees clusters in the qcow2 file (on/off; +default: on) + +@item pass-discard-other +Whether discard requests for the data source should be issued on other +occasions where a cluster gets freed (on/off; default: off) + +@item overlap-check +Which overlap checks to perform for writes to the image +(none/constant/cached/all; default: cached). For details or finer +granularity control refer to the QAPI documentation of @code{blockdev-add}. +@end table + +Example 1: +@example +-blockdev driver=file,node-name=my_file,filename=/tmp/disk.qcow2 +-blockdev driver=qcow2,node-name=hda,file=my_file,overlap-check=none,cache-size=16777216 +@end example +Example 2: +@example +-blockdev driver=qcow2,node-name=disk,file.driver=http,file.filename=http://example.com/image.qcow2 +@end example + +@item Driver-specific options for other drivers +Please refer to the QAPI documentation of the @code{blockdev-add} QMP command. + @end table ETEXI -- 1.8.3.1 From: Alberto Garcia <berto@igalia.com> There used to be throttle_timers_{detach,attach}_aio_context() calls in bdrv_set_aio_context(), but since 7ca7f0f6db1fedd28d490795d778cf239 they are now in blk_set_aio_context(). Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/throttle-groups.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/throttle-groups.c b/block/throttle-groups.c index XXXXXXX..XXXXXXX 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -XXX,XX +XXX,XX @@ * Again, all this is handled internally and is mostly transparent to * the outside. The 'throttle_timers' field however has an additional * constraint because it may be temporarily invalid (see for example - * bdrv_set_aio_context()). Therefore in this file a thread will + * blk_set_aio_context()). Therefore in this file a thread will * access some other BlockBackend's timers only after verifying that * that BlockBackend has throttled requests in the queue. */ -- 1.8.3.1 From: Stefan Hajnoczi <stefanha@redhat.com> Old kvm.ko versions only supported a tiny number of ioeventfds so virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0. Do not check kvm_has_many_ioeventfds() when KVM is disabled since it always returns 0. Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f ("memory: emulate ioeventfd") it has been possible to use ioeventfds in qtest or TCG mode. This patch makes -device virtio-blk-pci,iothread=iothread0 work even when KVM is disabled. I have tested that virtio-blk-pci works under TCG both with and without iothread. Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/virtio/virtio-pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -XXX,XX +XXX,XX @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) bool pcie_port = pci_bus_is_express(pci_dev->bus) && !pci_bus_is_root(pci_dev->bus); - if (!kvm_has_many_ioeventfds()) { + if (kvm_enabled() && !kvm_has_many_ioeventfds()) { proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; } -- 1.8.3.1 From: Stefan Hajnoczi <stefanha@redhat.com> migration_incoming_state_destroy() uses qemu_fclose() on the vmstate file. Make sure to call it inside an AioContext acquire/release region. This fixes an 'qemu: qemu_mutex_unlock: Operation not permitted' abort in loadvm. This patch closes the vmstate file before ending the drained region. Previously we closed the vmstate file after ending the drained region. The order does not matter. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- migration/savevm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index XXXXXXX..XXXXXXX 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -XXX,XX +XXX,XX @@ int load_snapshot(const char *name, Error **errp) aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); + migration_incoming_state_destroy(); aio_context_release(aio_context); bdrv_drain_all_end(); - migration_incoming_state_destroy(); if (ret < 0) { error_setg(errp, "Error %d while loading VM state", ret); return ret; -- 1.8.3.1 From: Stefan Hajnoczi <stefanha@redhat.com> Avoid duplicating the QEMU command-line. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/068 | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/068 +++ b/tests/qemu-iotests/068 @@ -XXX,XX +XXX,XX @@ case "$QEMU_DEFAULT_MACHINE" in ;; esac -# Give qemu some time to boot before saving the VM state -bash -c 'sleep 1; echo -e "savevm 0\nquit"' |\ - $QEMU $platform_parm -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\ +_qemu() +{ + $QEMU $platform_parm -nographic -monitor stdio -serial none -hda "$TEST_IMG" \ + "$@" |\ _filter_qemu | _filter_hmp +} + +# Give qemu some time to boot before saving the VM state +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu # Now try to continue from that VM state (this should just work) -echo quit |\ - $QEMU $platform_parm -nographic -monitor stdio -serial none -hda "$TEST_IMG" -loadvm 0 |\ - _filter_qemu | _filter_hmp +echo quit | _qemu -loadvm 0 # success, all done echo "*** done" -- 1.8.3.1 From: Stefan Hajnoczi <stefanha@redhat.com> Perform the savevm/loadvm test with both iothread on and off. This covers the recently found savevm/loadvm hang when iothread is enabled. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/068 | 23 ++++++++++++++--------- tests/qemu-iotests/068.out | 11 ++++++++++- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/068 +++ b/tests/qemu-iotests/068 @@ -XXX,XX +XXX,XX @@ _supported_os Linux IMGOPTS="compat=1.1" IMG_SIZE=128K -echo -echo "=== Saving and reloading a VM state to/from a qcow2 image ===" -echo -_make_test_img $IMG_SIZE - case "$QEMU_DEFAULT_MACHINE" in s390-ccw-virtio) platform_parm="-no-shutdown" @@ -XXX,XX +XXX,XX @@ _qemu() _filter_qemu | _filter_hmp } -# Give qemu some time to boot before saving the VM state -bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu -# Now try to continue from that VM state (this should just work) -echo quit | _qemu -loadvm 0 +for extra_args in \ + "" \ + "-object iothread,id=iothread0 -set device.hba0.iothread=iothread0"; do + echo + echo "=== Saving and reloading a VM state to/from a qcow2 image ($extra_args) ===" + echo + + _make_test_img $IMG_SIZE + + # Give qemu some time to boot before saving the VM state + bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu $extra_args + # Now try to continue from that VM state (this should just work) + echo quit | _qemu $extra_args -loadvm 0 +done # success, all done echo "*** done" diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/068.out +++ b/tests/qemu-iotests/068.out @@ -XXX,XX +XXX,XX @@ QA output created by 068 -=== Saving and reloading a VM state to/from a qcow2 image === +=== Saving and reloading a VM state to/from a qcow2 image () === + +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 +QEMU X.Y.Z monitor - type 'help' for more information +(qemu) savevm 0 +(qemu) quit +QEMU X.Y.Z monitor - type 'help' for more information +(qemu) quit + +=== Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 QEMU X.Y.Z monitor - type 'help' for more information -- 1.8.3.1 From: Stephen Bates <sbates@raithlin.com> Add the ability for the NVMe model to support both the RDS and WDS modes in the Controller Memory Buffer. Although not currently supported in the upstreamed Linux kernel a fork with support exists [1] and user-space test programs that build on this also exist [2]. Useful for testing CMB functionality in preperation for real CMB enabled NVMe devices (coming soon). [1] https://github.com/sbates130272/linux-p2pmem [2] https://github.com/sbates130272/p2pmem-test Signed-off-by: Stephen Bates <sbates@raithlin.com> Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Reviewed-by: Keith Busch <keith.busch@intel.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/block/nvme.c | 83 +++++++++++++++++++++++++++++++++++++++------------------ hw/block/nvme.h | 1 + 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -XXX,XX +XXX,XX @@ * cmb_size_mb=<cmb_size_mb[optional]> * * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at - * offset 0 in BAR2 and supports SQS only for now. + * offset 0 in BAR2 and supports only WDS, RDS and SQS for now. */ #include "qemu/osdep.h" @@ -XXX,XX +XXX,XX @@ static void nvme_isr_notify(NvmeCtrl *n, NvmeCQueue *cq) } } -static uint16_t nvme_map_prp(QEMUSGList *qsg, uint64_t prp1, uint64_t prp2, - uint32_t len, NvmeCtrl *n) +static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1, + uint64_t prp2, uint32_t len, NvmeCtrl *n) { hwaddr trans_len = n->page_size - (prp1 % n->page_size); trans_len = MIN(len, trans_len); @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, uint64_t prp1, uint64_t prp2, if (!prp1) { return NVME_INVALID_FIELD | NVME_DNR; + } else if (n->cmbsz && prp1 >= n->ctrl_mem.addr && + prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) { + qsg->nsg = 0; + qemu_iovec_init(iov, num_prps); + qemu_iovec_add(iov, (void *)&n->cmbuf[prp1 - n->ctrl_mem.addr], trans_len); + } else { + pci_dma_sglist_init(qsg, &n->parent_obj, num_prps); + qemu_sglist_add(qsg, prp1, trans_len); } - - pci_dma_sglist_init(qsg, &n->parent_obj, num_prps); - qemu_sglist_add(qsg, prp1, trans_len); len -= trans_len; if (len) { if (!prp2) { @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, uint64_t prp1, uint64_t prp2, nents = (len + n->page_size - 1) >> n->page_bits; prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t); - pci_dma_read(&n->parent_obj, prp2, (void *)prp_list, prp_trans); + nvme_addr_read(n, prp2, (void *)prp_list, prp_trans); while (len != 0) { uint64_t prp_ent = le64_to_cpu(prp_list[i]); @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, uint64_t prp1, uint64_t prp2, i = 0; nents = (len + n->page_size - 1) >> n->page_bits; prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t); - pci_dma_read(&n->parent_obj, prp_ent, (void *)prp_list, + nvme_addr_read(n, prp_ent, (void *)prp_list, prp_trans); prp_ent = le64_to_cpu(prp_list[i]); } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, uint64_t prp1, uint64_t prp2, } trans_len = MIN(len, n->page_size); - qemu_sglist_add(qsg, prp_ent, trans_len); + if (qsg->nsg){ + qemu_sglist_add(qsg, prp_ent, trans_len); + } else { + qemu_iovec_add(iov, (void *)&n->cmbuf[prp_ent - n->ctrl_mem.addr], trans_len); + } len -= trans_len; i++; } @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, uint64_t prp1, uint64_t prp2, if (prp2 & (n->page_size - 1)) { goto unmap; } - qemu_sglist_add(qsg, prp2, len); + if (qsg->nsg) { + qemu_sglist_add(qsg, prp2, len); + } else { + qemu_iovec_add(iov, (void *)&n->cmbuf[prp2 - n->ctrl_mem.addr], trans_len); + } } } return NVME_SUCCESS; @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len, uint64_t prp1, uint64_t prp2) { QEMUSGList qsg; + QEMUIOVector iov; + uint16_t status = NVME_SUCCESS; - if (nvme_map_prp(&qsg, prp1, prp2, len, n)) { + if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) { return NVME_INVALID_FIELD | NVME_DNR; } - if (dma_buf_read(ptr, len, &qsg)) { + if (qsg.nsg > 0) { + if (dma_buf_read(ptr, len, &qsg)) { + status = NVME_INVALID_FIELD | NVME_DNR; + } qemu_sglist_destroy(&qsg); - return NVME_INVALID_FIELD | NVME_DNR; + } else { + if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) { + status = NVME_INVALID_FIELD | NVME_DNR; + } + qemu_iovec_destroy(&iov); } - qemu_sglist_destroy(&qsg); - return NVME_SUCCESS; + return status; } static void nvme_post_cqes(void *opaque) @@ -XXX,XX +XXX,XX @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, return NVME_LBA_RANGE | NVME_DNR; } - if (nvme_map_prp(&req->qsg, prp1, prp2, data_size, n)) { + if (nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, data_size, n)) { block_acct_invalid(blk_get_stats(n->conf.blk), acct); return NVME_INVALID_FIELD | NVME_DNR; } - assert((nlb << data_shift) == req->qsg.size); - - req->has_sg = true; dma_acct_start(n->conf.blk, &req->acct, &req->qsg, acct); - req->aiocb = is_write ? - dma_blk_write(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE, - nvme_rw_cb, req) : - dma_blk_read(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE, - nvme_rw_cb, req); + if (req->qsg.nsg > 0) { + req->has_sg = true; + req->aiocb = is_write ? + dma_blk_write(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE, + nvme_rw_cb, req) : + dma_blk_read(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE, + nvme_rw_cb, req); + } else { + req->has_sg = false; + req->aiocb = is_write ? + blk_aio_pwritev(n->conf.blk, data_offset, &req->iov, 0, nvme_rw_cb, + req) : + blk_aio_preadv(n->conf.blk, data_offset, &req->iov, 0, nvme_rw_cb, + req); + } return NVME_NO_COMPLETE; } @@ -XXX,XX +XXX,XX @@ static int nvme_init(PCIDevice *pci_dev) NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1); NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0); NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0); - NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 0); - NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 0); + NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1); + NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1); NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2); /* MBs */ NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->cmb_size_mb); + n->cmbloc = n->bar.cmbloc; + n->cmbsz = n->bar.cmbsz; + n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); memory_region_init_io(&n->ctrl_mem, OBJECT(n), &nvme_cmb_ops, n, "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); diff --git a/hw/block/nvme.h b/hw/block/nvme.h index XXXXXXX..XXXXXXX 100644 --- a/hw/block/nvme.h +++ b/hw/block/nvme.h @@ -XXX,XX +XXX,XX @@ typedef struct NvmeRequest { NvmeCqe cqe; BlockAcctCookie acct; QEMUSGList qsg; + QEMUIOVector iov; QTAILQ_ENTRY(NvmeRequest)entry; } NvmeRequest; -- 1.8.3.1 From: Alberto Garcia <berto@igalia.com> Qcow2COWRegion has two attributes: - The offset of the COW region from the start of the first cluster touched by the I/O request. Since it's always going to be positive and the maximum request size is at most INT_MAX, we can use a regular unsigned int to store this offset. - The size of the COW region in bytes. This is guaranteed to be >= 0, so we should use an unsigned type instead. In x86_64 this reduces the size of Qcow2COWRegion from 16 to 8 bytes. It will also help keep some assertions simpler now that we know that there are no negative numbers. The prototype of do_perform_cow() is also updated to reflect these changes. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 4 ++-- block/qcow2.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, static int coroutine_fn do_perform_cow(BlockDriverState *bs, uint64_t src_cluster_offset, uint64_t cluster_offset, - int offset_in_cluster, - int bytes) + unsigned offset_in_cluster, + unsigned bytes) { BDRVQcow2State *s = bs->opaque; QEMUIOVector qiov; diff --git a/block/qcow2.h b/block/qcow2.h index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -XXX,XX +XXX,XX @@ typedef struct Qcow2COWRegion { * Offset of the COW region in bytes from the start of the first cluster * touched by the request. */ - uint64_t offset; + unsigned offset; /** Number of bytes to copy */ - int nb_bytes; + unsigned nb_bytes; } Qcow2COWRegion; /** -- 1.8.3.1 From: Alberto Garcia <berto@igalia.com> Instead of calling perform_cow() twice with a different COW region each time, call it just once and make perform_cow() handle both regions. This patch simply moves code around. The next one will do the actual reordering of the COW operations. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs, struct iovec iov; int ret; + if (bytes == 0) { + return 0; + } + iov.iov_len = bytes; iov.iov_base = qemu_try_blockalign(bs, iov.iov_len); if (iov.iov_base == NULL) { @@ -XXX,XX +XXX,XX @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, return cluster_offset; } -static int perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r) +static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) { BDRVQcow2State *s = bs->opaque; + Qcow2COWRegion *start = &m->cow_start; + Qcow2COWRegion *end = &m->cow_end; int ret; - if (r->nb_bytes == 0) { + if (start->nb_bytes == 0 && end->nb_bytes == 0) { return 0; } qemu_co_mutex_unlock(&s->lock); - ret = do_perform_cow(bs, m->offset, m->alloc_offset, r->offset, r->nb_bytes); - qemu_co_mutex_lock(&s->lock); - + ret = do_perform_cow(bs, m->offset, m->alloc_offset, + start->offset, start->nb_bytes); if (ret < 0) { - return ret; + goto fail; } + ret = do_perform_cow(bs, m->offset, m->alloc_offset, + end->offset, end->nb_bytes); + +fail: + qemu_co_mutex_lock(&s->lock); + /* * Before we update the L2 table to actually point to the new cluster, we * need to be sure that the refcounts have been increased and COW was * handled. */ - qcow2_cache_depends_on_flush(s->l2_table_cache); + if (ret == 0) { + qcow2_cache_depends_on_flush(s->l2_table_cache); + } - return 0; + return ret; } int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) @@ -XXX,XX +XXX,XX @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) } /* copy content of unmodified sectors */ - ret = perform_cow(bs, m, &m->cow_start); - if (ret < 0) { - goto err; - } - - ret = perform_cow(bs, m, &m->cow_end); + ret = perform_cow(bs, m); if (ret < 0) { goto err; } -- 1.8.3.1 From: Alberto Garcia <berto@igalia.com> This patch splits do_perform_cow() into three separate functions to read, encrypt and write the COW regions. perform_cow() can now read both regions first, then encrypt them and finally write them to disk. The memory allocation is also done in this function now, using one single buffer large enough to hold both regions. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 117 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 87 insertions(+), 30 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, return 0; } -static int coroutine_fn do_perform_cow(BlockDriverState *bs, - uint64_t src_cluster_offset, - uint64_t cluster_offset, - unsigned offset_in_cluster, - unsigned bytes) +static int coroutine_fn do_perform_cow_read(BlockDriverState *bs, + uint64_t src_cluster_offset, + unsigned offset_in_cluster, + uint8_t *buffer, + unsigned bytes) { - BDRVQcow2State *s = bs->opaque; QEMUIOVector qiov; - struct iovec iov; + struct iovec iov = { .iov_base = buffer, .iov_len = bytes }; int ret; if (bytes == 0) { return 0; } - iov.iov_len = bytes; - iov.iov_base = qemu_try_blockalign(bs, iov.iov_len); - if (iov.iov_base == NULL) { - return -ENOMEM; - } - qemu_iovec_init_external(&qiov, &iov, 1); BLKDBG_EVENT(bs->file, BLKDBG_COW_READ); if (!bs->drv) { - ret = -ENOMEDIUM; - goto out; + return -ENOMEDIUM; } /* Call .bdrv_co_readv() directly instead of using the public block-layer @@ -XXX,XX +XXX,XX @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs, ret = bs->drv->bdrv_co_preadv(bs, src_cluster_offset + offset_in_cluster, bytes, &qiov, 0); if (ret < 0) { - goto out; + return ret; } - if (bs->encrypted) { + return 0; +} + +static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, + uint64_t src_cluster_offset, + unsigned offset_in_cluster, + uint8_t *buffer, + unsigned bytes) +{ + if (bytes && bs->encrypted) { + BDRVQcow2State *s = bs->opaque; int64_t sector = (src_cluster_offset + offset_in_cluster) >> BDRV_SECTOR_BITS; assert(s->cipher); assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0); assert((bytes & ~BDRV_SECTOR_MASK) == 0); - if (qcow2_encrypt_sectors(s, sector, iov.iov_base, iov.iov_base, + if (qcow2_encrypt_sectors(s, sector, buffer, buffer, bytes >> BDRV_SECTOR_BITS, true, NULL) < 0) { - ret = -EIO; - goto out; + return false; } } + return true; +} + +static int coroutine_fn do_perform_cow_write(BlockDriverState *bs, + uint64_t cluster_offset, + unsigned offset_in_cluster, + uint8_t *buffer, + unsigned bytes) +{ + QEMUIOVector qiov; + struct iovec iov = { .iov_base = buffer, .iov_len = bytes }; + int ret; + + if (bytes == 0) { + return 0; + } + + qemu_iovec_init_external(&qiov, &iov, 1); ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset + offset_in_cluster, bytes); if (ret < 0) { - goto out; + return ret; } BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE); ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster, bytes, &qiov, 0); if (ret < 0) { - goto out; + return ret; } - ret = 0; -out: - qemu_vfree(iov.iov_base); - return ret; + return 0; } @@ -XXX,XX +XXX,XX @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) BDRVQcow2State *s = bs->opaque; Qcow2COWRegion *start = &m->cow_start; Qcow2COWRegion *end = &m->cow_end; + unsigned buffer_size; + uint8_t *start_buffer, *end_buffer; int ret; + assert(start->nb_bytes <= UINT_MAX - end->nb_bytes); + if (start->nb_bytes == 0 && end->nb_bytes == 0) { return 0; } + /* Reserve a buffer large enough to store the data from both the + * start and end COW regions. Add some padding in the middle if + * necessary to make sure that the end region is optimally aligned */ + buffer_size = QEMU_ALIGN_UP(start->nb_bytes, bdrv_opt_mem_align(bs)) + + end->nb_bytes; + start_buffer = qemu_try_blockalign(bs, buffer_size); + if (start_buffer == NULL) { + return -ENOMEM; + } + /* The part of the buffer where the end region is located */ + end_buffer = start_buffer + buffer_size - end->nb_bytes; + qemu_co_mutex_unlock(&s->lock); - ret = do_perform_cow(bs, m->offset, m->alloc_offset, - start->offset, start->nb_bytes); + /* First we read the existing data from both COW regions */ + ret = do_perform_cow_read(bs, m->offset, start->offset, + start_buffer, start->nb_bytes); if (ret < 0) { goto fail; } - ret = do_perform_cow(bs, m->offset, m->alloc_offset, - end->offset, end->nb_bytes); + ret = do_perform_cow_read(bs, m->offset, end->offset, + end_buffer, end->nb_bytes); + if (ret < 0) { + goto fail; + } + + /* Encrypt the data if necessary before writing it */ + if (bs->encrypted) { + if (!do_perform_cow_encrypt(bs, m->offset, start->offset, + start_buffer, start->nb_bytes) || + !do_perform_cow_encrypt(bs, m->offset, end->offset, + end_buffer, end->nb_bytes)) { + ret = -EIO; + goto fail; + } + } + + /* And now we can write everything */ + ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, + start_buffer, start->nb_bytes); + if (ret < 0) { + goto fail; + } + ret = do_perform_cow_write(bs, m->alloc_offset, end->offset, + end_buffer, end->nb_bytes); fail: qemu_co_mutex_lock(&s->lock); @@ -XXX,XX +XXX,XX @@ fail: qcow2_cache_depends_on_flush(s->l2_table_cache); } + qemu_vfree(start_buffer); return ret; } -- 1.8.3.1 From: Alberto Garcia <berto@igalia.com> Instead of passing a single buffer pointer to do_perform_cow_write(), pass a QEMUIOVector. This will allow us to merge the write requests for the COW regions and the actual data into a single one. Although do_perform_cow_read() does not strictly need to change its API, we're doing it here as well for consistency. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 51 ++++++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, static int coroutine_fn do_perform_cow_read(BlockDriverState *bs, uint64_t src_cluster_offset, unsigned offset_in_cluster, - uint8_t *buffer, - unsigned bytes) + QEMUIOVector *qiov) { - QEMUIOVector qiov; - struct iovec iov = { .iov_base = buffer, .iov_len = bytes }; int ret; - if (bytes == 0) { + if (qiov->size == 0) { return 0; } - qemu_iovec_init_external(&qiov, &iov, 1); - BLKDBG_EVENT(bs->file, BLKDBG_COW_READ); if (!bs->drv) { @@ -XXX,XX +XXX,XX @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs, * which can lead to deadlock when block layer copy-on-read is enabled. */ ret = bs->drv->bdrv_co_preadv(bs, src_cluster_offset + offset_in_cluster, - bytes, &qiov, 0); + qiov->size, qiov, 0); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, static int coroutine_fn do_perform_cow_write(BlockDriverState *bs, uint64_t cluster_offset, unsigned offset_in_cluster, - uint8_t *buffer, - unsigned bytes) + QEMUIOVector *qiov) { - QEMUIOVector qiov; - struct iovec iov = { .iov_base = buffer, .iov_len = bytes }; int ret; - if (bytes == 0) { + if (qiov->size == 0) { return 0; } - qemu_iovec_init_external(&qiov, &iov, 1); - ret = qcow2_pre_write_overlap_check(bs, 0, - cluster_offset + offset_in_cluster, bytes); + cluster_offset + offset_in_cluster, qiov->size); if (ret < 0) { return ret; } BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE); ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster, - bytes, &qiov, 0); + qiov->size, qiov, 0); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) unsigned data_bytes = end->offset - (start->offset + start->nb_bytes); bool merge_reads; uint8_t *start_buffer, *end_buffer; + QEMUIOVector qiov; int ret; assert(start->nb_bytes <= UINT_MAX - end->nb_bytes); @@ -XXX,XX +XXX,XX @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) /* The part of the buffer where the end region is located */ end_buffer = start_buffer + buffer_size - end->nb_bytes; + qemu_iovec_init(&qiov, 1); + qemu_co_mutex_unlock(&s->lock); /* First we read the existing data from both COW regions. We * either read the whole region in one go, or the start and end * regions separately. */ if (merge_reads) { - ret = do_perform_cow_read(bs, m->offset, start->offset, - start_buffer, buffer_size); + qemu_iovec_add(&qiov, start_buffer, buffer_size); + ret = do_perform_cow_read(bs, m->offset, start->offset, &qiov); } else { - ret = do_perform_cow_read(bs, m->offset, start->offset, - start_buffer, start->nb_bytes); + qemu_iovec_add(&qiov, start_buffer, start->nb_bytes); + ret = do_perform_cow_read(bs, m->offset, start->offset, &qiov); if (ret < 0) { goto fail; } - ret = do_perform_cow_read(bs, m->offset, end->offset, - end_buffer, end->nb_bytes); + qemu_iovec_reset(&qiov); + qemu_iovec_add(&qiov, end_buffer, end->nb_bytes); + ret = do_perform_cow_read(bs, m->offset, end->offset, &qiov); } if (ret < 0) { goto fail; @@ -XXX,XX +XXX,XX @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) } /* And now we can write everything */ - ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, - start_buffer, start->nb_bytes); + qemu_iovec_reset(&qiov); + qemu_iovec_add(&qiov, start_buffer, start->nb_bytes); + ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, &qiov); if (ret < 0) { goto fail; } - ret = do_perform_cow_write(bs, m->alloc_offset, end->offset, - end_buffer, end->nb_bytes); + qemu_iovec_reset(&qiov); + qemu_iovec_add(&qiov, end_buffer, end->nb_bytes); + ret = do_perform_cow_write(bs, m->alloc_offset, end->offset, &qiov); fail: qemu_co_mutex_lock(&s->lock); @@ -XXX,XX +XXX,XX @@ fail: } qemu_vfree(start_buffer); + qemu_iovec_destroy(&qiov); return ret; } -- 1.8.3.1 From: Alberto Garcia <berto@igalia.com> If the guest tries to write data that results on the allocation of a new cluster, instead of writing the guest data first and then the data from the COW regions, write everything together using one single I/O operation. This can improve the write performance by 25% or more, depending on several factors such as the media type, the cluster size and the I/O request size. Signed-off-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 40 ++++++++++++++++++++++++-------- block/qcow2.c | 64 +++++++++++++++++++++++++++++++++++++++++++-------- block/qcow2.h | 7 ++++++ 3 files changed, 91 insertions(+), 20 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) assert(start->nb_bytes <= UINT_MAX - end->nb_bytes); assert(start->nb_bytes + end->nb_bytes <= UINT_MAX - data_bytes); assert(start->offset + start->nb_bytes <= end->offset); + assert(!m->data_qiov || m->data_qiov->size == data_bytes); if (start->nb_bytes == 0 && end->nb_bytes == 0) { return 0; @@ -XXX,XX +XXX,XX @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) /* The part of the buffer where the end region is located */ end_buffer = start_buffer + buffer_size - end->nb_bytes; - qemu_iovec_init(&qiov, 1); + qemu_iovec_init(&qiov, 2 + (m->data_qiov ? m->data_qiov->niov : 0)); qemu_co_mutex_unlock(&s->lock); /* First we read the existing data from both COW regions. We @@ -XXX,XX +XXX,XX @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) } } - /* And now we can write everything */ - qemu_iovec_reset(&qiov); - qemu_iovec_add(&qiov, start_buffer, start->nb_bytes); - ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, &qiov); - if (ret < 0) { - goto fail; + /* And now we can write everything. If we have the guest data we + * can write everything in one single operation */ + if (m->data_qiov) { + qemu_iovec_reset(&qiov); + if (start->nb_bytes) { + qemu_iovec_add(&qiov, start_buffer, start->nb_bytes); + } + qemu_iovec_concat(&qiov, m->data_qiov, 0, data_bytes); + if (end->nb_bytes) { + qemu_iovec_add(&qiov, end_buffer, end->nb_bytes); + } + /* NOTE: we have a write_aio blkdebug event here followed by + * a cow_write one in do_perform_cow_write(), but there's only + * one single I/O operation */ + BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); + ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, &qiov); + } else { + /* If there's no guest data then write both COW regions separately */ + qemu_iovec_reset(&qiov); + qemu_iovec_add(&qiov, start_buffer, start->nb_bytes); + ret = do_perform_cow_write(bs, m->alloc_offset, start->offset, &qiov); + if (ret < 0) { + goto fail; + } + + qemu_iovec_reset(&qiov); + qemu_iovec_add(&qiov, end_buffer, end->nb_bytes); + ret = do_perform_cow_write(bs, m->alloc_offset, end->offset, &qiov); } - qemu_iovec_reset(&qiov); - qemu_iovec_add(&qiov, end_buffer, end->nb_bytes); - ret = do_perform_cow_write(bs, m->alloc_offset, end->offset, &qiov); fail: qemu_co_mutex_lock(&s->lock); diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ fail: return ret; } +/* Check if it's possible to merge a write request with the writing of + * the data from the COW regions */ +static bool merge_cow(uint64_t offset, unsigned bytes, + QEMUIOVector *hd_qiov, QCowL2Meta *l2meta) +{ + QCowL2Meta *m; + + for (m = l2meta; m != NULL; m = m->next) { + /* If both COW regions are empty then there's nothing to merge */ + if (m->cow_start.nb_bytes == 0 && m->cow_end.nb_bytes == 0) { + continue; + } + + /* The data (middle) region must be immediately after the + * start region */ + if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) { + continue; + } + + /* The end region must be immediately after the data (middle) + * region */ + if (m->offset + m->cow_end.offset != offset + bytes) { + continue; + } + + /* Make sure that adding both COW regions to the QEMUIOVector + * does not exceed IOV_MAX */ + if (hd_qiov->niov > IOV_MAX - 2) { + continue; + } + + m->data_qiov = hd_qiov; + return true; + } + + return false; +} + static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) @@ -XXX,XX +XXX,XX @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset, goto fail; } - qemu_co_mutex_unlock(&s->lock); - BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); - trace_qcow2_writev_data(qemu_coroutine_self(), - cluster_offset + offset_in_cluster); - ret = bdrv_co_pwritev(bs->file, - cluster_offset + offset_in_cluster, - cur_bytes, &hd_qiov, 0); - qemu_co_mutex_lock(&s->lock); - if (ret < 0) { - goto fail; + /* If we need to do COW, check if it's possible to merge the + * writing of the guest data together with that of the COW regions. + * If it's not possible (or not necessary) then write the + * guest data now. */ + if (!merge_cow(offset, cur_bytes, &hd_qiov, l2meta)) { + qemu_co_mutex_unlock(&s->lock); + BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); + trace_qcow2_writev_data(qemu_coroutine_self(), + cluster_offset + offset_in_cluster); + ret = bdrv_co_pwritev(bs->file, + cluster_offset + offset_in_cluster, + cur_bytes, &hd_qiov, 0); + qemu_co_mutex_lock(&s->lock); + if (ret < 0) { + goto fail; + } } while (l2meta != NULL) { diff --git a/block/qcow2.h b/block/qcow2.h index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -XXX,XX +XXX,XX @@ typedef struct QCowL2Meta */ Qcow2COWRegion cow_end; + /** + * The I/O vector with the data from the actual guest write request. + * If non-NULL, this is meant to be merged together with the data + * from @cow_start and @cow_end into one single write operation. + */ + QEMUIOVector *data_qiov; + /** Pointer to next L2Meta of the same write request */ struct QCowL2Meta *next; -- 1.8.3.1 From: Alberto Garcia <berto@igalia.com> We already have functions for doing these calculations, so let's use them instead of doing everything by hand. This makes the code a bit more readable. Signed-off-by: Alberto Garcia <berto@igalia.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 4 ++-- block/qcow2.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, /* find the cluster offset for the given disk offset */ - l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1); + l2_index = offset_to_l2_index(s, offset); *cluster_offset = be64_to_cpu(l2_table[l2_index]); nb_clusters = size_to_clusters(s, bytes_needed); @@ -XXX,XX +XXX,XX @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset, /* find the cluster offset for the given disk offset */ - l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1); + l2_index = offset_to_l2_index(s, offset); *new_l2_table = l2_table; *new_l2_index = l2_index; diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int validate_table_offset(BlockDriverState *bs, uint64_t offset, } /* Tables must be cluster aligned */ - if (offset & (s->cluster_size - 1)) { + if (offset_into_cluster(s, offset) != 0) { return -EINVAL; } -- 1.8.3.1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed-cluster.c | 94 ++++++++++++++++++----------------------------------- block/qed-table.c | 15 +++------ block/qed.h | 3 +- 3 files changed, 36 insertions(+), 76 deletions(-) diff --git a/block/qed-cluster.c b/block/qed-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed-cluster.c +++ b/block/qed-cluster.c @@ -XXX,XX +XXX,XX @@ static unsigned int qed_count_contiguous_clusters(BDRVQEDState *s, return i - index; } -typedef struct { - BDRVQEDState *s; - uint64_t pos; - size_t len; - - QEDRequest *request; - - /* User callback */ - QEDFindClusterFunc *cb; - void *opaque; -} QEDFindClusterCB; - -static void qed_find_cluster_cb(void *opaque, int ret) -{ - QEDFindClusterCB *find_cluster_cb = opaque; - BDRVQEDState *s = find_cluster_cb->s; - QEDRequest *request = find_cluster_cb->request; - uint64_t offset = 0; - size_t len = 0; - unsigned int index; - unsigned int n; - - qed_acquire(s); - if (ret) { - goto out; - } - - index = qed_l2_index(s, find_cluster_cb->pos); - n = qed_bytes_to_clusters(s, - qed_offset_into_cluster(s, find_cluster_cb->pos) + - find_cluster_cb->len); - n = qed_count_contiguous_clusters(s, request->l2_table->table, - index, n, &offset); - - if (qed_offset_is_unalloc_cluster(offset)) { - ret = QED_CLUSTER_L2; - } else if (qed_offset_is_zero_cluster(offset)) { - ret = QED_CLUSTER_ZERO; - } else if (qed_check_cluster_offset(s, offset)) { - ret = QED_CLUSTER_FOUND; - } else { - ret = -EINVAL; - } - - len = MIN(find_cluster_cb->len, n * s->header.cluster_size - - qed_offset_into_cluster(s, find_cluster_cb->pos)); - -out: - find_cluster_cb->cb(find_cluster_cb->opaque, ret, offset, len); - qed_release(s); - g_free(find_cluster_cb); -} - /** * Find the offset of a data cluster * @@ -XXX,XX +XXX,XX @@ out: void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, size_t len, QEDFindClusterFunc *cb, void *opaque) { - QEDFindClusterCB *find_cluster_cb; uint64_t l2_offset; + uint64_t offset = 0; + unsigned int index; + unsigned int n; + int ret; /* Limit length to L2 boundary. Requests are broken up at the L2 boundary * so that a request acts on one L2 table at a time. @@ -XXX,XX +XXX,XX @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, return; } - find_cluster_cb = g_malloc(sizeof(*find_cluster_cb)); - find_cluster_cb->s = s; - find_cluster_cb->pos = pos; - find_cluster_cb->len = len; - find_cluster_cb->cb = cb; - find_cluster_cb->opaque = opaque; - find_cluster_cb->request = request; + ret = qed_read_l2_table(s, request, l2_offset); + qed_acquire(s); + if (ret) { + goto out; + } + + index = qed_l2_index(s, pos); + n = qed_bytes_to_clusters(s, + qed_offset_into_cluster(s, pos) + len); + n = qed_count_contiguous_clusters(s, request->l2_table->table, + index, n, &offset); + + if (qed_offset_is_unalloc_cluster(offset)) { + ret = QED_CLUSTER_L2; + } else if (qed_offset_is_zero_cluster(offset)) { + ret = QED_CLUSTER_ZERO; + } else if (qed_check_cluster_offset(s, offset)) { + ret = QED_CLUSTER_FOUND; + } else { + ret = -EINVAL; + } + + len = MIN(len, + n * s->header.cluster_size - qed_offset_into_cluster(s, pos)); - qed_read_l2_table(s, request, l2_offset, - qed_find_cluster_cb, find_cluster_cb); +out: + cb(opaque, ret, offset, len); + qed_release(s); } diff --git a/block/qed-table.c b/block/qed-table.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed-table.c +++ b/block/qed-table.c @@ -XXX,XX +XXX,XX @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index, return ret; } -void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset, - BlockCompletionFunc *cb, void *opaque) +int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset) { int ret; @@ -XXX,XX +XXX,XX @@ void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset, /* Check for cached L2 entry */ request->l2_table = qed_find_l2_cache_entry(&s->l2_cache, offset); if (request->l2_table) { - cb(opaque, 0); - return; + return 0; } request->l2_table = qed_alloc_l2_cache_entry(&s->l2_cache); @@ -XXX,XX +XXX,XX @@ void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset, } qed_release(s); - cb(opaque, ret); + return ret; } int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset) { - int ret = -EINPROGRESS; - - qed_read_l2_table(s, request, offset, qed_sync_cb, &ret); - BDRV_POLL_WHILE(s->bs, ret == -EINPROGRESS); - - return ret; + return qed_read_l2_table(s, request, offset); } void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request, diff --git a/block/qed.h b/block/qed.h index XXXXXXX..XXXXXXX 100644 --- a/block/qed.h +++ b/block/qed.h @@ -XXX,XX +XXX,XX @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index, unsigned int n); int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset); -void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset, - BlockCompletionFunc *cb, void *opaque); +int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset); void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request, unsigned int index, unsigned int n, bool flush, BlockCompletionFunc *cb, void *opaque); -- 1.8.3.1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed-cluster.c | 39 ++++++++++++++++++++++----------------- block/qed.c | 24 +++++++++++------------- block/qed.h | 4 ++-- 3 files changed, 35 insertions(+), 32 deletions(-) diff --git a/block/qed-cluster.c b/block/qed-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed-cluster.c +++ b/block/qed-cluster.c @@ -XXX,XX +XXX,XX @@ static unsigned int qed_count_contiguous_clusters(BDRVQEDState *s, * @s: QED state * @request: L2 cache entry * @pos: Byte position in device - * @len: Number of bytes - * @cb: Completion function - * @opaque: User data for completion function + * @len: Number of bytes (may be shortened on return) + * @img_offset: Contains offset in the image file on success * * This function translates a position in the block device to an offset in the - * image file. It invokes the cb completion callback to report back the - * translated offset or unallocated range in the image file. + * image file. The translated offset or unallocated range in the image file is + * reported back in *img_offset and *len. * * If the L2 table exists, request->l2_table points to the L2 table cache entry * and the caller must free the reference when they are finished. The cache * entry is exposed in this way to avoid callers having to read the L2 table * again later during request processing. If request->l2_table is non-NULL it * will be unreferenced before taking on the new cache entry. + * + * On success QED_CLUSTER_FOUND is returned and img_offset/len are a contiguous + * range in the image file. + * + * On failure QED_CLUSTER_L2 or QED_CLUSTER_L1 is returned for missing L2 or L1 + * table offset, respectively. len is number of contiguous unallocated bytes. */ -void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, - size_t len, QEDFindClusterFunc *cb, void *opaque) +int qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, + size_t *len, uint64_t *img_offset) { uint64_t l2_offset; uint64_t offset = 0; @@ -XXX,XX +XXX,XX @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, /* Limit length to L2 boundary. Requests are broken up at the L2 boundary * so that a request acts on one L2 table at a time. */ - len = MIN(len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos); + *len = MIN(*len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos); l2_offset = s->l1_table->offsets[qed_l1_index(s, pos)]; if (qed_offset_is_unalloc_cluster(l2_offset)) { - cb(opaque, QED_CLUSTER_L1, 0, len); - return; + *img_offset = 0; + return QED_CLUSTER_L1; } if (!qed_check_table_offset(s, l2_offset)) { - cb(opaque, -EINVAL, 0, 0); - return; + *img_offset = *len = 0; + return -EINVAL; } ret = qed_read_l2_table(s, request, l2_offset); @@ -XXX,XX +XXX,XX @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, } index = qed_l2_index(s, pos); - n = qed_bytes_to_clusters(s, - qed_offset_into_cluster(s, pos) + len); + n = qed_bytes_to_clusters(s, qed_offset_into_cluster(s, pos) + *len); n = qed_count_contiguous_clusters(s, request->l2_table->table, index, n, &offset); @@ -XXX,XX +XXX,XX @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, ret = -EINVAL; } - len = MIN(len, - n * s->header.cluster_size - qed_offset_into_cluster(s, pos)); + *len = MIN(*len, + n * s->header.cluster_size - qed_offset_into_cluster(s, pos)); out: - cb(opaque, ret, offset, len); + *img_offset = offset; qed_release(s); + return ret; } diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs, .file = file, }; QEDRequest request = { .l2_table = NULL }; + uint64_t offset; + int ret; - qed_find_cluster(s, &request, cb.pos, len, qed_is_allocated_cb, &cb); + ret = qed_find_cluster(s, &request, cb.pos, &len, &offset); + qed_is_allocated_cb(&cb, ret, offset, len); - /* Now sleep if the callback wasn't invoked immediately */ - while (cb.status == BDRV_BLOCK_OFFSET_MASK) { - cb.co = qemu_coroutine_self(); - qemu_coroutine_yield(); - } + /* The callback was invoked immediately */ + assert(cb.status != BDRV_BLOCK_OFFSET_MASK); qed_unref_l2_cache_entry(request.l2_table); @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) * or -errno * @offset: Cluster offset in bytes * @len: Length in bytes - * - * Callback from qed_find_cluster(). */ static void qed_aio_write_data(void *opaque, int ret, uint64_t offset, size_t len) @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_data(void *opaque, int ret, * or -errno * @offset: Cluster offset in bytes * @len: Length in bytes - * - * Callback from qed_find_cluster(). */ static void qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len) @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret) BDRVQEDState *s = acb_to_s(acb); QEDFindClusterFunc *io_fn = (acb->flags & QED_AIOCB_WRITE) ? qed_aio_write_data : qed_aio_read_data; + uint64_t offset; + size_t len; trace_qed_aio_next_io(s, acb, ret, acb->cur_pos + acb->cur_qiov.size); @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret) } /* Find next cluster and start I/O */ - qed_find_cluster(s, &acb->request, - acb->cur_pos, acb->end_pos - acb->cur_pos, - io_fn, acb); + len = acb->end_pos - acb->cur_pos; + ret = qed_find_cluster(s, &acb->request, acb->cur_pos, &len, &offset); + io_fn(acb, ret, offset, len); } static BlockAIOCB *qed_aio_setup(BlockDriverState *bs, diff --git a/block/qed.h b/block/qed.h index XXXXXXX..XXXXXXX 100644 --- a/block/qed.h +++ b/block/qed.h @@ -XXX,XX +XXX,XX @@ int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request, /** * Cluster functions */ -void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, - size_t len, QEDFindClusterFunc *cb, void *opaque); +int qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, + size_t *len, uint64_t *img_offset); /** * Consistency check -- 1.8.3.1 With this change, qed_aio_write_prefill() and qed_aio_write_postfill() collapse into a single function. This is reflected by a rename of the combined function to qed_aio_write_cow(). Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 57 +++++++++++++++++++++++---------------------------------- 1 file changed, 23 insertions(+), 34 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static int qed_read_backing_file(BDRVQEDState *s, uint64_t pos, * @pos: Byte position in device * @len: Number of bytes * @offset: Byte offset in image file - * @cb: Completion function - * @opaque: User data for completion function */ -static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos, - uint64_t len, uint64_t offset, - BlockCompletionFunc *cb, - void *opaque) +static int qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos, + uint64_t len, uint64_t offset) { QEMUIOVector qiov; QEMUIOVector *backing_qiov = NULL; @@ -XXX,XX +XXX,XX @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos, /* Skip copy entirely if there is no work to do */ if (len == 0) { - cb(opaque, 0); - return; + return 0; } iov = (struct iovec) { @@ -XXX,XX +XXX,XX @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos, ret = 0; out: qemu_vfree(iov.iov_base); - cb(opaque, ret); + return ret; } /** @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_main(void *opaque, int ret) } /** - * Populate back untouched region of new data cluster + * Populate untouched regions of new data cluster */ -static void qed_aio_write_postfill(void *opaque, int ret) +static void qed_aio_write_cow(void *opaque, int ret) { QEDAIOCB *acb = opaque; BDRVQEDState *s = acb_to_s(acb); - uint64_t start = acb->cur_pos + acb->cur_qiov.size; - uint64_t len = - qed_start_of_cluster(s, start + s->header.cluster_size - 1) - start; - uint64_t offset = acb->cur_cluster + - qed_offset_into_cluster(s, acb->cur_pos) + - acb->cur_qiov.size; + uint64_t start, len, offset; + + /* Populate front untouched region of new data cluster */ + start = qed_start_of_cluster(s, acb->cur_pos); + len = qed_offset_into_cluster(s, acb->cur_pos); + trace_qed_aio_write_prefill(s, acb, start, len, acb->cur_cluster); + ret = qed_copy_from_backing_file(s, start, len, acb->cur_cluster); if (ret) { qed_aio_complete(acb, ret); return; } - trace_qed_aio_write_postfill(s, acb, start, len, offset); - qed_copy_from_backing_file(s, start, len, offset, - qed_aio_write_main, acb); -} + /* Populate back untouched region of new data cluster */ + start = acb->cur_pos + acb->cur_qiov.size; + len = qed_start_of_cluster(s, start + s->header.cluster_size - 1) - start; + offset = acb->cur_cluster + + qed_offset_into_cluster(s, acb->cur_pos) + + acb->cur_qiov.size; -/** - * Populate front untouched region of new data cluster - */ -static void qed_aio_write_prefill(void *opaque, int ret) -{ - QEDAIOCB *acb = opaque; - BDRVQEDState *s = acb_to_s(acb); - uint64_t start = qed_start_of_cluster(s, acb->cur_pos); - uint64_t len = qed_offset_into_cluster(s, acb->cur_pos); + trace_qed_aio_write_postfill(s, acb, start, len, offset); + ret = qed_copy_from_backing_file(s, start, len, offset); - trace_qed_aio_write_prefill(s, acb, start, len, acb->cur_cluster); - qed_copy_from_backing_file(s, start, len, acb->cur_cluster, - qed_aio_write_postfill, acb); + qed_aio_write_main(acb, ret); } /** @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) cb = qed_aio_write_zero_cluster; } else { - cb = qed_aio_write_prefill; + cb = qed_aio_write_cow; acb->cur_cluster = qed_alloc_clusters(s, acb->cur_nclusters); } -- 1.8.3.1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ int qed_write_header_sync(BDRVQEDState *s) * This function only updates known header fields in-place and does not affect * extra data after the QED header. */ -static void qed_write_header(BDRVQEDState *s, BlockCompletionFunc cb, - void *opaque) +static int qed_write_header(BDRVQEDState *s) { /* We must write full sectors for O_DIRECT but cannot necessarily generate * the data following the header if an unrecognized compat feature is @@ -XXX,XX +XXX,XX @@ static void qed_write_header(BDRVQEDState *s, BlockCompletionFunc cb, ret = 0; out: qemu_vfree(buf); - cb(opaque, ret); + return ret; } static uint64_t qed_max_image_size(uint32_t cluster_size, uint32_t table_size) @@ -XXX,XX +XXX,XX @@ static void qed_unplug_allocating_write_reqs(BDRVQEDState *s) } } -static void qed_finish_clear_need_check(void *opaque, int ret) -{ - /* Do nothing */ -} - -static void qed_flush_after_clear_need_check(void *opaque, int ret) -{ - BDRVQEDState *s = opaque; - - bdrv_aio_flush(s->bs, qed_finish_clear_need_check, s); - - /* No need to wait until flush completes */ - qed_unplug_allocating_write_reqs(s); -} - static void qed_clear_need_check(void *opaque, int ret) { BDRVQEDState *s = opaque; @@ -XXX,XX +XXX,XX @@ static void qed_clear_need_check(void *opaque, int ret) } s->header.features &= ~QED_F_NEED_CHECK; - qed_write_header(s, qed_flush_after_clear_need_check, s); + ret = qed_write_header(s); + (void) ret; + + qed_unplug_allocating_write_reqs(s); + + ret = bdrv_flush(s->bs); + (void) ret; } static void qed_need_check_timer_cb(void *opaque) @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) { BDRVQEDState *s = acb_to_s(acb); BlockCompletionFunc *cb; + int ret; /* Cancel timer when the first allocating request comes in */ if (QSIMPLEQ_EMPTY(&s->allocating_write_reqs)) { @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) if (qed_should_set_need_check(s)) { s->header.features |= QED_F_NEED_CHECK; - qed_write_header(s, cb, acb); + ret = qed_write_header(s); + cb(acb, ret); } else { cb(acb, 0); } -- 1.8.3.1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed-table.c | 47 ++++++++++++----------------------------------- block/qed.c | 12 +++++++----- block/qed.h | 8 +++----- 3 files changed, 22 insertions(+), 45 deletions(-) diff --git a/block/qed-table.c b/block/qed-table.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed-table.c +++ b/block/qed-table.c @@ -XXX,XX +XXX,XX @@ out: * @index: Index of first element * @n: Number of elements * @flush: Whether or not to sync to disk - * @cb: Completion function - * @opaque: Argument for completion function */ -static void qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table, - unsigned int index, unsigned int n, bool flush, - BlockCompletionFunc *cb, void *opaque) +static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table, + unsigned int index, unsigned int n, bool flush) { unsigned int sector_mask = BDRV_SECTOR_SIZE / sizeof(uint64_t) - 1; unsigned int start, end, i; @@ -XXX,XX +XXX,XX @@ static void qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table, ret = 0; out: qemu_vfree(new_table); - cb(opaque, ret); -} - -/** - * Propagate return value from async callback - */ -static void qed_sync_cb(void *opaque, int ret) -{ - *(int *)opaque = ret; + return ret; } int qed_read_l1_table_sync(BDRVQEDState *s) @@ -XXX,XX +XXX,XX @@ int qed_read_l1_table_sync(BDRVQEDState *s) return qed_read_table(s, s->header.l1_table_offset, s->l1_table); } -void qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n, - BlockCompletionFunc *cb, void *opaque) +int qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n) { BLKDBG_EVENT(s->bs->file, BLKDBG_L1_UPDATE); - qed_write_table(s, s->header.l1_table_offset, - s->l1_table, index, n, false, cb, opaque); + return qed_write_table(s, s->header.l1_table_offset, + s->l1_table, index, n, false); } int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index, unsigned int n) { - int ret = -EINPROGRESS; - - qed_write_l1_table(s, index, n, qed_sync_cb, &ret); - BDRV_POLL_WHILE(s->bs, ret == -EINPROGRESS); - - return ret; + return qed_write_l1_table(s, index, n); } int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset) @@ -XXX,XX +XXX,XX @@ int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset return qed_read_l2_table(s, request, offset); } -void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request, - unsigned int index, unsigned int n, bool flush, - BlockCompletionFunc *cb, void *opaque) +int qed_write_l2_table(BDRVQEDState *s, QEDRequest *request, + unsigned int index, unsigned int n, bool flush) { BLKDBG_EVENT(s->bs->file, BLKDBG_L2_UPDATE); - qed_write_table(s, request->l2_table->offset, - request->l2_table->table, index, n, flush, cb, opaque); + return qed_write_table(s, request->l2_table->offset, + request->l2_table->table, index, n, flush); } int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request, unsigned int index, unsigned int n, bool flush) { - int ret = -EINPROGRESS; - - qed_write_l2_table(s, request, index, n, flush, qed_sync_cb, &ret); - BDRV_POLL_WHILE(s->bs, ret == -EINPROGRESS); - - return ret; + return qed_write_l2_table(s, request, index, n, flush); } diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_l1_update(void *opaque, int ret) index = qed_l1_index(s, acb->cur_pos); s->l1_table->offsets[index] = acb->request.l2_table->offset; - qed_write_l1_table(s, index, 1, qed_commit_l2_update, acb); + ret = qed_write_l1_table(s, index, 1); + qed_commit_l2_update(acb, ret); } /** @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset) if (need_alloc) { /* Write out the whole new L2 table */ - qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true, - qed_aio_write_l1_update, acb); + ret = qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true); + qed_aio_write_l1_update(acb, ret); } else { /* Write out only the updated part of the L2 table */ - qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters, false, - qed_aio_next_io_cb, acb); + ret = qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters, + false); + qed_aio_next_io(acb, ret); } return; diff --git a/block/qed.h b/block/qed.h index XXXXXXX..XXXXXXX 100644 --- a/block/qed.h +++ b/block/qed.h @@ -XXX,XX +XXX,XX @@ void qed_commit_l2_cache_entry(L2TableCache *l2_cache, CachedL2Table *l2_table); * Table I/O functions */ int qed_read_l1_table_sync(BDRVQEDState *s); -void qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n, - BlockCompletionFunc *cb, void *opaque); +int qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n); int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index, unsigned int n); int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset); int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset); -void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request, - unsigned int index, unsigned int n, bool flush, - BlockCompletionFunc *cb, void *opaque); +int qed_write_l2_table(BDRVQEDState *s, QEDRequest *request, + unsigned int index, unsigned int n, bool flush); int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request, unsigned int index, unsigned int n, bool flush); -- 1.8.3.1 Note that this code is generally not running in coroutine context, so this is an actual blocking synchronous operation. We'll fix this in a moment. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 61 +++++++++++++++++++------------------------------------------ 1 file changed, 19 insertions(+), 42 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void qed_aio_start_io(QEDAIOCB *acb) qed_aio_next_io(acb, 0); } -static void qed_aio_next_io_cb(void *opaque, int ret) -{ - QEDAIOCB *acb = opaque; - - qed_aio_next_io(acb, ret); -} - static void qed_plug_allocating_write_reqs(BDRVQEDState *s) { assert(!s->allocating_write_reqs_plugged); @@ -XXX,XX +XXX,XX @@ err: qed_aio_complete(acb, ret); } -static void qed_aio_write_l2_update_cb(void *opaque, int ret) -{ - QEDAIOCB *acb = opaque; - qed_aio_write_l2_update(acb, ret, acb->cur_cluster); -} - -/** - * Flush new data clusters before updating the L2 table - * - * This flush is necessary when a backing file is in use. A crash during an - * allocating write could result in empty clusters in the image. If the write - * only touched a subregion of the cluster, then backing image sectors have - * been lost in the untouched region. The solution is to flush after writing a - * new data cluster and before updating the L2 table. - */ -static void qed_aio_write_flush_before_l2_update(void *opaque, int ret) -{ - QEDAIOCB *acb = opaque; - BDRVQEDState *s = acb_to_s(acb); - - if (!bdrv_aio_flush(s->bs->file->bs, qed_aio_write_l2_update_cb, opaque)) { - qed_aio_complete(acb, -EIO); - } -} - /** * Write data to the image file */ @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_main(void *opaque, int ret) BDRVQEDState *s = acb_to_s(acb); uint64_t offset = acb->cur_cluster + qed_offset_into_cluster(s, acb->cur_pos); - BlockCompletionFunc *next_fn; trace_qed_aio_write_main(s, acb, ret, offset, acb->cur_qiov.size); @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_main(void *opaque, int ret) return; } + BLKDBG_EVENT(s->bs->file, BLKDBG_WRITE_AIO); + ret = bdrv_pwritev(s->bs->file, offset, &acb->cur_qiov); + if (ret >= 0) { + ret = 0; + } + if (acb->find_cluster_ret == QED_CLUSTER_FOUND) { - next_fn = qed_aio_next_io_cb; + qed_aio_next_io(acb, ret); } else { if (s->bs->backing) { - next_fn = qed_aio_write_flush_before_l2_update; - } else { - next_fn = qed_aio_write_l2_update_cb; + /* + * Flush new data clusters before updating the L2 table + * + * This flush is necessary when a backing file is in use. A crash + * during an allocating write could result in empty clusters in the + * image. If the write only touched a subregion of the cluster, + * then backing image sectors have been lost in the untouched + * region. The solution is to flush after writing a new data + * cluster and before updating the L2 table. + */ + ret = bdrv_flush(s->bs->file->bs); } + qed_aio_write_l2_update(acb, ret, acb->cur_cluster); } - - BLKDBG_EVENT(s->bs->file, BLKDBG_WRITE_AIO); - bdrv_aio_writev(s->bs->file, offset / BDRV_SECTOR_SIZE, - &acb->cur_qiov, acb->cur_qiov.size / BDRV_SECTOR_SIZE, - next_fn, acb); } /** -- 1.8.3.1 qed_commit_l2_update() is unconditionally called at the end of qed_aio_write_l1_update(). Inline it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void qed_aio_complete(QEDAIOCB *acb, int ret) } /** - * Commit the current L2 table to the cache + * Update L1 table with new L2 table offset and write it out */ -static void qed_commit_l2_update(void *opaque, int ret) +static void qed_aio_write_l1_update(void *opaque, int ret) { QEDAIOCB *acb = opaque; BDRVQEDState *s = acb_to_s(acb); CachedL2Table *l2_table = acb->request.l2_table; uint64_t l2_offset = l2_table->offset; + int index; + + if (ret) { + qed_aio_complete(acb, ret); + return; + } + index = qed_l1_index(s, acb->cur_pos); + s->l1_table->offsets[index] = l2_table->offset; + + ret = qed_write_l1_table(s, index, 1); + + /* Commit the current L2 table to the cache */ qed_commit_l2_cache_entry(&s->l2_cache, l2_table); /* This is guaranteed to succeed because we just committed the entry to the @@ -XXX,XX +XXX,XX @@ static void qed_commit_l2_update(void *opaque, int ret) qed_aio_next_io(acb, ret); } -/** - * Update L1 table with new L2 table offset and write it out - */ -static void qed_aio_write_l1_update(void *opaque, int ret) -{ - QEDAIOCB *acb = opaque; - BDRVQEDState *s = acb_to_s(acb); - int index; - - if (ret) { - qed_aio_complete(acb, ret); - return; - } - - index = qed_l1_index(s, acb->cur_pos); - s->l1_table->offsets[index] = acb->request.l2_table->offset; - - ret = qed_write_l1_table(s, index, 1); - qed_commit_l2_update(acb, ret); -} /** * Update L2 table with new cluster offsets and write them out -- 1.8.3.1 Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but just return an error code and let the caller handle it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void qed_aio_complete(QEDAIOCB *acb, int ret) /** * Update L1 table with new L2 table offset and write it out */ -static void qed_aio_write_l1_update(void *opaque, int ret) +static int qed_aio_write_l1_update(QEDAIOCB *acb) { - QEDAIOCB *acb = opaque; BDRVQEDState *s = acb_to_s(acb); CachedL2Table *l2_table = acb->request.l2_table; uint64_t l2_offset = l2_table->offset; - int index; - - if (ret) { - qed_aio_complete(acb, ret); - return; - } + int index, ret; index = qed_l1_index(s, acb->cur_pos); s->l1_table->offsets[index] = l2_table->offset; @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_l1_update(void *opaque, int ret) acb->request.l2_table = qed_find_l2_cache_entry(&s->l2_cache, l2_offset); assert(acb->request.l2_table != NULL); - qed_aio_next_io(acb, ret); + return ret; } @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset) if (need_alloc) { /* Write out the whole new L2 table */ ret = qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true); - qed_aio_write_l1_update(acb, ret); + if (ret) { + goto err; + } + ret = qed_aio_write_l1_update(acb); + qed_aio_next_io(acb, ret); + } else { /* Write out only the updated part of the L2 table */ ret = qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters, -- 1.8.3.1 Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but just return an error code and let the caller handle it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_l1_update(QEDAIOCB *acb) /** * Update L2 table with new cluster offsets and write them out */ -static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset) +static int qed_aio_write_l2_update(QEDAIOCB *acb, uint64_t offset) { BDRVQEDState *s = acb_to_s(acb); bool need_alloc = acb->find_cluster_ret == QED_CLUSTER_L1; - int index; - - if (ret) { - goto err; - } + int index, ret; if (need_alloc) { qed_unref_l2_cache_entry(acb->request.l2_table); @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset) /* Write out the whole new L2 table */ ret = qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true); if (ret) { - goto err; + return ret; } - ret = qed_aio_write_l1_update(acb); - qed_aio_next_io(acb, ret); - + return qed_aio_write_l1_update(acb); } else { /* Write out only the updated part of the L2 table */ ret = qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters, false); - qed_aio_next_io(acb, ret); + if (ret) { + return ret; + } } - return; - -err: - qed_aio_complete(acb, ret); + return 0; } /** @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_main(void *opaque, int ret) */ ret = bdrv_flush(s->bs->file->bs); } - qed_aio_write_l2_update(acb, ret, acb->cur_cluster); + if (ret) { + goto err; + } + ret = qed_aio_write_l2_update(acb, acb->cur_cluster); + if (ret) { + goto err; + } + qed_aio_next_io(acb, 0); } + return; + +err: + qed_aio_complete(acb, ret); } /** @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_zero_cluster(void *opaque, int ret) return; } - qed_aio_write_l2_update(acb, 0, 1); + ret = qed_aio_write_l2_update(acb, 1); + if (ret < 0) { + qed_aio_complete(acb, ret); + return; + } + qed_aio_next_io(acb, 0); } /** -- 1.8.3.1 Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but just return an error code and let the caller handle it. While refactoring qed_aio_write_alloc() to accomodate the change, qed_aio_write_zero_cluster() ended up with a single line, so I chose to inline that line and remove the function completely. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 58 +++++++++++++++++++++------------------------------------- 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_main(QEDAIOCB *acb) /** * Populate untouched regions of new data cluster */ -static void qed_aio_write_cow(void *opaque, int ret) +static int qed_aio_write_cow(QEDAIOCB *acb) { - QEDAIOCB *acb = opaque; BDRVQEDState *s = acb_to_s(acb); uint64_t start, len, offset; + int ret; /* Populate front untouched region of new data cluster */ start = qed_start_of_cluster(s, acb->cur_pos); @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_cow(void *opaque, int ret) trace_qed_aio_write_prefill(s, acb, start, len, acb->cur_cluster); ret = qed_copy_from_backing_file(s, start, len, acb->cur_cluster); - if (ret) { - qed_aio_complete(acb, ret); - return; + if (ret < 0) { + return ret; } /* Populate back untouched region of new data cluster */ @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_cow(void *opaque, int ret) trace_qed_aio_write_postfill(s, acb, start, len, offset); ret = qed_copy_from_backing_file(s, start, len, offset); - if (ret) { - qed_aio_complete(acb, ret); - return; - } - - ret = qed_aio_write_main(acb); if (ret < 0) { - qed_aio_complete(acb, ret); - return; + return ret; } - qed_aio_next_io(acb, 0); + + return qed_aio_write_main(acb); } /** @@ -XXX,XX +XXX,XX @@ static bool qed_should_set_need_check(BDRVQEDState *s) return !(s->header.features & QED_F_NEED_CHECK); } -static void qed_aio_write_zero_cluster(void *opaque, int ret) -{ - QEDAIOCB *acb = opaque; - - if (ret) { - qed_aio_complete(acb, ret); - return; - } - - ret = qed_aio_write_l2_update(acb, 1); - if (ret < 0) { - qed_aio_complete(acb, ret); - return; - } - qed_aio_next_io(acb, 0); -} - /** * Write new data cluster * @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_zero_cluster(void *opaque, int ret) static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) { BDRVQEDState *s = acb_to_s(acb); - BlockCompletionFunc *cb; int ret; /* Cancel timer when the first allocating request comes in */ @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) qed_aio_start_io(acb); return; } - - cb = qed_aio_write_zero_cluster; } else { - cb = qed_aio_write_cow; acb->cur_cluster = qed_alloc_clusters(s, acb->cur_nclusters); } if (qed_should_set_need_check(s)) { s->header.features |= QED_F_NEED_CHECK; ret = qed_write_header(s); - cb(acb, ret); + if (ret < 0) { + qed_aio_complete(acb, ret); + return; + } + } + + if (acb->flags & QED_AIOCB_ZERO) { + ret = qed_aio_write_l2_update(acb, 1); } else { - cb(acb, 0); + ret = qed_aio_write_cow(acb); } + if (ret < 0) { + qed_aio_complete(acb, ret); + return; + } + qed_aio_next_io(acb, 0); } /** -- 1.8.3.1 Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but just return an error code and let the caller handle it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static bool qed_should_set_need_check(BDRVQEDState *s) * * This path is taken when writing to previously unallocated clusters. */ -static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) +static int qed_aio_write_alloc(QEDAIOCB *acb, size_t len) { BDRVQEDState *s = acb_to_s(acb); int ret; @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) } if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs) || s->allocating_write_reqs_plugged) { - return; /* wait for existing request to finish */ + return -EINPROGRESS; /* wait for existing request to finish */ } acb->cur_nclusters = qed_bytes_to_clusters(s, @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) if (acb->flags & QED_AIOCB_ZERO) { /* Skip ahead if the clusters are already zero */ if (acb->find_cluster_ret == QED_CLUSTER_ZERO) { - qed_aio_start_io(acb); - return; + return 0; } } else { acb->cur_cluster = qed_alloc_clusters(s, acb->cur_nclusters); @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) s->header.features |= QED_F_NEED_CHECK; ret = qed_write_header(s); if (ret < 0) { - qed_aio_complete(acb, ret); - return; + return ret; } } @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) ret = qed_aio_write_cow(acb); } if (ret < 0) { - qed_aio_complete(acb, ret); - return; + return ret; } - qed_aio_next_io(acb, 0); + return 0; } /** @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) * * This path is taken when writing to already allocated clusters. */ -static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) +static int qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) { - int ret; - /* Allocate buffer for zero writes */ if (acb->flags & QED_AIOCB_ZERO) { struct iovec *iov = acb->qiov->iov; @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) if (!iov->iov_base) { iov->iov_base = qemu_try_blockalign(acb->common.bs, iov->iov_len); if (iov->iov_base == NULL) { - qed_aio_complete(acb, -ENOMEM); - return; + return -ENOMEM; } memset(iov->iov_base, 0, iov->iov_len); } @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len); /* Do the actual write */ - ret = qed_aio_write_main(acb); - if (ret < 0) { - qed_aio_complete(acb, ret); - return; - } - qed_aio_next_io(acb, 0); + return qed_aio_write_main(acb); } /** @@ -XXX,XX +XXX,XX @@ static void qed_aio_write_data(void *opaque, int ret, switch (ret) { case QED_CLUSTER_FOUND: - qed_aio_write_inplace(acb, offset, len); + ret = qed_aio_write_inplace(acb, offset, len); break; case QED_CLUSTER_L2: case QED_CLUSTER_L1: case QED_CLUSTER_ZERO: - qed_aio_write_alloc(acb, len); + ret = qed_aio_write_alloc(acb, len); break; default: - qed_aio_complete(acb, ret); + assert(ret < 0); break; } + + if (ret < 0) { + if (ret != -EINPROGRESS) { + qed_aio_complete(acb, ret); + } + return; + } + qed_aio_next_io(acb, 0); } /** -- 1.8.3.1 All callers pass ret = 0, so we can just remove it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static CachedL2Table *qed_new_l2_table(BDRVQEDState *s) return l2_table; } -static void qed_aio_next_io(QEDAIOCB *acb, int ret); +static void qed_aio_next_io(QEDAIOCB *acb); static void qed_aio_start_io(QEDAIOCB *acb) { - qed_aio_next_io(acb, 0); + qed_aio_next_io(acb); } static void qed_plug_allocating_write_reqs(BDRVQEDState *s) @@ -XXX,XX +XXX,XX @@ static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len) /** * Begin next I/O or complete the request */ -static void qed_aio_next_io(QEDAIOCB *acb, int ret) +static void qed_aio_next_io(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); uint64_t offset; size_t len; + int ret; - trace_qed_aio_next_io(s, acb, ret, acb->cur_pos + acb->cur_qiov.size); + trace_qed_aio_next_io(s, acb, 0, acb->cur_pos + acb->cur_qiov.size); if (acb->backing_qiov) { qemu_iovec_destroy(acb->backing_qiov); @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret) acb->backing_qiov = NULL; } - /* Handle I/O error */ - if (ret) { - qed_aio_complete(acb, ret); - return; - } - acb->qiov_offset += acb->cur_qiov.size; acb->cur_pos += acb->cur_qiov.size; qemu_iovec_reset(&acb->cur_qiov); @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret) } return; } - qed_aio_next_io(acb, 0); + qed_aio_next_io(acb); } static BlockAIOCB *qed_aio_setup(BlockDriverState *bs, -- 1.8.3.1 Most of the qed code is now synchronous and matches the coroutine model. One notable exception is the serialisation between requests which can still schedule a callback. Before we can replace this with coroutine locks, let's convert the driver's external interfaces to the coroutine versions. We need to be careful to handle both requests that call the completion callback directly from the calling coroutine (i.e. fully synchronous code) and requests that involve some callback, so that we need to yield and wait for the completion callback coming from outside the coroutine. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Manos Pitsidianakis <el13635@mail.ntua.gr> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 97 ++++++++++++++++++++++++++----------------------------------- 1 file changed, 42 insertions(+), 55 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb) } } -static BlockAIOCB *qed_aio_setup(BlockDriverState *bs, - int64_t sector_num, - QEMUIOVector *qiov, int nb_sectors, - BlockCompletionFunc *cb, - void *opaque, int flags) +typedef struct QEDRequestCo { + Coroutine *co; + bool done; + int ret; +} QEDRequestCo; + +static void qed_co_request_cb(void *opaque, int ret) { - QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, cb, opaque); + QEDRequestCo *co = opaque; - trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors, - opaque, flags); + co->done = true; + co->ret = ret; + qemu_coroutine_enter_if_inactive(co->co); +} + +static int coroutine_fn qed_co_request(BlockDriverState *bs, int64_t sector_num, + QEMUIOVector *qiov, int nb_sectors, + int flags) +{ + QEDRequestCo co = { + .co = qemu_coroutine_self(), + .done = false, + }; + QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, qed_co_request_cb, &co); + + trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors, &co, flags); acb->flags = flags; acb->qiov = qiov; @@ -XXX,XX +XXX,XX @@ static BlockAIOCB *qed_aio_setup(BlockDriverState *bs, /* Start request */ qed_aio_start_io(acb); - return &acb->common; -} -static BlockAIOCB *bdrv_qed_aio_readv(BlockDriverState *bs, - int64_t sector_num, - QEMUIOVector *qiov, int nb_sectors, - BlockCompletionFunc *cb, - void *opaque) -{ - return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0); + if (!co.done) { + qemu_coroutine_yield(); + } + + return co.ret; } -static BlockAIOCB *bdrv_qed_aio_writev(BlockDriverState *bs, - int64_t sector_num, - QEMUIOVector *qiov, int nb_sectors, - BlockCompletionFunc *cb, - void *opaque) +static int coroutine_fn bdrv_qed_co_readv(BlockDriverState *bs, + int64_t sector_num, int nb_sectors, + QEMUIOVector *qiov) { - return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb, - opaque, QED_AIOCB_WRITE); + return qed_co_request(bs, sector_num, qiov, nb_sectors, 0); } -typedef struct { - Coroutine *co; - int ret; - bool done; -} QEDWriteZeroesCB; - -static void coroutine_fn qed_co_pwrite_zeroes_cb(void *opaque, int ret) +static int coroutine_fn bdrv_qed_co_writev(BlockDriverState *bs, + int64_t sector_num, int nb_sectors, + QEMUIOVector *qiov) { - QEDWriteZeroesCB *cb = opaque; - - cb->done = true; - cb->ret = ret; - if (cb->co) { - aio_co_wake(cb->co); - } + return qed_co_request(bs, sector_num, qiov, nb_sectors, QED_AIOCB_WRITE); } static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs, @@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs, int count, BdrvRequestFlags flags) { - BlockAIOCB *blockacb; BDRVQEDState *s = bs->opaque; - QEDWriteZeroesCB cb = { .done = false }; QEMUIOVector qiov; struct iovec iov; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs, iov.iov_len = count; qemu_iovec_init_external(&qiov, &iov, 1); - blockacb = qed_aio_setup(bs, offset >> BDRV_SECTOR_BITS, &qiov, - count >> BDRV_SECTOR_BITS, - qed_co_pwrite_zeroes_cb, &cb, - QED_AIOCB_WRITE | QED_AIOCB_ZERO); - if (!blockacb) { - return -EIO; - } - if (!cb.done) { - cb.co = qemu_coroutine_self(); - qemu_coroutine_yield(); - } - assert(cb.done); - return cb.ret; + return qed_co_request(bs, offset >> BDRV_SECTOR_BITS, &qiov, + count >> BDRV_SECTOR_BITS, + QED_AIOCB_WRITE | QED_AIOCB_ZERO); } static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp) @@ -XXX,XX +XXX,XX @@ static BlockDriver bdrv_qed = { .bdrv_create = bdrv_qed_create, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_get_block_status = bdrv_qed_co_get_block_status, - .bdrv_aio_readv = bdrv_qed_aio_readv, - .bdrv_aio_writev = bdrv_qed_aio_writev, + .bdrv_co_readv = bdrv_qed_co_readv, + .bdrv_co_writev = bdrv_qed_co_writev, .bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes, .bdrv_truncate = bdrv_qed_truncate, .bdrv_getlength = bdrv_qed_getlength, -- 1.8.3.1 Now that we're running in coroutine context, the ad-hoc serialisation code (which drops a request that has to wait out of coroutine context) can be replaced by a CoQueue. This means that when we resume a serialised request, it is running in coroutine context again and its I/O isn't blocking any more. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 49 +++++++++++++++++-------------------------------- block/qed.h | 3 ++- 2 files changed, 19 insertions(+), 33 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void qed_plug_allocating_write_reqs(BDRVQEDState *s) static void qed_unplug_allocating_write_reqs(BDRVQEDState *s) { - QEDAIOCB *acb; - assert(s->allocating_write_reqs_plugged); s->allocating_write_reqs_plugged = false; - - acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs); - if (acb) { - qed_aio_start_io(acb); - } + qemu_co_enter_next(&s->allocating_write_reqs); } static void qed_clear_need_check(void *opaque, int ret) @@ -XXX,XX +XXX,XX @@ static void qed_need_check_timer_cb(void *opaque) BDRVQEDState *s = opaque; /* The timer should only fire when allocating writes have drained */ - assert(!QSIMPLEQ_FIRST(&s->allocating_write_reqs)); + assert(!s->allocating_acb); trace_qed_need_check_timer_cb(s); @@ -XXX,XX +XXX,XX @@ static int bdrv_qed_do_open(BlockDriverState *bs, QDict *options, int flags, int ret; s->bs = bs; - QSIMPLEQ_INIT(&s->allocating_write_reqs); + qemu_co_queue_init(&s->allocating_write_reqs); ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header)); if (ret < 0) { @@ -XXX,XX +XXX,XX @@ static void qed_aio_complete_bh(void *opaque) qed_release(s); } -static void qed_resume_alloc_bh(void *opaque) -{ - qed_aio_start_io(opaque); -} - static void qed_aio_complete(QEDAIOCB *acb, int ret) { BDRVQEDState *s = acb_to_s(acb); @@ -XXX,XX +XXX,XX @@ static void qed_aio_complete(QEDAIOCB *acb, int ret) * next request in the queue. This ensures that we don't cycle through * requests multiple times but rather finish one at a time completely. */ - if (acb == QSIMPLEQ_FIRST(&s->allocating_write_reqs)) { - QEDAIOCB *next_acb; - QSIMPLEQ_REMOVE_HEAD(&s->allocating_write_reqs, next); - next_acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs); - if (next_acb) { - aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs), - qed_resume_alloc_bh, next_acb); + if (acb == s->allocating_acb) { + s->allocating_acb = NULL; + if (!qemu_co_queue_empty(&s->allocating_write_reqs)) { + qemu_co_enter_next(&s->allocating_write_reqs); } else if (s->header.features & QED_F_NEED_CHECK) { qed_start_need_check_timer(s); } @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_alloc(QEDAIOCB *acb, size_t len) int ret; /* Cancel timer when the first allocating request comes in */ - if (QSIMPLEQ_EMPTY(&s->allocating_write_reqs)) { + if (s->allocating_acb == NULL) { qed_cancel_need_check_timer(s); } /* Freeze this request if another allocating write is in progress */ - if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs)) { - QSIMPLEQ_INSERT_TAIL(&s->allocating_write_reqs, acb, next); - } - if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs) || - s->allocating_write_reqs_plugged) { - return -EINPROGRESS; /* wait for existing request to finish */ + if (s->allocating_acb != acb || s->allocating_write_reqs_plugged) { + if (s->allocating_acb != NULL) { + qemu_co_queue_wait(&s->allocating_write_reqs, NULL); + assert(s->allocating_acb == NULL); + } + s->allocating_acb = acb; + return -EAGAIN; /* start over with looking up table entries */ } acb->cur_nclusters = qed_bytes_to_clusters(s, @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb) ret = qed_aio_read_data(acb, ret, offset, len); } - if (ret < 0) { - if (ret != -EINPROGRESS) { - qed_aio_complete(acb, ret); - } + if (ret < 0 && ret != -EAGAIN) { + qed_aio_complete(acb, ret); return; } } diff --git a/block/qed.h b/block/qed.h index XXXXXXX..XXXXXXX 100644 --- a/block/qed.h +++ b/block/qed.h @@ -XXX,XX +XXX,XX @@ typedef struct { uint32_t l2_mask; /* Allocating write request queue */ - QSIMPLEQ_HEAD(, QEDAIOCB) allocating_write_reqs; + QEDAIOCB *allocating_acb; + CoQueue allocating_write_reqs; bool allocating_write_reqs_plugged; /* Periodic flush and clear need check flag */ -- 1.8.3.1 Now that we process a request in the same coroutine from beginning to end and don't drop out of it any more, we can look like a proper coroutine-based driver and simply call qed_aio_next_io() and get a return value from it instead of spawning an additional coroutine that reenters the parent when it's done. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 101 +++++++++++++----------------------------------------------- block/qed.h | 3 +- 2 files changed, 22 insertions(+), 82 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ #include "qapi/qmp/qerror.h" #include "sysemu/block-backend.h" -static const AIOCBInfo qed_aiocb_info = { - .aiocb_size = sizeof(QEDAIOCB), -}; - static int bdrv_qed_probe(const uint8_t *buf, int buf_size, const char *filename) { @@ -XXX,XX +XXX,XX @@ static CachedL2Table *qed_new_l2_table(BDRVQEDState *s) return l2_table; } -static void qed_aio_next_io(QEDAIOCB *acb); - -static void qed_aio_start_io(QEDAIOCB *acb) -{ - qed_aio_next_io(acb); -} - static void qed_plug_allocating_write_reqs(BDRVQEDState *s) { assert(!s->allocating_write_reqs_plugged); @@ -XXX,XX +XXX,XX @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs, static BDRVQEDState *acb_to_s(QEDAIOCB *acb) { - return acb->common.bs->opaque; + return acb->bs->opaque; } /** @@ -XXX,XX +XXX,XX @@ static void qed_update_l2_table(BDRVQEDState *s, QEDTable *table, int index, } } -static void qed_aio_complete_bh(void *opaque) -{ - QEDAIOCB *acb = opaque; - BDRVQEDState *s = acb_to_s(acb); - BlockCompletionFunc *cb = acb->common.cb; - void *user_opaque = acb->common.opaque; - int ret = acb->bh_ret; - - qemu_aio_unref(acb); - - /* Invoke callback */ - qed_acquire(s); - cb(user_opaque, ret); - qed_release(s); -} - -static void qed_aio_complete(QEDAIOCB *acb, int ret) +static void qed_aio_complete(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); - trace_qed_aio_complete(s, acb, ret); - /* Free resources */ qemu_iovec_destroy(&acb->cur_qiov); qed_unref_l2_cache_entry(acb->request.l2_table); @@ -XXX,XX +XXX,XX @@ static void qed_aio_complete(QEDAIOCB *acb, int ret) acb->qiov->iov[0].iov_base = NULL; } - /* Arrange for a bh to invoke the completion function */ - acb->bh_ret = ret; - aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs), - qed_aio_complete_bh, acb); - /* Start next allocating write request waiting behind this one. Note that * requests enqueue themselves when they first hit an unallocated cluster * but they wait until the entire request is finished before waking up the @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) struct iovec *iov = acb->qiov->iov; if (!iov->iov_base) { - iov->iov_base = qemu_try_blockalign(acb->common.bs, iov->iov_len); + iov->iov_base = qemu_try_blockalign(acb->bs, iov->iov_len); if (iov->iov_base == NULL) { return -ENOMEM; } @@ -XXX,XX +XXX,XX @@ static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len) { QEDAIOCB *acb = opaque; BDRVQEDState *s = acb_to_s(acb); - BlockDriverState *bs = acb->common.bs; + BlockDriverState *bs = acb->bs; /* Adjust offset into cluster */ offset += qed_offset_into_cluster(s, acb->cur_pos); @@ -XXX,XX +XXX,XX @@ static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len) /** * Begin next I/O or complete the request */ -static void qed_aio_next_io(QEDAIOCB *acb) +static int qed_aio_next_io(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); uint64_t offset; @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb) /* Complete request */ if (acb->cur_pos >= acb->end_pos) { - qed_aio_complete(acb, 0); - return; + ret = 0; + break; } /* Find next cluster and start I/O */ len = acb->end_pos - acb->cur_pos; ret = qed_find_cluster(s, &acb->request, acb->cur_pos, &len, &offset); if (ret < 0) { - qed_aio_complete(acb, ret); - return; + break; } if (acb->flags & QED_AIOCB_WRITE) { @@ -XXX,XX +XXX,XX @@ static void qed_aio_next_io(QEDAIOCB *acb) } if (ret < 0 && ret != -EAGAIN) { - qed_aio_complete(acb, ret); - return; + break; } } -} -typedef struct QEDRequestCo { - Coroutine *co; - bool done; - int ret; -} QEDRequestCo; - -static void qed_co_request_cb(void *opaque, int ret) -{ - QEDRequestCo *co = opaque; - - co->done = true; - co->ret = ret; - qemu_coroutine_enter_if_inactive(co->co); + trace_qed_aio_complete(s, acb, ret); + qed_aio_complete(acb); + return ret; } static int coroutine_fn qed_co_request(BlockDriverState *bs, int64_t sector_num, QEMUIOVector *qiov, int nb_sectors, int flags) { - QEDRequestCo co = { - .co = qemu_coroutine_self(), - .done = false, + QEDAIOCB acb = { + .bs = bs, + .cur_pos = (uint64_t) sector_num * BDRV_SECTOR_SIZE, + .end_pos = (sector_num + nb_sectors) * BDRV_SECTOR_SIZE, + .qiov = qiov, + .flags = flags, }; - QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, qed_co_request_cb, &co); - - trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors, &co, flags); + qemu_iovec_init(&acb.cur_qiov, qiov->niov); - acb->flags = flags; - acb->qiov = qiov; - acb->qiov_offset = 0; - acb->cur_pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE; - acb->end_pos = acb->cur_pos + nb_sectors * BDRV_SECTOR_SIZE; - acb->backing_qiov = NULL; - acb->request.l2_table = NULL; - qemu_iovec_init(&acb->cur_qiov, qiov->niov); + trace_qed_aio_setup(bs->opaque, &acb, sector_num, nb_sectors, NULL, flags); /* Start request */ - qed_aio_start_io(acb); - - if (!co.done) { - qemu_coroutine_yield(); - } - - return co.ret; + return qed_aio_next_io(&acb); } static int coroutine_fn bdrv_qed_co_readv(BlockDriverState *bs, diff --git a/block/qed.h b/block/qed.h index XXXXXXX..XXXXXXX 100644 --- a/block/qed.h +++ b/block/qed.h @@ -XXX,XX +XXX,XX @@ enum { }; typedef struct QEDAIOCB { - BlockAIOCB common; - int bh_ret; /* final return status for completion bh */ + BlockDriverState *bs; QSIMPLEQ_ENTRY(QEDAIOCB) next; /* next request */ int flags; /* QED_AIOCB_* bits ORed together */ uint64_t end_pos; /* request end on block device, in bytes */ -- 1.8.3.1 This fixes the last place where we degraded from AIO to actual blocking synchronous I/O requests. Putting it into a coroutine means that instead of blocking, the coroutine simply yields while doing I/O. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static void qed_unplug_allocating_write_reqs(BDRVQEDState *s) qemu_co_enter_next(&s->allocating_write_reqs); } -static void qed_clear_need_check(void *opaque, int ret) +static void qed_need_check_timer_entry(void *opaque) { BDRVQEDState *s = opaque; + int ret; - if (ret) { + /* The timer should only fire when allocating writes have drained */ + assert(!s->allocating_acb); + + trace_qed_need_check_timer_cb(s); + + qed_acquire(s); + qed_plug_allocating_write_reqs(s); + + /* Ensure writes are on disk before clearing flag */ + ret = bdrv_co_flush(s->bs->file->bs); + qed_release(s); + if (ret < 0) { qed_unplug_allocating_write_reqs(s); return; } @@ -XXX,XX +XXX,XX @@ static void qed_clear_need_check(void *opaque, int ret) qed_unplug_allocating_write_reqs(s); - ret = bdrv_flush(s->bs); + ret = bdrv_co_flush(s->bs); (void) ret; } static void qed_need_check_timer_cb(void *opaque) { - BDRVQEDState *s = opaque; - - /* The timer should only fire when allocating writes have drained */ - assert(!s->allocating_acb); - - trace_qed_need_check_timer_cb(s); - - qed_acquire(s); - qed_plug_allocating_write_reqs(s); - - /* Ensure writes are on disk before clearing flag */ - bdrv_aio_flush(s->bs->file->bs, qed_clear_need_check, s); - qed_release(s); + Coroutine *co = qemu_coroutine_create(qed_need_check_timer_entry, opaque); + qemu_coroutine_enter(co); } void qed_acquire(BDRVQEDState *s) -- 1.8.3.1 Now that we stay in coroutine context for the whole request when doing reads or writes, we can add coroutine_fn annotations to many functions that can do I/O or yield directly. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed-cluster.c | 5 +++-- block/qed.c | 44 ++++++++++++++++++++++++-------------------- block/qed.h | 5 +++-- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/block/qed-cluster.c b/block/qed-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed-cluster.c +++ b/block/qed-cluster.c @@ -XXX,XX +XXX,XX @@ static unsigned int qed_count_contiguous_clusters(BDRVQEDState *s, * On failure QED_CLUSTER_L2 or QED_CLUSTER_L1 is returned for missing L2 or L1 * table offset, respectively. len is number of contiguous unallocated bytes. */ -int qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, - size_t *len, uint64_t *img_offset) +int coroutine_fn qed_find_cluster(BDRVQEDState *s, QEDRequest *request, + uint64_t pos, size_t *len, + uint64_t *img_offset) { uint64_t l2_offset; uint64_t offset = 0; diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ int qed_write_header_sync(BDRVQEDState *s) * This function only updates known header fields in-place and does not affect * extra data after the QED header. */ -static int qed_write_header(BDRVQEDState *s) +static int coroutine_fn qed_write_header(BDRVQEDState *s) { /* We must write full sectors for O_DIRECT but cannot necessarily generate * the data following the header if an unrecognized compat feature is @@ -XXX,XX +XXX,XX @@ static void qed_unplug_allocating_write_reqs(BDRVQEDState *s) qemu_co_enter_next(&s->allocating_write_reqs); } -static void qed_need_check_timer_entry(void *opaque) +static void coroutine_fn qed_need_check_timer_entry(void *opaque) { BDRVQEDState *s = opaque; int ret; @@ -XXX,XX +XXX,XX @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb) * This function reads qiov->size bytes starting at pos from the backing file. * If there is no backing file then zeroes are read. */ -static int qed_read_backing_file(BDRVQEDState *s, uint64_t pos, - QEMUIOVector *qiov, - QEMUIOVector **backing_qiov) +static int coroutine_fn qed_read_backing_file(BDRVQEDState *s, uint64_t pos, + QEMUIOVector *qiov, + QEMUIOVector **backing_qiov) { uint64_t backing_length = 0; size_t size; @@ -XXX,XX +XXX,XX @@ static int qed_read_backing_file(BDRVQEDState *s, uint64_t pos, * @len: Number of bytes * @offset: Byte offset in image file */ -static int qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos, - uint64_t len, uint64_t offset) +static int coroutine_fn qed_copy_from_backing_file(BDRVQEDState *s, + uint64_t pos, uint64_t len, + uint64_t offset) { QEMUIOVector qiov; QEMUIOVector *backing_qiov = NULL; @@ -XXX,XX +XXX,XX @@ out: * The cluster offset may be an allocated byte offset in the image file, the * zero cluster marker, or the unallocated cluster marker. */ -static void qed_update_l2_table(BDRVQEDState *s, QEDTable *table, int index, - unsigned int n, uint64_t cluster) +static void coroutine_fn qed_update_l2_table(BDRVQEDState *s, QEDTable *table, + int index, unsigned int n, + uint64_t cluster) { int i; for (i = index; i < index + n; i++) { @@ -XXX,XX +XXX,XX @@ static void qed_update_l2_table(BDRVQEDState *s, QEDTable *table, int index, } } -static void qed_aio_complete(QEDAIOCB *acb) +static void coroutine_fn qed_aio_complete(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); @@ -XXX,XX +XXX,XX @@ static void qed_aio_complete(QEDAIOCB *acb) /** * Update L1 table with new L2 table offset and write it out */ -static int qed_aio_write_l1_update(QEDAIOCB *acb) +static int coroutine_fn qed_aio_write_l1_update(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); CachedL2Table *l2_table = acb->request.l2_table; @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_l1_update(QEDAIOCB *acb) /** * Update L2 table with new cluster offsets and write them out */ -static int qed_aio_write_l2_update(QEDAIOCB *acb, uint64_t offset) +static int coroutine_fn qed_aio_write_l2_update(QEDAIOCB *acb, uint64_t offset) { BDRVQEDState *s = acb_to_s(acb); bool need_alloc = acb->find_cluster_ret == QED_CLUSTER_L1; @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_l2_update(QEDAIOCB *acb, uint64_t offset) /** * Write data to the image file */ -static int qed_aio_write_main(QEDAIOCB *acb) +static int coroutine_fn qed_aio_write_main(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); uint64_t offset = acb->cur_cluster + @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_main(QEDAIOCB *acb) /** * Populate untouched regions of new data cluster */ -static int qed_aio_write_cow(QEDAIOCB *acb) +static int coroutine_fn qed_aio_write_cow(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); uint64_t start, len, offset; @@ -XXX,XX +XXX,XX @@ static bool qed_should_set_need_check(BDRVQEDState *s) * * This path is taken when writing to previously unallocated clusters. */ -static int qed_aio_write_alloc(QEDAIOCB *acb, size_t len) +static int coroutine_fn qed_aio_write_alloc(QEDAIOCB *acb, size_t len) { BDRVQEDState *s = acb_to_s(acb); int ret; @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_alloc(QEDAIOCB *acb, size_t len) * * This path is taken when writing to already allocated clusters. */ -static int qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) +static int coroutine_fn qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, + size_t len) { /* Allocate buffer for zero writes */ if (acb->flags & QED_AIOCB_ZERO) { @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len) * @offset: Cluster offset in bytes * @len: Length in bytes */ -static int qed_aio_write_data(void *opaque, int ret, - uint64_t offset, size_t len) +static int coroutine_fn qed_aio_write_data(void *opaque, int ret, + uint64_t offset, size_t len) { QEDAIOCB *acb = opaque; @@ -XXX,XX +XXX,XX @@ static int qed_aio_write_data(void *opaque, int ret, * @offset: Cluster offset in bytes * @len: Length in bytes */ -static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len) +static int coroutine_fn qed_aio_read_data(void *opaque, int ret, + uint64_t offset, size_t len) { QEDAIOCB *acb = opaque; BDRVQEDState *s = acb_to_s(acb); @@ -XXX,XX +XXX,XX @@ static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len) /** * Begin next I/O or complete the request */ -static int qed_aio_next_io(QEDAIOCB *acb) +static int coroutine_fn qed_aio_next_io(QEDAIOCB *acb) { BDRVQEDState *s = acb_to_s(acb); uint64_t offset; diff --git a/block/qed.h b/block/qed.h index XXXXXXX..XXXXXXX 100644 --- a/block/qed.h +++ b/block/qed.h @@ -XXX,XX +XXX,XX @@ int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request, /** * Cluster functions */ -int qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, - size_t *len, uint64_t *img_offset); +int coroutine_fn qed_find_cluster(BDRVQEDState *s, QEDRequest *request, + uint64_t pos, size_t *len, + uint64_t *img_offset); /** * Consistency check -- 1.8.3.1 All functions that are marked coroutine_fn can directly call the bdrv_co_* version of functions instead of going through the wrapper. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Manos Pitsidianakis <el13635@mail.ntua.gr> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/qed.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qed_write_header(BDRVQEDState *s) }; qemu_iovec_init_external(&qiov, &iov, 1); - ret = bdrv_preadv(s->bs->file, 0, &qiov); + ret = bdrv_co_preadv(s->bs->file, 0, qiov.size, &qiov, 0); if (ret < 0) { goto out; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qed_write_header(BDRVQEDState *s) /* Update header */ qed_header_cpu_to_le(&s->header, (QEDHeader *) buf); - ret = bdrv_pwritev(s->bs->file, 0, &qiov); + ret = bdrv_co_pwritev(s->bs->file, 0, qiov.size, &qiov, 0); if (ret < 0) { goto out; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qed_read_backing_file(BDRVQEDState *s, uint64_t pos, qemu_iovec_concat(*backing_qiov, qiov, 0, size); BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO); - ret = bdrv_preadv(s->bs->backing, pos, *backing_qiov); + ret = bdrv_co_preadv(s->bs->backing, pos, size, *backing_qiov, 0); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qed_copy_from_backing_file(BDRVQEDState *s, } BLKDBG_EVENT(s->bs->file, BLKDBG_COW_WRITE); - ret = bdrv_pwritev(s->bs->file, offset, &qiov); + ret = bdrv_co_pwritev(s->bs->file, offset, qiov.size, &qiov, 0); if (ret < 0) { goto out; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qed_aio_write_main(QEDAIOCB *acb) trace_qed_aio_write_main(s, acb, 0, offset, acb->cur_qiov.size); BLKDBG_EVENT(s->bs->file, BLKDBG_WRITE_AIO); - ret = bdrv_pwritev(s->bs->file, offset, &acb->cur_qiov); + ret = bdrv_co_pwritev(s->bs->file, offset, acb->cur_qiov.size, + &acb->cur_qiov, 0); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qed_aio_write_main(QEDAIOCB *acb) * region. The solution is to flush after writing a new data * cluster and before updating the L2 table. */ - ret = bdrv_flush(s->bs->file->bs); + ret = bdrv_co_flush(s->bs->file->bs); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qed_aio_read_data(void *opaque, int ret, } BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO); - ret = bdrv_preadv(bs->file, offset, &acb->cur_qiov); + ret = bdrv_co_preadv(bs->file, offset, acb->cur_qiov.size, + &acb->cur_qiov, 0); if (ret < 0) { return ret; } -- 1.8.3.1 From: "sochin.jiang" <sochin.jiang@huawei.com> img_commit could fall into an infinite loop calling run_block_job() if its blockjob fails on any I/O error, fix this already known problem. Signed-off-by: sochin.jiang <sochin.jiang@huawei.com> Message-id: 1497509253-28941-1-git-send-email-sochin.jiang@huawei.com Signed-off-by: Max Reitz <mreitz@redhat.com> --- blockjob.c | 4 ++-- include/block/blockjob.h | 18 ++++++++++++++++++ qemu-img.c | 20 +++++++++++++------- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/blockjob.c b/blockjob.c index XXXXXXX..XXXXXXX 100644 --- a/blockjob.c +++ b/blockjob.c @@ -XXX,XX +XXX,XX @@ static void block_job_resume(BlockJob *job) block_job_enter(job); } -static void block_job_ref(BlockJob *job) +void block_job_ref(BlockJob *job) { ++job->refcnt; } @@ -XXX,XX +XXX,XX @@ static void block_job_attached_aio_context(AioContext *new_context, void *opaque); static void block_job_detach_aio_context(void *opaque); -static void block_job_unref(BlockJob *job) +void block_job_unref(BlockJob *job) { if (--job->refcnt == 0) { BlockDriverState *bs = blk_bs(job->blk); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -XXX,XX +XXX,XX @@ void block_job_iostatus_reset(BlockJob *job); BlockJobTxn *block_job_txn_new(void); /** + * block_job_ref: + * + * Add a reference to BlockJob refcnt, it will be decreased with + * block_job_unref, and then be freed if it comes to be the last + * reference. + */ +void block_job_ref(BlockJob *job); + +/** + * block_job_unref: + * + * Release a reference that was previously acquired with block_job_ref + * or block_job_create. If it's the last reference to the object, it will be + * freed. + */ +void block_job_unref(BlockJob *job); + +/** * block_job_txn_unref: * * Release a reference that was previously acquired with block_job_txn_add_job diff --git a/qemu-img.c b/qemu-img.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -XXX,XX +XXX,XX @@ static void common_block_job_cb(void *opaque, int ret) static void run_block_job(BlockJob *job, Error **errp) { AioContext *aio_context = blk_get_aio_context(job->blk); + int ret = 0; - /* FIXME In error cases, the job simply goes away and we access a dangling - * pointer below. */ aio_context_acquire(aio_context); + block_job_ref(job); do { aio_poll(aio_context, true); qemu_progress_print(job->len ? ((float)job->offset / job->len * 100.f) : 0.0f, 0); - } while (!job->ready); + } while (!job->ready && !job->completed); - block_job_complete_sync(job, errp); + if (!job->completed) { + ret = block_job_complete_sync(job, errp); + } else { + ret = job->ret; + } + block_job_unref(job); aio_context_release(aio_context); - /* A block job may finish instantaneously without publishing any progress, - * so just signal completion here */ - qemu_progress_print(100.f, 0); + /* publish completion progress only when success */ + if (!ret) { + qemu_progress_print(100.f, 0); + } } static int img_commit(int argc, char **argv) -- 1.8.3.1 From: Max Reitz <mreitz@redhat.com> The bs->exact_filename field may not be sufficient to store the full blkdebug node filename. In this case, we should not generate a filename at all instead of an unusable one. Cc: qemu-stable@nongnu.org Reported-by: Qu Wenruo <quwenruo@cn.fujitsu.com> Signed-off-by: Max Reitz <mreitz@redhat.com> Message-id: 20170613172006.19685-2-mreitz@redhat.com Reviewed-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/blkdebug.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/block/blkdebug.c b/block/blkdebug.c index XXXXXXX..XXXXXXX 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -XXX,XX +XXX,XX @@ static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options) } if (!force_json && bs->file->bs->exact_filename[0]) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), - "blkdebug:%s:%s", s->config_file ?: "", - bs->file->bs->exact_filename); + int ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename), + "blkdebug:%s:%s", s->config_file ?: "", + bs->file->bs->exact_filename); + if (ret >= sizeof(bs->exact_filename)) { + /* An overflow makes the filename unusable, so do not report any */ + bs->exact_filename[0] = 0; + } } opts = qdict_new(); -- 1.8.3.1 From: Max Reitz <mreitz@redhat.com> The bs->exact_filename field may not be sufficient to store the full blkverify node filename. In this case, we should not generate a filename at all instead of an unusable one. Cc: qemu-stable@nongnu.org Reported-by: Qu Wenruo <quwenruo@cn.fujitsu.com> Signed-off-by: Max Reitz <mreitz@redhat.com> Message-id: 20170613172006.19685-3-mreitz@redhat.com Reviewed-by: Alberto Garcia <berto@igalia.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/blkverify.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/block/blkverify.c b/block/blkverify.c index XXXXXXX..XXXXXXX 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -XXX,XX +XXX,XX @@ static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options) if (bs->file->bs->exact_filename[0] && s->test_file->bs->exact_filename[0]) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), - "blkverify:%s:%s", - bs->file->bs->exact_filename, - s->test_file->bs->exact_filename); + int ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename), + "blkverify:%s:%s", + bs->file->bs->exact_filename, + s->test_file->bs->exact_filename); + if (ret >= sizeof(bs->exact_filename)) { + /* An overflow makes the filename unusable, so do not report any */ + bs->exact_filename[0] = 0; + } } } -- 1.8.3.1 From: Max Reitz <mreitz@redhat.com> uri_parse(...)->scheme may be NULL. In fact, probably every field may be NULL, and the callers do test this for all of the other fields but not for scheme (except for block/gluster.c; block/vxhs.c does not access that field at all). We can easily fix this by using g_strcmp0() instead of strcmp(). Cc: qemu-stable@nongnu.org Signed-off-by: Max Reitz <mreitz@redhat.com> Message-id: 20170613205726.13544-1-mreitz@redhat.com Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/nbd.c | 6 +++--- block/nfs.c | 2 +- block/sheepdog.c | 6 +++--- block/ssh.c | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index XXXXXXX..XXXXXXX 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -XXX,XX +XXX,XX @@ static int nbd_parse_uri(const char *filename, QDict *options) } /* transport */ - if (!strcmp(uri->scheme, "nbd")) { + if (!g_strcmp0(uri->scheme, "nbd")) { is_unix = false; - } else if (!strcmp(uri->scheme, "nbd+tcp")) { + } else if (!g_strcmp0(uri->scheme, "nbd+tcp")) { is_unix = false; - } else if (!strcmp(uri->scheme, "nbd+unix")) { + } else if (!g_strcmp0(uri->scheme, "nbd+unix")) { is_unix = true; } else { ret = -EINVAL; diff --git a/block/nfs.c b/block/nfs.c index XXXXXXX..XXXXXXX 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -XXX,XX +XXX,XX @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp) error_setg(errp, "Invalid URI specified"); goto out; } - if (strcmp(uri->scheme, "nfs") != 0) { + if (g_strcmp0(uri->scheme, "nfs") != 0) { error_setg(errp, "URI scheme must be 'nfs'"); goto out; } diff --git a/block/sheepdog.c b/block/sheepdog.c index XXXXXXX..XXXXXXX 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -XXX,XX +XXX,XX @@ static void sd_parse_uri(SheepdogConfig *cfg, const char *filename, } /* transport */ - if (!strcmp(uri->scheme, "sheepdog")) { + if (!g_strcmp0(uri->scheme, "sheepdog")) { is_unix = false; - } else if (!strcmp(uri->scheme, "sheepdog+tcp")) { + } else if (!g_strcmp0(uri->scheme, "sheepdog+tcp")) { is_unix = false; - } else if (!strcmp(uri->scheme, "sheepdog+unix")) { + } else if (!g_strcmp0(uri->scheme, "sheepdog+unix")) { is_unix = true; } else { error_setg(&err, "URI scheme must be 'sheepdog', 'sheepdog+tcp'," diff --git a/block/ssh.c b/block/ssh.c index XXXXXXX..XXXXXXX 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -XXX,XX +XXX,XX @@ static int parse_uri(const char *filename, QDict *options, Error **errp) return -EINVAL; } - if (strcmp(uri->scheme, "ssh") != 0) { + if (g_strcmp0(uri->scheme, "ssh") != 0) { error_setg(errp, "URI scheme must be 'ssh'"); goto err; } -- 1.8.3.1 The following changes since commit 16aaacb307ed607b9780c12702c44f0fe52edc7e: Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20200430' into staging (2020-04-30 14:00:36 +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 eaae29ef89d498d0eac553c77b554f310a47f809: qemu-storage-daemon: Fix non-string --object properties (2020-04-30 17:51:07 +0200) ---------------------------------------------------------------- Block layer patches: - Fix resize (extending) of short overlays - nvme: introduce PMR support from NVMe 1.4 spec - qemu-storage-daemon: Fix non-string --object properties ---------------------------------------------------------------- Alberto Garcia (1): qcow2: Add incompatibility note between backing files and raw external data files Andrzej Jakowski (1): nvme: introduce PMR support from NVMe 1.4 spec Kevin Wolf (12): block: Add flags to BlockDriver.bdrv_co_truncate() block: Add flags to bdrv(_co)_truncate() block-backend: Add flags to blk_truncate() qcow2: Support BDRV_REQ_ZERO_WRITE for truncate raw-format: Support BDRV_REQ_ZERO_WRITE for truncate file-posix: Support BDRV_REQ_ZERO_WRITE for truncate block: truncate: Don't make backing file data visible iotests: Filter testfiles out in filter_img_info() iotests: Test committing to short backing file qcow2: Forward ZERO_WRITE flag for full preallocation qom: Factor out user_creatable_add_dict() qemu-storage-daemon: Fix non-string --object properties Paolo Bonzini (1): qemu-iotests: allow qcow2 external discarded clusters to contain stale data docs/interop/qcow2.txt | 3 + hw/block/nvme.h | 2 + include/block/block.h | 5 +- include/block/block_int.h | 10 +- include/block/nvme.h | 172 ++++++++++++++++++++++++++ include/qom/object_interfaces.h | 16 +++ include/sysemu/block-backend.h | 2 +- block.c | 3 +- block/block-backend.c | 4 +- block/commit.c | 4 +- block/crypto.c | 7 +- block/file-posix.c | 6 +- block/file-win32.c | 2 +- block/gluster.c | 1 + block/io.c | 43 ++++++- block/iscsi.c | 2 +- block/mirror.c | 2 +- block/nfs.c | 3 +- block/parallels.c | 6 +- block/qcow.c | 4 +- block/qcow2-cluster.c | 2 +- block/qcow2-refcount.c | 2 +- block/qcow2.c | 73 +++++++++-- block/qed.c | 3 +- block/raw-format.c | 6 +- block/rbd.c | 1 + block/sheepdog.c | 4 +- block/ssh.c | 2 +- block/vdi.c | 2 +- block/vhdx-log.c | 2 +- block/vhdx.c | 6 +- block/vmdk.c | 8 +- block/vpc.c | 2 +- blockdev.c | 2 +- hw/block/nvme.c | 109 ++++++++++++++++ qemu-img.c | 2 +- qemu-io-cmds.c | 2 +- qemu-storage-daemon.c | 4 +- qom/object_interfaces.c | 31 +++++ qom/qom-qmp-cmds.c | 24 +--- tests/test-block-iothread.c | 9 +- tests/qemu-iotests/iotests.py | 5 +- hw/block/Makefile.objs | 2 +- hw/block/trace-events | 4 + tests/qemu-iotests/244 | 10 +- tests/qemu-iotests/244.out | 9 +- tests/qemu-iotests/274 | 155 +++++++++++++++++++++++ tests/qemu-iotests/274.out | 268 ++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 49 files changed, 951 insertions(+), 96 deletions(-) create mode 100755 tests/qemu-iotests/274 create mode 100644 tests/qemu-iotests/274.out From: Alberto Garcia <berto@igalia.com> Backing files and raw external data files are mutually exclusive. The documentation of the raw external data bit (in autoclear_features) already indicates that, but we should also mention it on the other side. Suggested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Alberto Garcia <berto@igalia.com> Message-Id: <20200410121816.8334-1-berto@igalia.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- docs/interop/qcow2.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index XXXXXXX..XXXXXXX 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -XXX,XX +XXX,XX @@ The first cluster of a qcow2 image contains the file header: is stored (NB: The string is not null terminated). 0 if the image doesn't have a backing file. + Note: backing files are incompatible with raw external data + files (auto-clear feature bit 1). + 16 - 19: backing_file_size Length of the backing file name in bytes. Must not be longer than 1023 bytes. Undefined if the image doesn't have -- 2.25.3 From: Paolo Bonzini <pbonzini@redhat.com> Test 244 checks the expected behavior of qcow2 external data files with respect to zero and discarded clusters. Filesystems however are free to ignore discard requests, and this seems to be the case for overlayfs. Relax the tests to skip checks on the external data file for discarded areas, which implies not using qemu-img compare in the data_file_raw=on case. This fixes docker tests on RHEL8. Cc: Kevin Wolf <kwolf@redhat.com> Cc: qemu-block@nongnu.org Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20200409191006.24429-1-pbonzini@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/244 | 10 ++++++++-- tests/qemu-iotests/244.out | 9 ++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/244 +++ b/tests/qemu-iotests/244 @@ -XXX,XX +XXX,XX @@ $QEMU_IO -c 'read -P 0 0 1M' \ echo $QEMU_IO -c 'read -P 0 0 1M' \ -c 'read -P 0x11 1M 1M' \ - -c 'read -P 0 2M 2M' \ -c 'read -P 0x11 4M 1M' \ -c 'read -P 0 5M 1M' \ -f raw "$TEST_IMG.data" | @@ -XXX,XX +XXX,XX @@ $QEMU_IO -c 'read -P 0 0 1M' \ -f $IMGFMT "$TEST_IMG" | _filter_qemu_io +# Discarded clusters are only marked as such in the qcow2 metadata, but +# they can contain stale data in the external data file. Instead, zero +# clusters must be zeroed in the external data file too. echo -$QEMU_IMG compare "$TEST_IMG" "$TEST_IMG.data" +$QEMU_IO -c 'read -P 0 0 1M' \ + -c 'read -P 0x11 1M 1M' \ + -c 'read -P 0 3M 3M' \ + -f raw "$TEST_IMG".data | + _filter_qemu_io echo -n "qcow2 file size after I/O: " du -b $TEST_IMG | cut -f1 diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/244.out +++ b/tests/qemu-iotests/244.out @@ -XXX,XX +XXX,XX @@ read 1048576/1048576 bytes at offset 0 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 1048576/1048576 bytes at offset 1048576 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -read 2097152/2097152 bytes at offset 2097152 -2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 1048576/1048576 bytes at offset 4194304 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 1048576/1048576 bytes at offset 5242880 @@ -XXX,XX +XXX,XX @@ read 1048576/1048576 bytes at offset 1048576 read 4194304/4194304 bytes at offset 2097152 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -Images are identical. +read 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 3145728/3145728 bytes at offset 3145728 +3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) qcow2 file size after I/O: 327680 === bdrv_co_block_status test for file and offset=0 === -- 2.25.3 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> --- include/block/block_int.h | 10 +++++++++- block/crypto.c | 3 ++- block/file-posix.c | 2 +- block/file-win32.c | 2 +- block/gluster.c | 1 + block/io.c | 8 +++++++- block/iscsi.c | 2 +- block/nfs.c | 3 ++- block/qcow2.c | 2 +- block/qed.c | 1 + block/raw-format.c | 2 +- block/rbd.c | 1 + block/sheepdog.c | 4 ++-- block/ssh.c | 2 +- tests/test-block-iothread.c | 3 ++- 15 files changed, 33 insertions(+), 13 deletions(-) diff --git a/include/block/block_int.h b/include/block/block_int.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -XXX,XX +XXX,XX @@ struct BlockDriver { */ int coroutine_fn (*bdrv_co_truncate)(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp); + BdrvRequestFlags flags, Error **errp); int64_t (*bdrv_getlength)(BlockDriverState *bs); bool has_variable_length; @@ -XXX,XX +XXX,XX @@ struct BlockDriverState { /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA, * BDRV_REQ_MAY_UNMAP, BDRV_REQ_WRITE_UNCHANGED) */ unsigned int supported_zero_flags; + /* + * Flags honoured during truncate (so far: BDRV_REQ_ZERO_WRITE). + * + * If BDRV_REQ_ZERO_WRITE is given, the truncate operation must make sure + * that any added space reads as all zeros. If this can't be guaranteed, + * the operation must fail. + */ + unsigned int supported_truncate_flags; /* the following member gives a name to every node on the bs graph. */ char node_name[32]; diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ static int block_crypto_co_create_generic(BlockDriverState *bs, static int coroutine_fn block_crypto_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp) + PreallocMode prealloc, BdrvRequestFlags flags, + Error **errp) { BlockCrypto *crypto = bs->opaque; uint64_t payload_offset = diff --git a/block/file-posix.c b/block/file-posix.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -XXX,XX +XXX,XX @@ raw_regular_truncate(BlockDriverState *bs, int fd, int64_t offset, static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp) + BdrvRequestFlags flags, Error **errp) { BDRVRawState *s = bs->opaque; struct stat st; diff --git a/block/file-win32.c b/block/file-win32.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-win32.c +++ b/block/file-win32.c @@ -XXX,XX +XXX,XX @@ static void raw_close(BlockDriverState *bs) static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp) + BdrvRequestFlags flags, Error **errp) { BDRVRawState *s = bs->opaque; LONG low, high; diff --git a/block/gluster.c b/block/gluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -XXX,XX +XXX,XX @@ static coroutine_fn int qemu_gluster_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, + BdrvRequestFlags flags, Error **errp) { BDRVGlusterState *s = bs->opaque; diff --git a/block/io.c b/block/io.c index XXXXXXX..XXXXXXX 100644 --- a/block/io.c +++ b/block/io.c @@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, BlockDriverState *bs = child->bs; BlockDriver *drv = bs->drv; BdrvTrackedRequest req; + BdrvRequestFlags flags = 0; int64_t old_size, new_bytes; int ret; @@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, } if (drv->bdrv_co_truncate) { - ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, errp); + if (flags & ~bs->supported_truncate_flags) { + error_setg(errp, "Block driver does not support requested flags"); + ret = -ENOTSUP; + goto out; + } + ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, flags, errp); } else if (bs->file && drv->is_filter) { ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, errp); } else { diff --git a/block/iscsi.c b/block/iscsi.c index XXXXXXX..XXXXXXX 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -XXX,XX +XXX,XX @@ static void iscsi_reopen_commit(BDRVReopenState *reopen_state) static int coroutine_fn iscsi_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp) + BdrvRequestFlags flags, Error **errp) { IscsiLun *iscsilun = bs->opaque; int64_t cur_length; diff --git a/block/nfs.c b/block/nfs.c index XXXXXXX..XXXXXXX 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -XXX,XX +XXX,XX @@ static int64_t nfs_get_allocated_file_size(BlockDriverState *bs) static int coroutine_fn nfs_file_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp) + PreallocMode prealloc, BdrvRequestFlags flags, + Error **errp) { NFSClient *client = bs->opaque; int ret; diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ fail: static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp) + BdrvRequestFlags flags, Error **errp) { BDRVQcow2State *s = bs->opaque; uint64_t old_length; diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_qed_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, + BdrvRequestFlags flags, Error **errp) { BDRVQEDState *s = bs->opaque; diff --git a/block/raw-format.c b/block/raw-format.c index XXXXXXX..XXXXXXX 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -XXX,XX +XXX,XX @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp) + BdrvRequestFlags flags, Error **errp) { BDRVRawState *s = bs->opaque; diff --git a/block/rbd.c b/block/rbd.c index XXXXXXX..XXXXXXX 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qemu_rbd_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, + BdrvRequestFlags flags, Error **errp) { int r; diff --git a/block/sheepdog.c b/block/sheepdog.c index XXXXXXX..XXXXXXX 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -XXX,XX +XXX,XX @@ static int64_t sd_getlength(BlockDriverState *bs) static int coroutine_fn sd_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp) + BdrvRequestFlags flags, Error **errp) { BDRVSheepdogState *s = bs->opaque; int ret, fd; @@ -XXX,XX +XXX,XX @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num, assert(!flags); if (offset > s->inode.vdi_size) { - ret = sd_co_truncate(bs, offset, false, PREALLOC_MODE_OFF, NULL); + ret = sd_co_truncate(bs, offset, false, PREALLOC_MODE_OFF, 0, NULL); if (ret < 0) { return ret; } diff --git a/block/ssh.c b/block/ssh.c index XXXXXXX..XXXXXXX 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -XXX,XX +XXX,XX @@ static int64_t ssh_getlength(BlockDriverState *bs) static int coroutine_fn ssh_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, PreallocMode prealloc, - Error **errp) + BdrvRequestFlags flags, Error **errp) { BDRVSSHState *s = bs->opaque; diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-block-iothread.c +++ b/tests/test-block-iothread.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_test_co_pdiscard(BlockDriverState *bs, static int coroutine_fn bdrv_test_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp) + PreallocMode prealloc, BdrvRequestFlags flags, + Error **errp) { return 0; } -- 2.25.3 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> --- include/block/block.h | 5 +++-- block/block-backend.c | 2 +- block/crypto.c | 2 +- block/io.c | 12 +++++++----- block/parallels.c | 6 +++--- block/qcow.c | 4 ++-- block/qcow2-refcount.c | 2 +- block/qcow2.c | 15 +++++++++------ block/raw-format.c | 2 +- block/vhdx-log.c | 2 +- block/vhdx.c | 2 +- block/vmdk.c | 2 +- tests/test-block-iothread.c | 6 +++--- 13 files changed, 34 insertions(+), 28 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, void bdrv_refresh_filename(BlockDriverState *bs); int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp); + PreallocMode prealloc, BdrvRequestFlags flags, + Error **errp); int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp); + PreallocMode prealloc, BdrvRequestFlags flags, Error **errp); int64_t bdrv_nb_sectors(BlockDriverState *bs); int64_t bdrv_getlength(BlockDriverState *bs); diff --git a/block/block-backend.c b/block/block-backend.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -XXX,XX +XXX,XX @@ int blk_truncate(BlockBackend *blk, int64_t offset, bool exact, return -ENOMEDIUM; } - return bdrv_truncate(blk->root, offset, exact, prealloc, errp); + return bdrv_truncate(blk->root, offset, exact, prealloc, 0, errp); } int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf, diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ block_crypto_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, offset += payload_offset; - return bdrv_co_truncate(bs->file, offset, exact, prealloc, errp); + return bdrv_co_truncate(bs->file, offset, exact, prealloc, 0, errp); } static void block_crypto_close(BlockDriverState *bs) diff --git a/block/io.c b/block/io.c index XXXXXXX..XXXXXXX 100644 --- a/block/io.c +++ b/block/io.c @@ -XXX,XX +XXX,XX @@ static void bdrv_parent_cb_resize(BlockDriverState *bs) * 'offset' bytes in length. */ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp) + PreallocMode prealloc, BdrvRequestFlags flags, + Error **errp) { BlockDriverState *bs = child->bs; BlockDriver *drv = bs->drv; BdrvTrackedRequest req; - BdrvRequestFlags flags = 0; int64_t old_size, new_bytes; int ret; @@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, } ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, flags, errp); } else if (bs->file && drv->is_filter) { - ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, errp); + ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp); } else { error_setg(errp, "Image format driver does not support resize"); ret = -ENOTSUP; @@ -XXX,XX +XXX,XX @@ typedef struct TruncateCo { int64_t offset; bool exact; PreallocMode prealloc; + BdrvRequestFlags flags; Error **errp; int ret; } TruncateCo; @@ -XXX,XX +XXX,XX @@ static void coroutine_fn bdrv_truncate_co_entry(void *opaque) { TruncateCo *tco = opaque; tco->ret = bdrv_co_truncate(tco->child, tco->offset, tco->exact, - tco->prealloc, tco->errp); + tco->prealloc, tco->flags, tco->errp); aio_wait_kick(); } int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp) + PreallocMode prealloc, BdrvRequestFlags flags, Error **errp) { Coroutine *co; TruncateCo tco = { @@ -XXX,XX +XXX,XX @@ int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact, .offset = offset, .exact = exact, .prealloc = prealloc, + .flags = flags, .errp = errp, .ret = NOT_DONE, }; diff --git a/block/parallels.c b/block/parallels.c index XXXXXXX..XXXXXXX 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -XXX,XX +XXX,XX @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, } else { ret = bdrv_truncate(bs->file, (s->data_end + space) << BDRV_SECTOR_BITS, - false, PREALLOC_MODE_OFF, NULL); + false, PREALLOC_MODE_OFF, 0, NULL); } if (ret < 0) { return ret; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, * That means we have to pass exact=true. */ ret = bdrv_truncate(bs->file, res->image_end_offset, true, - PREALLOC_MODE_OFF, &local_err); + PREALLOC_MODE_OFF, 0, &local_err); if (ret < 0) { error_report_err(local_err); res->check_errors++; @@ -XXX,XX +XXX,XX @@ static void parallels_close(BlockDriverState *bs) /* errors are ignored, so we might as well pass exact=true */ bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true, - PREALLOC_MODE_OFF, NULL); + PREALLOC_MODE_OFF, 0, NULL); } g_free(s->bat_dirty_bmap); diff --git a/block/qcow.c b/block/qcow.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs, return -E2BIG; } ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size, - false, PREALLOC_MODE_OFF, NULL); + false, PREALLOC_MODE_OFF, 0, NULL); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int qcow_make_empty(BlockDriverState *bs) l1_length) < 0) return -1; ret = bdrv_truncate(bs->file, s->l1_table_offset + l1_length, false, - PREALLOC_MODE_OFF, NULL); + PREALLOC_MODE_OFF, 0, NULL); if (ret < 0) return ret; diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -XXX,XX +XXX,XX @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res, } ret = bdrv_truncate(bs->file, offset + s->cluster_size, false, - PREALLOC_MODE_OFF, &local_err); + PREALLOC_MODE_OFF, 0, &local_err); if (ret < 0) { error_report_err(local_err); goto resize_fail; diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset, mode = PREALLOC_MODE_OFF; } ret = bdrv_co_truncate(s->data_file, host_offset + cur_bytes, false, - mode, errp); + mode, 0, errp); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, * always fulfilled, so there is no need to pass it on.) */ bdrv_co_truncate(bs->file, (last_cluster + 1) * s->cluster_size, - false, PREALLOC_MODE_OFF, &local_err); + false, PREALLOC_MODE_OFF, 0, &local_err); if (local_err) { warn_reportf_err(local_err, "Failed to truncate the tail of the image: "); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, * file should be resized to the exact target size, too, * so we pass @exact here. */ - ret = bdrv_co_truncate(s->data_file, offset, exact, prealloc, errp); + ret = bdrv_co_truncate(s->data_file, offset, exact, prealloc, 0, + errp); if (ret < 0) { goto fail; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, new_file_size = allocation_start + nb_new_data_clusters * s->cluster_size; /* Image file grows, so @exact does not matter */ - ret = bdrv_co_truncate(bs->file, new_file_size, false, prealloc, errp); + ret = bdrv_co_truncate(bs->file, new_file_size, false, prealloc, 0, + errp); if (ret < 0) { error_prepend(errp, "Failed to resize underlying file: "); qcow2_free_clusters(bs, allocation_start, @@ -XXX,XX +XXX,XX @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs, if (len < 0) { return len; } - return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL); + return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, 0, + NULL); } if (offset_into_cluster(s, offset)) { @@ -XXX,XX +XXX,XX @@ static int make_completely_empty(BlockDriverState *bs) } ret = bdrv_truncate(bs->file, (3 + l1_clusters) * s->cluster_size, false, - PREALLOC_MODE_OFF, &local_err); + PREALLOC_MODE_OFF, 0, &local_err); if (ret < 0) { error_report_err(local_err); goto fail; diff --git a/block/raw-format.c b/block/raw-format.c index XXXXXXX..XXXXXXX 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, s->size = offset; offset += s->offset; - return bdrv_co_truncate(bs->file, offset, exact, prealloc, errp); + return bdrv_co_truncate(bs->file, offset, exact, prealloc, 0, errp); } static void raw_eject(BlockDriverState *bs, bool eject_flag) diff --git a/block/vhdx-log.c b/block/vhdx-log.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -XXX,XX +XXX,XX @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, goto exit; } ret = bdrv_truncate(bs->file, new_file_size, false, - PREALLOC_MODE_OFF, NULL); + PREALLOC_MODE_OFF, 0, NULL); if (ret < 0) { goto exit; } diff --git a/block/vhdx.c b/block/vhdx.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -XXX,XX +XXX,XX @@ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s, } return bdrv_truncate(bs->file, *new_offset + s->block_size, false, - PREALLOC_MODE_OFF, NULL); + PREALLOC_MODE_OFF, 0, NULL); } /* diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, } length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE); ret = bdrv_truncate(s->extents[i].file, length, false, - PREALLOC_MODE_OFF, NULL); + PREALLOC_MODE_OFF, 0, NULL); if (ret < 0) { return ret; } diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c index XXXXXXX..XXXXXXX 100644 --- a/tests/test-block-iothread.c +++ b/tests/test-block-iothread.c @@ -XXX,XX +XXX,XX @@ static void test_sync_op_truncate(BdrvChild *c) int ret; /* Normal success path */ - ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, NULL); + ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, 0, NULL); g_assert_cmpint(ret, ==, 0); /* Early error: Negative offset */ - ret = bdrv_truncate(c, -2, false, PREALLOC_MODE_OFF, NULL); + ret = bdrv_truncate(c, -2, false, PREALLOC_MODE_OFF, 0, NULL); g_assert_cmpint(ret, ==, -EINVAL); /* Error: Read-only image */ c->bs->read_only = true; c->bs->open_flags &= ~BDRV_O_RDWR; - ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, NULL); + ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, 0, NULL); g_assert_cmpint(ret, ==, -EACCES); c->bs->read_only = false; -- 2.25.3 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> --- include/sysemu/block-backend.h | 2 +- block.c | 3 ++- block/block-backend.c | 4 ++-- block/commit.c | 4 ++-- block/crypto.c | 2 +- block/mirror.c | 2 +- block/qcow2.c | 4 ++-- block/qed.c | 2 +- block/vdi.c | 2 +- block/vhdx.c | 4 ++-- block/vmdk.c | 6 +++--- block/vpc.c | 2 +- blockdev.c | 2 +- qemu-img.c | 2 +- qemu-io-cmds.c | 2 +- 15 files changed, 22 insertions(+), 21 deletions(-) diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -XXX,XX +XXX,XX @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, int bytes); int blk_truncate(BlockBackend *blk, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp); + PreallocMode prealloc, BdrvRequestFlags flags, Error **errp); int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes); int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf, int64_t pos, int size); diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ static int64_t create_file_fallback_truncate(BlockBackend *blk, int64_t size; int ret; - ret = blk_truncate(blk, minimum_size, false, PREALLOC_MODE_OFF, &local_err); + ret = blk_truncate(blk, minimum_size, false, PREALLOC_MODE_OFF, 0, + &local_err); if (ret < 0 && ret != -ENOTSUP) { error_propagate(errp, local_err); return ret; diff --git a/block/block-backend.c b/block/block-backend.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -XXX,XX +XXX,XX @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, } int blk_truncate(BlockBackend *blk, int64_t offset, bool exact, - PreallocMode prealloc, Error **errp) + PreallocMode prealloc, BdrvRequestFlags flags, Error **errp) { if (!blk_is_available(blk)) { error_setg(errp, "No medium inserted"); return -ENOMEDIUM; } - return bdrv_truncate(blk->root, offset, exact, prealloc, 0, errp); + return bdrv_truncate(blk->root, offset, exact, prealloc, flags, errp); } int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf, diff --git a/block/commit.c b/block/commit.c index XXXXXXX..XXXXXXX 100644 --- a/block/commit.c +++ b/block/commit.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_run(Job *job, Error **errp) } if (base_len < len) { - ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, NULL); + ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, 0, NULL); if (ret) { goto out; } @@ -XXX,XX +XXX,XX @@ int bdrv_commit(BlockDriverState *bs) * grow the backing file image if possible. If not possible, * we must return an error */ if (length > backing_length) { - ret = blk_truncate(backing, length, false, PREALLOC_MODE_OFF, + ret = blk_truncate(backing, length, false, PREALLOC_MODE_OFF, 0, &local_err); if (ret < 0) { error_report_err(local_err); diff --git a/block/crypto.c b/block/crypto.c index XXXXXXX..XXXXXXX 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -XXX,XX +XXX,XX @@ static ssize_t block_crypto_init_func(QCryptoBlock *block, * which will be used by the crypto header */ return blk_truncate(data->blk, data->size + headerlen, false, - data->prealloc, errp); + data->prealloc, 0, errp); } diff --git a/block/mirror.c b/block/mirror.c index XXXXXXX..XXXXXXX 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_run(Job *job, Error **errp) if (s->bdev_length > base_length) { ret = blk_truncate(s->target, s->bdev_length, false, - PREALLOC_MODE_OFF, NULL); + PREALLOC_MODE_OFF, 0, NULL); if (ret < 0) { goto immediate_exit; } diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) /* Okay, now that we have a valid image, let's give it the right size */ ret = blk_truncate(blk, qcow2_opts->size, false, qcow2_opts->preallocation, - errp); + 0, errp); if (ret < 0) { error_prepend(errp, "Could not resize image: "); goto out; @@ -XXX,XX +XXX,XX @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, * Amending image options should ensure that the image has * exactly the given new values, so pass exact=true here. */ - ret = blk_truncate(blk, new_size, true, PREALLOC_MODE_OFF, errp); + ret = blk_truncate(blk, new_size, true, PREALLOC_MODE_OFF, 0, errp); blk_unref(blk); if (ret < 0) { return ret; diff --git a/block/qed.c b/block/qed.c index XXXXXXX..XXXXXXX 100644 --- a/block/qed.c +++ b/block/qed.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts, * The QED format associates file length with allocation status, * so a new file (which is empty) must have a length of 0. */ - ret = blk_truncate(blk, 0, true, PREALLOC_MODE_OFF, errp); + ret = blk_truncate(blk, 0, true, PREALLOC_MODE_OFF, 0, errp); if (ret < 0) { goto out; } diff --git a/block/vdi.c b/block/vdi.c index XXXXXXX..XXXXXXX 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options, if (image_type == VDI_TYPE_STATIC) { ret = blk_truncate(blk, offset + blocks * block_size, false, - PREALLOC_MODE_OFF, errp); + PREALLOC_MODE_OFF, 0, errp); if (ret < 0) { error_prepend(errp, "Failed to statically allocate file"); goto exit; diff --git a/block/vhdx.c b/block/vhdx.c index XXXXXXX..XXXXXXX 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -XXX,XX +XXX,XX @@ static int vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s, /* All zeroes, so we can just extend the file - the end of the BAT * is the furthest thing we have written yet */ ret = blk_truncate(blk, data_file_offset, false, PREALLOC_MODE_OFF, - errp); + 0, errp); if (ret < 0) { goto exit; } } else if (type == VHDX_TYPE_FIXED) { ret = blk_truncate(blk, data_file_offset + image_size, false, - PREALLOC_MODE_OFF, errp); + PREALLOC_MODE_OFF, 0, errp); if (ret < 0) { goto exit; } diff --git a/block/vmdk.c b/block/vmdk.c index XXXXXXX..XXXXXXX 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -XXX,XX +XXX,XX @@ static int vmdk_init_extent(BlockBackend *blk, int gd_buf_size; if (flat) { - ret = blk_truncate(blk, filesize, false, PREALLOC_MODE_OFF, errp); + ret = blk_truncate(blk, filesize, false, PREALLOC_MODE_OFF, 0, errp); goto exit; } magic = cpu_to_be32(VMDK4_MAGIC); @@ -XXX,XX +XXX,XX @@ static int vmdk_init_extent(BlockBackend *blk, } ret = blk_truncate(blk, le64_to_cpu(header.grain_offset) << 9, false, - PREALLOC_MODE_OFF, errp); + PREALLOC_MODE_OFF, 0, errp); if (ret < 0) { goto exit; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn vmdk_co_do_create(int64_t size, /* bdrv_pwrite write padding zeros to align to sector, we don't need that * for description file */ if (desc_offset == 0) { - ret = blk_truncate(blk, desc_len, false, PREALLOC_MODE_OFF, errp); + ret = blk_truncate(blk, desc_len, false, PREALLOC_MODE_OFF, 0, errp); if (ret < 0) { goto exit; } diff --git a/block/vpc.c b/block/vpc.c index XXXXXXX..XXXXXXX 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -XXX,XX +XXX,XX @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf, /* Add footer to total size */ total_size += HEADER_SIZE; - ret = blk_truncate(blk, total_size, false, PREALLOC_MODE_OFF, errp); + ret = blk_truncate(blk, total_size, false, PREALLOC_MODE_OFF, 0, errp); if (ret < 0) { return ret; } diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ void qmp_block_resize(bool has_device, const char *device, } bdrv_drained_begin(bs); - ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp); + ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, 0, errp); bdrv_drained_end(bs); out: diff --git a/qemu-img.c b/qemu-img.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -XXX,XX +XXX,XX @@ static int img_resize(int argc, char **argv) * resizing, so pass @exact=true. It is of no use to report * success when the image has not actually been resized. */ - ret = blk_truncate(blk, total_size, true, prealloc, &err); + ret = blk_truncate(blk, total_size, true, prealloc, 0, &err); if (!ret) { qprintf(quiet, "Image resized.\n"); } else { diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -XXX,XX +XXX,XX @@ static int truncate_f(BlockBackend *blk, int argc, char **argv) * exact=true. It is better to err on the "emit more errors" side * than to be overly permissive. */ - ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, &local_err); + ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, 0, &local_err); if (ret < 0) { error_report_err(local_err); return ret; -- 2.25.3 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> --- block/qcow2-cluster.c | 2 +- block/qcow2.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -XXX,XX +XXX,XX @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset, /* Caller must pass aligned values, except at image end */ assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) || - end_offset == bs->total_sectors << BDRV_SECTOR_BITS); + end_offset >= bs->total_sectors << BDRV_SECTOR_BITS); /* The zero flag is only supported by version 3 and newer */ if (s->qcow_version < 3) { diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, bs->supported_zero_flags = header.version >= 3 ? BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0; + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; /* Repair image if dirty */ if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only && @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, g_assert_not_reached(); } + if ((flags & BDRV_REQ_ZERO_WRITE) && offset > old_length) { + uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size); + + /* + * Use zero clusters as much as we can. qcow2_cluster_zeroize() + * requires a cluster-aligned start. The end may be unaligned if it is + * at the end of the image (which it is here). + */ + ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to zero out new clusters"); + goto fail; + } + + /* Write explicit zeros for the unaligned head */ + if (zero_start > old_length) { + uint64_t len = zero_start - old_length; + uint8_t *buf = qemu_blockalign0(bs, len); + QEMUIOVector qiov; + qemu_iovec_init_buf(&qiov, buf, len); + + qemu_co_mutex_unlock(&s->lock); + ret = qcow2_co_pwritev_part(bs, old_length, len, &qiov, 0, 0); + qemu_co_mutex_lock(&s->lock); + + qemu_vfree(buf); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to zero out the new area"); + goto fail; + } + } + } + if (prealloc != PREALLOC_MODE_OFF) { /* Flush metadata before actually changing the image size */ ret = qcow2_write_caches(bs); -- 2.25.3 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> --- block/raw-format.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/raw-format.c b/block/raw-format.c index XXXXXXX..XXXXXXX 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset, s->size = offset; offset += s->offset; - return bdrv_co_truncate(bs->file, offset, exact, prealloc, 0, errp); + return bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp); } static void raw_eject(BlockDriverState *bs, bool eject_flag) @@ -XXX,XX +XXX,XX @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & bs->file->bs->supported_zero_flags); + bs->supported_truncate_flags = bs->file->bs->supported_truncate_flags & + BDRV_REQ_ZERO_WRITE; if (bs->probed && !bdrv_is_read_only(bs)) { bdrv_refresh_filename(bs->file->bs); -- 2.25.3 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> --- block/file-posix.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/block/file-posix.c b/block/file-posix.c index XXXXXXX..XXXXXXX 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options, #endif bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK; + if (S_ISREG(st.st_mode)) { + /* When extending regular files, we get zeros from the OS */ + bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; + } ret = 0; fail: if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) { -- 2.25.3 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> --- block/io.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/block/io.c b/block/io.c index XXXXXXX..XXXXXXX 100644 --- a/block/io.c +++ b/block/io.c @@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, goto out; } + /* + * If the image has a backing file that is large enough that it would + * provide data for the new area, we cannot leave it unallocated because + * then the backing file content would become visible. Instead, zero-fill + * the new area. + * + * Note that if the image has a backing file, but was opened without the + * backing file, taking care of keeping things consistent with that backing + * file is the user's responsibility. + */ + if (new_bytes && bs->backing) { + int64_t backing_len; + + backing_len = bdrv_getlength(backing_bs(bs)); + if (backing_len < 0) { + ret = backing_len; + error_setg_errno(errp, -ret, "Could not get backing file size"); + goto out; + } + + if (backing_len > old_size) { + flags |= BDRV_REQ_ZERO_WRITE; + } + } + if (drv->bdrv_co_truncate) { if (flags & ~bs->supported_truncate_flags) { error_setg(errp, "Block driver does not support requested flags"); -- 2.25.3 We want to keep TEST_IMG for the full path of the main test image, but filter_testfiles() must be called for other test images before replacing other things like the image format because the test directory path could contain the format as a substring. Insert a filter_testfiles() call between both. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20200424125448.63318-9-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/iotests.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -XXX,XX +XXX,XX @@ def filter_img_info(output, filename): for line in output.split('\n'): if 'disk size' in line or 'actual-size' in line: continue - line = line.replace(filename, 'TEST_IMG') \ - .replace(imgfmt, 'IMGFMT') + line = line.replace(filename, 'TEST_IMG') + line = filter_testfiles(line) + line = line.replace(imgfmt, 'IMGFMT') line = re.sub('iters: [0-9]+', 'iters: XXX', line) line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line) line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line) -- 2.25.3 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200424125448.63318-10-kwolf@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/274 | 155 +++++++++++++++++++++ tests/qemu-iotests/274.out | 268 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 424 insertions(+) create mode 100755 tests/qemu-iotests/274 create mode 100644 tests/qemu-iotests/274.out diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/274 @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2019 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 <http://www.gnu.org/licenses/>. +# +# Creator/Owner: Kevin Wolf <kwolf@redhat.com> +# +# Some tests for short backing files and short overlays + +import iotests + +iotests.verify_image_format(supported_fmts=['qcow2']) +iotests.verify_platform(['linux']) + +size_short = 1 * 1024 * 1024 +size_long = 2 * 1024 * 1024 +size_diff = size_long - size_short + +def create_chain() -> None: + iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, + str(size_long)) + iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid, + str(size_short)) + iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top, + str(size_long)) + + iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base) + +def create_vm() -> iotests.VM: + vm = iotests.VM() + vm.add_blockdev('file,filename=%s,node-name=base-file' % base) + vm.add_blockdev('%s,file=base-file,node-name=base' % iotests.imgfmt) + vm.add_blockdev('file,filename=%s,node-name=mid-file' % mid) + vm.add_blockdev('%s,file=mid-file,node-name=mid,backing=base' + % iotests.imgfmt) + vm.add_drive(top, 'backing=mid,node-name=top') + return vm + +with iotests.FilePath('base') as base, \ + iotests.FilePath('mid') as mid, \ + iotests.FilePath('top') as top: + + iotests.log('== Commit tests ==') + + create_chain() + + iotests.log('=== Check visible data ===') + + iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, top) + iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), top) + + iotests.log('=== Checking allocation status ===') + + iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short, + '-c', 'alloc %d %d' % (size_short, size_diff), + base) + + iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short, + '-c', 'alloc %d %d' % (size_short, size_diff), + mid) + + iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short, + '-c', 'alloc %d %d' % (size_short, size_diff), + top) + + iotests.log('=== Checking map ===') + + iotests.qemu_img_log('map', '--output=json', base) + iotests.qemu_img_log('map', '--output=human', base) + iotests.qemu_img_log('map', '--output=json', mid) + iotests.qemu_img_log('map', '--output=human', mid) + iotests.qemu_img_log('map', '--output=json', top) + iotests.qemu_img_log('map', '--output=human', top) + + iotests.log('=== Testing qemu-img commit (top -> mid) ===') + + iotests.qemu_img_log('commit', top) + iotests.img_info_log(mid) + iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid) + iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid) + + iotests.log('=== Testing HMP commit (top -> mid) ===') + + create_chain() + with create_vm() as vm: + vm.launch() + vm.qmp_log('human-monitor-command', command_line='commit drive0') + + iotests.img_info_log(mid) + iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid) + iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid) + + iotests.log('=== Testing QMP active commit (top -> mid) ===') + + create_chain() + with create_vm() as vm: + vm.launch() + vm.qmp_log('block-commit', device='top', base_node='mid', + job_id='job0', auto_dismiss=False) + vm.run_job('job0', wait=5) + + iotests.img_info_log(mid) + iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid) + iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid) + + + iotests.log('== Resize tests ==') + + # Use different sizes for different allocation modes: + # + # We want to have at least one test where 32 bit truncation in the size of + # the overlapping area becomes visible. This is covered by the + # prealloc='off' case (1G to 6G is an overlap of 5G). + # + # However, we can only do this for modes that don't preallocate data + # because otherwise we might run out of space on the test host. + # + # We also want to test some unaligned combinations. + for (prealloc, base_size, top_size_old, top_size_new, off) in [ + ('off', '6G', '1G', '8G', '5G'), + ('metadata', '32G', '30G', '33G', '31G'), + ('falloc', '10M', '5M', '15M', '9M'), + ('full', '16M', '8M', '12M', '11M'), + ('off', '384k', '253k', '512k', '253k'), + ('off', '400k', '256k', '512k', '336k'), + ('off', '512k', '256k', '500k', '436k')]: + + iotests.log('=== preallocation=%s ===' % prealloc) + iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, base_size) + iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, top, + top_size_old) + iotests.qemu_io_log('-c', 'write -P 1 %s 64k' % off, base) + + # After this, top_size_old to base_size should be allocated/zeroed. + # + # In theory, leaving base_size to top_size_new unallocated would be + # correct, but in practice, if we zero out anything, we zero out + # everything up to top_size_new. + iotests.qemu_img_log('resize', '-f', iotests.imgfmt, + '--preallocation', prealloc, top, top_size_new) + iotests.qemu_io_log('-c', 'read -P 0 %s 64k' % off, top) + iotests.qemu_io_log('-c', 'map', top) + iotests.qemu_img_log('map', '--output=json', top) diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/274.out @@ -XXX,XX +XXX,XX @@ +== Commit tests == +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 2097152/2097152 bytes at offset 0 +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +=== Check visible data === +read 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +read 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +=== Checking allocation status === +1048576/1048576 bytes allocated at offset 0 bytes +1048576/1048576 bytes allocated at offset 1 MiB + +0/1048576 bytes allocated at offset 0 bytes +0/0 bytes allocated at offset 1 MiB + +0/1048576 bytes allocated at offset 0 bytes +0/1048576 bytes allocated at offset 1 MiB + +=== Checking map === +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 327680}] + +Offset Length Mapped to File +0 0x200000 0x50000 TEST_DIR/PID-base + +[{ "start": 0, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680}] + +Offset Length Mapped to File +0 0x100000 0x50000 TEST_DIR/PID-base + +[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true, "offset": 327680}, +{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": false}] + +Offset Length Mapped to File +0 0x100000 0x50000 TEST_DIR/PID-base + +=== Testing qemu-img commit (top -> mid) === +Image committed. + +image: TEST_IMG +file format: IMGFMT +virtual size: 2 MiB (2097152 bytes) +cluster_size: 65536 +backing file: TEST_DIR/PID-base +Format specific information: + compat: 1.1 + lazy refcounts: false + refcount bits: 16 + corrupt: false + +read 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +read 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +=== Testing HMP commit (top -> mid) === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 2097152/2097152 bytes at offset 0 +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +{"execute": "human-monitor-command", "arguments": {"command-line": "commit drive0"}} +{"return": ""} +image: TEST_IMG +file format: IMGFMT +virtual size: 2 MiB (2097152 bytes) +cluster_size: 65536 +backing file: TEST_DIR/PID-base +Format specific information: + compat: 1.1 + lazy refcounts: false + refcount bits: 16 + corrupt: false + +read 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +read 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +=== Testing QMP active commit (top -> mid) === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 2097152/2097152 bytes at offset 0 +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +{"execute": "block-commit", "arguments": {"auto-dismiss": false, "base-node": "mid", "device": "top", "job-id": "job0"}} +{"return": {}} +{"execute": "job-complete", "arguments": {"id": "job0"}} +{"return": {}} +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"execute": "job-dismiss", "arguments": {"id": "job0"}} +{"return": {}} +image: TEST_IMG +file format: IMGFMT +virtual size: 2 MiB (2097152 bytes) +cluster_size: 65536 +backing file: TEST_DIR/PID-base +Format specific information: + compat: 1.1 + lazy refcounts: false + refcount bits: 16 + corrupt: false + +read 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +read 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== Resize tests == +=== preallocation=off === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=6442450944 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=1073741824 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 65536/65536 bytes at offset 5368709120 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Image resized. + +read 65536/65536 bytes at offset 5368709120 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +1 GiB (0x40000000) bytes not allocated at offset 0 bytes (0x0) +7 GiB (0x1c0000000) bytes allocated at offset 1 GiB (0x40000000) + +[{ "start": 0, "length": 1073741824, "depth": 1, "zero": true, "data": false}, +{ "start": 1073741824, "length": 7516192768, "depth": 0, "zero": true, "data": false}] + +=== preallocation=metadata === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=34359738368 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=32212254720 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 65536/65536 bytes at offset 33285996544 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Image resized. + +read 65536/65536 bytes at offset 33285996544 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +30 GiB (0x780000000) bytes not allocated at offset 0 bytes (0x0) +3 GiB (0xc0000000) bytes allocated at offset 30 GiB (0x780000000) + +[{ "start": 0, "length": 32212254720, "depth": 1, "zero": true, "data": false}, +{ "start": 32212254720, "length": 536870912, "depth": 0, "zero": true, "data": false, "offset": 327680}, +{ "start": 32749125632, "length": 536870912, "depth": 0, "zero": true, "data": false, "offset": 537264128}, +{ "start": 33285996544, "length": 536870912, "depth": 0, "zero": true, "data": false, "offset": 1074200576}, +{ "start": 33822867456, "length": 536870912, "depth": 0, "zero": true, "data": false, "offset": 1611137024}, +{ "start": 34359738368, "length": 536870912, "depth": 0, "zero": true, "data": false, "offset": 2148139008}, +{ "start": 34896609280, "length": 536870912, "depth": 0, "zero": true, "data": false, "offset": 2685075456}] + +=== preallocation=falloc === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=10485760 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=5242880 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 65536/65536 bytes at offset 9437184 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Image resized. + +read 65536/65536 bytes at offset 9437184 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +5 MiB (0x500000) bytes not allocated at offset 0 bytes (0x0) +10 MiB (0xa00000) bytes allocated at offset 5 MiB (0x500000) + +[{ "start": 0, "length": 5242880, "depth": 1, "zero": true, "data": false}, +{ "start": 5242880, "length": 10485760, "depth": 0, "zero": true, "data": false, "offset": 327680}] + +=== preallocation=full === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=16777216 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=8388608 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 65536/65536 bytes at offset 11534336 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Image resized. + +read 65536/65536 bytes at offset 11534336 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +8 MiB (0x800000) bytes not allocated at offset 0 bytes (0x0) +4 MiB (0x400000) bytes allocated at offset 8 MiB (0x800000) + +[{ "start": 0, "length": 8388608, "depth": 1, "zero": true, "data": false}, +{ "start": 8388608, "length": 4194304, "depth": 0, "zero": true, "data": false, "offset": 327680}] + +=== preallocation=off === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=393216 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=259072 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 65536/65536 bytes at offset 259072 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Image resized. + +read 65536/65536 bytes at offset 259072 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +192 KiB (0x30000) bytes not allocated at offset 0 bytes (0x0) +320 KiB (0x50000) bytes allocated at offset 192 KiB (0x30000) + +[{ "start": 0, "length": 196608, "depth": 1, "zero": true, "data": false}, +{ "start": 196608, "length": 65536, "depth": 0, "zero": false, "data": true, "offset": 327680}, +{ "start": 262144, "length": 262144, "depth": 0, "zero": true, "data": false}] + +=== preallocation=off === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=409600 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=262144 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 65536/65536 bytes at offset 344064 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Image resized. + +read 65536/65536 bytes at offset 344064 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +256 KiB (0x40000) bytes not allocated at offset 0 bytes (0x0) +256 KiB (0x40000) bytes allocated at offset 256 KiB (0x40000) + +[{ "start": 0, "length": 262144, "depth": 1, "zero": true, "data": false}, +{ "start": 262144, "length": 262144, "depth": 0, "zero": true, "data": false}] + +=== preallocation=off === +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=524288 cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=262144 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16 + +wrote 65536/65536 bytes at offset 446464 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +Image resized. + +read 65536/65536 bytes at offset 446464 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +256 KiB (0x40000) bytes not allocated at offset 0 bytes (0x0) +244 KiB (0x3d000) bytes allocated at offset 256 KiB (0x40000) + +[{ "start": 0, "length": 262144, "depth": 1, "zero": true, "data": false}, +{ "start": 262144, "length": 249856, "depth": 0, "zero": true, "data": false}] + diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -XXX,XX +XXX,XX @@ 270 rw backing quick 272 rw 273 backing quick +274 rw backing 277 rw quick 279 rw backing quick 280 rw migration quick -- 2.25.3 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> --- block/qcow2.c | 22 +++++++++++++++++++--- tests/qemu-iotests/274.out | 4 ++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, /* Allocate the data area */ new_file_size = allocation_start + nb_new_data_clusters * s->cluster_size; - /* Image file grows, so @exact does not matter */ - ret = bdrv_co_truncate(bs->file, new_file_size, false, prealloc, 0, - errp); + /* + * Image file grows, so @exact does not matter. + * + * If we need to zero out the new area, try first whether the protocol + * driver can already take care of this. + */ + if (flags & BDRV_REQ_ZERO_WRITE) { + ret = bdrv_co_truncate(bs->file, new_file_size, false, prealloc, + BDRV_REQ_ZERO_WRITE, NULL); + if (ret >= 0) { + flags &= ~BDRV_REQ_ZERO_WRITE; + } + } else { + ret = -1; + } + if (ret < 0) { + ret = bdrv_co_truncate(bs->file, new_file_size, false, prealloc, 0, + errp); + } if (ret < 0) { error_prepend(errp, "Failed to resize underlying file: "); qcow2_free_clusters(bs, allocation_start, diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/274.out +++ b/tests/qemu-iotests/274.out @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 9437184 10 MiB (0xa00000) bytes allocated at offset 5 MiB (0x500000) [{ "start": 0, "length": 5242880, "depth": 1, "zero": true, "data": false}, -{ "start": 5242880, "length": 10485760, "depth": 0, "zero": true, "data": false, "offset": 327680}] +{ "start": 5242880, "length": 10485760, "depth": 0, "zero": false, "data": true, "offset": 327680}] === preallocation=full === Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=16777216 cluster_size=65536 lazy_refcounts=off refcount_bits=16 @@ -XXX,XX +XXX,XX @@ read 65536/65536 bytes at offset 11534336 4 MiB (0x400000) bytes allocated at offset 8 MiB (0x800000) [{ "start": 0, "length": 8388608, "depth": 1, "zero": true, "data": false}, -{ "start": 8388608, "length": 4194304, "depth": 0, "zero": true, "data": false, "offset": 327680}] +{ "start": 8388608, "length": 4194304, "depth": 0, "zero": false, "data": true, "offset": 327680}] === preallocation=off === Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=393216 cluster_size=65536 lazy_refcounts=off refcount_bits=16 -- 2.25.3 From: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> This patch introduces support for PMR that has been defined as part of NVMe 1.4 spec. User can now specify a pmrdev option that should point to HostMemoryBackend. pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated NVMe device. Guest OS can perform mmio read and writes to the PMR region that will stay persistent across system reboot. Signed-off-by: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20200330164656.9348-1-andrzej.jakowski@linux.intel.com> Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/block/nvme.h | 2 + include/block/nvme.h | 172 +++++++++++++++++++++++++++++++++++++++++ hw/block/nvme.c | 109 ++++++++++++++++++++++++++ hw/block/Makefile.objs | 2 +- hw/block/trace-events | 4 + 5 files changed, 288 insertions(+), 1 deletion(-) diff --git a/hw/block/nvme.h b/hw/block/nvme.h index XXXXXXX..XXXXXXX 100644 --- a/hw/block/nvme.h +++ b/hw/block/nvme.h @@ -XXX,XX +XXX,XX @@ typedef struct NvmeCtrl { uint64_t timestamp_set_qemu_clock_ms; /* QEMU clock time */ char *serial; + HostMemoryBackend *pmrdev; + NvmeNamespace *namespaces; NvmeSQueue **sq; NvmeCQueue **cq; diff --git a/include/block/nvme.h b/include/block/nvme.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/nvme.h +++ b/include/block/nvme.h @@ -XXX,XX +XXX,XX @@ typedef struct NvmeBar { uint64_t acq; uint32_t cmbloc; uint32_t cmbsz; + uint8_t padding[3520]; /* not used by QEMU */ + uint32_t pmrcap; + uint32_t pmrctl; + uint32_t pmrsts; + uint32_t pmrebs; + uint32_t pmrswtp; + uint32_t pmrmsc; } NvmeBar; enum NvmeCapShift { @@ -XXX,XX +XXX,XX @@ enum NvmeCapShift { CAP_CSS_SHIFT = 37, CAP_MPSMIN_SHIFT = 48, CAP_MPSMAX_SHIFT = 52, + CAP_PMR_SHIFT = 56, }; enum NvmeCapMask { @@ -XXX,XX +XXX,XX @@ enum NvmeCapMask { CAP_CSS_MASK = 0xff, CAP_MPSMIN_MASK = 0xf, CAP_MPSMAX_MASK = 0xf, + CAP_PMR_MASK = 0x1, }; #define NVME_CAP_MQES(cap) (((cap) >> CAP_MQES_SHIFT) & CAP_MQES_MASK) @@ -XXX,XX +XXX,XX @@ enum NvmeCapMask { << CAP_MPSMIN_SHIFT) #define NVME_CAP_SET_MPSMAX(cap, val) (cap |= (uint64_t)(val & CAP_MPSMAX_MASK)\ << CAP_MPSMAX_SHIFT) +#define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)\ + << CAP_PMR_SHIFT) enum NvmeCcShift { CC_EN_SHIFT = 0, @@ -XXX,XX +XXX,XX @@ enum NvmeCmbszMask { #define NVME_CMBSZ_GETSIZE(cmbsz) \ (NVME_CMBSZ_SZ(cmbsz) * (1 << (12 + 4 * NVME_CMBSZ_SZU(cmbsz)))) +enum NvmePmrcapShift { + PMRCAP_RDS_SHIFT = 3, + PMRCAP_WDS_SHIFT = 4, + PMRCAP_BIR_SHIFT = 5, + PMRCAP_PMRTU_SHIFT = 8, + PMRCAP_PMRWBM_SHIFT = 10, + PMRCAP_PMRTO_SHIFT = 16, + PMRCAP_CMSS_SHIFT = 24, +}; + +enum NvmePmrcapMask { + PMRCAP_RDS_MASK = 0x1, + PMRCAP_WDS_MASK = 0x1, + PMRCAP_BIR_MASK = 0x7, + PMRCAP_PMRTU_MASK = 0x3, + PMRCAP_PMRWBM_MASK = 0xf, + PMRCAP_PMRTO_MASK = 0xff, + PMRCAP_CMSS_MASK = 0x1, +}; + +#define NVME_PMRCAP_RDS(pmrcap) \ + ((pmrcap >> PMRCAP_RDS_SHIFT) & PMRCAP_RDS_MASK) +#define NVME_PMRCAP_WDS(pmrcap) \ + ((pmrcap >> PMRCAP_WDS_SHIFT) & PMRCAP_WDS_MASK) +#define NVME_PMRCAP_BIR(pmrcap) \ + ((pmrcap >> PMRCAP_BIR_SHIFT) & PMRCAP_BIR_MASK) +#define NVME_PMRCAP_PMRTU(pmrcap) \ + ((pmrcap >> PMRCAP_PMRTU_SHIFT) & PMRCAP_PMRTU_MASK) +#define NVME_PMRCAP_PMRWBM(pmrcap) \ + ((pmrcap >> PMRCAP_PMRWBM_SHIFT) & PMRCAP_PMRWBM_MASK) +#define NVME_PMRCAP_PMRTO(pmrcap) \ + ((pmrcap >> PMRCAP_PMRTO_SHIFT) & PMRCAP_PMRTO_MASK) +#define NVME_PMRCAP_CMSS(pmrcap) \ + ((pmrcap >> PMRCAP_CMSS_SHIFT) & PMRCAP_CMSS_MASK) + +#define NVME_PMRCAP_SET_RDS(pmrcap, val) \ + (pmrcap |= (uint64_t)(val & PMRCAP_RDS_MASK) << PMRCAP_RDS_SHIFT) +#define NVME_PMRCAP_SET_WDS(pmrcap, val) \ + (pmrcap |= (uint64_t)(val & PMRCAP_WDS_MASK) << PMRCAP_WDS_SHIFT) +#define NVME_PMRCAP_SET_BIR(pmrcap, val) \ + (pmrcap |= (uint64_t)(val & PMRCAP_BIR_MASK) << PMRCAP_BIR_SHIFT) +#define NVME_PMRCAP_SET_PMRTU(pmrcap, val) \ + (pmrcap |= (uint64_t)(val & PMRCAP_PMRTU_MASK) << PMRCAP_PMRTU_SHIFT) +#define NVME_PMRCAP_SET_PMRWBM(pmrcap, val) \ + (pmrcap |= (uint64_t)(val & PMRCAP_PMRWBM_MASK) << PMRCAP_PMRWBM_SHIFT) +#define NVME_PMRCAP_SET_PMRTO(pmrcap, val) \ + (pmrcap |= (uint64_t)(val & PMRCAP_PMRTO_MASK) << PMRCAP_PMRTO_SHIFT) +#define NVME_PMRCAP_SET_CMSS(pmrcap, val) \ + (pmrcap |= (uint64_t)(val & PMRCAP_CMSS_MASK) << PMRCAP_CMSS_SHIFT) + +enum NvmePmrctlShift { + PMRCTL_EN_SHIFT = 0, +}; + +enum NvmePmrctlMask { + PMRCTL_EN_MASK = 0x1, +}; + +#define NVME_PMRCTL_EN(pmrctl) ((pmrctl >> PMRCTL_EN_SHIFT) & PMRCTL_EN_MASK) + +#define NVME_PMRCTL_SET_EN(pmrctl, val) \ + (pmrctl |= (uint64_t)(val & PMRCTL_EN_MASK) << PMRCTL_EN_SHIFT) + +enum NvmePmrstsShift { + PMRSTS_ERR_SHIFT = 0, + PMRSTS_NRDY_SHIFT = 8, + PMRSTS_HSTS_SHIFT = 9, + PMRSTS_CBAI_SHIFT = 12, +}; + +enum NvmePmrstsMask { + PMRSTS_ERR_MASK = 0xff, + PMRSTS_NRDY_MASK = 0x1, + PMRSTS_HSTS_MASK = 0x7, + PMRSTS_CBAI_MASK = 0x1, +}; + +#define NVME_PMRSTS_ERR(pmrsts) \ + ((pmrsts >> PMRSTS_ERR_SHIFT) & PMRSTS_ERR_MASK) +#define NVME_PMRSTS_NRDY(pmrsts) \ + ((pmrsts >> PMRSTS_NRDY_SHIFT) & PMRSTS_NRDY_MASK) +#define NVME_PMRSTS_HSTS(pmrsts) \ + ((pmrsts >> PMRSTS_HSTS_SHIFT) & PMRSTS_HSTS_MASK) +#define NVME_PMRSTS_CBAI(pmrsts) \ + ((pmrsts >> PMRSTS_CBAI_SHIFT) & PMRSTS_CBAI_MASK) + +#define NVME_PMRSTS_SET_ERR(pmrsts, val) \ + (pmrsts |= (uint64_t)(val & PMRSTS_ERR_MASK) << PMRSTS_ERR_SHIFT) +#define NVME_PMRSTS_SET_NRDY(pmrsts, val) \ + (pmrsts |= (uint64_t)(val & PMRSTS_NRDY_MASK) << PMRSTS_NRDY_SHIFT) +#define NVME_PMRSTS_SET_HSTS(pmrsts, val) \ + (pmrsts |= (uint64_t)(val & PMRSTS_HSTS_MASK) << PMRSTS_HSTS_SHIFT) +#define NVME_PMRSTS_SET_CBAI(pmrsts, val) \ + (pmrsts |= (uint64_t)(val & PMRSTS_CBAI_MASK) << PMRSTS_CBAI_SHIFT) + +enum NvmePmrebsShift { + PMREBS_PMRSZU_SHIFT = 0, + PMREBS_RBB_SHIFT = 4, + PMREBS_PMRWBZ_SHIFT = 8, +}; + +enum NvmePmrebsMask { + PMREBS_PMRSZU_MASK = 0xf, + PMREBS_RBB_MASK = 0x1, + PMREBS_PMRWBZ_MASK = 0xffffff, +}; + +#define NVME_PMREBS_PMRSZU(pmrebs) \ + ((pmrebs >> PMREBS_PMRSZU_SHIFT) & PMREBS_PMRSZU_MASK) +#define NVME_PMREBS_RBB(pmrebs) \ + ((pmrebs >> PMREBS_RBB_SHIFT) & PMREBS_RBB_MASK) +#define NVME_PMREBS_PMRWBZ(pmrebs) \ + ((pmrebs >> PMREBS_PMRWBZ_SHIFT) & PMREBS_PMRWBZ_MASK) + +#define NVME_PMREBS_SET_PMRSZU(pmrebs, val) \ + (pmrebs |= (uint64_t)(val & PMREBS_PMRSZU_MASK) << PMREBS_PMRSZU_SHIFT) +#define NVME_PMREBS_SET_RBB(pmrebs, val) \ + (pmrebs |= (uint64_t)(val & PMREBS_RBB_MASK) << PMREBS_RBB_SHIFT) +#define NVME_PMREBS_SET_PMRWBZ(pmrebs, val) \ + (pmrebs |= (uint64_t)(val & PMREBS_PMRWBZ_MASK) << PMREBS_PMRWBZ_SHIFT) + +enum NvmePmrswtpShift { + PMRSWTP_PMRSWTU_SHIFT = 0, + PMRSWTP_PMRSWTV_SHIFT = 8, +}; + +enum NvmePmrswtpMask { + PMRSWTP_PMRSWTU_MASK = 0xf, + PMRSWTP_PMRSWTV_MASK = 0xffffff, +}; + +#define NVME_PMRSWTP_PMRSWTU(pmrswtp) \ + ((pmrswtp >> PMRSWTP_PMRSWTU_SHIFT) & PMRSWTP_PMRSWTU_MASK) +#define NVME_PMRSWTP_PMRSWTV(pmrswtp) \ + ((pmrswtp >> PMRSWTP_PMRSWTV_SHIFT) & PMRSWTP_PMRSWTV_MASK) + +#define NVME_PMRSWTP_SET_PMRSWTU(pmrswtp, val) \ + (pmrswtp |= (uint64_t)(val & PMRSWTP_PMRSWTU_MASK) << PMRSWTP_PMRSWTU_SHIFT) +#define NVME_PMRSWTP_SET_PMRSWTV(pmrswtp, val) \ + (pmrswtp |= (uint64_t)(val & PMRSWTP_PMRSWTV_MASK) << PMRSWTP_PMRSWTV_SHIFT) + +enum NvmePmrmscShift { + PMRMSC_CMSE_SHIFT = 1, + PMRMSC_CBA_SHIFT = 12, +}; + +enum NvmePmrmscMask { + PMRMSC_CMSE_MASK = 0x1, + PMRMSC_CBA_MASK = 0xfffffffffffff, +}; + +#define NVME_PMRMSC_CMSE(pmrmsc) \ + ((pmrmsc >> PMRMSC_CMSE_SHIFT) & PMRMSC_CMSE_MASK) +#define NVME_PMRMSC_CBA(pmrmsc) \ + ((pmrmsc >> PMRMSC_CBA_SHIFT) & PMRMSC_CBA_MASK) + +#define NVME_PMRMSC_SET_CMSE(pmrmsc, val) \ + (pmrmsc |= (uint64_t)(val & PMRMSC_CMSE_MASK) << PMRMSC_CMSE_SHIFT) +#define NVME_PMRMSC_SET_CBA(pmrmsc, val) \ + (pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT) + typedef struct NvmeCmd { uint8_t opcode; uint8_t fuse; diff --git a/hw/block/nvme.c b/hw/block/nvme.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -XXX,XX +XXX,XX @@ * -drive file=<file>,if=none,id=<drive_id> * -device nvme,drive=<drive_id>,serial=<serial>,id=<id[optional]>, \ * cmb_size_mb=<cmb_size_mb[optional]>, \ + * [pmrdev=<mem_backend_file_id>,] \ * num_queues=<N[optional]> * * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at * offset 0 in BAR2 and supports only WDS, RDS and SQS for now. + * + * cmb_size_mb= and pmrdev= options are mutually exclusive due to limitation + * in available BAR's. cmb_size_mb= will take precedence over pmrdev= when + * both provided. + * Enabling pmr emulation can be achieved by pointing to memory-backend-file. + * For example: + * -object memory-backend-file,id=<mem_id>,share=on,mem-path=<file_path>, \ + * size=<size> .... -device nvme,...,pmrdev=<mem_id> */ #include "qemu/osdep.h" @@ -XXX,XX +XXX,XX @@ #include "sysemu/sysemu.h" #include "qapi/error.h" #include "qapi/visitor.h" +#include "sysemu/hostmem.h" #include "sysemu/block-backend.h" +#include "exec/ram_addr.h" #include "qemu/log.h" #include "qemu/module.h" @@ -XXX,XX +XXX,XX @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, NVME_GUEST_ERR(nvme_ub_mmiowr_cmbsz_readonly, "invalid write to read only CMBSZ, ignored"); return; + case 0xE00: /* PMRCAP */ + NVME_GUEST_ERR(nvme_ub_mmiowr_pmrcap_readonly, + "invalid write to PMRCAP register, ignored"); + return; + case 0xE04: /* TODO PMRCTL */ + break; + case 0xE08: /* PMRSTS */ + NVME_GUEST_ERR(nvme_ub_mmiowr_pmrsts_readonly, + "invalid write to PMRSTS register, ignored"); + return; + case 0xE0C: /* PMREBS */ + NVME_GUEST_ERR(nvme_ub_mmiowr_pmrebs_readonly, + "invalid write to PMREBS register, ignored"); + return; + case 0xE10: /* PMRSWTP */ + NVME_GUEST_ERR(nvme_ub_mmiowr_pmrswtp_readonly, + "invalid write to PMRSWTP register, ignored"); + return; + case 0xE14: /* TODO PMRMSC */ + break; default: NVME_GUEST_ERR(nvme_ub_mmiowr_invalid, "invalid MMIO write," @@ -XXX,XX +XXX,XX @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size) } if (addr < sizeof(n->bar)) { + /* + * When PMRWBM bit 1 is set then read from + * from PMRSTS should ensure prior writes + * made it to persistent media + */ + if (addr == 0xE08 && + (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) { + qemu_ram_writeback(n->pmrdev->mr.ram_block, + 0, n->pmrdev->size); + } memcpy(&val, ptr + addr, size); } else { NVME_GUEST_ERR(nvme_ub_mmiord_invalid_ofs, @@ -XXX,XX +XXX,XX @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) error_setg(errp, "serial property not set"); return; } + + if (!n->cmb_size_mb && n->pmrdev) { + if (host_memory_backend_is_mapped(n->pmrdev)) { + char *path = object_get_canonical_path_component(OBJECT(n->pmrdev)); + error_setg(errp, "can't use already busy memdev: %s", path); + g_free(path); + return; + } + + if (!is_power_of_2(n->pmrdev->size)) { + error_setg(errp, "pmr backend size needs to be power of 2 in size"); + return; + } + + host_memory_backend_set_mapped(n->pmrdev, true); + } + blkconf_blocksizes(&n->conf); if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk), false, errp)) { @@ -XXX,XX +XXX,XX @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64 | PCI_BASE_ADDRESS_MEM_PREFETCH, &n->ctrl_mem); + } else if (n->pmrdev) { + /* Controller Capabilities register */ + NVME_CAP_SET_PMRS(n->bar.cap, 1); + + /* PMR Capabities register */ + n->bar.pmrcap = 0; + NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 0); + NVME_PMRCAP_SET_WDS(n->bar.pmrcap, 0); + NVME_PMRCAP_SET_BIR(n->bar.pmrcap, 2); + NVME_PMRCAP_SET_PMRTU(n->bar.pmrcap, 0); + /* Turn on bit 1 support */ + NVME_PMRCAP_SET_PMRWBM(n->bar.pmrcap, 0x02); + NVME_PMRCAP_SET_PMRTO(n->bar.pmrcap, 0); + NVME_PMRCAP_SET_CMSS(n->bar.pmrcap, 0); + + /* PMR Control register */ + n->bar.pmrctl = 0; + NVME_PMRCTL_SET_EN(n->bar.pmrctl, 0); + + /* PMR Status register */ + n->bar.pmrsts = 0; + NVME_PMRSTS_SET_ERR(n->bar.pmrsts, 0); + NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 0); + NVME_PMRSTS_SET_HSTS(n->bar.pmrsts, 0); + NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 0); + + /* PMR Elasticity Buffer Size register */ + n->bar.pmrebs = 0; + NVME_PMREBS_SET_PMRSZU(n->bar.pmrebs, 0); + NVME_PMREBS_SET_RBB(n->bar.pmrebs, 0); + NVME_PMREBS_SET_PMRWBZ(n->bar.pmrebs, 0); + + /* PMR Sustained Write Throughput register */ + n->bar.pmrswtp = 0; + NVME_PMRSWTP_SET_PMRSWTU(n->bar.pmrswtp, 0); + NVME_PMRSWTP_SET_PMRSWTV(n->bar.pmrswtp, 0); + + /* PMR Memory Space Control register */ + n->bar.pmrmsc = 0; + NVME_PMRMSC_SET_CMSE(n->bar.pmrmsc, 0); + NVME_PMRMSC_SET_CBA(n->bar.pmrmsc, 0); + + pci_register_bar(pci_dev, NVME_PMRCAP_BIR(n->bar.pmrcap), + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64 | + PCI_BASE_ADDRESS_MEM_PREFETCH, &n->pmrdev->mr); } for (i = 0; i < n->num_namespaces; i++) { @@ -XXX,XX +XXX,XX @@ static void nvme_exit(PCIDevice *pci_dev) if (n->cmb_size_mb) { g_free(n->cmbuf); } + + if (n->pmrdev) { + host_memory_backend_set_mapped(n->pmrdev, false); + } msix_uninit_exclusive_bar(pci_dev); } static Property nvme_props[] = { DEFINE_BLOCK_PROPERTIES(NvmeCtrl, conf), + DEFINE_PROP_LINK("pmrdev", NvmeCtrl, pmrdev, TYPE_MEMORY_BACKEND, + HostMemoryBackend *), DEFINE_PROP_STRING("serial", NvmeCtrl, serial), DEFINE_PROP_UINT32("cmb_size_mb", NvmeCtrl, cmb_size_mb, 0), DEFINE_PROP_UINT32("num_queues", NvmeCtrl, num_queues, 64), diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs index XXXXXXX..XXXXXXX 100644 --- a/hw/block/Makefile.objs +++ b/hw/block/Makefile.objs @@ -XXX,XX +XXX,XX @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o common-obj-$(CONFIG_XEN) += xen-block.o common-obj-$(CONFIG_ECC) += ecc.o common-obj-$(CONFIG_ONENAND) += onenand.o -common-obj-$(CONFIG_NVME_PCI) += nvme.o common-obj-$(CONFIG_SWIM) += swim.o common-obj-$(CONFIG_SH4) += tc58128.o obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o +obj-$(CONFIG_NVME_PCI) += nvme.o obj-y += dataplane/ diff --git a/hw/block/trace-events b/hw/block/trace-events index XXXXXXX..XXXXXXX 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -XXX,XX +XXX,XX @@ nvme_ub_mmiowr_ssreset_w1c_unsupported(void) "attempted to W1C CSTS.NSSRO but CA nvme_ub_mmiowr_ssreset_unsupported(void) "attempted NVM subsystem reset but CAP.NSSRS is zero (not supported)" nvme_ub_mmiowr_cmbloc_reserved(void) "invalid write to reserved CMBLOC when CMBSZ is zero, ignored" nvme_ub_mmiowr_cmbsz_readonly(void) "invalid write to read only CMBSZ, ignored" +nvme_ub_mmiowr_pmrcap_readonly(void) "invalid write to read only PMRCAP, ignored" +nvme_ub_mmiowr_pmrsts_readonly(void) "invalid write to read only PMRSTS, ignored" +nvme_ub_mmiowr_pmrebs_readonly(void) "invalid write to read only PMREBS, ignored" +nvme_ub_mmiowr_pmrswtp_readonly(void) "invalid write to read only PMRSWTP, ignored" nvme_ub_mmiowr_invalid(uint64_t offset, uint64_t data) "invalid MMIO write, offset=0x%"PRIx64", data=0x%"PRIx64"" nvme_ub_mmiord_misaligned32(uint64_t offset) "MMIO read not 32-bit aligned, offset=0x%"PRIx64"" nvme_ub_mmiord_toosmall(uint64_t offset) "MMIO read smaller than 32-bits, offset=0x%"PRIx64"" -- 2.25.3 The QMP handler qmp_object_add() and the implementation of --object in qemu-storage-daemon can share most of the code. Currently, qemu-storage-daemon calls qmp_object_add(), but this is not correct because different visitors need to be used. As a first step towards a fix, make qmp_object_add() a wrapper around a new function user_creatable_add_dict() that can get an additional parameter. The handling of "props" is only required for compatibility and not required for the qemu-storage-daemon command line, so it stays in qmp_object_add(). Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qom/object_interfaces.h | 12 ++++++++++++ qom/object_interfaces.c | 27 +++++++++++++++++++++++++++ qom/qom-qmp-cmds.c | 24 +----------------------- 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h index XXXXXXX..XXXXXXX 100644 --- a/include/qom/object_interfaces.h +++ b/include/qom/object_interfaces.h @@ -XXX,XX +XXX,XX @@ Object *user_creatable_add_type(const char *type, const char *id, const QDict *qdict, Visitor *v, Error **errp); +/** + * user_creatable_add_dict: + * @qdict: the object definition + * @errp: if an error occurs, a pointer to an area to store the error + * + * Create an instance of the user creatable object that is defined by + * @qdict. The object type is taken from the QDict key 'qom-type', its + * ID from the key 'id'. The remaining entries in @qdict are used to + * initialize the object properties. + */ +void user_creatable_add_dict(QDict *qdict, Error **errp); + /** * user_creatable_add_opts: * @opts: the object definition diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index XXXXXXX..XXXXXXX 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -XXX,XX +XXX,XX @@ #include "qapi/qmp/qerror.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/qstring.h" +#include "qapi/qobject-input-visitor.h" #include "qom/object_interfaces.h" #include "qemu/help_option.h" #include "qemu/module.h" @@ -XXX,XX +XXX,XX @@ out: return obj; } +void user_creatable_add_dict(QDict *qdict, Error **errp) +{ + Visitor *v; + Object *obj; + g_autofree char *type = NULL; + g_autofree char *id = NULL; + + type = g_strdup(qdict_get_try_str(qdict, "qom-type")); + if (!type) { + error_setg(errp, QERR_MISSING_PARAMETER, "qom-type"); + return; + } + qdict_del(qdict, "qom-type"); + + id = g_strdup(qdict_get_try_str(qdict, "id")); + if (!id) { + error_setg(errp, QERR_MISSING_PARAMETER, "id"); + return; + } + qdict_del(qdict, "id"); + + v = qobject_input_visitor_new(QOBJECT(qdict)); + obj = user_creatable_add_type(type, id, qdict, v, errp); + visit_free(v); + object_unref(obj); +} Object *user_creatable_add_opts(QemuOpts *opts, Error **errp) { diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/qom/qom-qmp-cmds.c +++ b/qom/qom-qmp-cmds.c @@ -XXX,XX +XXX,XX @@ #include "qapi/qapi-commands-qom.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" -#include "qapi/qobject-input-visitor.h" #include "qemu/cutils.h" #include "qom/object_interfaces.h" #include "qom/qom-qobject.h" @@ -XXX,XX +XXX,XX @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp) { QObject *props; QDict *pdict; - Visitor *v; - Object *obj; - g_autofree char *type = NULL; - g_autofree char *id = NULL; - - type = g_strdup(qdict_get_try_str(qdict, "qom-type")); - if (!type) { - error_setg(errp, QERR_MISSING_PARAMETER, "qom-type"); - return; - } - qdict_del(qdict, "qom-type"); - - id = g_strdup(qdict_get_try_str(qdict, "id")); - if (!id) { - error_setg(errp, QERR_MISSING_PARAMETER, "id"); - return; - } - qdict_del(qdict, "id"); props = qdict_get(qdict, "props"); if (props) { @@ -XXX,XX +XXX,XX @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp) qobject_unref(pdict); } - v = qobject_input_visitor_new(QOBJECT(qdict)); - obj = user_creatable_add_type(type, id, qdict, v, errp); - visit_free(v); - object_unref(obj); + user_creatable_add_dict(qdict, errp); } void qmp_object_del(const char *id, Error **errp) -- 2.25.3 After processing the option string with the keyval parser, we get a QDict that contains only strings. This QDict must be fed to a keyval visitor which converts the strings into the right data types. qmp_object_add(), however, uses the normal QObject input visitor, which expects a QDict where all properties already have the QType that matches the data type required by the QOM object type. Change the --object implementation in qemu-storage-daemon so that it doesn't call qmp_object_add(), but calls user_creatable_add_dict() directly instead and pass it a new keyval boolean that decides which visitor must be used. Reported-by: Coiby Xu <coiby.xu@gmail.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qom/object_interfaces.h | 6 +++++- qemu-storage-daemon.c | 4 +--- qom/object_interfaces.c | 8 ++++++-- qom/qom-qmp-cmds.c | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h index XXXXXXX..XXXXXXX 100644 --- a/include/qom/object_interfaces.h +++ b/include/qom/object_interfaces.h @@ -XXX,XX +XXX,XX @@ Object *user_creatable_add_type(const char *type, const char *id, /** * user_creatable_add_dict: * @qdict: the object definition + * @keyval: if true, use a keyval visitor for processing @qdict (i.e. + * assume that all @qdict values are strings); otherwise, use + * the normal QObject visitor (i.e. assume all @qdict values + * have the QType expected by the QOM object type) * @errp: if an error occurs, a pointer to an area to store the error * * Create an instance of the user creatable object that is defined by @@ -XXX,XX +XXX,XX @@ Object *user_creatable_add_type(const char *type, const char *id, * ID from the key 'id'. The remaining entries in @qdict are used to * initialize the object properties. */ -void user_creatable_add_dict(QDict *qdict, Error **errp); +void user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp); /** * user_creatable_add_opts: diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c index XXXXXXX..XXXXXXX 100644 --- a/qemu-storage-daemon.c +++ b/qemu-storage-daemon.c @@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[]) QemuOpts *opts; const char *type; QDict *args; - QObject *ret_data = NULL; /* FIXME The keyval parser rejects 'help' arguments, so we must * unconditionall try QemuOpts first. */ @@ -XXX,XX +XXX,XX @@ static void process_options(int argc, char *argv[]) qemu_opts_del(opts); args = keyval_parse(optarg, "qom-type", &error_fatal); - qmp_object_add(args, &ret_data, &error_fatal); + user_creatable_add_dict(args, true, &error_fatal); qobject_unref(args); - qobject_unref(ret_data); break; } default: diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index XXXXXXX..XXXXXXX 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -XXX,XX +XXX,XX @@ out: return obj; } -void user_creatable_add_dict(QDict *qdict, Error **errp) +void user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp) { Visitor *v; Object *obj; @@ -XXX,XX +XXX,XX @@ void user_creatable_add_dict(QDict *qdict, Error **errp) } qdict_del(qdict, "id"); - v = qobject_input_visitor_new(QOBJECT(qdict)); + if (keyval) { + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + } else { + v = qobject_input_visitor_new(QOBJECT(qdict)); + } obj = user_creatable_add_type(type, id, qdict, v, errp); visit_free(v); object_unref(obj); diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/qom/qom-qmp-cmds.c +++ b/qom/qom-qmp-cmds.c @@ -XXX,XX +XXX,XX @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp) qobject_unref(pdict); } - user_creatable_add_dict(qdict, errp); + user_creatable_add_dict(qdict, false, errp); } void qmp_object_del(const char *id, Error **errp) -- 2.25.3