The following changes since commit ec397e90d21269037280633b6058d1f280e27667: Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20210901-2' into staging (2021-09-01 08:33:02 +0100) are available in the Git repository at: https://github.com/XanClic/qemu.git tags/pull-block-2021-09-01 for you to fetch changes up to ebd979c74e2b8a7275090475df36dde4ab858320: block/file-win32: add reopen handlers (2021-09-01 14:38:08 +0200) **Note:** I’ve signed the pull request tag with my new GPG key, which I have uploaded here: https://xanclic.moe/A1FA40D098019CDF Included in that key should be the signature I made with my old key (F407DB0061D5CF40), and I hope that’s sufficient to establish a reasonable level of trust. (I’ve also tried uploading this key to several keyservers, but it appears to me that keyservers are kind of a thing of the past now, especially when it comes to uploading keys with signatures on them...) ---------------------------------------------------------------- Block patches: - Make the backup-top filter driver available for user-created block nodes (i.e. via blockdev-add) - Allow running iotests with gdb or valgrind being attached to qemu instances - Fix the raw format driver's permissions: There is no metadata, so we only need WRITE or RESIZE when the parent needs it - Basic reopen implementation for win32 files (file-win32.c) so that qemu-img commit can work - uclibc/musl build fix for the FUSE export code - Some iotests delinting - block-hmp-cmds.c refactoring ---------------------------------------------------------------- Emanuele Giuseppe Esposito (15): python: qemu: add timer parameter for qmp.accept socket python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine docs/devel/testing: add debug section to the QEMU iotests chapter qemu-iotests: add option to attach gdbserver qemu-iotests: delay QMP socket timers qemu_iotests: insert gdbserver command line as wrapper for qemu binary qemu-iotests: add gdbserver option to script tests too docs/devel/testing: add -gdb option to the debugging section of QEMU iotests qemu-iotests: extend the check script to prepare supporting valgrind for python tests qemu-iotests: extend QMP socket timeout when using valgrind qemu-iotests: allow valgrind to read/delete the generated log file qemu-iotests: insert valgrind command line as wrapper for qemu binary docs/devel/testing: add -valgrind option to the debug section of QEMU iotests qemu-iotests: add option to show qemu binary logs on stdout docs/devel/testing: add -p option to the debug section of QEMU iotests Fabrice Fontaine (1): block/export/fuse.c: fix fuse-lseek on uclibc or musl John Snow (3): python: Reduce strictness of pylint's duplicate-code check iotests: use with-statement for open() calls iotests: use subprocess.DEVNULL instead of open("/dev/null") Mao Zhongyi (1): block/monitor: Consolidate hmp_handle_error calls to reduce redundant code Stefan Hajnoczi (1): raw-format: drop WRITE and RESIZE child perms when possible Viktor Prutyanov (1): block/file-win32: add reopen handlers Vladimir Sementsov-Ogievskiy (34): block: introduce bdrv_replace_child_bs() block: introduce blk_replace_bs qdev-properties: PropertyInfo: add realized_set_allowed field qdev: allow setting drive property for realized device block: rename backup-top to copy-before-write block-copy: move detecting fleecing scheme to block-copy block/block-copy: introduce block_copy_set_copy_opts() block/backup: set copy_range and compress after filter insertion block/backup: move cluster size calculation to block-copy block/copy-before-write: relax permission requirements when no parents block/copy-before-write: drop extra bdrv_unref on failure path block/copy-before-write: use file child instead of backing block/copy-before-write: bdrv_cbw_append(): replace child at last block/copy-before-write: introduce cbw_init() block/copy-before-write: cbw_init(): rename variables block/copy-before-write: cbw_init(): use file child after attaching block/copy-before-write: bdrv_cbw_append(): drop unused compress arg block/copy-before-write: cbw_init(): use options block/copy-before-write: initialize block-copy bitmap block/block-copy: make setting progress optional block/copy-before-write: make public block driver qapi: publish copy-before-write filter python/qemu/machine.py: refactor _qemu_args() python/qemu/machine: QEMUMachine: improve qmp() method python:QEMUMachine: template typing for self returning methods iotests/222: fix pylint and mypy complains iotests/222: constantly use single quotes for strings iotests: move 222 to tests/image-fleecing iotests.py: hmp_qemu_io: support qdev iotests/image-fleecing: proper source device iotests/image-fleecing: rename tgt_node iotests/image-fleecing: prepare for adding new test-case iotests/image-fleecing: add test-case for copy-before-write filter block/block-copy: block_copy_state_new(): drop extra arguments docs/devel/testing.rst | 29 +++ qapi/block-core.json | 25 +- block/{backup-top.h => copy-before-write.h} | 25 +- include/block/block-copy.h | 6 +- include/block/block.h | 2 + include/hw/qdev-properties.h | 1 + include/sysemu/block-backend.h | 1 + block.c | 31 +++ block/backup-top.c | 253 ------------------- block/backup.c | 116 ++------- block/block-backend.c | 8 + block/block-copy.c | 136 ++++++++--- block/copy-before-write.c | 256 ++++++++++++++++++++ block/export/fuse.c | 3 + block/file-win32.c | 101 +++++++- block/monitor/block-hmp-cmds.c | 12 +- block/raw-format.c | 21 +- hw/core/qdev-properties-system.c | 43 +++- hw/core/qdev-properties.c | 6 +- .gitlab-ci.d/buildtest.yml | 6 +- MAINTAINERS | 4 +- block/meson.build | 2 +- python/qemu/machine/machine.py | 56 +++-- python/qemu/machine/qtest.py | 9 +- python/setup.cfg | 5 + tests/qemu-iotests/222 | 159 ------------ tests/qemu-iotests/222.out | 67 ----- tests/qemu-iotests/283 | 35 ++- tests/qemu-iotests/283.out | 4 +- tests/qemu-iotests/297 | 2 +- tests/qemu-iotests/check | 15 +- tests/qemu-iotests/common.qemu | 7 +- tests/qemu-iotests/common.rc | 8 +- tests/qemu-iotests/iotests.py | 75 ++++-- tests/qemu-iotests/testenv.py | 23 +- tests/qemu-iotests/tests/image-fleecing | 192 +++++++++++++++ tests/qemu-iotests/tests/image-fleecing.out | 139 +++++++++++ 37 files changed, 1172 insertions(+), 711 deletions(-) rename block/{backup-top.h => copy-before-write.h} (56%) delete mode 100644 block/backup-top.c create mode 100644 block/copy-before-write.c delete mode 100755 tests/qemu-iotests/222 delete mode 100644 tests/qemu-iotests/222.out create mode 100755 tests/qemu-iotests/tests/image-fleecing create mode 100644 tests/qemu-iotests/tests/image-fleecing.out -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Also add a new _qmp_timer field to the QEMUMachine class. Let's change the default socket timeout to None, so that if a subclass needs to add a timer, it can be done by modifying this private field. At the same time, restore the timer to be 15 seconds in iotests.py, to give an upper bound to the QMP monitor test command execution. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: John Snow <jsnow@redhat.com> Acked-by: John Snow <jsnow@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-2-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- python/qemu/machine/machine.py | 7 +++++-- python/qemu/machine/qtest.py | 5 +++-- tests/qemu-iotests/iotests.py | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index XXXXXXX..XXXXXXX 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -XXX,XX +XXX,XX @@ def __init__(self, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None, - log_dir: Optional[str] = None): + log_dir: Optional[str] = None, + qmp_timer: Optional[float] = None): ''' Initialize a QEMUMachine @@ -XXX,XX +XXX,XX @@ def __init__(self, @param drain_console: (optional) True to drain console socket to buffer @param console_log: (optional) path to console log file @param log_dir: where to create and keep log files + @param qmp_timer: (optional) default QMP socket timeout @note: Qemu process is not started until launch() is used. ''' # pylint: disable=too-many-arguments @@ -XXX,XX +XXX,XX @@ def __init__(self, self._binary = binary self._args = list(args) self._wrapper = wrapper + self._qmp_timer = qmp_timer self._name = name or "qemu-%d" % os.getpid() self._base_temp_dir = base_temp_dir @@ -XXX,XX +XXX,XX @@ def _pre_launch(self) -> None: def _post_launch(self) -> None: if self._qmp_connection: - self._qmp.accept() + self._qmp.accept(self._qmp_timer) def _post_shutdown(self) -> None: """ diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py index XXXXXXX..XXXXXXX 100644 --- a/python/qemu/machine/qtest.py +++ b/python/qemu/machine/qtest.py @@ -XXX,XX +XXX,XX @@ def __init__(self, name: Optional[str] = None, base_temp_dir: str = "/var/tmp", socket_scm_helper: Optional[str] = None, - sock_dir: Optional[str] = None): + sock_dir: Optional[str] = None, + qmp_timer: Optional[float] = None): # pylint: disable=too-many-arguments if name is None: @@ -XXX,XX +XXX,XX @@ def __init__(self, sock_dir = base_temp_dir super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, socket_scm_helper=socket_scm_helper, - sock_dir=sock_dir) + sock_dir=sock_dir, qmp_timer=qmp_timer) self._qtest: Optional[QEMUQtestProtocol] = None self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock") 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 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) + timer = 15.0 super().__init__(qemu_prog, qemu_opts, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, - sock_dir=sock_dir) + sock_dir=sock_dir, qmp_timer=timer) self._num_drives = 0 def add_object(self, opts): -- 2.31.1 From: John Snow <jsnow@redhat.com> Pylint prior to 2.8.3 (We pin at >= 2.8.0) includes function and method signatures as part of its duplicate checking algorithm. This check does not listen to pragmas, so the only way to disable it is to turn it off completely or increase the minimum duplicate lines so that it doesn't trigger for functions with long, multi-line signatures. When we decide to upgrade to pylint 2.8.3 or greater, we will be able to use 'ignore-signatures = true' to the config instead. I'd prefer not to keep us on the very bleeding edge of pylint if I can help it -- 2.8.3 came out only three days ago at time of writing. See: https://github.com/PyCQA/pylint/pull/4474 Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Acked-by: John Snow <jsnow@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-3-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- python/setup.cfg | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/python/setup.cfg b/python/setup.cfg index XXXXXXX..XXXXXXX 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -XXX,XX +XXX,XX @@ good-names=i, # Ignore imports when computing similarities. ignore-imports=yes +# Minimum lines number of a similarity. +# TODO: Remove after we opt in to Pylint 2.8.3. See commit msg. +min-similarity-lines=6 + + [isort] force_grid_wrap=4 force_sort_within_sections=True -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: John Snow <jsnow@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Acked-by: John Snow <jsnow@redhat.com> Message-Id: <20210809090114.64834-4-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- python/qemu/machine/qtest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py index XXXXXXX..XXXXXXX 100644 --- a/python/qemu/machine/qtest.py +++ b/python/qemu/machine/qtest.py @@ -XXX,XX +XXX,XX @@ class QEMUQtestMachine(QEMUMachine): def __init__(self, binary: str, args: Sequence[str] = (), + wrapper: Sequence[str] = (), name: Optional[str] = None, base_temp_dir: str = "/var/tmp", socket_scm_helper: Optional[str] = None, @@ -XXX,XX +XXX,XX @@ def __init__(self, name = "qemu-%d" % os.getpid() if sock_dir is None: sock_dir = base_temp_dir - super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, + super().__init__(binary, args, wrapper=wrapper, name=name, + base_temp_dir=base_temp_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir, qmp_timer=qmp_timer) self._qtest: Optional[QEMUQtestProtocol] = None -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Introduce the "Debugging a test case" section, in preparation to the additional flags that will be added in the next patches. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-5-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- docs/devel/testing.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -XXX,XX +XXX,XX @@ another application on the host may have locked the file, possibly leading to a test failure. If using such devices are explicitly desired, consider adding ``locking=off`` option to disable image locking. +Debugging a test case +----------------------- +The following options to the ``check`` script can be useful when debugging +a failing test: + +* ``-d`` (debug) just increases the logging verbosity, showing + for example the QMP commands and answers. + Test case groups ---------------- -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Define -gdb flag and GDB_OPTIONS environment variable to python tests to attach a gdbserver to each qemu instance. This patch only adds and parses this flag, it does not yet add the implementation for it. if -gdb is not provided but $GDB_OPTIONS is set, ignore the environment variable. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-6-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/check | 6 +++++- tests/qemu-iotests/iotests.py | 5 +++++ tests/qemu-iotests/testenv.py | 17 +++++++++++++++-- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -XXX,XX +XXX,XX @@ def make_argparser() -> argparse.ArgumentParser: help='pretty print output for make check') p.add_argument('-d', dest='debug', action='store_true', help='debug') + p.add_argument('-gdb', action='store_true', + help="start gdbserver with $GDB_OPTIONS options \ + ('localhost:12345' if $GDB_OPTIONS is empty)") p.add_argument('-misalign', action='store_true', help='misalign memory allocations') p.add_argument('--color', choices=['on', 'off', 'auto'], @@ -XXX,XX +XXX,XX @@ if __name__ == '__main__': env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto, aiomode=args.aiomode, cachemode=args.cachemode, imgopts=args.imgopts, misalign=args.misalign, - debug=args.debug, valgrind=args.valgrind) + debug=args.debug, valgrind=args.valgrind, + gdb=args.gdb) if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--': if not args.tests: 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 @@ qemu_prog = os.environ.get('QEMU_PROG', 'qemu') qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ') +gdb_qemu_env = os.environ.get('GDB_OPTIONS') +qemu_gdb = [] +if gdb_qemu_env: + qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ') + imgfmt = os.environ.get('IMGFMT', 'raw') imgproto = os.environ.get('IMGPROTO', 'file') output_dir = os.environ.get('OUTPUT_DIR', '.') diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/testenv.py +++ b/tests/qemu-iotests/testenv.py @@ -XXX,XX +XXX,XX @@ import glob from typing import List, Dict, Any, Optional, ContextManager +DEF_GDB_OPTIONS = 'localhost:12345' def isxfile(path: str) -> bool: return os.path.isfile(path) and os.access(path, os.X_OK) @@ -XXX,XX +XXX,XX @@ class TestEnv(ContextManager['TestEnv']): 'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO', 'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU', 'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX', - 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_'] + 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_', + 'GDB_OPTIONS'] def prepare_subprocess(self, args: List[str]) -> Dict[str, str]: if self.debug: @@ -XXX,XX +XXX,XX @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str, imgopts: Optional[str] = None, misalign: bool = False, debug: bool = False, - valgrind: bool = False) -> None: + valgrind: bool = False, + gdb: bool = False) -> None: self.imgfmt = imgfmt self.imgproto = imgproto self.aiomode = aiomode @@ -XXX,XX +XXX,XX @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str, self.misalign = misalign self.debug = debug + if gdb: + self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS) + if not self.gdb_options: + # cover the case 'export GDB_OPTIONS=' + self.gdb_options = DEF_GDB_OPTIONS + elif 'GDB_OPTIONS' in os.environ: + # to not propagate it in prepare_subprocess() + del os.environ['GDB_OPTIONS'] + if valgrind: self.valgrind_qemu = 'y' @@ -XXX,XX +XXX,XX @@ def print_env(self) -> None: TEST_DIR -- {TEST_DIR} SOCK_DIR -- {SOCK_DIR} SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER} +GDB_OPTIONS -- {GDB_OPTIONS} """ args = collections.defaultdict(str, self.get_env()) -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Attaching gdbserver implies that the qmp socket should wait indefinitely for an answer from QEMU. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-7-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 __init__(self, seconds, errmsg="Timeout"): self.seconds = seconds self.errmsg = errmsg def __enter__(self): + if qemu_gdb: + return self signal.signal(signal.SIGALRM, self.timeout) signal.setitimer(signal.ITIMER_REAL, self.seconds) return self def __exit__(self, exc_type, value, traceback): + if qemu_gdb: + return False signal.setitimer(signal.ITIMER_REAL, 0) return False def timeout(self, signum, frame): @@ -XXX,XX +XXX,XX @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) - timer = 15.0 + timer = 15.0 if not qemu_gdb else None super().__init__(qemu_prog, qemu_opts, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-8-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) timer = 15.0 if not qemu_gdb else None - super().__init__(qemu_prog, qemu_opts, name=name, + super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb, + name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir, qmp_timer=timer) -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Remove read timer in test script when GDB_OPTIONS are set, so that the bash tests won't timeout while running gdb. The only limitation here is that running a script with gdbserver will make the test output mismatch with the expected results, making the test fail. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20210809090114.64834-9-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/common.qemu | 7 ++++++- tests/qemu-iotests/common.rc | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) 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 @@ _timed_wait_for() timeout=yes QEMU_STATUS[$h]=0 - while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]} + read_timeout="-t ${QEMU_COMM_TIMEOUT}" + if [ -n "${GDB_OPTIONS}" ]; then + read_timeout= + fi + + while IFS= read ${read_timeout} resp <&${QEMU_OUT[$h]} do if [ -n "$capture_events" ]; then capture=0 diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -XXX,XX +XXX,XX @@ _qemu_wrapper() if [ -n "${QEMU_NEED_PID}" ]; then echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid" fi + + GDB="" + if [ -n "${GDB_OPTIONS}" ]; then + GDB="gdbserver ${GDB_OPTIONS}" + fi + VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \ - "$QEMU_PROG" $QEMU_OPTIONS "$@" + $GDB "$QEMU_PROG" $QEMU_OPTIONS "$@" ) RETVAL=$? _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-10-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- docs/devel/testing.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -XXX,XX +XXX,XX @@ Debugging a test case The following options to the ``check`` script can be useful when debugging a failing test: +* ``-gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a + connection from a gdb client. The options given to ``gdbserver`` (e.g. the + address on which to listen for connections) are taken from the ``$GDB_OPTIONS`` + environment variable. By default (if ``$GDB_OPTIONS`` is empty), it listens on + ``localhost:12345``. + It is possible to connect to it for example with + ``gdb -iex "target remote $addr"``, where ``$addr`` is the address + ``gdbserver`` listens on. + If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored, + regardless of whether it is set or not. + * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Currently, the check script only parses the option and sets the VALGRIND_QEMU environmental variable to "y". Add another local python variable that prepares the command line, identical to the one provided in the test scripts. Because the python script does not know in advance the valgrind PID to assign to the log file name, use the "%p" flag in valgrind log file name that automatically puts the process PID at runtime. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20210809090114.64834-11-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/check | 7 ++++--- tests/qemu-iotests/iotests.py | 11 +++++++++++ tests/qemu-iotests/testenv.py | 1 + 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -XXX,XX +XXX,XX @@ def make_argparser() -> argparse.ArgumentParser: p.add_argument('-gdb', action='store_true', help="start gdbserver with $GDB_OPTIONS options \ ('localhost:12345' if $GDB_OPTIONS is empty)") + p.add_argument('-valgrind', action='store_true', + help='use valgrind, sets VALGRIND_QEMU environment ' + 'variable') + p.add_argument('-misalign', action='store_true', help='misalign memory allocations') p.add_argument('--color', choices=['on', 'off', 'auto'], @@ -XXX,XX +XXX,XX @@ def make_argparser() -> argparse.ArgumentParser: g_bash.add_argument('-o', dest='imgopts', help='options to pass to qemu-img create/convert, ' 'sets IMGOPTS environment variable') - g_bash.add_argument('-valgrind', action='store_true', - help='use valgrind, sets VALGRIND_QEMU environment ' - 'variable') g_sel = p.add_argument_group('test selecting options', 'The following options specify test set ' 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 @@ sys.stderr.write('Please run this test via the "check" script\n') sys.exit(os.EX_USAGE) +qemu_valgrind = [] +if os.environ.get('VALGRIND_QEMU') == "y" and \ + os.environ.get('NO_VALGRIND') != "y": + valgrind_logfile = "--log-file=" + test_dir + # %p allows to put the valgrind process PID, since + # we don't know it a priori (subprocess.Popen is + # not yet invoked) + valgrind_logfile += "/%p.valgrind" + + qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99'] + socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper') luks_default_secret_object = 'secret,id=keysec0,data=' + \ diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/testenv.py +++ b/tests/qemu-iotests/testenv.py @@ -XXX,XX +XXX,XX @@ def print_env(self) -> None: SOCK_DIR -- {SOCK_DIR} SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER} GDB_OPTIONS -- {GDB_OPTIONS} +VALGRIND_QEMU -- {VALGRIND_QEMU} """ args = collections.defaultdict(str, self.get_env()) -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> As with gdbserver, valgrind delays the test execution, so the default QMP socket timeout and the generic class Timeout in iotests.py timeouts too soon. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-12-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 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 __init__(self, seconds, errmsg="Timeout"): self.seconds = seconds self.errmsg = errmsg def __enter__(self): - if qemu_gdb: + if qemu_gdb or qemu_valgrind: return self signal.signal(signal.SIGALRM, self.timeout) signal.setitimer(signal.ITIMER_REAL, self.seconds) return self def __exit__(self, exc_type, value, traceback): - if qemu_gdb: + if qemu_gdb or qemu_valgrind: return False signal.setitimer(signal.ITIMER_REAL, 0) return False @@ -XXX,XX +XXX,XX @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) - timer = 15.0 if not qemu_gdb else None + timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb, name=name, base_temp_dir=test_dir, -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> When using -valgrind on the script tests, it generates a log file in $TEST_DIR that is either read (if valgrind finds problems) or otherwise deleted. Provide the same exact behavior when using -valgrind on the python tests. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-13-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 __init__(self, path_suffix=''): sock_dir=sock_dir, qmp_timer=timer) self._num_drives = 0 + def _post_shutdown(self) -> None: + super()._post_shutdown() + if not qemu_valgrind or not self._popen: + return + valgrind_filename = f"{test_dir}/{self._popen.pid}.valgrind" + if self.exitcode() == 99: + with open(valgrind_filename) as f: + print(f.read()) + else: + os.remove(valgrind_filename) + def add_object(self, opts): self._args.append('-object') self._args.append(opts) -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> If -gdb and -valgrind are both defined, return an error. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-14-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None - super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb, + if qemu_gdb and qemu_valgrind: + sys.stderr.write('gdb and valgrind are mutually exclusive\n') + sys.exit(1) + wrapper = qemu_gdb if qemu_gdb else qemu_valgrind + super().__init__(qemu_prog, qemu_opts, wrapper=wrapper, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20210809090114.64834-15-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- docs/devel/testing.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -XXX,XX +XXX,XX @@ a failing test: If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored, regardless of whether it is set or not. +* ``-valgrind`` attaches a valgrind instance to QEMU. If it detects + warnings, it will print and save the log in + ``$TEST_DIR/<valgrind_pid>.valgrind``. + The final command line will be ``valgrind --log-file=$TEST_DIR/ + <valgrind_pid>.valgrind --error-exitcode=99 $QEMU ...`` + * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Using the flag -p, allow the qemu binary to print to stdout. Also create the common function _close_qemu_log_file() to avoid accessing machine.py private fields directly and have duplicate code. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-16-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- python/qemu/machine/machine.py | 9 ++++++--- tests/qemu-iotests/check | 4 +++- tests/qemu-iotests/iotests.py | 8 ++++++++ tests/qemu-iotests/testenv.py | 9 +++++++-- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index XXXXXXX..XXXXXXX 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -XXX,XX +XXX,XX @@ def _post_launch(self) -> None: if self._qmp_connection: self._qmp.accept(self._qmp_timer) + def _close_qemu_log_file(self) -> None: + if self._qemu_log_file is not None: + self._qemu_log_file.close() + self._qemu_log_file = None + def _post_shutdown(self) -> None: """ Called to cleanup the VM instance after the process has exited. @@ -XXX,XX +XXX,XX @@ def _post_shutdown(self) -> None: self._qmp.close() self._qmp_connection = None - if self._qemu_log_file is not None: - self._qemu_log_file.close() - self._qemu_log_file = None + self._close_qemu_log_file() self._load_io_log() diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -XXX,XX +XXX,XX @@ def make_argparser() -> argparse.ArgumentParser: help='pretty print output for make check') p.add_argument('-d', dest='debug', action='store_true', help='debug') + p.add_argument('-p', dest='print', action='store_true', + help='redirects qemu\'s stdout and stderr to the test output') p.add_argument('-gdb', action='store_true', help="start gdbserver with $GDB_OPTIONS options \ ('localhost:12345' if $GDB_OPTIONS is empty)") @@ -XXX,XX +XXX,XX @@ if __name__ == '__main__': aiomode=args.aiomode, cachemode=args.cachemode, imgopts=args.imgopts, misalign=args.misalign, debug=args.debug, valgrind=args.valgrind, - gdb=args.gdb) + gdb=args.gdb, qprint=args.print) if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--': if not args.tests: 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 @@ if gdb_qemu_env: qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ') +qemu_print = os.environ.get('PRINT_QEMU', False) + imgfmt = os.environ.get('IMGFMT', 'raw') imgproto = os.environ.get('IMGPROTO', 'file') output_dir = os.environ.get('OUTPUT_DIR', '.') @@ -XXX,XX +XXX,XX @@ def _post_shutdown(self) -> None: else: os.remove(valgrind_filename) + def _pre_launch(self) -> None: + super()._pre_launch() + if qemu_print: + # set QEMU binary output to stdout + self._close_qemu_log_file() + def add_object(self, opts): self._args.append('-object') self._args.append(opts) diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/testenv.py +++ b/tests/qemu-iotests/testenv.py @@ -XXX,XX +XXX,XX @@ class TestEnv(ContextManager['TestEnv']): 'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU', 'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX', 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_', - 'GDB_OPTIONS'] + 'GDB_OPTIONS', 'PRINT_QEMU'] def prepare_subprocess(self, args: List[str]) -> Dict[str, str]: if self.debug: @@ -XXX,XX +XXX,XX @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str, misalign: bool = False, debug: bool = False, valgrind: bool = False, - gdb: bool = False) -> None: + gdb: bool = False, + qprint: bool = False) -> None: self.imgfmt = imgfmt self.imgproto = imgproto self.aiomode = aiomode @@ -XXX,XX +XXX,XX @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str, self.misalign = misalign self.debug = debug + if qprint: + self.print_qemu = 'y' + if gdb: self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS) if not self.gdb_options: @@ -XXX,XX +XXX,XX @@ def print_env(self) -> None: SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER} GDB_OPTIONS -- {GDB_OPTIONS} VALGRIND_QEMU -- {VALGRIND_QEMU} +PRINT_QEMU_OUTPUT -- {PRINT_QEMU} """ args = collections.defaultdict(str, self.get_env()) -- 2.31.1 From: Emanuele Giuseppe Esposito <eesposit@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210809090114.64834-17-eesposit@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- docs/devel/testing.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -XXX,XX +XXX,XX @@ a failing test: * ``-d`` (debug) just increases the logging verbosity, showing for example the QMP commands and answers. +* ``-p`` (print) redirects QEMU’s stdout and stderr to the test output, + instead of saving it into a log file in + ``$TEST_DIR/qemu-machine-<random_string>``. + Test case groups ---------------- -- 2.31.1 From: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Message-Id: <20210802062507.347555-1-maozhongyi@cmss.chinamobile.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/monitor/block-hmp-cmds.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index XXXXXXX..XXXXXXX 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -XXX,XX +XXX,XX @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict) if (!filename) { error_setg(&err, QERR_MISSING_PARAMETER, "target"); - hmp_handle_error(mon, err); - return; + goto end; } qmp_drive_mirror(&mirror, &err); +end: hmp_handle_error(mon, err); } @@ -XXX,XX +XXX,XX @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) if (!filename) { error_setg(&err, QERR_MISSING_PARAMETER, "target"); - hmp_handle_error(mon, err); - return; + goto end; } qmp_drive_backup(&backup, &err); +end: hmp_handle_error(mon, err); } @@ -XXX,XX +XXX,XX @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) * will be taken internally. Today it's actually required. */ error_setg(&err, QERR_MISSING_PARAMETER, "snapshot-file"); - hmp_handle_error(mon, err); - return; + goto end; } mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS; @@ -XXX,XX +XXX,XX @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) filename, false, NULL, !!format, format, true, mode, &err); +end: hmp_handle_error(mon, err); } -- 2.31.1 From: Stefan Hajnoczi <stefanha@redhat.com> The following command-line fails due to a permissions conflict: $ qemu-storage-daemon \ --blockdev driver=nvme,node-name=nvme0,device=0000:08:00.0,namespace=1 \ --blockdev driver=raw,node-name=l1-1,file=nvme0,offset=0,size=1073741824 \ --blockdev driver=raw,node-name=l1-2,file=nvme0,offset=1073741824,size=1073741824 \ --nbd-server addr.type=unix,addr.path=/tmp/nbd.sock,max-connections=2 \ --export type=nbd,id=nbd-l1-1,node-name=l1-1,name=l1-1,writable=on \ --export type=nbd,id=nbd-l1-2,node-name=l1-2,name=l1-2,writable=on qemu-storage-daemon: --export type=nbd,id=nbd-l1-1,node-name=l1-1,name=l1-1,writable=on: Permission conflict on node 'nvme0': permissions 'resize' are both required by node 'l1-1' (uses node 'nvme0' as 'file' child) and unshared by node 'l1-2' (uses node 'nvme0' as 'file' child). The problem is that block/raw-format.c relies on bdrv_default_perms() to set permissions on the nvme node. The default permissions add RESIZE in anticipation of a format driver like qcow2 that needs to grow the image file. This fails because RESIZE is unshared, so we cannot get the RESIZE permission. Max Reitz pointed out that block/crypto.c already handles this case by implementing a custom ->bdrv_child_perm() function that adjusts the result of bdrv_default_perms(). This patch takes the same approach in block/raw-format.c so that RESIZE is only required if it's actually necessary (e.g. the parent is qcow2). Cc: Max Reitz <mreitz@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20210726122839.822900-1-stefanha@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/raw-format.c | 21 ++++++++++++++++++++- 1 file changed, 20 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 void raw_cancel_in_flight(BlockDriverState *bs) bdrv_cancel_in_flight(bs->file->bs); } +static void raw_child_perm(BlockDriverState *bs, BdrvChild *c, + BdrvChildRole role, + BlockReopenQueue *reopen_queue, + uint64_t parent_perm, uint64_t parent_shared, + uint64_t *nperm, uint64_t *nshared) +{ + bdrv_default_perms(bs, c, role, reopen_queue, parent_perm, + parent_shared, nperm, nshared); + + /* + * bdrv_default_perms() may add WRITE and/or RESIZE (see comment in + * bdrv_default_perms_for_storage() for an explanation) but we only need + * them if they are in parent_perm. Drop WRITE and RESIZE whenever possible + * to avoid permission conflicts. + */ + *nperm &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE); + *nperm |= parent_perm & (BLK_PERM_WRITE | BLK_PERM_RESIZE); +} + BlockDriver bdrv_raw = { .format_name = "raw", .instance_size = sizeof(BDRVRawState), @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_raw = { .bdrv_reopen_commit = &raw_reopen_commit, .bdrv_reopen_abort = &raw_reopen_abort, .bdrv_open = &raw_open, - .bdrv_child_perm = bdrv_default_perms, + .bdrv_child_perm = raw_child_perm, .bdrv_co_create_opts = &raw_co_create_opts, .bdrv_co_preadv = &raw_co_preadv, .bdrv_co_pwritev = &raw_co_pwritev, -- 2.31.1 From: John Snow <jsnow@redhat.com> Silences a new pylint warning. The dangers of *not* doing this are somewhat unclear; I believe the file object gets garbage collected eventually, but possibly the way in which it happens is non-deterministic. Maybe this is a valid warning, but if there are consequences of not doing it, I am not aware of them at present. Signed-off-by: John Snow <jsnow@redhat.com> Message-Id: <20210720173336.1876937-2-jsnow@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 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 notrun(reason): # Each test in qemu-iotests has a number ("seq") seq = os.path.basename(sys.argv[0]) - open('%s/%s.notrun' % (output_dir, seq), 'w').write(reason + '\n') + with open('%s/%s.notrun' % (output_dir, seq), 'w') as outfile: + outfile.write(reason + '\n') logger.warning("%s not run: %s", seq, reason) sys.exit(0) @@ -XXX,XX +XXX,XX @@ def case_notrun(reason): # Each test in qemu-iotests has a number ("seq") seq = os.path.basename(sys.argv[0]) - open('%s/%s.casenotrun' % (output_dir, seq), 'a').write( - ' [case not run] ' + reason + '\n') + with open('%s/%s.casenotrun' % (output_dir, seq), 'a') as outfile: + outfile.write(' [case not run] ' + reason + '\n') def _verify_image_format(supported_fmts: Sequence[str] = (), unsupported_fmts: Sequence[str] = ()) -> None: -- 2.31.1 From: John Snow <jsnow@redhat.com> Avoids a warning from pylint not to use open() outside of a with-statement, and is ... probably more portable anyway. Not that I think we care too much about running tests *on* Windows, but... eh. Signed-off-by: John Snow <jsnow@redhat.com> Message-Id: <20210720173336.1876937-3-jsnow@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 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 qemu_io_silent(*args): default_args = qemu_io_args args = default_args + list(args) - exitcode = subprocess.call(args, stdout=open('/dev/null', 'w')) - if exitcode < 0: + result = subprocess.run(args, stdout=subprocess.DEVNULL, check=False) + if result.returncode < 0: sys.stderr.write('qemu-io received signal %i: %s\n' % - (-exitcode, ' '.join(args))) - return exitcode + (-result.returncode, ' '.join(args))) + return result.returncode def qemu_io_silent_check(*args): '''Run qemu-io and return the true if subprocess returned 0''' args = qemu_io_args + list(args) - exitcode = subprocess.call(args, stdout=open('/dev/null', 'w'), - stderr=subprocess.STDOUT) - return exitcode == 0 + result = subprocess.run(args, stdout=subprocess.DEVNULL, + stderr=subprocess.STDOUT, check=False) + return result.returncode == 0 class QemuIoInteractive: def __init__(self, *args): -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Add function to transactionally replace bs inside BdrvChild. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-2-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/block/block.h | 2 ++ block.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) 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 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp); int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, Error **errp); +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, + Error **errp); BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options, int flags, Error **errp); int bdrv_drop_filter(BlockDriverState *bs, Error **errp); diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ out: return ret; } +/* Not for empty child */ +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, + Error **errp) +{ + int ret; + Transaction *tran = tran_new(); + g_autoptr(GHashTable) found = NULL; + g_autoptr(GSList) refresh_list = NULL; + BlockDriverState *old_bs = child->bs; + + bdrv_ref(old_bs); + bdrv_drained_begin(old_bs); + bdrv_drained_begin(new_bs); + + bdrv_replace_child_tran(child, new_bs, tran); + + found = g_hash_table_new(NULL, NULL); + refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs); + refresh_list = bdrv_topological_dfs(refresh_list, found, new_bs); + + ret = bdrv_list_refresh_perms(refresh_list, NULL, tran, errp); + + tran_finalize(tran, ret); + + bdrv_drained_end(old_bs); + bdrv_drained_end(new_bs); + bdrv_unref(old_bs); + + return ret; +} + static void bdrv_delete(BlockDriverState *bs) { assert(bdrv_op_blocker_is_empty(bs)); -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Add function to change bs inside blk. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-3-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/sysemu/block-backend.h | 1 + block/block-backend.c | 8 ++++++++ 2 files changed, 9 insertions(+) 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 @@ BlockBackend *blk_by_public(BlockBackendPublic *public); BlockDriverState *blk_bs(BlockBackend *blk); void blk_remove_bs(BlockBackend *blk); int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp); +int blk_replace_bs(BlockBackend *blk, BlockDriverState *new_bs, Error **errp); bool bdrv_has_blk(BlockDriverState *bs); bool bdrv_is_root_node(BlockDriverState *bs); int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm, 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_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp) return 0; } +/* + * Change BlockDriverState associated with @blk. + */ +int blk_replace_bs(BlockBackend *blk, BlockDriverState *new_bs, Error **errp) +{ + return bdrv_replace_child_bs(blk->root, new_bs, errp); +} + /* * Sets the permission bitmasks that the user of the BlockBackend needs. */ -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Add field, so property can declare support for setting the property when device is realized. To be used in the following commit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-4-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/hw/qdev-properties.h | 1 + hw/core/qdev-properties.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -XXX,XX +XXX,XX @@ struct PropertyInfo { const char *name; const char *description; const QEnumLookup *enum_table; + bool realized_set_allowed; /* allow setting property on realized device */ int (*print)(Object *obj, Property *prop, char *dest, size_t len); void (*set_default_value)(ObjectProperty *op, const Property *prop); ObjectProperty *(*create)(ObjectClass *oc, const char *name, diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index XXXXXXX..XXXXXXX 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -XXX,XX +XXX,XX @@ void qdev_prop_set_after_realize(DeviceState *dev, const char *name, /* returns: true if property is allowed to be set, false otherwise */ static bool qdev_prop_allow_set(Object *obj, const char *name, - Error **errp) + const PropertyInfo *info, Error **errp) { DeviceState *dev = DEVICE(obj); - if (dev->realized) { + if (dev->realized && !info->realized_set_allowed) { qdev_prop_set_after_realize(dev, name, errp); return false; } @@ -XXX,XX +XXX,XX @@ static void field_prop_set(Object *obj, Visitor *v, const char *name, { Property *prop = opaque; - if (!qdev_prop_allow_set(obj, name, errp)) { + if (!qdev_prop_allow_set(obj, name, prop->info, errp)) { return; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We need an ability to insert filters above top block node, attached to block device. It can't be achieved with blockdev-reopen command. So, we want do it with help of qom-set. Intended usage: Assume there is a node A that is attached to some guest device. 1. blockdev-add to create a filter node B that has A as its child. 2. qom-set to change the node attached to the guest device’s BlockBackend from A to B. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-5-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- hw/core/qdev-properties-system.c | 43 +++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index XXXXXXX..XXXXXXX 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -XXX,XX +XXX,XX @@ static bool check_prop_still_unset(Object *obj, const char *name, const void *old_val, const char *new_val, - Error **errp) + bool allow_override, Error **errp) { const GlobalProperty *prop = qdev_find_global_prop(obj, name); - if (!old_val) { + if (!old_val || (!prop && allow_override)) { return true; } @@ -XXX,XX +XXX,XX @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name, BlockBackend *blk; bool blk_created = false; int ret; + BlockDriverState *bs; + AioContext *ctx; if (!visit_type_str(v, name, &str, errp)) { return; } - /* - * TODO Should this really be an error? If no, the old value - * needs to be released before we store the new one. - */ - if (!check_prop_still_unset(obj, name, *ptr, str, errp)) { + if (!check_prop_still_unset(obj, name, *ptr, str, true, errp)) { + return; + } + + if (*ptr) { + /* BlockBackend alread exists. So, we want to change attached node */ + blk = *ptr; + ctx = blk_get_aio_context(blk); + bs = bdrv_lookup_bs(NULL, str, errp); + if (!bs) { + return; + } + + if (ctx != bdrv_get_aio_context(bs)) { + error_setg(errp, "Different aio context is not supported for new " + "node"); + } + + aio_context_acquire(ctx); + blk_replace_bs(blk, bs, errp); + aio_context_release(ctx); return; } @@ -XXX,XX +XXX,XX @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name, blk = blk_by_name(str); if (!blk) { - BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); + bs = bdrv_lookup_bs(NULL, str, NULL); if (bs) { /* * If the device supports iothreads, it will make sure to move the @@ -XXX,XX +XXX,XX @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name, * aware of iothreads require their BlockBackends to be in the main * AioContext. */ - AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : - qemu_get_aio_context(); + ctx = iothread ? bdrv_get_aio_context(bs) : qemu_get_aio_context(); blk = blk_new(ctx, 0, BLK_PERM_ALL); blk_created = true; @@ -XXX,XX +XXX,XX @@ static void release_drive(Object *obj, const char *name, void *opaque) const PropertyInfo qdev_prop_drive = { .name = "str", .description = "Node name or ID of a block device to use as a backend", + .realized_set_allowed = true, .get = get_drive, .set = set_drive, .release = release_drive, @@ -XXX,XX +XXX,XX @@ const PropertyInfo qdev_prop_drive = { const PropertyInfo qdev_prop_drive_iothread = { .name = "str", .description = "Node name or ID of a block device to use as a backend", + .realized_set_allowed = true, .get = get_drive, .set = set_drive_iothread, .release = release_drive, @@ -XXX,XX +XXX,XX @@ static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque, * TODO Should this really be an error? If no, the old value * needs to be released before we store the new one. */ - if (!check_prop_still_unset(obj, name, be->chr, str, errp)) { + if (!check_prop_still_unset(obj, name, be->chr, str, false, errp)) { return; } @@ -XXX,XX +XXX,XX @@ static void set_netdev(Object *obj, Visitor *v, const char *name, * TODO Should this really be an error? If no, the old value * needs to be released before we store the new one. */ - if (!check_prop_still_unset(obj, name, ncs[i], str, errp)) { + if (!check_prop_still_unset(obj, name, ncs[i], str, false, errp)) { goto out; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We are going to convert backup_top to full featured public filter, which can be used in separate of backup job. Start from renaming from "how it used" to "what it does". While updating comments in 283 iotest, drop and rephrase also things about ".active", as this field is now dropped, and filter doesn't have "inactive" mode. Note that this change may be considered as incompatible interface change, as backup-top filter format name was visible through query-block and query-named-block-nodes. Still, consider the following reasoning: 1. backup-top was never documented, so if someone depends on format name (for driver that can't be used other than it is automatically inserted on backup job start), it's a kind of "undocumented feature use". So I think we are free to change it. 2. There is a hope, that there is no such users: it's a lot more native to give a good node-name to backup-top filter if need to operate with it somehow, and don't touch format name. 3. Another "incompatible" change in further commit would be moving copy-before-write filter from using backing child to file child. And this is even more reasonable than renaming: for now all public filters are file-child based. So, it's a risky change, but risk seems small and good interface worth it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-6-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/{backup-top.h => copy-before-write.h} | 28 +++--- block/backup.c | 22 ++--- block/{backup-top.c => copy-before-write.c} | 100 ++++++++++---------- MAINTAINERS | 4 +- block/meson.build | 2 +- tests/qemu-iotests/283 | 35 +++---- tests/qemu-iotests/283.out | 4 +- 7 files changed, 95 insertions(+), 100 deletions(-) rename block/{backup-top.h => copy-before-write.h} (56%) rename block/{backup-top.c => copy-before-write.c} (62%) diff --git a/block/backup-top.h b/block/copy-before-write.h similarity index 56% rename from block/backup-top.h rename to block/copy-before-write.h index XXXXXXX..XXXXXXX 100644 --- a/block/backup-top.h +++ b/block/copy-before-write.h @@ -XXX,XX +XXX,XX @@ /* - * backup-top filter driver + * copy-before-write filter driver * * The driver performs Copy-Before-Write (CBW) operation: it is injected above * some node, and before each write it copies _old_ data to the target node. * - * Copyright (c) 2018-2019 Virtuozzo International GmbH. + * Copyright (c) 2018-2021 Virtuozzo International GmbH. * * Author: * Sementsov-Ogievskiy Vladimir <vsementsov@virtuozzo.com> @@ -XXX,XX +XXX,XX @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ -#ifndef BACKUP_TOP_H -#define BACKUP_TOP_H +#ifndef COPY_BEFORE_WRITE_H +#define COPY_BEFORE_WRITE_H #include "block/block_int.h" #include "block/block-copy.h" -BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, - BlockDriverState *target, - const char *filter_node_name, - uint64_t cluster_size, - BackupPerf *perf, - BdrvRequestFlags write_flags, - BlockCopyState **bcs, - Error **errp); -void bdrv_backup_top_drop(BlockDriverState *bs); +BlockDriverState *bdrv_cbw_append(BlockDriverState *source, + BlockDriverState *target, + const char *filter_node_name, + uint64_t cluster_size, + BackupPerf *perf, + BdrvRequestFlags write_flags, + BlockCopyState **bcs, + Error **errp); +void bdrv_cbw_drop(BlockDriverState *bs); -#endif /* BACKUP_TOP_H */ +#endif /* COPY_BEFORE_WRITE_H */ diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ #include "qemu/bitmap.h" #include "qemu/error-report.h" -#include "block/backup-top.h" +#include "block/copy-before-write.h" #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16) typedef struct BackupBlockJob { BlockJob common; - BlockDriverState *backup_top; + BlockDriverState *cbw; BlockDriverState *source_bs; BlockDriverState *target_bs; @@ -XXX,XX +XXX,XX @@ static void backup_clean(Job *job) { BackupBlockJob *s = container_of(job, BackupBlockJob, common.job); block_job_remove_all_bdrv(&s->common); - bdrv_backup_top_drop(s->backup_top); + bdrv_cbw_drop(s->cbw); } void backup_do_checkpoint(BlockJob *job, Error **errp) @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, BackupBlockJob *job = NULL; int64_t cluster_size; BdrvRequestFlags write_flags; - BlockDriverState *backup_top = NULL; + BlockDriverState *cbw = NULL; BlockCopyState *bcs = NULL; assert(bs); @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, write_flags = (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) | (compress ? BDRV_REQ_WRITE_COMPRESSED : 0), - backup_top = bdrv_backup_top_append(bs, target, filter_node_name, + cbw = bdrv_cbw_append(bs, target, filter_node_name, cluster_size, perf, write_flags, &bcs, errp); - if (!backup_top) { + if (!cbw) { goto error; } /* job->len is fixed, so we can't allow resize */ - job = block_job_create(job_id, &backup_job_driver, txn, backup_top, + job = block_job_create(job_id, &backup_job_driver, txn, cbw, 0, BLK_PERM_ALL, speed, creation_flags, cb, opaque, errp); if (!job) { goto error; } - job->backup_top = backup_top; + job->cbw = cbw; job->source_bs = bs; job->target_bs = target; job->on_source_error = on_source_error; @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, block_copy_set_progress_meter(bcs, &job->common.job.progress); block_copy_set_speed(bcs, speed); - /* Required permissions are already taken by backup-top target */ + /* Required permissions are taken by copy-before-write filter target */ block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL, &error_abort); @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, if (sync_bitmap) { bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL); } - if (backup_top) { - bdrv_backup_top_drop(backup_top); + if (cbw) { + bdrv_cbw_drop(cbw); } return NULL; diff --git a/block/backup-top.c b/block/copy-before-write.c similarity index 62% rename from block/backup-top.c rename to block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup-top.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ /* - * backup-top filter driver + * copy-before-write filter driver * * The driver performs Copy-Before-Write (CBW) operation: it is injected above * some node, and before each write it copies _old_ data to the target node. * - * Copyright (c) 2018-2019 Virtuozzo International GmbH. + * Copyright (c) 2018-2021 Virtuozzo International GmbH. * * Author: * Sementsov-Ogievskiy Vladimir <vsementsov@virtuozzo.com> @@ -XXX,XX +XXX,XX @@ #include "block/qdict.h" #include "block/block-copy.h" -#include "block/backup-top.h" +#include "block/copy-before-write.h" -typedef struct BDRVBackupTopState { +typedef struct BDRVCopyBeforeWriteState { BlockCopyState *bcs; BdrvChild *target; int64_t cluster_size; -} BDRVBackupTopState; +} BDRVCopyBeforeWriteState; -static coroutine_fn int backup_top_co_preadv( +static coroutine_fn int cbw_co_preadv( BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) { return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); } -static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset, - uint64_t bytes, BdrvRequestFlags flags) +static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs, + uint64_t offset, uint64_t bytes, BdrvRequestFlags flags) { - BDRVBackupTopState *s = bs->opaque; + BDRVCopyBeforeWriteState *s = bs->opaque; uint64_t off, end; if (flags & BDRV_REQ_WRITE_UNCHANGED) { @@ -XXX,XX +XXX,XX @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset, return block_copy(s->bcs, off, end - off, true); } -static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs, - int64_t offset, int bytes) +static int coroutine_fn cbw_co_pdiscard(BlockDriverState *bs, + int64_t offset, int bytes) { - int ret = backup_top_cbw(bs, offset, bytes, 0); + int ret = cbw_do_copy_before_write(bs, offset, bytes, 0); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs, return bdrv_co_pdiscard(bs->backing, offset, bytes); } -static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs, +static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes, BdrvRequestFlags flags) { - int ret = backup_top_cbw(bs, offset, bytes, flags); + int ret = cbw_do_copy_before_write(bs, offset, bytes, flags); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs, return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags); } -static coroutine_fn int backup_top_co_pwritev(BlockDriverState *bs, - uint64_t offset, - uint64_t bytes, - QEMUIOVector *qiov, int flags) +static coroutine_fn int cbw_co_pwritev(BlockDriverState *bs, + uint64_t offset, + uint64_t bytes, + QEMUIOVector *qiov, int flags) { - int ret = backup_top_cbw(bs, offset, bytes, flags); + int ret = cbw_do_copy_before_write(bs, offset, bytes, flags); if (ret < 0) { return ret; } @@ -XXX,XX +XXX,XX @@ static coroutine_fn int backup_top_co_pwritev(BlockDriverState *bs, return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags); } -static int coroutine_fn backup_top_co_flush(BlockDriverState *bs) +static int coroutine_fn cbw_co_flush(BlockDriverState *bs) { if (!bs->backing) { return 0; @@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_top_co_flush(BlockDriverState *bs) return bdrv_co_flush(bs->backing->bs); } -static void backup_top_refresh_filename(BlockDriverState *bs) +static void cbw_refresh_filename(BlockDriverState *bs) { if (bs->backing == NULL) { /* @@ -XXX,XX +XXX,XX @@ static void backup_top_refresh_filename(BlockDriverState *bs) bs->backing->bs->filename); } -static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c, - BdrvChildRole role, - BlockReopenQueue *reopen_queue, - uint64_t perm, uint64_t shared, - uint64_t *nperm, uint64_t *nshared) +static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, + BdrvChildRole role, + BlockReopenQueue *reopen_queue, + uint64_t perm, uint64_t shared, + uint64_t *nperm, uint64_t *nshared) { if (!(role & BDRV_CHILD_FILTERED)) { /* @@ -XXX,XX +XXX,XX @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c, } } -BlockDriver bdrv_backup_top_filter = { - .format_name = "backup-top", - .instance_size = sizeof(BDRVBackupTopState), +BlockDriver bdrv_cbw_filter = { + .format_name = "copy-before-write", + .instance_size = sizeof(BDRVCopyBeforeWriteState), - .bdrv_co_preadv = backup_top_co_preadv, - .bdrv_co_pwritev = backup_top_co_pwritev, - .bdrv_co_pwrite_zeroes = backup_top_co_pwrite_zeroes, - .bdrv_co_pdiscard = backup_top_co_pdiscard, - .bdrv_co_flush = backup_top_co_flush, + .bdrv_co_preadv = cbw_co_preadv, + .bdrv_co_pwritev = cbw_co_pwritev, + .bdrv_co_pwrite_zeroes = cbw_co_pwrite_zeroes, + .bdrv_co_pdiscard = cbw_co_pdiscard, + .bdrv_co_flush = cbw_co_flush, - .bdrv_refresh_filename = backup_top_refresh_filename, + .bdrv_refresh_filename = cbw_refresh_filename, - .bdrv_child_perm = backup_top_child_perm, + .bdrv_child_perm = cbw_child_perm, .is_filter = true, }; -BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, - BlockDriverState *target, - const char *filter_node_name, - uint64_t cluster_size, - BackupPerf *perf, - BdrvRequestFlags write_flags, - BlockCopyState **bcs, - Error **errp) +BlockDriverState *bdrv_cbw_append(BlockDriverState *source, + BlockDriverState *target, + const char *filter_node_name, + uint64_t cluster_size, + BackupPerf *perf, + BdrvRequestFlags write_flags, + BlockCopyState **bcs, + Error **errp) { ERRP_GUARD(); int ret; - BDRVBackupTopState *state; + BDRVCopyBeforeWriteState *state; BlockDriverState *top; bool appended = false; assert(source->total_sectors == target->total_sectors); - top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name, + top = bdrv_new_open_driver(&bdrv_cbw_filter, filter_node_name, BDRV_O_RDWR, errp); if (!top) { return NULL; @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, ret = bdrv_append(top, source, errp); if (ret < 0) { - error_prepend(errp, "Cannot append backup-top filter: "); + error_prepend(errp, "Cannot append copy-before-write filter: "); goto fail; } appended = true; @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, fail: if (appended) { - bdrv_backup_top_drop(top); + bdrv_cbw_drop(top); } else { bdrv_unref(top); } @@ -XXX,XX +XXX,XX @@ fail: return NULL; } -void bdrv_backup_top_drop(BlockDriverState *bs) +void bdrv_cbw_drop(BlockDriverState *bs) { - BDRVBackupTopState *s = bs->opaque; + BDRVCopyBeforeWriteState *s = bs->opaque; bdrv_drop_filter(bs, &error_abort); diff --git a/MAINTAINERS b/MAINTAINERS index XXXXXXX..XXXXXXX 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -XXX,XX +XXX,XX @@ F: block/mirror.c F: qapi/job.json F: block/block-copy.c F: include/block/block-copy.c -F: block/backup-top.h -F: block/backup-top.c +F: block/copy-before-write.h +F: block/copy-before-write.c F: include/block/aio_task.h F: block/aio_task.c F: util/qemu-co-shared-resource.c diff --git a/block/meson.build b/block/meson.build index XXXXXXX..XXXXXXX 100644 --- a/block/meson.build +++ b/block/meson.build @@ -XXX,XX +XXX,XX @@ block_ss.add(files( 'aio_task.c', 'amend.c', 'backup.c', - 'backup-top.c', + 'copy-before-write.c', 'blkdebug.c', 'blklogwrites.c', 'blkverify.c', diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/283 +++ b/tests/qemu-iotests/283 @@ -XXX,XX +XXX,XX @@ #!/usr/bin/env python3 # group: auto quick # -# Test for backup-top filter permission activation failure +# Test for copy-before-write filter permission conflict # # Copyright (c) 2019 Virtuozzo International GmbH. # @@ -XXX,XX +XXX,XX @@ size = 1024 * 1024 """ Test description When performing a backup, all writes on the source subtree must go through the -backup-top filter so it can copy all data to the target before it is changed. -backup-top filter is appended above source node, to achieve this thing, so all -parents of source node are handled. A configuration with side parents of source -sub-tree with write permission is unsupported (we'd have append several -backup-top filter like nodes to handle such parents). The test create an -example of such configuration and checks that a backup is then not allowed -(blockdev-backup command should fail). +copy-before-write filter so it can copy all data to the target before it is +changed. copy-before-write filter is appended above source node, to achieve +this thing, so all parents of source node are handled. A configuration with +side parents of source sub-tree with write permission is unsupported (we'd have +append several copy-before-write filter like nodes to handle such parents). The +test create an example of such configuration and checks that a backup is then +not allowed (blockdev-backup command should fail). The configuration: @@ -XXX,XX +XXX,XX @@ The configuration: │ base │ ◀──────────── │ other │ └─────────────┘ └───────┘ -On activation (see .active field of backup-top state in block/backup-top.c), -backup-top is going to unshare write permission on its source child. Write -unsharing will be propagated to the "source->base" link and will conflict with -other node write permission. So permission update will fail and backup job will -not be started. +copy-before-write filter wants to unshare write permission on its source child. +Write unsharing will be propagated to the "source->base" link and will conflict +with other node write permission. So permission update will fail and backup job +will not be started. Note, that the only thing which prevents backup of running on such configuration is default permission propagation scheme. It may be altered by @@ -XXX,XX +XXX,XX @@ vm.qmp_log('blockdev-backup', sync='full', device='source', target='target') vm.shutdown() -print('\n=== backup-top should be gone after job-finalize ===\n') +print('\n=== copy-before-write filter should be gone after job-finalize ===\n') -# Check that the backup-top node is gone after job-finalize. -# -# During finalization, the node becomes inactive and can no longer -# function. If it is still present, new parents might be attached, and -# there would be no meaningful way to handle their I/O requests. +# Check that the copy-before-write node is gone after job-finalize. vm = iotests.VM() vm.launch() @@ -XXX,XX +XXX,XX @@ vm.qmp_log('blockdev-backup', vm.event_wait('BLOCK_JOB_PENDING', 5.0) -# The backup-top filter should still be present prior to finalization +# The copy-before-write filter should still be present prior to finalization assert vm.node_info('backup-filter') is not None vm.qmp_log('job-finalize', id='backup') diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -XXX,XX +XXX,XX @@ {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} {"return": {}} {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} -{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Permission conflict on node 'base': permissions 'write' are both required by node 'other' (uses node 'base' as 'image' child) and unshared by node 'source' (uses node 'base' as 'image' child)."}} +{"error": {"class": "GenericError", "desc": "Cannot append copy-before-write filter: Permission conflict on node 'base': permissions 'write' are both required by node 'other' (uses node 'base' as 'image' child) and unshared by node 'source' (uses node 'base' as 'image' child)."}} -=== backup-top should be gone after job-finalize === +=== copy-before-write filter should be gone after job-finalize === {"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "source"}} {"return": {}} -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We want to simplify initialization interface of copy-before-write filter as we are going to make it public. So, let's detect fleecing scheme exactly in block-copy code, to not pass this information through extra levels. Why not just set BDRV_REQ_SERIALISING unconditionally: because we are going to implement new more efficient fleecing scheme which will not rely on backing feature. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20210824083856.17408-7-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.h | 2 +- include/block/block-copy.h | 3 +-- block/backup.c | 21 +-------------------- block/block-copy.c | 24 +++++++++++++++++++++--- block/copy-before-write.c | 4 ++-- 5 files changed, 26 insertions(+), 28 deletions(-) diff --git a/block/copy-before-write.h b/block/copy-before-write.h index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.h +++ b/block/copy-before-write.h @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, const char *filter_node_name, uint64_t cluster_size, BackupPerf *perf, - BdrvRequestFlags write_flags, + bool compress, BlockCopyState **bcs, Error **errp); void bdrv_cbw_drop(BlockDriverState *bs); diff --git a/include/block/block-copy.h b/include/block/block-copy.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -XXX,XX +XXX,XX @@ typedef struct BlockCopyCallState BlockCopyCallState; BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, int64_t cluster_size, bool use_copy_range, - BdrvRequestFlags write_flags, - Error **errp); + bool compress, Error **errp); void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm); diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, int64_t len, target_len; BackupBlockJob *job = NULL; int64_t cluster_size; - BdrvRequestFlags write_flags; BlockDriverState *cbw = NULL; BlockCopyState *bcs = NULL; @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, goto error; } - /* - * If source is in backing chain of target assume that target is going to be - * used for "image fleecing", i.e. it should represent a kind of snapshot of - * source at backup-start point in time. And target is going to be read by - * somebody (for example, used as NBD export) during backup job. - * - * In this case, we need to add BDRV_REQ_SERIALISING write flag to avoid - * intersection of backup writes and third party reads from target, - * otherwise reading from target we may occasionally read already updated by - * guest data. - * - * For more information see commit f8d59dfb40bb and test - * tests/qemu-iotests/222 - */ - write_flags = (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) | - (compress ? BDRV_REQ_WRITE_COMPRESSED : 0), - cbw = bdrv_cbw_append(bs, target, filter_node_name, - cluster_size, perf, - write_flags, &bcs, errp); + cluster_size, perf, compress, &bcs, errp); if (!cbw) { goto error; } diff --git a/block/block-copy.c b/block/block-copy.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -XXX,XX +XXX,XX @@ static uint32_t block_copy_max_transfer(BdrvChild *source, BdrvChild *target) BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, int64_t cluster_size, bool use_copy_range, - BdrvRequestFlags write_flags, Error **errp) + bool compress, Error **errp) { BlockCopyState *s; BdrvDirtyBitmap *copy_bitmap; + bool is_fleecing; copy_bitmap = bdrv_create_dirty_bitmap(source->bs, cluster_size, NULL, errp); @@ -XXX,XX +XXX,XX @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, } bdrv_disable_dirty_bitmap(copy_bitmap); + /* + * If source is in backing chain of target assume that target is going to be + * used for "image fleecing", i.e. it should represent a kind of snapshot of + * source at backup-start point in time. And target is going to be read by + * somebody (for example, used as NBD export) during backup job. + * + * In this case, we need to add BDRV_REQ_SERIALISING write flag to avoid + * intersection of backup writes and third party reads from target, + * otherwise reading from target we may occasionally read already updated by + * guest data. + * + * For more information see commit f8d59dfb40bb and test + * tests/qemu-iotests/222 + */ + is_fleecing = bdrv_chain_contains(target->bs, source->bs); + s = g_new(BlockCopyState, 1); *s = (BlockCopyState) { .source = source, @@ -XXX,XX +XXX,XX @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, .copy_bitmap = copy_bitmap, .cluster_size = cluster_size, .len = bdrv_dirty_bitmap_size(copy_bitmap), - .write_flags = write_flags, + .write_flags = (is_fleecing ? BDRV_REQ_SERIALISING : 0) | + (compress ? BDRV_REQ_WRITE_COMPRESSED : 0), .mem = shres_create(BLOCK_COPY_MAX_MEM), .max_transfer = QEMU_ALIGN_DOWN( block_copy_max_transfer(source, target), @@ -XXX,XX +XXX,XX @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, * behalf). */ s->method = COPY_READ_WRITE_CLUSTER; - } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) { + } else if (compress) { /* Compression supports only cluster-size writes and no copy-range. */ s->method = COPY_READ_WRITE_CLUSTER; } else { diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, const char *filter_node_name, uint64_t cluster_size, BackupPerf *perf, - BdrvRequestFlags write_flags, + bool compress, BlockCopyState **bcs, Error **errp) { @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, state->cluster_size = cluster_size; state->bcs = block_copy_state_new(top->backing, state->target, cluster_size, perf->use_copy_range, - write_flags, errp); + compress, errp); if (!state->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); goto fail; -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We'll need a possibility to set compress and use_copy_range options after initialization of the state. So make corresponding part of block_copy_state_new() separate and public. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20210824083856.17408-8-vsementsov@virtuozzo.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/block/block-copy.h | 3 +++ block/block-copy.c | 49 ++++++++++++++++++++++---------------- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/include/block/block-copy.h b/include/block/block-copy.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -XXX,XX +XXX,XX @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, int64_t cluster_size, bool use_copy_range, bool compress, Error **errp); +/* Function should be called prior any actual copy request */ +void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range, + bool compress); void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm); void block_copy_state_free(BlockCopyState *s); diff --git a/block/block-copy.c b/block/block-copy.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -XXX,XX +XXX,XX @@ static uint32_t block_copy_max_transfer(BdrvChild *source, BdrvChild *target) target->bs->bl.max_transfer)); } +void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range, + bool compress) +{ + /* Keep BDRV_REQ_SERIALISING set (or not set) in block_copy_state_new() */ + s->write_flags = (s->write_flags & BDRV_REQ_SERIALISING) | + (compress ? BDRV_REQ_WRITE_COMPRESSED : 0); + + if (s->max_transfer < s->cluster_size) { + /* + * copy_range does not respect max_transfer. We don't want to bother + * with requests smaller than block-copy cluster size, so fallback to + * buffered copying (read and write respect max_transfer on their + * behalf). + */ + s->method = COPY_READ_WRITE_CLUSTER; + } else if (compress) { + /* Compression supports only cluster-size writes and no copy-range. */ + s->method = COPY_READ_WRITE_CLUSTER; + } else { + /* + * If copy range enabled, start with COPY_RANGE_SMALL, until first + * successful copy_range (look at block_copy_do_copy). + */ + s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE; + } +} + BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, int64_t cluster_size, bool use_copy_range, bool compress, Error **errp) @@ -XXX,XX +XXX,XX @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, .copy_bitmap = copy_bitmap, .cluster_size = cluster_size, .len = bdrv_dirty_bitmap_size(copy_bitmap), - .write_flags = (is_fleecing ? BDRV_REQ_SERIALISING : 0) | - (compress ? BDRV_REQ_WRITE_COMPRESSED : 0), + .write_flags = (is_fleecing ? BDRV_REQ_SERIALISING : 0), .mem = shres_create(BLOCK_COPY_MAX_MEM), .max_transfer = QEMU_ALIGN_DOWN( block_copy_max_transfer(source, target), cluster_size), }; - if (s->max_transfer < cluster_size) { - /* - * copy_range does not respect max_transfer. We don't want to bother - * with requests smaller than block-copy cluster size, so fallback to - * buffered copying (read and write respect max_transfer on their - * behalf). - */ - s->method = COPY_READ_WRITE_CLUSTER; - } else if (compress) { - /* Compression supports only cluster-size writes and no copy-range. */ - s->method = COPY_READ_WRITE_CLUSTER; - } else { - /* - * If copy range enabled, start with COPY_RANGE_SMALL, until first - * successful copy_range (look at block_copy_do_copy). - */ - s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE; - } + block_copy_set_copy_opts(s, use_copy_range, compress); ratelimit_init(&s->rate_limit); qemu_co_mutex_init(&s->lock); -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We are going to publish copy-before-write filter, so it would be initialized through options. Still we don't want to publish compress and copy-range options, as 1. Modern way to enable compression is to use compress filter. 2. For copy-range it's unclean how to make proper interface: - it's has experimental prefix for backup job anyway - the whole BackupPerf structure doesn't make sense for the filter So, let's just add copy-range possibility to the filter later if needed. Still, we are going to continue support for compression and experimental copy-range in backup job. So, set these options after filter creation. Note, that we can drop "compress" argument of bdrv_cbw_append() now, as well as "perf". The only reason not doing so is that now, when I prepare this patch the big series around it is already reviewed and I want to avoid extra rebase conflicts to simplify review of the following version. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20210824083856.17408-9-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.h | 1 - block/backup.c | 3 ++- block/copy-before-write.c | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/block/copy-before-write.h b/block/copy-before-write.h index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.h +++ b/block/copy-before-write.h @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, uint64_t cluster_size, - BackupPerf *perf, bool compress, BlockCopyState **bcs, Error **errp); diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, } cbw = bdrv_cbw_append(bs, target, filter_node_name, - cluster_size, perf, compress, &bcs, errp); + cluster_size, false, &bcs, errp); if (!cbw) { goto error; } @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, job->len = len; job->perf = *perf; + block_copy_set_copy_opts(bcs, perf->use_copy_range, compress); block_copy_set_progress_meter(bcs, &job->common.job.progress); block_copy_set_speed(bcs, speed); diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, uint64_t cluster_size, - BackupPerf *perf, bool compress, BlockCopyState **bcs, Error **errp) @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, state->cluster_size = cluster_size; state->bcs = block_copy_state_new(top->backing, state->target, - cluster_size, perf->use_copy_range, - compress, errp); + cluster_size, false, compress, errp); if (!state->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); goto fail; -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> The main consumer of cluster-size is block-copy. Let's calculate it here instead of passing through backup-top. We are going to publish copy-before-write filter soon, so it will be created through options. But we don't want for now to make explicit option for cluster-size, let's continue to calculate it automatically. So, now is the time to get rid of cluster_size argument for bdrv_cbw_append(). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-10-vsementsov@virtuozzo.com> [hreitz: Add qemu/error-report.h include to block/block-copy.c] Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.h | 1 - include/block/block-copy.h | 5 +-- block/backup.c | 62 ++++++-------------------------------- block/block-copy.c | 52 +++++++++++++++++++++++++++++++- block/copy-before-write.c | 10 +++--- 5 files changed, 67 insertions(+), 63 deletions(-) diff --git a/block/copy-before-write.h b/block/copy-before-write.h index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.h +++ b/block/copy-before-write.h @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, - uint64_t cluster_size, bool compress, BlockCopyState **bcs, Error **errp); diff --git a/include/block/block-copy.h b/include/block/block-copy.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -XXX,XX +XXX,XX @@ typedef struct BlockCopyState BlockCopyState; typedef struct BlockCopyCallState BlockCopyCallState; BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, - int64_t cluster_size, bool use_copy_range, - bool compress, Error **errp); + bool use_copy_range, bool compress, + Error **errp); /* Function should be called prior any actual copy request */ void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range, @@ -XXX,XX +XXX,XX @@ void block_copy_kick(BlockCopyCallState *call_state); void block_copy_call_cancel(BlockCopyCallState *call_state); BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s); +int64_t block_copy_cluster_size(BlockCopyState *s); void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip); #endif /* BLOCK_COPY_H */ diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ #include "block/copy-before-write.h" -#define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16) - typedef struct BackupBlockJob { BlockJob common; BlockDriverState *cbw; @@ -XXX,XX +XXX,XX @@ static const BlockJobDriver backup_job_driver = { .set_speed = backup_set_speed, }; -static int64_t backup_calculate_cluster_size(BlockDriverState *target, - Error **errp) -{ - int ret; - BlockDriverInfo bdi; - bool target_does_cow = bdrv_backing_chain_next(target); - - /* - * If there is no backing file on the target, we cannot rely on COW if our - * backup cluster size is smaller than the target cluster size. Even for - * targets with a backing file, try to avoid COW if possible. - */ - ret = bdrv_get_info(target, &bdi); - if (ret == -ENOTSUP && !target_does_cow) { - /* Cluster size is not defined */ - warn_report("The target block device doesn't provide " - "information about the block size and it doesn't have a " - "backing file. The default block size of %u bytes is " - "used. If the actual block size of the target exceeds " - "this default, the backup may be unusable", - BACKUP_CLUSTER_SIZE_DEFAULT); - return BACKUP_CLUSTER_SIZE_DEFAULT; - } else if (ret < 0 && !target_does_cow) { - error_setg_errno(errp, -ret, - "Couldn't determine the cluster size of the target image, " - "which has no backing file"); - error_append_hint(errp, - "Aborting, since this may create an unusable destination image\n"); - return ret; - } else if (ret < 0 && target_does_cow) { - /* Not fatal; just trudge on ahead. */ - return BACKUP_CLUSTER_SIZE_DEFAULT; - } - - return MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size); -} - BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, BlockDriverState *target, int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, return NULL; } - cluster_size = backup_calculate_cluster_size(target, errp); - if (cluster_size < 0) { - goto error; - } - if (perf->max_workers < 1) { error_setg(errp, "max-workers must be greater than zero"); return NULL; @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, return NULL; } - if (perf->max_chunk && perf->max_chunk < cluster_size) { - error_setg(errp, "Required max-chunk (%" PRIi64 ") is less than backup " - "cluster size (%" PRIi64 ")", perf->max_chunk, cluster_size); - return NULL; - } - - if (sync_bitmap) { /* If we need to write to this bitmap, check that we can: */ if (bitmap_mode != BITMAP_SYNC_MODE_NEVER && @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, goto error; } - cbw = bdrv_cbw_append(bs, target, filter_node_name, - cluster_size, false, &bcs, errp); + cbw = bdrv_cbw_append(bs, target, filter_node_name, false, &bcs, errp); if (!cbw) { goto error; } + cluster_size = block_copy_cluster_size(bcs); + + if (perf->max_chunk && perf->max_chunk < cluster_size) { + error_setg(errp, "Required max-chunk (%" PRIi64 ") is less than backup " + "cluster size (%" PRIi64 ")", perf->max_chunk, cluster_size); + goto error; + } + /* job->len is fixed, so we can't allow resize */ job = block_job_create(job_id, &backup_job_driver, txn, cbw, 0, BLK_PERM_ALL, diff --git a/block/block-copy.c b/block/block-copy.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -XXX,XX +XXX,XX @@ #include "qemu/units.h" #include "qemu/coroutine.h" #include "block/aio_task.h" +#include "qemu/error-report.h" #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB) #define BLOCK_COPY_MAX_BUFFER (1 * MiB) #define BLOCK_COPY_MAX_MEM (128 * MiB) #define BLOCK_COPY_MAX_WORKERS 64 #define BLOCK_COPY_SLICE_TIME 100000000ULL /* ns */ +#define BLOCK_COPY_CLUSTER_SIZE_DEFAULT (1 << 16) typedef enum { COPY_READ_WRITE_CLUSTER, @@ -XXX,XX +XXX,XX @@ void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range, } } +static int64_t block_copy_calculate_cluster_size(BlockDriverState *target, + Error **errp) +{ + int ret; + BlockDriverInfo bdi; + bool target_does_cow = bdrv_backing_chain_next(target); + + /* + * If there is no backing file on the target, we cannot rely on COW if our + * backup cluster size is smaller than the target cluster size. Even for + * targets with a backing file, try to avoid COW if possible. + */ + ret = bdrv_get_info(target, &bdi); + if (ret == -ENOTSUP && !target_does_cow) { + /* Cluster size is not defined */ + warn_report("The target block device doesn't provide " + "information about the block size and it doesn't have a " + "backing file. The default block size of %u bytes is " + "used. If the actual block size of the target exceeds " + "this default, the backup may be unusable", + BLOCK_COPY_CLUSTER_SIZE_DEFAULT); + return BLOCK_COPY_CLUSTER_SIZE_DEFAULT; + } else if (ret < 0 && !target_does_cow) { + error_setg_errno(errp, -ret, + "Couldn't determine the cluster size of the target image, " + "which has no backing file"); + error_append_hint(errp, + "Aborting, since this may create an unusable destination image\n"); + return ret; + } else if (ret < 0 && target_does_cow) { + /* Not fatal; just trudge on ahead. */ + return BLOCK_COPY_CLUSTER_SIZE_DEFAULT; + } + + return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size); +} + BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, - int64_t cluster_size, bool use_copy_range, + bool use_copy_range, bool compress, Error **errp) { BlockCopyState *s; + int64_t cluster_size; BdrvDirtyBitmap *copy_bitmap; bool is_fleecing; + cluster_size = block_copy_calculate_cluster_size(target->bs, errp); + if (cluster_size < 0) { + return NULL; + } + copy_bitmap = bdrv_create_dirty_bitmap(source->bs, cluster_size, NULL, errp); if (!copy_bitmap) { @@ -XXX,XX +XXX,XX @@ BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s) return s->copy_bitmap; } +int64_t block_copy_cluster_size(BlockCopyState *s) +{ + return s->cluster_size; +} + void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip) { qatomic_set(&s->skip_unallocated, skip); diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ typedef struct BDRVCopyBeforeWriteState { BlockCopyState *bcs; BdrvChild *target; - int64_t cluster_size; } BDRVCopyBeforeWriteState; static coroutine_fn int cbw_co_preadv( @@ -XXX,XX +XXX,XX @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs, { BDRVCopyBeforeWriteState *s = bs->opaque; uint64_t off, end; + int64_t cluster_size = block_copy_cluster_size(s->bcs); if (flags & BDRV_REQ_WRITE_UNCHANGED) { return 0; } - off = QEMU_ALIGN_DOWN(offset, s->cluster_size); - end = QEMU_ALIGN_UP(offset + bytes, s->cluster_size); + off = QEMU_ALIGN_DOWN(offset, cluster_size); + end = QEMU_ALIGN_UP(offset + bytes, cluster_size); return block_copy(s->bcs, off, end - off, true); } @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_cbw_filter = { BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, - uint64_t cluster_size, bool compress, BlockCopyState **bcs, Error **errp) @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, } appended = true; - state->cluster_size = cluster_size; state->bcs = block_copy_state_new(top->backing, state->target, - cluster_size, false, compress, errp); + false, compress, errp); if (!state->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); goto fail; -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We are going to publish copy-before-write filter. So, user should be able to create it with blockdev-add first, specifying both filtered and target children. And then do blockdev-reopen, to actually insert the filter where needed. Currently, filter unshares write permission unconditionally on source node. It's good, but it will not allow to do blockdev-add. So, let's relax restrictions when filter doesn't have any parent. Test output is modified, as now permission conflict happens only when job creates a blk parent for filter node. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-11-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 8 +++++--- tests/qemu-iotests/283.out | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm, nshared); - if (perm & BLK_PERM_WRITE) { - *nperm = *nperm | BLK_PERM_CONSISTENT_READ; + if (!QLIST_EMPTY(&bs->parents)) { + if (perm & BLK_PERM_WRITE) { + *nperm = *nperm | BLK_PERM_CONSISTENT_READ; + } + *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE); } - *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE); } } diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -XXX,XX +XXX,XX @@ {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} {"return": {}} {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} -{"error": {"class": "GenericError", "desc": "Cannot append copy-before-write filter: Permission conflict on node 'base': permissions 'write' are both required by node 'other' (uses node 'base' as 'image' child) and unshared by node 'source' (uses node 'base' as 'image' child)."}} +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'base': permissions 'write' are both required by node 'other' (uses node 'base' as 'image' child) and unshared by node 'source' (uses node 'base' as 'image' child)."}} === copy-before-write filter should be gone after job-finalize === -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> bdrv_attach_child() do bdrv_unref() on failure, so we shouldn't do it by hand here. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-12-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 1 - 1 file changed, 1 deletion(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, state->target = bdrv_attach_child(top, target, "target", &child_of_bds, BDRV_CHILD_DATA, errp); if (!state->target) { - bdrv_unref(target); bdrv_unref(top); return NULL; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We are going to publish copy-before-write filter, and there no public backing-child-based filter in Qemu. No reason to create a precedent, so let's refactor copy-before-write filter instead. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-13-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static coroutine_fn int cbw_co_preadv( BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) { - return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); + return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags); } static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs, @@ -XXX,XX +XXX,XX @@ static int coroutine_fn cbw_co_pdiscard(BlockDriverState *bs, return ret; } - return bdrv_co_pdiscard(bs->backing, offset, bytes); + return bdrv_co_pdiscard(bs->file, offset, bytes); } static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState *bs, @@ -XXX,XX +XXX,XX @@ static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState *bs, return ret; } - return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags); + return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags); } static coroutine_fn int cbw_co_pwritev(BlockDriverState *bs, @@ -XXX,XX +XXX,XX @@ static coroutine_fn int cbw_co_pwritev(BlockDriverState *bs, return ret; } - return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags); + return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags); } static int coroutine_fn cbw_co_flush(BlockDriverState *bs) { - if (!bs->backing) { + if (!bs->file) { return 0; } - return bdrv_co_flush(bs->backing->bs); + return bdrv_co_flush(bs->file->bs); } static void cbw_refresh_filename(BlockDriverState *bs) { - if (bs->backing == NULL) { - /* - * we can be here after failed bdrv_attach_child in - * bdrv_set_backing_hd - */ - return; - } pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), - bs->backing->bs->filename); + bs->file->bs->filename); } static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, top = bdrv_new_open_driver(&bdrv_cbw_filter, filter_node_name, BDRV_O_RDWR, errp); if (!top) { + error_prepend(errp, "Cannot open driver: "); return NULL; } @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, state->target = bdrv_attach_child(top, target, "target", &child_of_bds, BDRV_CHILD_DATA, errp); if (!state->target) { + error_prepend(errp, "Cannot attach target child: "); + bdrv_unref(top); + return NULL; + } + + bdrv_ref(source); + top->file = bdrv_attach_child(top, source, "file", &child_of_bds, + BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, + errp); + if (!top->file) { + error_prepend(errp, "Cannot attach file child: "); bdrv_unref(top); return NULL; } bdrv_drained_begin(source); - ret = bdrv_append(top, source, errp); + ret = bdrv_replace_node(source, top, errp); if (ret < 0) { error_prepend(errp, "Cannot append copy-before-write filter: "); goto fail; } appended = true; - state->bcs = block_copy_state_new(top->backing, state->target, - false, compress, errp); + state->bcs = block_copy_state_new(top->file, state->target, false, compress, + errp); if (!state->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); goto fail; -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Refactor the function to replace child at last. Thus we don't need to revert it and code is simplified. block-copy state initialization being done before replacing the child doesn't need any drained section. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-14-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, int ret; BDRVCopyBeforeWriteState *state; BlockDriverState *top; - bool appended = false; assert(source->total_sectors == target->total_sectors); @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BDRV_CHILD_DATA, errp); if (!state->target) { error_prepend(errp, "Cannot attach target child: "); - bdrv_unref(top); - return NULL; + goto fail; } bdrv_ref(source); @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, errp); if (!top->file) { error_prepend(errp, "Cannot attach file child: "); - bdrv_unref(top); - return NULL; - } - - bdrv_drained_begin(source); - - ret = bdrv_replace_node(source, top, errp); - if (ret < 0) { - error_prepend(errp, "Cannot append copy-before-write filter: "); goto fail; } - appended = true; state->bcs = block_copy_state_new(top->file, state->target, false, compress, errp); @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, error_prepend(errp, "Cannot create block-copy-state: "); goto fail; } - *bcs = state->bcs; + bdrv_drained_begin(source); + ret = bdrv_replace_node(source, top, errp); bdrv_drained_end(source); + if (ret < 0) { + error_prepend(errp, "Cannot append copy-before-write filter: "); + goto fail; + } + + *bcs = state->bcs; return top; fail: - if (appended) { - bdrv_cbw_drop(top); - } else { - bdrv_unref(top); - } - - bdrv_drained_end(source); - + block_copy_state_free(state->bcs); + bdrv_unref(top); return NULL; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Move part of bdrv_cbw_append() to new function cbw_open(). It's an intermediate step for adding normal .bdrv_open() handler to the filter. With this commit no logic is changed, but we have a function which will be turned into .bdrv_open() handler in future commit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-15-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 69 +++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, } } +static int cbw_init(BlockDriverState *top, BlockDriverState *source, + BlockDriverState *target, bool compress, Error **errp) +{ + BDRVCopyBeforeWriteState *state = top->opaque; + + top->total_sectors = source->total_sectors; + top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | + (BDRV_REQ_FUA & source->supported_write_flags); + top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | + ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & + source->supported_zero_flags); + + bdrv_ref(target); + state->target = bdrv_attach_child(top, target, "target", &child_of_bds, + BDRV_CHILD_DATA, errp); + if (!state->target) { + error_prepend(errp, "Cannot attach target child: "); + return -EINVAL; + } + + bdrv_ref(source); + top->file = bdrv_attach_child(top, source, "file", &child_of_bds, + BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, + errp); + if (!top->file) { + error_prepend(errp, "Cannot attach file child: "); + return -EINVAL; + } + + state->bcs = block_copy_state_new(top->file, state->target, false, compress, + errp); + if (!state->bcs) { + error_prepend(errp, "Cannot create block-copy-state: "); + return -EINVAL; + } + + return 0; +} + BlockDriver bdrv_cbw_filter = { .format_name = "copy-before-write", .instance_size = sizeof(BDRVCopyBeforeWriteState), @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, error_prepend(errp, "Cannot open driver: "); return NULL; } - state = top->opaque; - top->total_sectors = source->total_sectors; - top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | - (BDRV_REQ_FUA & source->supported_write_flags); - top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | - ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & - source->supported_zero_flags); - - bdrv_ref(target); - state->target = bdrv_attach_child(top, target, "target", &child_of_bds, - BDRV_CHILD_DATA, errp); - if (!state->target) { - error_prepend(errp, "Cannot attach target child: "); - goto fail; - } - bdrv_ref(source); - top->file = bdrv_attach_child(top, source, "file", &child_of_bds, - BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - errp); - if (!top->file) { - error_prepend(errp, "Cannot attach file child: "); - goto fail; - } - - state->bcs = block_copy_state_new(top->file, state->target, false, compress, - errp); - if (!state->bcs) { - error_prepend(errp, "Cannot create block-copy-state: "); + ret = cbw_init(top, source, target, compress, errp); + if (ret < 0) { goto fail; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> One more step closer to real .bdrv_open() handler: use more usual names for bs being initialized and its state. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-16-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, } } -static int cbw_init(BlockDriverState *top, BlockDriverState *source, +static int cbw_init(BlockDriverState *bs, BlockDriverState *source, BlockDriverState *target, bool compress, Error **errp) { - BDRVCopyBeforeWriteState *state = top->opaque; + BDRVCopyBeforeWriteState *s = bs->opaque; - top->total_sectors = source->total_sectors; - top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | + bs->total_sectors = source->total_sectors; + bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | (BDRV_REQ_FUA & source->supported_write_flags); - top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | + bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & source->supported_zero_flags); bdrv_ref(target); - state->target = bdrv_attach_child(top, target, "target", &child_of_bds, - BDRV_CHILD_DATA, errp); - if (!state->target) { + s->target = bdrv_attach_child(bs, target, "target", &child_of_bds, + BDRV_CHILD_DATA, errp); + if (!s->target) { error_prepend(errp, "Cannot attach target child: "); return -EINVAL; } bdrv_ref(source); - top->file = bdrv_attach_child(top, source, "file", &child_of_bds, - BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - errp); - if (!top->file) { + bs->file = bdrv_attach_child(bs, source, "file", &child_of_bds, + BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, + errp); + if (!bs->file) { error_prepend(errp, "Cannot attach file child: "); return -EINVAL; } - state->bcs = block_copy_state_new(top->file, state->target, false, compress, - errp); - if (!state->bcs) { + s->bcs = block_copy_state_new(bs->file, s->target, false, compress, errp); + if (!s->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); return -EINVAL; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> In the next commit we'll get rid of source argument of cbw_init(). Prepare to it now, to make next commit simpler: move the code block that uses source below attaching the child and use bs->file->bs instead of source variable. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-17-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static int cbw_init(BlockDriverState *bs, BlockDriverState *source, { BDRVCopyBeforeWriteState *s = bs->opaque; - bs->total_sectors = source->total_sectors; - bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | - (BDRV_REQ_FUA & source->supported_write_flags); - bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | - ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & - source->supported_zero_flags); - bdrv_ref(target); s->target = bdrv_attach_child(bs, target, "target", &child_of_bds, BDRV_CHILD_DATA, errp); @@ -XXX,XX +XXX,XX @@ static int cbw_init(BlockDriverState *bs, BlockDriverState *source, return -EINVAL; } + bs->total_sectors = bs->file->bs->total_sectors; + bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | + (BDRV_REQ_FUA & bs->file->bs->supported_write_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); + s->bcs = block_copy_state_new(bs->file, s->target, false, compress, errp); if (!s->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20210824083856.17408-18-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.h | 1 - block/backup.c | 2 +- block/copy-before-write.c | 7 +++---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/block/copy-before-write.h b/block/copy-before-write.h index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.h +++ b/block/copy-before-write.h @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, - bool compress, BlockCopyState **bcs, Error **errp); void bdrv_cbw_drop(BlockDriverState *bs); diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, goto error; } - cbw = bdrv_cbw_append(bs, target, filter_node_name, false, &bcs, errp); + cbw = bdrv_cbw_append(bs, target, filter_node_name, &bcs, errp); if (!cbw) { goto error; } diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, } static int cbw_init(BlockDriverState *bs, BlockDriverState *source, - BlockDriverState *target, bool compress, Error **errp) + BlockDriverState *target, Error **errp) { BDRVCopyBeforeWriteState *s = bs->opaque; @@ -XXX,XX +XXX,XX @@ static int cbw_init(BlockDriverState *bs, BlockDriverState *source, ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & bs->file->bs->supported_zero_flags); - s->bcs = block_copy_state_new(bs->file, s->target, false, compress, errp); + s->bcs = block_copy_state_new(bs->file, s->target, false, false, errp); if (!s->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); return -EINVAL; @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_cbw_filter = { BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, - bool compress, BlockCopyState **bcs, Error **errp) { @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, } state = top->opaque; - ret = cbw_init(top, source, target, compress, errp); + ret = cbw_init(top, source, target, errp); if (ret < 0) { goto fail; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> One more step closer to .bdrv_open(): use options instead of plain arguments. Move to bdrv_open_child() calls, native for drive open handlers. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20210824083856.17408-19-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, } } -static int cbw_init(BlockDriverState *bs, BlockDriverState *source, - BlockDriverState *target, Error **errp) +static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp) { BDRVCopyBeforeWriteState *s = bs->opaque; - bdrv_ref(target); - s->target = bdrv_attach_child(bs, target, "target", &child_of_bds, - BDRV_CHILD_DATA, errp); - if (!s->target) { - error_prepend(errp, "Cannot attach target child: "); + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, + BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, + false, errp); + if (!bs->file) { return -EINVAL; } - bdrv_ref(source); - bs->file = bdrv_attach_child(bs, source, "file", &child_of_bds, - BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - errp); - if (!bs->file) { - error_prepend(errp, "Cannot attach file child: "); + s->target = bdrv_open_child(NULL, options, "target", bs, &child_of_bds, + BDRV_CHILD_DATA, false, errp); + if (!s->target) { return -EINVAL; } @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, int ret; BDRVCopyBeforeWriteState *state; BlockDriverState *top; + QDict *opts; assert(source->total_sectors == target->total_sectors); @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, } state = top->opaque; - ret = cbw_init(top, source, target, errp); + opts = qdict_new(); + qdict_put_str(opts, "file", bdrv_get_node_name(source)); + qdict_put_str(opts, "target", bdrv_get_node_name(target)); + + ret = cbw_init(top, opts, errp); + qobject_unref(opts); if (ret < 0) { goto fail; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We are going to publish copy-before-write filter to be used in separate of backup. Future step would support bitmap for the filter. But let's start from full set bitmap. We have to modify backup, as bitmap is first initialized by copy-before-write filter, and then backup modifies it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-20-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/backup.c | 16 +++++++--------- block/copy-before-write.c | 4 ++++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/block/backup.c b/block/backup.c index XXXXXXX..XXXXXXX 100644 --- a/block/backup.c +++ b/block/backup.c @@ -XXX,XX +XXX,XX @@ static void backup_init_bcs_bitmap(BackupBlockJob *job) BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs); if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) { + bdrv_clear_dirty_bitmap(bcs_bitmap, NULL); ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap, NULL, true); assert(ret); - } else { - if (job->sync_mode == MIRROR_SYNC_MODE_TOP) { - /* - * We can't hog the coroutine to initialize this thoroughly. - * Set a flag and resume work when we are able to yield safely. - */ - block_copy_set_skip_unallocated(job->bcs, true); - } - bdrv_set_dirty_bitmap(bcs_bitmap, 0, job->len); + } else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) { + /* + * We can't hog the coroutine to initialize this thoroughly. + * Set a flag and resume work when we are able to yield safely. + */ + block_copy_set_skip_unallocated(job->bcs, true); } estimate = bdrv_get_dirty_count(bcs_bitmap); diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp) { BDRVCopyBeforeWriteState *s = bs->opaque; + BdrvDirtyBitmap *copy_bitmap; bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, @@ -XXX,XX +XXX,XX @@ static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp) return -EINVAL; } + copy_bitmap = block_copy_dirty_bitmap(s->bcs); + bdrv_set_dirty_bitmap(copy_bitmap, 0, bdrv_dirty_bitmap_size(copy_bitmap)); + return 0; } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Now block-copy will crash if user don't set progress meter by block_copy_set_progress_meter(). copy-before-write filter will be used in separate of backup job, and it doesn't want any progress meter (for now). So, allow not setting it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-21-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/block-copy.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/block/block-copy.c b/block/block-copy.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -XXX,XX +XXX,XX @@ static void coroutine_fn block_copy_task_end(BlockCopyTask *task, int ret) bdrv_set_dirty_bitmap(task->s->copy_bitmap, task->offset, task->bytes); } QLIST_REMOVE(task, list); - progress_set_remaining(task->s->progress, - bdrv_get_dirty_count(task->s->copy_bitmap) + - task->s->in_flight_bytes); + if (task->s->progress) { + progress_set_remaining(task->s->progress, + bdrv_get_dirty_count(task->s->copy_bitmap) + + task->s->in_flight_bytes); + } qemu_co_queue_restart_all(&task->wait_queue); } @@ -XXX,XX +XXX,XX @@ static coroutine_fn int block_copy_task_entry(AioTask *task) t->call_state->ret = ret; t->call_state->error_is_read = error_is_read; } - } else { + } else if (s->progress) { progress_work_done(s->progress, t->bytes); } } @@ -XXX,XX +XXX,XX @@ int64_t block_copy_reset_unallocated(BlockCopyState *s, if (!ret) { qemu_co_mutex_lock(&s->lock); bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes); - progress_set_remaining(s->progress, - bdrv_get_dirty_count(s->copy_bitmap) + - s->in_flight_bytes); + if (s->progress) { + progress_set_remaining(s->progress, + bdrv_get_dirty_count(s->copy_bitmap) + + s->in_flight_bytes); + } qemu_co_mutex_unlock(&s->lock); } -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Finally, copy-before-write gets own .bdrv_open and .bdrv_close handlers, block_init() call and becomes available through bdrv_open(). To achieve this: - cbw_init gets unused flags argument and becomes cbw_open - block_copy_state_free() call moved to new cbw_close() - in bdrv_cbw_append: - options are completed with driver and node-name, and we can simply use bdrv_insert_node() to do both open and drained replacing - in bdrv_cbw_drop: - cbw_close() is now responsible for freeing s->bcs, so don't do it here Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-22-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/copy-before-write.c | 60 ++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, } } -static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp) +static int cbw_open(BlockDriverState *bs, QDict *options, int flags, + Error **errp) { BDRVCopyBeforeWriteState *s = bs->opaque; BdrvDirtyBitmap *copy_bitmap; @@ -XXX,XX +XXX,XX @@ static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp) return 0; } +static void cbw_close(BlockDriverState *bs) +{ + BDRVCopyBeforeWriteState *s = bs->opaque; + + block_copy_state_free(s->bcs); + s->bcs = NULL; +} + BlockDriver bdrv_cbw_filter = { .format_name = "copy-before-write", .instance_size = sizeof(BDRVCopyBeforeWriteState), + .bdrv_open = cbw_open, + .bdrv_close = cbw_close, + .bdrv_co_preadv = cbw_co_preadv, .bdrv_co_pwritev = cbw_co_pwritev, .bdrv_co_pwrite_zeroes = cbw_co_pwrite_zeroes, @@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, Error **errp) { ERRP_GUARD(); - int ret; BDRVCopyBeforeWriteState *state; BlockDriverState *top; QDict *opts; assert(source->total_sectors == target->total_sectors); - top = bdrv_new_open_driver(&bdrv_cbw_filter, filter_node_name, - BDRV_O_RDWR, errp); - if (!top) { - error_prepend(errp, "Cannot open driver: "); - return NULL; - } - state = top->opaque; - opts = qdict_new(); + qdict_put_str(opts, "driver", "copy-before-write"); + if (filter_node_name) { + qdict_put_str(opts, "node-name", filter_node_name); + } qdict_put_str(opts, "file", bdrv_get_node_name(source)); qdict_put_str(opts, "target", bdrv_get_node_name(target)); - ret = cbw_init(top, opts, errp); - qobject_unref(opts); - if (ret < 0) { - goto fail; - } - - bdrv_drained_begin(source); - ret = bdrv_replace_node(source, top, errp); - bdrv_drained_end(source); - if (ret < 0) { - error_prepend(errp, "Cannot append copy-before-write filter: "); - goto fail; + top = bdrv_insert_node(source, opts, BDRV_O_RDWR, errp); + if (!top) { + return NULL; } + state = top->opaque; *bcs = state->bcs; return top; - -fail: - block_copy_state_free(state->bcs); - bdrv_unref(top); - return NULL; } void bdrv_cbw_drop(BlockDriverState *bs) { - BDRVCopyBeforeWriteState *s = bs->opaque; - bdrv_drop_filter(bs, &error_abort); - - block_copy_state_free(s->bcs); - bdrv_unref(bs); } + +static void cbw_init(void) +{ + bdrv_register(&bdrv_cbw_filter); +} + +block_init(cbw_init); -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Acked-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20210824083856.17408-23-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- qapi/block-core.json | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -XXX,XX +XXX,XX @@ # @blklogwrites: Since 3.0 # @blkreplay: Since 4.2 # @compress: Since 5.0 +# @copy-before-write: Since 6.2 # # Since: 2.9 ## { 'enum': 'BlockdevDriver', 'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs', - 'cloop', 'compress', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', - 'gluster', + 'cloop', 'compress', 'copy-before-write', 'copy-on-read', 'dmg', + 'file', 'ftp', 'ftps', 'gluster', {'name': 'host_cdrom', 'if': 'HAVE_HOST_BLOCK_DEVICE' }, {'name': 'host_device', 'if': 'HAVE_HOST_BLOCK_DEVICE' }, 'http', 'https', 'iscsi', @@ -XXX,XX +XXX,XX @@ 'base': 'BlockdevOptionsGenericFormat', 'data': { '*bottom': 'str' } } +## +# @BlockdevOptionsCbw: +# +# Driver specific block device options for the copy-before-write driver, +# which does so called copy-before-write operations: when data is +# written to the filter, the filter first reads corresponding blocks +# from its file child and copies them to @target child. After successfully +# copying, the write request is propagated to file child. If copying +# fails, the original write request is failed too and no data is written +# to file child. +# +# @target: The target for copy-before-write operations. +# +# Since: 6.2 +## +{ 'struct': 'BlockdevOptionsCbw', + 'base': 'BlockdevOptionsGenericFormat', + 'data': { 'target': 'BlockdevRef' } } + ## # @BlockdevOptions: # @@ -XXX,XX +XXX,XX @@ 'bochs': 'BlockdevOptionsGenericFormat', 'cloop': 'BlockdevOptionsGenericFormat', 'compress': 'BlockdevOptionsGenericFormat', + 'copy-before-write':'BlockdevOptionsCbw', 'copy-on-read':'BlockdevOptionsCor', 'dmg': 'BlockdevOptionsGenericFormat', 'file': 'BlockdevOptionsFile', -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> - use shorter construction - don't create new dict if not needed - drop extra unpacking key-val arguments - drop extra default values Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-Id: <20210824083856.17408-24-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- python/qemu/machine/machine.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index XXXXXXX..XXXXXXX 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -XXX,XX +XXX,XX @@ def _qmp(self) -> QEMUMonitorProtocol: return self._qmp_connection @classmethod - def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]: - qmp_args = dict() - for key, value in args.items(): - if _conv_keys: - qmp_args[key.replace('_', '-')] = value - else: - qmp_args[key] = value - return qmp_args + def _qmp_args(cls, conv_keys: bool, + args: Dict[str, Any]) -> Dict[str, object]: + if conv_keys: + return {k.replace('_', '-'): v for k, v in args.items()} + + return args def qmp(self, cmd: str, conv_keys: bool = True, @@ -XXX,XX +XXX,XX @@ def qmp(self, cmd: str, """ Invoke a QMP command and return the response dict """ - qmp_args = self._qmp_args(conv_keys, **args) + qmp_args = self._qmp_args(conv_keys, args) return self._qmp.cmd(cmd, args=qmp_args) def command(self, cmd: str, @@ -XXX,XX +XXX,XX @@ def command(self, cmd: str, On success return the response dict. On failure raise an exception. """ - qmp_args = self._qmp_args(conv_keys, **args) + qmp_args = self._qmp_args(conv_keys, args) return self._qmp.command(cmd, **qmp_args) def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]: -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We often call qmp() with unpacking dict, like qmp('foo', **{...}). mypy don't really like it, it thinks that passed unpacked dict is a positional argument and complains that it type should be bool (because second argument of qmp() is conv_keys: bool). Allow passing dict directly, simplifying interface, and giving a way to satisfy mypy. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-Id: <20210824083856.17408-25-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- python/qemu/machine/machine.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index XXXXXXX..XXXXXXX 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -XXX,XX +XXX,XX @@ def _qmp_args(cls, conv_keys: bool, return args def qmp(self, cmd: str, - conv_keys: bool = True, + args_dict: Optional[Dict[str, object]] = None, + conv_keys: Optional[bool] = None, **args: Any) -> QMPMessage: """ Invoke a QMP command and return the response dict """ + if args_dict is not None: + assert not args + assert conv_keys is None + args = args_dict + conv_keys = False + + if conv_keys is None: + conv_keys = True + qmp_args = self._qmp_args(conv_keys, args) return self._qmp.cmd(cmd, args=qmp_args) -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> mypy thinks that return value of these methods in subclusses is QEMUMachine, which is wrong. So, make typing smarter. Suggested-by: John Snow <jsnow@redhat.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20210824083856.17408-26-vsementsov@virtuozzo.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- python/qemu/machine/machine.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index XXXXXXX..XXXXXXX 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -XXX,XX +XXX,XX @@ Sequence, Tuple, Type, + TypeVar, ) from qemu.qmp import ( # pylint: disable=import-error @@ -XXX,XX +XXX,XX @@ class AbnormalShutdown(QEMUMachineError): """ +_T = TypeVar('_T', bound='QEMUMachine') + + class QEMUMachine: """ A QEMU VM. @@ -XXX,XX +XXX,XX @@ def __init__(self, self._remove_files: List[str] = [] self._user_killed = False - def __enter__(self) -> 'QEMUMachine': + def __enter__(self: _T) -> _T: return self def __exit__(self, @@ -XXX,XX +XXX,XX @@ def add_monitor_null(self) -> None: self._args.append('-monitor') self._args.append('null') - def add_fd(self, fd: int, fdset: int, - opaque: str, opts: str = '') -> 'QEMUMachine': + def add_fd(self: _T, fd: int, fdset: int, + opaque: str, opts: str = '') -> _T: """ Pass a file descriptor to the VM """ -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Here: - long line - move to new interface of vm.qmp() (direct passing dict), to avoid mypy false-positive, as it thinks that unpacked dict is a positional argument. - extra parenthesis - handle event_wait possible None value Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-27-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/222 | 20 +++++++++++--------- tests/qemu-iotests/297 | 2 +- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/222 +++ b/tests/qemu-iotests/222 @@ -XXX,XX +XXX,XX @@ remainder = [("0xd5", "0x108000", "32k"), # Right-end of partial-left [1] with iotests.FilePath('base.img') as base_img_path, \ iotests.FilePath('fleece.img') as fleece_img_path, \ - iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \ + iotests.FilePath('nbd.sock', + base_dir=iotests.sock_dir) as nbd_sock_path, \ iotests.VM() as vm: log('--- Setting up images ---') @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ tgt_node = "fleeceNode" # create tgt_node backed by src_node - log(vm.qmp("blockdev-add", **{ + log(vm.qmp("blockdev-add", { "driver": "qcow2", "node-name": tgt_node, "file": { @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ nbd_uri = 'nbd+unix:///%s?socket=%s' % (tgt_node, nbd_sock_path) log(vm.qmp("nbd-server-start", - **{"addr": { "type": "unix", - "data": { "path": nbd_sock_path } } })) + {"addr": { "type": "unix", + "data": { "path": nbd_sock_path } } })) log(vm.qmp("nbd-server-add", device=tgt_node)) @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Sanity Check ---') log('') - for p in (patterns + zeroes): + for p in patterns + zeroes: cmd = "read -P%s %s %s" % p log(cmd) assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0 @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Verifying Data ---') log('') - for p in (patterns + zeroes): + for p in patterns + zeroes: cmd = "read -P%s %s %s" % p log(cmd) assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0 @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('') log(vm.qmp('block-job-cancel', device=src_node)) - log(vm.event_wait('BLOCK_JOB_CANCELLED'), - filters=[iotests.filter_qmp_event]) + e = vm.event_wait('BLOCK_JOB_CANCELLED') + assert e is not None + log(e, filters=[iotests.filter_qmp_event]) log(vm.qmp('nbd-server-stop')) log(vm.qmp('blockdev-del', node_name=tgt_node)) vm.shutdown() @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Confirming writes ---') log('') - for p in (overwrite + remainder): + for p in overwrite + remainder: cmd = "read -P%s %s %s" % p log(cmd) assert qemu_io_silent(base_img_path, '-c', cmd) == 0 diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -XXX,XX +XXX,XX @@ SKIP_FILES = ( '096', '118', '124', '132', '136', '139', '147', '148', '149', '151', '152', '155', '163', '165', '169', '194', '196', '199', '202', '203', '205', '206', '207', '208', '210', '211', '212', '213', '216', - '218', '219', '222', '224', '228', '234', '235', '236', '237', '238', + '218', '219', '224', '228', '234', '235', '236', '237', '238', '240', '242', '245', '246', '248', '255', '256', '257', '258', '260', '262', '264', '266', '274', '277', '280', '281', '295', '296', '298', '299', '302', '303', '304', '307', -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> The file use both single and double quotes for strings. Let's be consistent. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-28-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/222 | 68 +++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222 index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/222 +++ b/tests/qemu-iotests/222 @@ -XXX,XX +XXX,XX @@ iotests.script_initialize( supported_platforms=['linux'], ) -patterns = [("0x5d", "0", "64k"), - ("0xd5", "1M", "64k"), - ("0xdc", "32M", "64k"), - ("0xcd", "0x3ff0000", "64k")] # 64M - 64K +patterns = [('0x5d', '0', '64k'), + ('0xd5', '1M', '64k'), + ('0xdc', '32M', '64k'), + ('0xcd', '0x3ff0000', '64k')] # 64M - 64K -overwrite = [("0xab", "0", "64k"), # Full overwrite - ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K) - ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K) - ("0xea", "0x3fe0000", "64k")] # Adjacent-left (64M - 128K) +overwrite = [('0xab', '0', '64k'), # Full overwrite + ('0xad', '0x00f8000', '64k'), # Partial-left (1M-32K) + ('0x1d', '0x2008000', '64k'), # Partial-right (32M+32K) + ('0xea', '0x3fe0000', '64k')] # Adjacent-left (64M - 128K) -zeroes = [("0", "0x00f8000", "32k"), # Left-end of partial-left (1M-32K) - ("0", "0x2010000", "32k"), # Right-end of partial-right (32M+64K) - ("0", "0x3fe0000", "64k")] # overwrite[3] +zeroes = [('0', '0x00f8000', '32k'), # Left-end of partial-left (1M-32K) + ('0', '0x2010000', '32k'), # Right-end of partial-right (32M+64K) + ('0', '0x3fe0000', '64k')] # overwrite[3] -remainder = [("0xd5", "0x108000", "32k"), # Right-end of partial-left [1] - ("0xdc", "32M", "32k"), # Left-end of partial-right [2] - ("0xcd", "0x3ff0000", "64k")] # patterns[3] +remainder = [('0xd5', '0x108000', '32k'), # Right-end of partial-left [1] + ('0xdc', '32M', '32k'), # Left-end of partial-right [2] + ('0xcd', '0x3ff0000', '64k')] # patterns[3] with iotests.FilePath('base.img') as base_img_path, \ iotests.FilePath('fleece.img') as fleece_img_path, \ @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('') assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0 - assert qemu_img('create', '-f', "qcow2", fleece_img_path, '64M') == 0 + assert qemu_img('create', '-f', 'qcow2', fleece_img_path, '64M') == 0 for p in patterns: qemu_io('-f', iotests.imgfmt, @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Setting up Fleecing Graph ---') log('') - src_node = "drive0" - tgt_node = "fleeceNode" + src_node = 'drive0' + tgt_node = 'fleeceNode' # create tgt_node backed by src_node - log(vm.qmp("blockdev-add", { - "driver": "qcow2", - "node-name": tgt_node, - "file": { - "driver": "file", - "filename": fleece_img_path, + log(vm.qmp('blockdev-add', { + 'driver': 'qcow2', + 'node-name': tgt_node, + 'file': { + 'driver': 'file', + 'filename': fleece_img_path, }, - "backing": src_node, + 'backing': src_node, })) # Establish COW from source to fleecing node - log(vm.qmp("blockdev-backup", + log(vm.qmp('blockdev-backup', device=src_node, target=tgt_node, - sync="none")) + sync='none')) log('') log('--- Setting up NBD Export ---') log('') nbd_uri = 'nbd+unix:///%s?socket=%s' % (tgt_node, nbd_sock_path) - log(vm.qmp("nbd-server-start", - {"addr": { "type": "unix", - "data": { "path": nbd_sock_path } } })) + log(vm.qmp('nbd-server-start', + {'addr': { 'type': 'unix', + 'data': { 'path': nbd_sock_path } } })) - log(vm.qmp("nbd-server-add", device=tgt_node)) + log(vm.qmp('nbd-server-add', device=tgt_node)) log('') log('--- Sanity Check ---') log('') for p in patterns + zeroes: - cmd = "read -P%s %s %s" % p + cmd = 'read -P%s %s %s' % p log(cmd) assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0 @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('') for p in overwrite: - cmd = "write -P%s %s %s" % p + cmd = 'write -P%s %s %s' % p log(cmd) log(vm.hmp_qemu_io(src_node, cmd)) @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('') for p in patterns + zeroes: - cmd = "read -P%s %s %s" % p + cmd = 'read -P%s %s %s' % p log(cmd) assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0 @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('') for p in overwrite + remainder: - cmd = "read -P%s %s %s" % p + cmd = 'read -P%s %s %s' % p log(cmd) assert qemu_io_silent(base_img_path, '-c', cmd) == 0 -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Give a good name to test file. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-29-vsementsov@virtuozzo.com> [hreitz: Adjust .gitlab-ci.d/buildtest.yml] Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- .gitlab-ci.d/buildtest.yml | 6 +++--- tests/qemu-iotests/{222 => tests/image-fleecing} | 0 tests/qemu-iotests/{222.out => tests/image-fleecing.out} | 0 3 files changed, 3 insertions(+), 3 deletions(-) rename tests/qemu-iotests/{222 => tests/image-fleecing} (100%) rename tests/qemu-iotests/{222.out => tests/image-fleecing.out} (100%) diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml index XXXXXXX..XXXXXXX 100644 --- a/.gitlab-ci.d/buildtest.yml +++ b/.gitlab-ci.d/buildtest.yml @@ -XXX,XX +XXX,XX @@ build-tcg-disabled: - cd tests/qemu-iotests/ - ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048 052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163 - 170 171 183 184 192 194 208 221 222 226 227 236 253 277 + 170 171 183 184 192 194 208 221 226 227 236 253 277 image-fleecing - ./check -qcow2 028 051 056 057 058 065 068 082 085 091 095 096 102 122 124 132 139 142 144 145 151 152 155 157 165 194 196 200 202 - 208 209 216 218 222 227 234 246 247 248 250 254 255 257 258 - 260 261 262 263 264 270 272 273 277 279 + 208 209 216 218 227 234 246 247 248 250 254 255 257 258 + 260 261 262 263 264 270 272 273 277 279 image-fleecing build-user: extends: .native_build_job_template diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/tests/image-fleecing similarity index 100% rename from tests/qemu-iotests/222 rename to tests/qemu-iotests/tests/image-fleecing diff --git a/tests/qemu-iotests/222.out b/tests/qemu-iotests/tests/image-fleecing.out similarity index 100% rename from tests/qemu-iotests/222.out rename to tests/qemu-iotests/tests/image-fleecing.out -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: John Snow <jsnow@redhat.com> Message-Id: <20210824083856.17408-30-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@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 resume_drive(self, drive: str) -> None: self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"') def hmp_qemu_io(self, drive: str, cmd: str, - use_log: bool = False) -> QMPMessage: + use_log: bool = False, qdev: bool = False) -> QMPMessage: """Write to a given drive using an HMP command""" - return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log) + d = '-d ' if qdev else '' + return self.hmp(f'qemu-io {d}{drive} "{cmd}"', use_log=use_log) def flatten_qmp_object(self, obj, output=None, basestr=''): if output is None: -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Define scsi device to operate with it by qom-set in further patch. Give a new node-name to source block node, to not look like device name. Job now don't want to work without giving explicit id, so, let's call it "fleecing". Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-31-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/tests/image-fleecing | 12 ++++++++---- tests/qemu-iotests/tests/image-fleecing.out | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/image-fleecing +++ b/tests/qemu-iotests/tests/image-fleecing @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Launching VM ---') log('') - vm.add_drive(base_img_path) + src_node = 'source' + vm.add_blockdev(f'driver={iotests.imgfmt},file.driver=file,' + f'file.filename={base_img_path},node-name={src_node}') + vm.add_device('virtio-scsi') + vm.add_device(f'scsi-hd,id=sda,drive={src_node}') vm.launch() log('Done') @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Setting up Fleecing Graph ---') log('') - src_node = 'drive0' tgt_node = 'fleeceNode' # create tgt_node backed by src_node @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ # Establish COW from source to fleecing node log(vm.qmp('blockdev-backup', + job_id='fleecing', device=src_node, target=tgt_node, sync='none')) @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ for p in overwrite: cmd = 'write -P%s %s %s' % p log(cmd) - log(vm.hmp_qemu_io(src_node, cmd)) + log(vm.hmp_qemu_io('/machine/peripheral/sda', cmd, qdev=True)) log('') log('--- Verifying Data ---') @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Cleanup ---') log('') - log(vm.qmp('block-job-cancel', device=src_node)) + log(vm.qmp('block-job-cancel', device='fleecing')) e = vm.event_wait('BLOCK_JOB_CANCELLED') assert e is not None log(e, filters=[iotests.filter_qmp_event]) diff --git a/tests/qemu-iotests/tests/image-fleecing.out b/tests/qemu-iotests/tests/image-fleecing.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/tests/image-fleecing.out +++ b/tests/qemu-iotests/tests/image-fleecing.out @@ -XXX,XX +XXX,XX @@ read -P0 0x3fe0000 64k --- Cleanup --- {"return": {}} -{"data": {"device": "drive0", "len": 67108864, "offset": 393216, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +{"data": {"device": "fleecing", "len": 67108864, "offset": 393216, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} {"return": {}} {"return": {}} -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Actually target of backup(sync=None) is not a final backup target: image fleecing is intended to be used with external tool, which will copy data from fleecing node to some real backup target. Also, we are going to add a test case for "push backup with fleecing", where instead of exporting fleecing node by NBD, we'll start a backup job from fleecing node to real backup target. To avoid confusion, let's rename temporary fleecing node now. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-32-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/tests/image-fleecing | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/image-fleecing +++ b/tests/qemu-iotests/tests/image-fleecing @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('') src_node = 'source' + tmp_node = 'temp' vm.add_blockdev(f'driver={iotests.imgfmt},file.driver=file,' f'file.filename={base_img_path},node-name={src_node}') vm.add_device('virtio-scsi') @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('--- Setting up Fleecing Graph ---') log('') - tgt_node = 'fleeceNode' - # create tgt_node backed by src_node + # create tmp_node backed by src_node log(vm.qmp('blockdev-add', { 'driver': 'qcow2', - 'node-name': tgt_node, + 'node-name': tmp_node, 'file': { 'driver': 'file', 'filename': fleece_img_path, @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log(vm.qmp('blockdev-backup', job_id='fleecing', device=src_node, - target=tgt_node, + target=tmp_node, sync='none')) log('') log('--- Setting up NBD Export ---') log('') - nbd_uri = 'nbd+unix:///%s?socket=%s' % (tgt_node, nbd_sock_path) + nbd_uri = 'nbd+unix:///%s?socket=%s' % (tmp_node, nbd_sock_path) log(vm.qmp('nbd-server-start', {'addr': { 'type': 'unix', 'data': { 'path': nbd_sock_path } } })) - log(vm.qmp('nbd-server-add', device=tgt_node)) + log(vm.qmp('nbd-server-add', device=tmp_node)) log('') log('--- Sanity Check ---') @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ assert e is not None log(e, filters=[iotests.filter_qmp_event]) log(vm.qmp('nbd-server-stop')) - log(vm.qmp('blockdev-del', node_name=tgt_node)) + log(vm.qmp('blockdev-del', node_name=tmp_node)) vm.shutdown() log('') -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> We are going to add a test-case with some behavior modifications. So, let's prepare a function to be reused. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-33-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/tests/image-fleecing | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/image-fleecing +++ b/tests/qemu-iotests/tests/image-fleecing @@ -XXX,XX +XXX,XX @@ remainder = [('0xd5', '0x108000', '32k'), # Right-end of partial-left [1] ('0xdc', '32M', '32k'), # Left-end of partial-right [2] ('0xcd', '0x3ff0000', '64k')] # patterns[3] -with iotests.FilePath('base.img') as base_img_path, \ - iotests.FilePath('fleece.img') as fleece_img_path, \ - iotests.FilePath('nbd.sock', - base_dir=iotests.sock_dir) as nbd_sock_path, \ - iotests.VM() as vm: - +def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm): log('--- Setting up images ---') log('') @@ -XXX,XX +XXX,XX @@ with iotests.FilePath('base.img') as base_img_path, \ log('') log('Done') + + +def test(): + with iotests.FilePath('base.img') as base_img_path, \ + iotests.FilePath('fleece.img') as fleece_img_path, \ + iotests.FilePath('nbd.sock', + base_dir=iotests.sock_dir) as nbd_sock_path, \ + iotests.VM() as vm: + do_test(base_img_path, fleece_img_path, nbd_sock_path, vm) + + +test() -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> New fleecing method becomes available: copy-before-write filter. Actually we don't need backup job to setup image fleecing. Add test for new recommended way of image fleecing. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Max Reitz <mreitz@redhat.com> Message-Id: <20210824083856.17408-34-vsementsov@virtuozzo.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- tests/qemu-iotests/tests/image-fleecing | 50 +++++++++----- tests/qemu-iotests/tests/image-fleecing.out | 72 +++++++++++++++++++++ 2 files changed, 107 insertions(+), 15 deletions(-) diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing index XXXXXXX..XXXXXXX 100755 --- a/tests/qemu-iotests/tests/image-fleecing +++ b/tests/qemu-iotests/tests/image-fleecing @@ -XXX,XX +XXX,XX @@ remainder = [('0xd5', '0x108000', '32k'), # Right-end of partial-left [1] ('0xdc', '32M', '32k'), # Left-end of partial-right [2] ('0xcd', '0x3ff0000', '64k')] # patterns[3] -def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm): +def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm): log('--- Setting up images ---') log('') @@ -XXX,XX +XXX,XX @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm): src_node = 'source' tmp_node = 'temp' + qom_path = '/machine/peripheral/sda' vm.add_blockdev(f'driver={iotests.imgfmt},file.driver=file,' f'file.filename={base_img_path},node-name={src_node}') vm.add_device('virtio-scsi') @@ -XXX,XX +XXX,XX @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm): 'backing': src_node, })) - # Establish COW from source to fleecing node - log(vm.qmp('blockdev-backup', - job_id='fleecing', - device=src_node, - target=tmp_node, - sync='none')) + # Establish CBW from source to fleecing node + if use_cbw: + log(vm.qmp('blockdev-add', { + 'driver': 'copy-before-write', + 'node-name': 'fl-cbw', + 'file': src_node, + 'target': tmp_node + })) + + log(vm.qmp('qom-set', path=qom_path, property='drive', value='fl-cbw')) + else: + log(vm.qmp('blockdev-backup', + job_id='fleecing', + device=src_node, + target=tmp_node, + sync='none')) log('') log('--- Setting up NBD Export ---') @@ -XXX,XX +XXX,XX @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm): for p in overwrite: cmd = 'write -P%s %s %s' % p log(cmd) - log(vm.hmp_qemu_io('/machine/peripheral/sda', cmd, qdev=True)) + log(vm.hmp_qemu_io(qom_path, cmd, qdev=True)) log('') log('--- Verifying Data ---') @@ -XXX,XX +XXX,XX @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm): log('--- Cleanup ---') log('') - log(vm.qmp('block-job-cancel', device='fleecing')) - e = vm.event_wait('BLOCK_JOB_CANCELLED') - assert e is not None - log(e, filters=[iotests.filter_qmp_event]) + if use_cbw: + log(vm.qmp('qom-set', path=qom_path, property='drive', value=src_node)) + log(vm.qmp('blockdev-del', node_name='fl-cbw')) + else: + log(vm.qmp('block-job-cancel', device='fleecing')) + e = vm.event_wait('BLOCK_JOB_CANCELLED') + assert e is not None + log(e, filters=[iotests.filter_qmp_event]) + log(vm.qmp('nbd-server-stop')) log(vm.qmp('blockdev-del', node_name=tmp_node)) vm.shutdown() @@ -XXX,XX +XXX,XX @@ def do_test(base_img_path, fleece_img_path, nbd_sock_path, vm): log('Done') -def test(): +def test(use_cbw): with iotests.FilePath('base.img') as base_img_path, \ iotests.FilePath('fleece.img') as fleece_img_path, \ iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \ iotests.VM() as vm: - do_test(base_img_path, fleece_img_path, nbd_sock_path, vm) + do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm) + +log('=== Test backup(sync=none) based fleecing ===\n') +test(False) -test() +log('=== Test filter based fleecing ===\n') +test(True) diff --git a/tests/qemu-iotests/tests/image-fleecing.out b/tests/qemu-iotests/tests/image-fleecing.out index XXXXXXX..XXXXXXX 100644 --- a/tests/qemu-iotests/tests/image-fleecing.out +++ b/tests/qemu-iotests/tests/image-fleecing.out @@ -XXX,XX +XXX,XX @@ +=== Test backup(sync=none) based fleecing === + --- Setting up images --- Done @@ -XXX,XX +XXX,XX @@ read -P0xdc 32M 32k read -P0xcd 0x3ff0000 64k Done +=== Test filter based fleecing === + +--- Setting up images --- + +Done + +--- Launching VM --- + +Done + +--- Setting up Fleecing Graph --- + +{"return": {}} +{"return": {}} +{"return": {}} + +--- Setting up NBD Export --- + +{"return": {}} +{"return": {}} + +--- Sanity Check --- + +read -P0x5d 0 64k +read -P0xd5 1M 64k +read -P0xdc 32M 64k +read -P0xcd 0x3ff0000 64k +read -P0 0x00f8000 32k +read -P0 0x2010000 32k +read -P0 0x3fe0000 64k + +--- Testing COW --- + +write -P0xab 0 64k +{"return": ""} +write -P0xad 0x00f8000 64k +{"return": ""} +write -P0x1d 0x2008000 64k +{"return": ""} +write -P0xea 0x3fe0000 64k +{"return": ""} + +--- Verifying Data --- + +read -P0x5d 0 64k +read -P0xd5 1M 64k +read -P0xdc 32M 64k +read -P0xcd 0x3ff0000 64k +read -P0 0x00f8000 32k +read -P0 0x2010000 32k +read -P0 0x3fe0000 64k + +--- Cleanup --- + +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} + +--- Confirming writes --- + +read -P0xab 0 64k +read -P0xad 0x00f8000 64k +read -P0x1d 0x2008000 64k +read -P0xea 0x3fe0000 64k +read -P0xd5 0x108000 32k +read -P0xdc 32M 32k +read -P0xcd 0x3ff0000 64k + +Done -- 2.31.1 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> The only caller pass copy_range and compress both false. Let's just drop these arguments. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20210824083856.17408-35-vsementsov@virtuozzo.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/block/block-copy.h | 1 - block/block-copy.c | 5 ++--- block/copy-before-write.c | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/include/block/block-copy.h b/include/block/block-copy.h index XXXXXXX..XXXXXXX 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -XXX,XX +XXX,XX @@ typedef struct BlockCopyState BlockCopyState; typedef struct BlockCopyCallState BlockCopyCallState; BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, - bool use_copy_range, bool compress, Error **errp); /* Function should be called prior any actual copy request */ diff --git a/block/block-copy.c b/block/block-copy.c index XXXXXXX..XXXXXXX 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -XXX,XX +XXX,XX @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target, } BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, - bool use_copy_range, - bool compress, Error **errp) + Error **errp) { BlockCopyState *s; int64_t cluster_size; @@ -XXX,XX +XXX,XX @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, cluster_size), }; - block_copy_set_copy_opts(s, use_copy_range, compress); + block_copy_set_copy_opts(s, false, false); ratelimit_init(&s->rate_limit); qemu_co_mutex_init(&s->lock); diff --git a/block/copy-before-write.c b/block/copy-before-write.c index XXXXXXX..XXXXXXX 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -XXX,XX +XXX,XX @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & bs->file->bs->supported_zero_flags); - s->bcs = block_copy_state_new(bs->file, s->target, false, false, errp); + s->bcs = block_copy_state_new(bs->file, s->target, errp); if (!s->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); return -EINVAL; -- 2.31.1 From: Fabrice Fontaine <fontaine.fabrice@gmail.com> Include linux/fs.h to avoid the following build failure on uclibc or musl raised since version 6.0.0: ../block/export/fuse.c: In function 'fuse_lseek': ../block/export/fuse.c:641:19: error: 'SEEK_HOLE' undeclared (first use in this function) 641 | if (whence != SEEK_HOLE && whence != SEEK_DATA) { | ^~~~~~~~~ ../block/export/fuse.c:641:19: note: each undeclared identifier is reported only once for each function it appears in ../block/export/fuse.c:641:42: error: 'SEEK_DATA' undeclared (first use in this function); did you mean 'SEEK_SET'? 641 | if (whence != SEEK_HOLE && whence != SEEK_DATA) { | ^~~~~~~~~ | SEEK_SET Fixes: - http://autobuild.buildroot.org/results/33c90ebf04997f4d3557cfa66abc9cf9a3076137 Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> Message-Id: <20210827220301.272887-1-fontaine.fabrice@gmail.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/export/fuse.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/export/fuse.c b/block/export/fuse.c index XXXXXXX..XXXXXXX 100644 --- a/block/export/fuse.c +++ b/block/export/fuse.c @@ -XXX,XX +XXX,XX @@ #include <fuse.h> #include <fuse_lowlevel.h> +#ifdef __linux__ +#include <linux/fs.h> +#endif /* Prevent overly long bounce buffer allocations */ #define FUSE_MAX_BOUNCE_BYTES (MIN(BDRV_REQUEST_MAX_BYTES, 64 * 1024 * 1024)) -- 2.31.1 From: Viktor Prutyanov <viktor.prutyanov@phystech.edu> Make 'qemu-img commit' work on Windows. Command 'commit' requires reopening backing file in RW mode. So, add reopen prepare/commit/abort handlers and change dwShareMode for CreateFile call in order to allow further read/write reopening. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/418 Suggested-by: Hanna Reitz <hreitz@redhat.com> Signed-off-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu> Tested-by: Helge Konetzka <hk@zapateado.de> Message-Id: <20210825173625.19415-1-viktor.prutyanov@phystech.edu> Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/file-win32.c | 101 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 100 insertions(+), 1 deletion(-) 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 @@ typedef struct BDRVRawState { QEMUWin32AIOState *aio; } BDRVRawState; +typedef struct BDRVRawReopenState { + HANDLE hfile; +} BDRVRawReopenState; + /* * Read/writes the data to/from a given linear buffer. * @@ -XXX,XX +XXX,XX @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, } s->hfile = CreateFile(filename, access_flags, - FILE_SHARE_READ, NULL, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, overlapped, NULL); if (s->hfile == INVALID_HANDLE_VALUE) { int err = GetLastError(); @@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_create_opts(BlockDriver *drv, return raw_co_create(&options, errp); } +static int raw_reopen_prepare(BDRVReopenState *state, + BlockReopenQueue *queue, Error **errp) +{ + BDRVRawState *s = state->bs->opaque; + BDRVRawReopenState *rs; + int access_flags; + DWORD overlapped; + int ret = 0; + + if (s->type != FTYPE_FILE) { + error_setg(errp, "Can only reopen files"); + return -EINVAL; + } + + rs = g_new0(BDRVRawReopenState, 1); + + /* + * We do not support changing any options (only flags). By leaving + * all options in state->options, we tell the generic reopen code + * that we do not support changing any of them, so it will verify + * that their values did not change. + */ + + raw_parse_flags(state->flags, s->aio != NULL, &access_flags, &overlapped); + rs->hfile = CreateFile(state->bs->filename, access_flags, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, + OPEN_EXISTING, overlapped, NULL); + + if (rs->hfile == INVALID_HANDLE_VALUE) { + int err = GetLastError(); + + error_setg_win32(errp, err, "Could not reopen '%s'", + state->bs->filename); + if (err == ERROR_ACCESS_DENIED) { + ret = -EACCES; + } else { + ret = -EINVAL; + } + goto fail; + } + + if (s->aio) { + ret = win32_aio_attach(s->aio, rs->hfile); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not enable AIO"); + CloseHandle(rs->hfile); + goto fail; + } + } + + state->opaque = rs; + + return 0; + +fail: + g_free(rs); + state->opaque = NULL; + + return ret; +} + +static void raw_reopen_commit(BDRVReopenState *state) +{ + BDRVRawState *s = state->bs->opaque; + BDRVRawReopenState *rs = state->opaque; + + assert(rs != NULL); + + CloseHandle(s->hfile); + s->hfile = rs->hfile; + + g_free(rs); + state->opaque = NULL; +} + +static void raw_reopen_abort(BDRVReopenState *state) +{ + BDRVRawReopenState *rs = state->opaque; + + if (!rs) { + return; + } + + if (rs->hfile != INVALID_HANDLE_VALUE) { + CloseHandle(rs->hfile); + } + + g_free(rs); + state->opaque = NULL; +} + static QemuOptsList raw_create_opts = { .name = "raw-create-opts", .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head), @@ -XXX,XX +XXX,XX @@ BlockDriver bdrv_file = { .bdrv_co_create_opts = raw_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_reopen_prepare = raw_reopen_prepare, + .bdrv_reopen_commit = raw_reopen_commit, + .bdrv_reopen_abort = raw_reopen_abort, + .bdrv_aio_preadv = raw_aio_preadv, .bdrv_aio_pwritev = raw_aio_pwritev, .bdrv_aio_flush = raw_aio_flush, -- 2.31.1 The following changes since commit eaefea537b476cb853e2edbdc68e969ec777e4bb: Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-fetch' into staging (2017-12-18 14:17:42 +0000) are available in the Git repository at: git://github.com/stefanha/qemu.git tags/block-pull-request for you to fetch changes up to 7a9dda0d7f9831c2432620dcfefdadbb7ae888dc: qemu-iotests: add 203 savevm with IOThreads test (2017-12-19 10:25:09 +0000) ---------------------------------------------------------------- Pull request v2: * Fixed incorrect virtio_blk_data_plane_create() local_err refactoring in "hw/block: Use errp directly rather than local_err" that broke virtio-blk over virtio-mmio [Peter] ---------------------------------------------------------------- Mao Zhongyi (4): hw/block/nvme: Convert to realize hw/block: Fix the return type hw/block: Use errp directly rather than local_err dev-storage: Fix the unusual function name Mark Kanda (2): virtio-blk: make queue size configurable virtio-blk: reject configs with logical block size > physical block size Paolo Bonzini (1): block: avoid recursive AioContext acquire in bdrv_inactivate_all() Stefan Hajnoczi (16): coroutine: simplify co_aio_sleep_ns() prototype qdev: drop unused #include "sysemu/iothread.h" blockdev: hold AioContext for bdrv_unref() in external_snapshot_clean() block: don't keep AioContext acquired after external_snapshot_prepare() block: don't keep AioContext acquired after drive_backup_prepare() block: don't keep AioContext acquired after blockdev_backup_prepare() block: don't keep AioContext acquired after internal_snapshot_prepare() block: drop unused BlockDirtyBitmapState->aio_context field iothread: add iothread_by_id() API blockdev: add x-blockdev-set-iothread testing command qemu-iotests: add 202 external snapshots IOThread test docs: mark nested AioContext locking as a legacy API blockdev: add x-blockdev-set-iothread force boolean iotests: add VM.add_object() iothread: fix iothread_stop() race condition qemu-iotests: add 203 savevm with IOThreads test docs/devel/multiple-iothreads.txt | 7 +- qapi/block-core.json | 40 ++++++ hw/block/dataplane/virtio-blk.h | 2 +- include/hw/block/block.h | 4 +- include/hw/virtio/virtio-blk.h | 1 + include/qemu/coroutine.h | 6 +- include/sysemu/iothread.h | 4 +- block.c | 14 ++- block/null.c | 3 +- block/sheepdog.c | 3 +- blockdev.c | 259 +++++++++++++++++++++++++++----------- hw/block/block.c | 15 ++- hw/block/dataplane/virtio-blk.c | 12 +- hw/block/fdc.c | 17 +-- hw/block/nvme.c | 23 ++-- hw/block/virtio-blk.c | 30 +++-- hw/core/qdev-properties-system.c | 1 - hw/ide/qdev.c | 12 +- hw/scsi/scsi-disk.c | 13 +- hw/usb/dev-storage.c | 29 ++--- iothread.c | 27 +++- util/qemu-coroutine-sleep.c | 4 +- tests/qemu-iotests/202 | 95 ++++++++++++++ tests/qemu-iotests/202.out | 11 ++ tests/qemu-iotests/203 | 59 +++++++++ tests/qemu-iotests/203.out | 6 + tests/qemu-iotests/group | 2 + tests/qemu-iotests/iotests.py | 5 + 28 files changed, 531 insertions(+), 173 deletions(-) create mode 100755 tests/qemu-iotests/202 create mode 100644 tests/qemu-iotests/202.out create mode 100755 tests/qemu-iotests/203 create mode 100644 tests/qemu-iotests/203.out -- 2.14.3 The AioContext pointer argument to co_aio_sleep_ns() is only used for the sleep timer. It does not affect where the caller coroutine is resumed. Due to changes to coroutine and AIO APIs it is now possible to drop the AioContext pointer argument. This is safe to do since no caller has specific requirements for which AioContext the timer must run in. This patch drops the AioContext pointer argument and renames the function to simplify the API. Reported-by: Paolo Bonzini <pbonzini@redhat.com> Reported-by: Eric Blake <eblake@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171109102652.6360-1-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/qemu/coroutine.h | 6 +----- block/null.c | 3 +-- block/sheepdog.c | 3 +-- util/qemu-coroutine-sleep.c | 4 ++-- 4 files changed, 5 insertions(+), 11 deletions(-) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index XXXXXXX..XXXXXXX 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -XXX,XX +XXX,XX @@ void qemu_co_rwlock_unlock(CoRwlock *lock); /** * Yield the coroutine for a given duration - * - * Behaves similarly to co_sleep_ns(), but the sleeping coroutine will be - * resumed when using aio_poll(). */ -void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type, - int64_t ns); +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns); /** * Yield until a file descriptor becomes readable diff --git a/block/null.c b/block/null.c index XXXXXXX..XXXXXXX 100644 --- a/block/null.c +++ b/block/null.c @@ -XXX,XX +XXX,XX @@ static coroutine_fn int null_co_common(BlockDriverState *bs) BDRVNullState *s = bs->opaque; if (s->latency_ns) { - co_aio_sleep_ns(bdrv_get_aio_context(bs), QEMU_CLOCK_REALTIME, - s->latency_ns); + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, s->latency_ns); } return 0; } 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 coroutine_fn void reconnect_to_sdog(void *opaque) if (s->fd < 0) { DPRINTF("Wait for connection to be established\n"); error_report_err(local_err); - co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME, - 1000000000ULL); + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000000ULL); } }; diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index XXXXXXX..XXXXXXX 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -XXX,XX +XXX,XX @@ static void co_sleep_cb(void *opaque) aio_co_wake(sleep_cb->co); } -void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type, - int64_t ns) +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) { + AioContext *ctx = qemu_get_current_aio_context(); CoSleepCB sleep_cb = { .co = qemu_coroutine_self(), }; -- 2.14.3 From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> Convert nvme_init() to realize and rename it to nvme_realize(). Cc: John Snow <jsnow@redhat.com> Cc: Keith Busch <keith.busch@intel.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> Message-id: 2882e72d795e04cbe2120f569d551aef2467ac60.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/nvme.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 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 @@ static const MemoryRegionOps nvme_cmb_ops = { }, }; -static int nvme_init(PCIDevice *pci_dev) +static void nvme_realize(PCIDevice *pci_dev, Error **errp) { NvmeCtrl *n = NVME(pci_dev); NvmeIdCtrl *id = &n->id_ctrl; @@ -XXX,XX +XXX,XX @@ static int nvme_init(PCIDevice *pci_dev) Error *local_err = NULL; if (!n->conf.blk) { - return -1; + error_setg(errp, "drive property not set"); + return; } bs_size = blk_getlength(n->conf.blk); if (bs_size < 0) { - return -1; + error_setg(errp, "could not get backing file size"); + return; } blkconf_serial(&n->conf, &n->serial); if (!n->serial) { - return -1; + error_setg(errp, "serial property not set"); + return; } blkconf_blocksizes(&n->conf); blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk), false, &local_err); if (local_err) { - error_report_err(local_err); - return -1; + error_propagate(errp, local_err); + return; } pci_conf = pci_dev->config; @@ -XXX,XX +XXX,XX @@ static int nvme_init(PCIDevice *pci_dev) cpu_to_le64(n->ns_size >> id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds); } - return 0; } static void nvme_exit(PCIDevice *pci_dev) @@ -XXX,XX +XXX,XX @@ static void nvme_class_init(ObjectClass *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc); PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc); - pc->init = nvme_init; + pc->realize = nvme_realize; pc->exit = nvme_exit; pc->class_id = PCI_CLASS_STORAGE_EXPRESS; pc->vendor_id = PCI_VENDOR_ID_INTEL; -- 2.14.3 From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> When the function no success value to transmit, it usually make the function return void. It has turned out not to be a success, because it means that the extra local_err variable and error_propagate() will be needed. It leads to cumbersome code, therefore, transmit success/ failure in the return value is worth. So fix the return type of blkconf_apply_backend_options(), blkconf_geometry() and virtio_blk_data_plane_create() to avoid it. Cc: John Snow <jsnow@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-id: ac0edc1fc70c4457e5cec94405eb7d1f89f9c2c1.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/dataplane/virtio-blk.h | 2 +- include/hw/block/block.h | 4 ++-- hw/block/block.c | 15 +++++++++------ hw/block/dataplane/virtio-blk.c | 12 +++++++----- 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h index XXXXXXX..XXXXXXX 100644 --- a/hw/block/dataplane/virtio-blk.h +++ b/hw/block/dataplane/virtio-blk.h @@ -XXX,XX +XXX,XX @@ typedef struct VirtIOBlockDataPlane VirtIOBlockDataPlane; -void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, +bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, VirtIOBlockDataPlane **dataplane, Error **errp); void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s); diff --git a/include/hw/block/block.h b/include/hw/block/block.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -XXX,XX +XXX,XX @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) /* Configuration helpers */ void blkconf_serial(BlockConf *conf, char **serial); -void blkconf_geometry(BlockConf *conf, int *trans, +bool blkconf_geometry(BlockConf *conf, int *trans, unsigned cyls_max, unsigned heads_max, unsigned secs_max, Error **errp); void blkconf_blocksizes(BlockConf *conf); -void blkconf_apply_backend_options(BlockConf *conf, bool readonly, +bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, bool resizable, Error **errp); /* Hard disk geometry */ diff --git a/hw/block/block.c b/hw/block/block.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -XXX,XX +XXX,XX @@ void blkconf_blocksizes(BlockConf *conf) } } -void blkconf_apply_backend_options(BlockConf *conf, bool readonly, +bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, bool resizable, Error **errp) { BlockBackend *blk = conf->blk; @@ -XXX,XX +XXX,XX @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly, ret = blk_set_perm(blk, perm, shared_perm, errp); if (ret < 0) { - return; + return false; } switch (conf->wce) { @@ -XXX,XX +XXX,XX @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly, blk_set_enable_write_cache(blk, wce); blk_set_on_error(blk, rerror, werror); + + return true; } -void blkconf_geometry(BlockConf *conf, int *ptrans, +bool blkconf_geometry(BlockConf *conf, int *ptrans, unsigned cyls_max, unsigned heads_max, unsigned secs_max, Error **errp) { @@ -XXX,XX +XXX,XX @@ void blkconf_geometry(BlockConf *conf, int *ptrans, if (conf->cyls || conf->heads || conf->secs) { if (conf->cyls < 1 || conf->cyls > cyls_max) { error_setg(errp, "cyls must be between 1 and %u", cyls_max); - return; + return false; } if (conf->heads < 1 || conf->heads > heads_max) { error_setg(errp, "heads must be between 1 and %u", heads_max); - return; + return false; } if (conf->secs < 1 || conf->secs > secs_max) { error_setg(errp, "secs must be between 1 and %u", secs_max); - return; + return false; } } + return true; } diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -XXX,XX +XXX,XX @@ static void notify_guest_bh(void *opaque) } /* Context: QEMU global mutex held */ -void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, +bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, VirtIOBlockDataPlane **dataplane, Error **errp) { @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, error_setg(errp, "device is incompatible with iothread " "(transport does not support notifiers)"); - return; + return false; } if (!virtio_device_ioeventfd_enabled(vdev)) { error_setg(errp, "ioeventfd is required for iothread"); - return; + return false; } /* If dataplane is (re-)enabled while the guest is running there could @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, */ if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) { error_prepend(errp, "cannot start virtio-blk dataplane: "); - return; + return false; } } /* Don't try if transport does not support notifiers. */ if (!virtio_device_ioeventfd_enabled(vdev)) { - return; + return false; } s = g_new0(VirtIOBlockDataPlane, 1); @@ -XXX,XX +XXX,XX @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, s->batch_notify_vqs = bitmap_new(conf->num_queues); *dataplane = s; + + return true; } /* Context: QEMU global mutex held */ -- 2.14.3 From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> [Drop virtio_blk_data_plane_create() change that misinterprets return value when the virtio transport does not support dataplane. --Stefan] Cc: John Snow <jsnow@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: Keith Busch <keith.busch@intel.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-id: e77848d3735ba590f23ffbf8094379c646c33d79.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/fdc.c | 17 ++++++----------- hw/block/nvme.c | 7 ++----- hw/block/virtio-blk.c | 13 +++++-------- hw/ide/qdev.c | 12 ++++-------- hw/scsi/scsi-disk.c | 13 ++++--------- hw/usb/dev-storage.c | 9 +++------ 6 files changed, 24 insertions(+), 47 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -XXX,XX +XXX,XX @@ static void fd_revalidate(FDrive *drv) static void fd_change_cb(void *opaque, bool load, Error **errp) { FDrive *drive = opaque; - Error *local_err = NULL; if (!load) { blk_set_perm(drive->blk, 0, BLK_PERM_ALL, &error_abort); } else { - blkconf_apply_backend_options(drive->conf, - blk_is_read_only(drive->blk), false, - &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!blkconf_apply_backend_options(drive->conf, + blk_is_read_only(drive->blk), false, + errp)) { return; } } @@ -XXX,XX +XXX,XX @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp) FloppyDrive *dev = FLOPPY_DRIVE(qdev); FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus); FDrive *drive; - Error *local_err = NULL; int ret; if (dev->unit == -1) { @@ -XXX,XX +XXX,XX @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp) dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO; dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO; - blkconf_apply_backend_options(&dev->conf, blk_is_read_only(dev->conf.blk), - false, &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!blkconf_apply_backend_options(&dev->conf, + blk_is_read_only(dev->conf.blk), + false, errp)) { return; } 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 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) int i; int64_t bs_size; uint8_t *pci_conf; - Error *local_err = NULL; if (!n->conf.blk) { error_setg(errp, "drive property not set"); @@ -XXX,XX +XXX,XX @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) return; } blkconf_blocksizes(&n->conf); - blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk), - false, &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk), + false, errp)) { return; } diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) } blkconf_serial(&conf->conf, &conf->serial); - blkconf_apply_backend_options(&conf->conf, - blk_is_read_only(conf->conf.blk), true, - &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&conf->conf, + blk_is_read_only(conf->conf.blk), true, + errp)) { return; } s->original_wce = blk_enable_write_cache(conf->conf.blk); - blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, errp)) { return; } + blkconf_blocksizes(&conf->conf); virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index XXXXXXX..XXXXXXX 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -XXX,XX +XXX,XX @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp) { IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus); IDEState *s = bus->ifs + dev->unit; - Error *err = NULL; int ret; if (!dev->conf.blk) { @@ -XXX,XX +XXX,XX @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp) blkconf_serial(&dev->conf, &dev->serial); if (kind != IDE_CD) { - blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, + errp)) { return; } } - blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD, - &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, + kind != IDE_CD, errp)) { return; } diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -XXX,XX +XXX,XX @@ static void scsi_disk_unit_attention_reported(SCSIDevice *dev) static void scsi_realize(SCSIDevice *dev, Error **errp) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); - Error *err = NULL; if (!s->qdev.conf.blk) { error_setg(errp, "drive property not set"); @@ -XXX,XX +XXX,XX @@ static void scsi_realize(SCSIDevice *dev, Error **errp) } if (dev->type == TYPE_DISK) { - blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, errp)) { return; } } - blkconf_apply_backend_options(&dev->conf, - blk_is_read_only(s->qdev.conf.blk), - dev->type == TYPE_DISK, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&dev->conf, + blk_is_read_only(s->qdev.conf.blk), + dev->type == TYPE_DISK, errp)) { return; } diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index XXXXXXX..XXXXXXX 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -XXX,XX +XXX,XX @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) MSDState *s = USB_STORAGE_DEV(dev); BlockBackend *blk = s->conf.blk; SCSIDevice *scsi_dev; - Error *err = NULL; if (!blk) { error_setg(errp, "drive property not set"); @@ -XXX,XX +XXX,XX @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) blkconf_serial(&s->conf, &dev->serial); blkconf_blocksizes(&s->conf); - blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true, + errp)) { return; } @@ -XXX,XX +XXX,XX @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) &usb_msd_scsi_info_storage, NULL); scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable, s->conf.bootindex, dev->serial, - &err); + errp); blk_unref(blk); if (!scsi_dev) { - error_propagate(errp, err); return; } usb_msd_handle_reset(dev); -- 2.14.3 From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> The function name of usb_msd_{realize,unrealize}_*, usb_msd_class_initfn_* are unusual. Rename it to usb_msd_*_{realize,unrealize}, usb_msd_class_*_initfn. Cc: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Message-id: 11e6003433abce35f3f4970e1acc71ee92dbcf51.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/usb/dev-storage.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index XXXXXXX..XXXXXXX 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -XXX,XX +XXX,XX @@ static void usb_msd_unrealize_storage(USBDevice *dev, Error **errp) object_unref(OBJECT(&s->bus)); } -static void usb_msd_realize_storage(USBDevice *dev, Error **errp) +static void usb_msd_storage_realize(USBDevice *dev, Error **errp) { MSDState *s = USB_STORAGE_DEV(dev); BlockBackend *blk = s->conf.blk; @@ -XXX,XX +XXX,XX @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) s->scsi_dev = scsi_dev; } -static void usb_msd_unrealize_bot(USBDevice *dev, Error **errp) +static void usb_msd_bot_unrealize(USBDevice *dev, Error **errp) { MSDState *s = USB_STORAGE_DEV(dev); object_unref(OBJECT(&s->bus)); } -static void usb_msd_realize_bot(USBDevice *dev, Error **errp) +static void usb_msd_bot_realize(USBDevice *dev, Error **errp) { MSDState *s = USB_STORAGE_DEV(dev); DeviceState *d = DEVICE(dev); @@ -XXX,XX +XXX,XX @@ static void usb_msd_class_initfn_common(ObjectClass *klass, void *data) dc->vmsd = &vmstate_usb_msd; } -static void usb_msd_class_initfn_storage(ObjectClass *klass, void *data) +static void usb_msd_class_storage_initfn(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); USBDeviceClass *uc = USB_DEVICE_CLASS(klass); - uc->realize = usb_msd_realize_storage; + uc->realize = usb_msd_storage_realize; uc->unrealize = usb_msd_unrealize_storage; dc->props = msd_properties; } @@ -XXX,XX +XXX,XX @@ static void usb_msd_instance_init(Object *obj) object_property_set_int(obj, -1, "bootindex", NULL); } -static void usb_msd_class_initfn_bot(ObjectClass *klass, void *data) +static void usb_msd_class_bot_initfn(ObjectClass *klass, void *data) { USBDeviceClass *uc = USB_DEVICE_CLASS(klass); - uc->realize = usb_msd_realize_bot; - uc->unrealize = usb_msd_unrealize_bot; + uc->realize = usb_msd_bot_realize; + uc->unrealize = usb_msd_bot_unrealize; uc->attached_settable = true; } static const TypeInfo msd_info = { .name = "usb-storage", .parent = TYPE_USB_STORAGE, - .class_init = usb_msd_class_initfn_storage, + .class_init = usb_msd_class_storage_initfn, .instance_init = usb_msd_instance_init, }; static const TypeInfo bot_info = { .name = "usb-bot", .parent = TYPE_USB_STORAGE, - .class_init = usb_msd_class_initfn_bot, + .class_init = usb_msd_class_bot_initfn, }; static void usb_msd_register_types(void) -- 2.14.3 Commit 1351d1ec89eabebc9fdff20451a62c413d7accc1 ("qdev: drop iothread property type") forgot to remove this include. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-id: 20171205133954.31006-1-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/core/qdev-properties-system.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index XXXXXXX..XXXXXXX 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -XXX,XX +XXX,XX @@ #include "qapi/visitor.h" #include "chardev/char-fe.h" #include "sysemu/tpm_backend.h" -#include "sysemu/iothread.h" static void get_pointer(Object *obj, Visitor *v, Property *prop, char *(*print)(void *ptr), -- 2.14.3 bdrv_unref() requires the AioContext lock because bdrv_flush() uses BDRV_POLL_WHILE(), which assumes the AioContext is currently held. If BDRV_POLL_WHILE() runs without AioContext held the pthread_mutex_unlock() call in aio_context_release() fails. This patch moves bdrv_unref() into the AioContext locked region to solve the following pthread_mutex_unlock() failure: #0 0x00007f566181969b in raise () at /lib64/libc.so.6 #1 0x00007f566181b3b1 in abort () at /lib64/libc.so.6 #2 0x00005592cd590458 in error_exit (err=<optimized out>, msg=msg@entry=0x5592cdaf6d60 <__func__.23977> "qemu_mutex_unlock") at util/qemu-thread-posix.c:36 #3 0x00005592cd96e738 in qemu_mutex_unlock (mutex=mutex@entry=0x5592ce9505e0) at util/qemu-thread-posix.c:96 #4 0x00005592cd969b69 in aio_context_release (ctx=ctx@entry=0x5592ce950580) at util/async.c:507 #5 0x00005592cd8ead78 in bdrv_flush (bs=bs@entry=0x5592cfa87210) at block/io.c:2478 #6 0x00005592cd89df30 in bdrv_close (bs=0x5592cfa87210) at block.c:3207 #7 0x00005592cd89df30 in bdrv_delete (bs=0x5592cfa87210) at block.c:3395 #8 0x00005592cd89df30 in bdrv_unref (bs=0x5592cfa87210) at block.c:4418 #9 0x00005592cd6b7f86 in qmp_transaction (dev_list=<optimized out>, has_props=<optimized out>, props=<optimized out>, errp=errp@entry=0x7ffe4a1fc9d8) at blockdev.c:2308 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-2-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ static void external_snapshot_clean(BlkActionState *common) DO_UPCAST(ExternalSnapshotState, common, common); if (state->aio_context) { bdrv_drained_end(state->old_bs); - aio_context_release(state->aio_context); bdrv_unref(state->new_bs); + aio_context_release(state->aio_context); } } -- 2.14.3 It is not necessary to hold AioContext across transactions anymore since bdrv_drained_begin/end() is used to keep the nodes quiesced. In fact, using the AioContext lock for this purpose was always buggy. This patch reduces the scope of AioContext locked regions. This is not just a cleanup but also fixes hangs that occur in BDRV_POLL_WHILE() because it is unware of recursive locking and does not release the AioContext the necessary number of times to allow progress to be made. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-3-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ typedef struct ExternalSnapshotState { BlkActionState common; BlockDriverState *old_bs; BlockDriverState *new_bs; - AioContext *aio_context; bool overlay_appended; } ExternalSnapshotState; @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, ExternalSnapshotState *state = DO_UPCAST(ExternalSnapshotState, common, common); TransactionAction *action = common->action; + AioContext *aio_context; /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar * purpose but a different set of parameters */ @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, return; } - /* Acquire AioContext now so any threads operating on old_bs stop */ - state->aio_context = bdrv_get_aio_context(state->old_bs); - aio_context_acquire(state->aio_context); + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + + /* Paired with .clean() */ bdrv_drained_begin(state->old_bs); if (!bdrv_is_inserted(state->old_bs)) { error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); - return; + goto out; } if (bdrv_op_is_blocked(state->old_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { - return; + goto out; } if (!bdrv_is_read_only(state->old_bs)) { if (bdrv_flush(state->old_bs)) { error_setg(errp, QERR_IO_ERROR); - return; + goto out; } } if (!bdrv_is_first_non_filter(state->old_bs)) { error_setg(errp, QERR_FEATURE_DISABLED, "snapshot"); - return; + goto out; } if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) { @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, if (node_name && !snapshot_node_name) { error_setg(errp, "New snapshot node name missing"); - return; + goto out; } if (snapshot_node_name && bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) { error_setg(errp, "New snapshot node name already in use"); - return; + goto out; } flags = state->old_bs->open_flags; @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, int64_t size = bdrv_getlength(state->old_bs); if (size < 0) { error_setg_errno(errp, -size, "bdrv_getlength failed"); - return; + goto out; } bdrv_img_create(new_image_file, format, state->old_bs->filename, @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, NULL, size, flags, false, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } } @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, errp); /* We will manually add the backing_hd field to the bs later */ if (!state->new_bs) { - return; + goto out; } if (bdrv_has_blk(state->new_bs)) { error_setg(errp, "The snapshot is already in use"); - return; + goto out; } if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { - return; + goto out; } if (state->new_bs->backing != NULL) { error_setg(errp, "The snapshot already has a backing image"); - return; + goto out; } if (!state->new_bs->drv->supports_backing) { error_setg(errp, "The snapshot does not support backing images"); - return; + goto out; } - bdrv_set_aio_context(state->new_bs, state->aio_context); + bdrv_set_aio_context(state->new_bs, aio_context); /* This removes our old bs and adds the new bs. This is an operation that * can fail, so we need to do it in .prepare; undoing it for abort is @@ -XXX,XX +XXX,XX @@ static void external_snapshot_prepare(BlkActionState *common, bdrv_append(state->new_bs, state->old_bs, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } state->overlay_appended = true; + +out: + aio_context_release(aio_context); } static void external_snapshot_commit(BlkActionState *common) { ExternalSnapshotState *state = DO_UPCAST(ExternalSnapshotState, common, common); + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); /* We don't need (or want) to use the transactional * bdrv_reopen_multiple() across all the entries at once, because we @@ -XXX,XX +XXX,XX @@ static void external_snapshot_commit(BlkActionState *common) bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR, NULL); } + + aio_context_release(aio_context); } static void external_snapshot_abort(BlkActionState *common) @@ -XXX,XX +XXX,XX @@ static void external_snapshot_abort(BlkActionState *common) DO_UPCAST(ExternalSnapshotState, common, common); if (state->new_bs) { if (state->overlay_appended) { + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + bdrv_ref(state->old_bs); /* we can't let bdrv_set_backind_hd() close state->old_bs; we need it */ bdrv_set_backing_hd(state->new_bs, NULL, &error_abort); bdrv_replace_node(state->new_bs, state->old_bs, &error_abort); bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */ + + aio_context_release(aio_context); } } } @@ -XXX,XX +XXX,XX @@ static void external_snapshot_clean(BlkActionState *common) { ExternalSnapshotState *state = DO_UPCAST(ExternalSnapshotState, common, common); - if (state->aio_context) { - bdrv_drained_end(state->old_bs); - bdrv_unref(state->new_bs); - aio_context_release(state->aio_context); + AioContext *aio_context; + + if (!state->old_bs) { + return; } + + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->old_bs); + bdrv_unref(state->new_bs); + + aio_context_release(aio_context); } typedef struct DriveBackupState { -- 2.14.3 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-4-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ static void external_snapshot_clean(BlkActionState *common) typedef struct DriveBackupState { BlkActionState common; BlockDriverState *bs; - AioContext *aio_context; BlockJob *job; } DriveBackupState; @@ -XXX,XX +XXX,XX @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); BlockDriverState *bs; DriveBackup *backup; + AioContext *aio_context; Error *local_err = NULL; assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); @@ -XXX,XX +XXX,XX @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) return; } - /* AioContext is released in .clean() */ - state->aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(state->aio_context); + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + /* Paired with .clean() */ bdrv_drained_begin(bs); + state->bs = bs; state->job = do_drive_backup(backup, common->block_job_txn, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } + +out: + aio_context_release(aio_context); } static void drive_backup_commit(BlkActionState *common) { DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + assert(state->job); block_job_start(state->job); + + aio_context_release(aio_context); } static void drive_backup_abort(BlkActionState *common) @@ -XXX,XX +XXX,XX @@ static void drive_backup_abort(BlkActionState *common) DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); if (state->job) { + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + block_job_cancel_sync(state->job); + + aio_context_release(aio_context); } } static void drive_backup_clean(BlkActionState *common) { DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + AioContext *aio_context; - if (state->aio_context) { - bdrv_drained_end(state->bs); - aio_context_release(state->aio_context); + if (!state->bs) { + return; } + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->bs); + + aio_context_release(aio_context); } typedef struct BlockdevBackupState { -- 2.14.3 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-5-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ typedef struct BlockdevBackupState { BlkActionState common; BlockDriverState *bs; BlockJob *job; - AioContext *aio_context; } BlockdevBackupState; static BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); BlockdevBackup *backup; BlockDriverState *bs, *target; + AioContext *aio_context; Error *local_err = NULL; assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) return; } - /* AioContext is released in .clean() */ - state->aio_context = bdrv_get_aio_context(bs); - if (state->aio_context != bdrv_get_aio_context(target)) { - state->aio_context = NULL; + aio_context = bdrv_get_aio_context(bs); + if (aio_context != bdrv_get_aio_context(target)) { error_setg(errp, "Backup between two IO threads is not implemented"); return; } - aio_context_acquire(state->aio_context); + aio_context_acquire(aio_context); state->bs = bs; + + /* Paired with .clean() */ bdrv_drained_begin(state->bs); state->job = do_blockdev_backup(backup, common->block_job_txn, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } + +out: + aio_context_release(aio_context); } static void blockdev_backup_commit(BlkActionState *common) { BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + assert(state->job); block_job_start(state->job); + + aio_context_release(aio_context); } static void blockdev_backup_abort(BlkActionState *common) @@ -XXX,XX +XXX,XX @@ static void blockdev_backup_abort(BlkActionState *common) BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); if (state->job) { + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + block_job_cancel_sync(state->job); + + aio_context_release(aio_context); } } static void blockdev_backup_clean(BlkActionState *common) { BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + AioContext *aio_context; - if (state->aio_context) { - bdrv_drained_end(state->bs); - aio_context_release(state->aio_context); + if (!state->bs) { + return; } + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->bs); + + aio_context_release(aio_context); } typedef struct BlockDirtyBitmapState { -- 2.14.3 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-6-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ struct BlkActionState { typedef struct InternalSnapshotState { BlkActionState common; BlockDriverState *bs; - AioContext *aio_context; QEMUSnapshotInfo sn; bool created; } InternalSnapshotState; @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_prepare(BlkActionState *common, qemu_timeval tv; BlockdevSnapshotInternal *internal; InternalSnapshotState *state; + AioContext *aio_context; int ret1; g_assert(common->action->type == @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_prepare(BlkActionState *common, return; } - /* AioContext is released in .clean() */ - state->aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(state->aio_context); + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); state->bs = bs; + + /* Paired with .clean() */ bdrv_drained_begin(bs); if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) { - return; + goto out; } if (bdrv_is_read_only(bs)) { error_setg(errp, "Device '%s' is read only", device); - return; + goto out; } if (!bdrv_can_snapshot(bs)) { error_setg(errp, "Block format '%s' used by device '%s' " "does not support internal snapshots", bs->drv->format_name, device); - return; + goto out; } if (!strlen(name)) { error_setg(errp, "Name is empty"); - return; + goto out; } /* check whether a snapshot with name exist */ @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_prepare(BlkActionState *common, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } else if (ret) { error_setg(errp, "Snapshot with name '%s' already exists on device '%s'", name, device); - return; + goto out; } /* 3. take the snapshot */ @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_prepare(BlkActionState *common, error_setg_errno(errp, -ret1, "Failed to create snapshot '%s' on device '%s'", name, device); - return; + goto out; } /* 4. succeed, mark a snapshot is created */ state->created = true; + +out: + aio_context_release(aio_context); } static void internal_snapshot_abort(BlkActionState *common) @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_abort(BlkActionState *common) DO_UPCAST(InternalSnapshotState, common, common); BlockDriverState *bs = state->bs; QEMUSnapshotInfo *sn = &state->sn; + AioContext *aio_context; Error *local_error = NULL; if (!state->created) { return; } + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) { error_reportf_err(local_error, "Failed to delete snapshot with id '%s' and " @@ -XXX,XX +XXX,XX @@ static void internal_snapshot_abort(BlkActionState *common) sn->id_str, sn->name, bdrv_get_device_name(bs)); } + + aio_context_release(aio_context); } static void internal_snapshot_clean(BlkActionState *common) { InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState, common, common); + AioContext *aio_context; - if (state->aio_context) { - if (state->bs) { - bdrv_drained_end(state->bs); - } - aio_context_release(state->aio_context); + if (!state->bs) { + return; } + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->bs); + + aio_context_release(aio_context); } /* external snapshot private data */ -- 2.14.3 The dirty bitmap actions in qmp_transaction have not used AioContext since the dirty bitmap locking discipline was introduced in commit 2119882c7eb7e2c612b24fc0c8d86f5887d6f1c3 ("block: introduce dirty_bitmap_mutex"). Remove the unused field. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-7-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ typedef struct BlockDirtyBitmapState { BlkActionState common; BdrvDirtyBitmap *bitmap; BlockDriverState *bs; - AioContext *aio_context; HBitmap *backup; bool prepared; } BlockDirtyBitmapState; @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, } bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); - /* AioContext is released in .clean() */ } static void block_dirty_bitmap_clear_abort(BlkActionState *common) @@ -XXX,XX +XXX,XX @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common) hbitmap_free(state->backup); } -static void block_dirty_bitmap_clear_clean(BlkActionState *common) -{ - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); - - if (state->aio_context) { - aio_context_release(state->aio_context); - } -} - static void abort_prepare(BlkActionState *common, Error **errp) { error_setg(errp, "Transaction aborted using Abort action"); @@ -XXX,XX +XXX,XX @@ static const BlkActionOps actions[] = { .prepare = block_dirty_bitmap_clear_prepare, .commit = block_dirty_bitmap_clear_commit, .abort = block_dirty_bitmap_clear_abort, - .clean = block_dirty_bitmap_clear_clean, } }; -- 2.14.3 Encapsulate IOThread QOM object lookup so that callers don't need to know how and where IOThread objects live. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-8-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/sysemu/iothread.h | 1 + iothread.c | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/iothread.h +++ b/include/sysemu/iothread.h @@ -XXX,XX +XXX,XX @@ typedef struct { OBJECT_CHECK(IOThread, obj, TYPE_IOTHREAD) char *iothread_get_id(IOThread *iothread); +IOThread *iothread_by_id(const char *id); AioContext *iothread_get_aio_context(IOThread *iothread); void iothread_stop_all(void); GMainContext *iothread_get_g_main_context(IOThread *iothread); diff --git a/iothread.c b/iothread.c index XXXXXXX..XXXXXXX 100644 --- a/iothread.c +++ b/iothread.c @@ -XXX,XX +XXX,XX @@ void iothread_destroy(IOThread *iothread) { object_unparent(OBJECT(iothread)); } + +/* Lookup IOThread by its id. Only finds user-created objects, not internal + * iothread_create() objects. */ +IOThread *iothread_by_id(const char *id) +{ + return IOTHREAD(object_resolve_path_type(id, TYPE_IOTHREAD, NULL)); +} -- 2.14.3 Currently there is no easy way for iotests to ensure that a BDS is bound to a particular IOThread. Normally the virtio-blk device calls blk_set_aio_context() when dataplane is enabled during guest driver initialization. This never happens in iotests since -machine accel=qtest means there is no guest activity (including device driver initialization). This patch adds a QMP command to explicitly assign IOThreads in test cases. See qapi/block-core.json for a description of the command. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-9-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- qapi/block-core.json | 36 ++++++++++++++++++++++++++++++++++++ blockdev.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/qapi/block-core.json b/qapi/block-core.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -XXX,XX +XXX,XX @@ 'data' : { 'parent': 'str', '*child': 'str', '*node': 'str' } } + +## +# @x-blockdev-set-iothread: +# +# Move @node and its children into the @iothread. If @iothread is null then +# move @node and its children into the main loop. +# +# The node must not be attached to a BlockBackend. +# +# @node-name: the name of the block driver node +# +# @iothread: the name of the IOThread object or null for the main loop +# +# Note: this command is experimental and intended for test cases that need +# control over IOThreads only. +# +# Since: 2.12 +# +# Example: +# +# 1. Move a node into an IOThread +# -> { "execute": "x-blockdev-set-iothread", +# "arguments": { "node-name": "disk1", +# "iothread": "iothread0" } } +# <- { "return": {} } +# +# 2. Move a node into the main loop +# -> { "execute": "x-blockdev-set-iothread", +# "arguments": { "node-name": "disk1", +# "iothread": null } } +# <- { "return": {} } +# +## +{ 'command': 'x-blockdev-set-iothread', + 'data' : { 'node-name': 'str', + 'iothread': 'StrOrNull' } } diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ #include "qapi/qmp/qerror.h" #include "qapi/qobject-output-visitor.h" #include "sysemu/sysemu.h" +#include "sysemu/iothread.h" #include "block/block_int.h" #include "qmp-commands.h" #include "block/trace.h" @@ -XXX,XX +XXX,XX @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) return head; } +void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, + Error **errp) +{ + AioContext *old_context; + AioContext *new_context; + BlockDriverState *bs; + + bs = bdrv_find_node(node_name); + if (!bs) { + error_setg(errp, "Cannot find node %s", node_name); + return; + } + + /* If we want to allow more extreme test scenarios this guard could be + * removed. For now it protects against accidents. */ + if (bdrv_has_blk(bs)) { + error_setg(errp, "Node %s is in use", node_name); + return; + } + + if (iothread->type == QTYPE_QSTRING) { + IOThread *obj = iothread_by_id(iothread->u.s); + if (!obj) { + error_setg(errp, "Cannot find iothread %s", iothread->u.s); + return; + } + + new_context = iothread_get_aio_context(obj); + } else { + new_context = qemu_get_aio_context(); + } + + old_context = bdrv_get_aio_context(bs); + aio_context_acquire(old_context); + + bdrv_set_aio_context(bs, new_context); + + aio_context_release(old_context); +} + QemuOptsList qemu_common_drive_opts = { .name = "drive", .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head), -- 2.14.3 QMP 'transaction' blockdev-snapshot-sync with multiple disks in an IOThread is an untested code path. Several bugs have been found in connection with this command. This patch adds a test case to prevent future regressions. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171206144550.22295-10-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tests/qemu-iotests/202 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/202.out | 11 ++++++ tests/qemu-iotests/group | 1 + 3 files changed, 107 insertions(+) create mode 100755 tests/qemu-iotests/202 create mode 100644 tests/qemu-iotests/202.out diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/202 @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env python +# +# 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: Stefan Hajnoczi <stefanha@redhat.com> +# +# Check that QMP 'transaction' blockdev-snapshot-sync with multiple drives on a +# single IOThread completes successfully. This particular command triggered a +# hang due to recursive AioContext locking and BDRV_POLL_WHILE(). Protect +# against regressions. + +import iotests + +iotests.verify_image_format(supported_fmts=['qcow2']) +iotests.verify_platform(['linux']) + +with iotests.FilePath('disk0.img') as disk0_img_path, \ + iotests.FilePath('disk1.img') as disk1_img_path, \ + iotests.FilePath('disk0-snap.img') as disk0_snap_img_path, \ + iotests.FilePath('disk1-snap.img') as disk1_snap_img_path, \ + iotests.VM() as vm: + + img_size = '10M' + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size) + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size) + + iotests.log('Launching VM...') + vm.launch() + + iotests.log('Adding IOThread...') + iotests.log(vm.qmp('object-add', + qom_type='iothread', + id='iothread0')) + + iotests.log('Adding blockdevs...') + iotests.log(vm.qmp('blockdev-add', + driver=iotests.imgfmt, + node_name='disk0', + file={ + 'driver': 'file', + 'filename': disk0_img_path, + })) + iotests.log(vm.qmp('blockdev-add', + driver=iotests.imgfmt, + node_name='disk1', + file={ + 'driver': 'file', + 'filename': disk1_img_path, + })) + + iotests.log('Setting iothread...') + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='disk0', + iothread='iothread0')) + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='disk1', + iothread='iothread0')) + + iotests.log('Creating external snapshots...') + iotests.log(vm.qmp( + 'transaction', + actions=[ + { + 'data': { + 'node-name': 'disk0', + 'snapshot-file': disk0_snap_img_path, + 'snapshot-node-name': 'disk0-snap', + 'mode': 'absolute-paths', + 'format': iotests.imgfmt, + }, + 'type': 'blockdev-snapshot-sync' + }, { + 'data': { + 'node-name': 'disk1', + 'snapshot-file': disk1_snap_img_path, + 'snapshot-node-name': 'disk1-snap', + 'mode': 'absolute-paths', + 'format': iotests.imgfmt + }, + 'type': 'blockdev-snapshot-sync' + } + ])) diff --git a/tests/qemu-iotests/202.out b/tests/qemu-iotests/202.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/202.out @@ -XXX,XX +XXX,XX @@ +Launching VM... +Adding IOThread... +{u'return': {}} +Adding blockdevs... +{u'return': {}} +{u'return': {}} +Setting iothread... +{u'return': {}} +{u'return': {}} +Creating external snapshots... +{u'return': {}} 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 @@ 197 rw auto quick 198 rw auto 200 rw auto +202 rw auto quick -- 2.14.3 From: Mark Kanda <mark.kanda@oracle.com> Depending on the configuration, it can be beneficial to adjust the virtio-blk queue size to something other than the current default of 128. Add a new property to make the queue size configurable. Signed-off-by: Mark Kanda <mark.kanda@oracle.com> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com> Reviewed-by: Ameya More <ameya.more@oracle.com> Message-id: 52e6d742811f10dbd16e996e86cf375b9577c187.1513005190.git.mark.kanda@oracle.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/hw/virtio/virtio-blk.h | 1 + hw/block/virtio-blk.c | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -XXX,XX +XXX,XX @@ struct VirtIOBlkConf uint32_t config_wce; uint32_t request_merging; uint16_t num_queues; + uint16_t queue_size; }; struct VirtIOBlockDataPlane; diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) error_setg(errp, "num-queues property must be larger than 0"); return; } + if (!is_power_of_2(conf->queue_size) || + conf->queue_size > VIRTQUEUE_MAX_SIZE) { + error_setg(errp, "invalid queue-size property (%" PRIu16 "), " + "must be a power of 2 (max %d)", + conf->queue_size, VIRTQUEUE_MAX_SIZE); + return; + } blkconf_serial(&conf->conf, &conf->serial); if (!blkconf_apply_backend_options(&conf->conf, @@ -XXX,XX +XXX,XX @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1; for (i = 0; i < conf->num_queues; i++) { - virtio_add_queue(vdev, 128, virtio_blk_handle_output); + virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output); } virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err); if (err != NULL) { @@ -XXX,XX +XXX,XX @@ static Property virtio_blk_properties[] = { DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, true), DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, IOThread *), DEFINE_PROP_END_OF_LIST(), -- 2.14.3 From: Mark Kanda <mark.kanda@oracle.com> virtio-blk logical block size should never be larger than physical block size because it doesn't make sense to have such configurations. QEMU doesn't have a way to effectively express this condition; the best it can do is report the physical block exponent as 0 - indicating the logical block size equals the physical block size. This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2 but applied to virtio-blk (instead of virtio-scsi). Signed-off-by: Mark Kanda <mark.kanda@oracle.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Ameya More <ameya.more@oracle.com> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-id: 773169891f9f2deb4cb7c4ef2655580dbe24c1d1.1513005190.git.mark.kanda@oracle.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/virtio-blk.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index XXXXXXX..XXXXXXX 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -XXX,XX +XXX,XX @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) blkconf_blocksizes(&conf->conf); + if (conf->conf.logical_block_size > + conf->conf.physical_block_size) { + error_setg(errp, + "logical_block_size > physical_block_size not supported"); + return; + } + virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, sizeof(struct virtio_blk_config)); -- 2.14.3 From: Paolo Bonzini <pbonzini@redhat.com> BDRV_POLL_WHILE() does not support recursive AioContext locking. It only releases the AioContext lock once regardless of how many times the caller has acquired it. This results in a hang since the IOThread does not make progress while the AioContext is still locked. The following steps trigger the hang: $ qemu-system-x86_64 -M accel=kvm -m 1G -cpu host \ -object iothread,id=iothread0 \ -device virtio-scsi-pci,iothread=iothread0 \ -drive if=none,id=drive0,file=test.img,format=raw \ -device scsi-hd,drive=drive0 \ -drive if=none,id=drive1,file=test.img,format=raw \ -device scsi-hd,drive=drive1 $ qemu-system-x86_64 ...same options... \ -incoming tcp::1234 (qemu) migrate tcp:127.0.0.1:1234 ...hang... Tested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171207201320.19284-2-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- block.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index XXXXXXX..XXXXXXX 100644 --- a/block.c +++ b/block.c @@ -XXX,XX +XXX,XX @@ int bdrv_inactivate_all(void) BdrvNextIterator it; int ret = 0; int pass; + GSList *aio_ctxs = NULL, *ctx; for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { - aio_context_acquire(bdrv_get_aio_context(bs)); + AioContext *aio_context = bdrv_get_aio_context(bs); + + if (!g_slist_find(aio_ctxs, aio_context)) { + aio_ctxs = g_slist_prepend(aio_ctxs, aio_context); + aio_context_acquire(aio_context); + } } /* We do two passes of inactivation. The first pass calls to drivers' @@ -XXX,XX +XXX,XX @@ int bdrv_inactivate_all(void) } out: - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { - aio_context_release(bdrv_get_aio_context(bs)); + for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) { + AioContext *aio_context = ctx->data; + aio_context_release(aio_context); } + g_slist_free(aio_ctxs); return ret; } -- 2.14.3 See the patch for why nested AioContext locking is no longer allowed. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171207201320.19284-3-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- docs/devel/multiple-iothreads.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt index XXXXXXX..XXXXXXX 100644 --- a/docs/devel/multiple-iothreads.txt +++ b/docs/devel/multiple-iothreads.txt @@ -XXX,XX +XXX,XX @@ -Copyright (c) 2014 Red Hat Inc. +Copyright (c) 2014-2017 Red Hat Inc. This work is licensed under the terms of the GNU GPL, version 2 or later. See the COPYING file in the top-level directory. @@ -XXX,XX +XXX,XX @@ aio_context_acquire()/aio_context_release() for mutual exclusion. Once the context is acquired no other thread can access it or run event loop iterations in this AioContext. -aio_context_acquire()/aio_context_release() calls may be nested. This -means you can call them if you're not sure whether #2 applies. +Legacy code sometimes nests aio_context_acquire()/aio_context_release() calls. +Do not use nesting anymore, it is incompatible with the BDRV_POLL_WHILE() macro +used in the block layer and can lead to hangs. There is currently no lock ordering rule if a thread needs to acquire multiple AioContexts simultaneously. Therefore, it is only safe for code holding the -- 2.14.3 When a node is already associated with a BlockBackend the x-blockdev-set-iothread command refuses to set the IOThread. This is to prevent accidentally changing the IOThread when the nodes are in use. When the nodes are created with -drive they automatically get a BlockBackend. In that case we know nothing is using them yet and it's safe to set the IOThread. Add a force boolean to override the check. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171207201320.19284-4-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- qapi/block-core.json | 6 +++++- blockdev.c | 11 ++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index XXXXXXX..XXXXXXX 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -XXX,XX +XXX,XX @@ # # @iothread: the name of the IOThread object or null for the main loop # +# @force: true if the node and its children should be moved when a BlockBackend +# is already attached +# # Note: this command is experimental and intended for test cases that need # control over IOThreads only. # @@ -XXX,XX +XXX,XX @@ ## { 'command': 'x-blockdev-set-iothread', 'data' : { 'node-name': 'str', - 'iothread': 'StrOrNull' } } + 'iothread': 'StrOrNull', + '*force': 'bool' } } diff --git a/blockdev.c b/blockdev.c index XXXXXXX..XXXXXXX 100644 --- a/blockdev.c +++ b/blockdev.c @@ -XXX,XX +XXX,XX @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) } void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, - Error **errp) + bool has_force, bool force, Error **errp) { AioContext *old_context; AioContext *new_context; @@ -XXX,XX +XXX,XX @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, return; } - /* If we want to allow more extreme test scenarios this guard could be - * removed. For now it protects against accidents. */ - if (bdrv_has_blk(bs)) { - error_setg(errp, "Node %s is in use", node_name); + /* Protects against accidents. */ + if (!(has_force && force) && bdrv_has_blk(bs)) { + error_setg(errp, "Node %s is associated with a BlockBackend and could " + "be in use (use force=true to override this check)", + node_name); return; } -- 2.14.3 The VM.add_object() method can be used to add IOThreads or memory backend objects. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171207201320.19284-5-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tests/qemu-iotests/iotests.py | 5 +++++ 1 file changed, 5 insertions(+) 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 @@ class VM(qtest.QEMUQtestMachine): socket_scm_helper=socket_scm_helper) self._num_drives = 0 + def add_object(self, opts): + self._args.append('-object') + self._args.append(opts) + return self + def add_device(self, opts): self._args.append('-device') self._args.append(opts) -- 2.14.3 There is a small chance that iothread_stop() hangs as follows: Thread 3 (Thread 0x7f63eba5f700 (LWP 16105)): #0 0x00007f64012c09b6 in ppoll () at /lib64/libc.so.6 #1 0x000055959992eac9 in ppoll (__ss=0x0, __timeout=0x0, __nfds=<optimized out>, __fds=<optimized out>) at /usr/include/bits/poll2.h:77 #2 0x000055959992eac9 in qemu_poll_ns (fds=<optimized out>, nfds=<optimized out>, timeout=<optimized out>) at util/qemu-timer.c:322 #3 0x0000559599930711 in aio_poll (ctx=0x55959bdb83c0, blocking=blocking@entry=true) at util/aio-posix.c:629 #4 0x00005595996806fe in iothread_run (opaque=0x55959bd78400) at iothread.c:59 #5 0x00007f640159f609 in start_thread () at /lib64/libpthread.so.0 #6 0x00007f64012cce6f in clone () at /lib64/libc.so.6 Thread 1 (Thread 0x7f640b45b280 (LWP 16103)): #0 0x00007f64015a0b6d in pthread_join () at /lib64/libpthread.so.0 #1 0x00005595999332ef in qemu_thread_join (thread=<optimized out>) at util/qemu-thread-posix.c:547 #2 0x00005595996808ae in iothread_stop (iothread=<optimized out>) at iothread.c:91 #3 0x000055959968094d in iothread_stop_iter (object=<optimized out>, opaque=<optimized out>) at iothread.c:102 #4 0x0000559599857d97 in do_object_child_foreach (obj=obj@entry=0x55959bdb8100, fn=fn@entry=0x559599680930 <iothread_stop_iter>, opaque=opaque@entry=0x0, recurse=recurse@entry=false) at qom/object.c:852 #5 0x0000559599859477 in object_child_foreach (obj=obj@entry=0x55959bdb8100, fn=fn@entry=0x559599680930 <iothread_stop_iter>, opaque=opaque@entry=0x0) at qom/object.c:867 #6 0x0000559599680a6e in iothread_stop_all () at iothread.c:341 #7 0x000055959955b1d5 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4913 The relevant code from iothread_run() is: while (!atomic_read(&iothread->stopping)) { aio_poll(iothread->ctx, true); and iothread_stop(): iothread->stopping = true; aio_notify(iothread->ctx); ... qemu_thread_join(&iothread->thread); The following scenario can occur: 1. IOThread: while (!atomic_read(&iothread->stopping)) -> stopping=false 2. Main loop: iothread->stopping = true; aio_notify(iothread->ctx); 3. IOThread: aio_poll(iothread->ctx, true); -> hang The bug is explained by the AioContext->notify_me doc comments: "If this field is 0, everything (file descriptors, bottom halves, timers) will be re-evaluated before the next blocking poll(), thus the event_notifier_set call can be skipped." The problem is that "everything" does not include checking iothread->stopping. This means iothread_run() will block in aio_poll() if aio_notify() was called just before aio_poll(). This patch fixes the hang by replacing aio_notify() with aio_bh_schedule_oneshot(). This makes aio_poll() or g_main_loop_run() to return. Implementing this properly required a new bool running flag. The new flag prevents races that are tricky if we try to use iothread->stopping. Now iothread->stopping is purely for iothread_stop() and iothread->running is purely for the iothread_run() thread. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171207201320.19284-6-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/sysemu/iothread.h | 3 ++- iothread.c | 20 +++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h index XXXXXXX..XXXXXXX 100644 --- a/include/sysemu/iothread.h +++ b/include/sysemu/iothread.h @@ -XXX,XX +XXX,XX @@ typedef struct { GOnce once; QemuMutex init_done_lock; QemuCond init_done_cond; /* is thread initialization done? */ - bool stopping; + bool stopping; /* has iothread_stop() been called? */ + bool running; /* should iothread_run() continue? */ int thread_id; /* AioContext poll parameters */ diff --git a/iothread.c b/iothread.c index XXXXXXX..XXXXXXX 100644 --- a/iothread.c +++ b/iothread.c @@ -XXX,XX +XXX,XX @@ static void *iothread_run(void *opaque) qemu_cond_signal(&iothread->init_done_cond); qemu_mutex_unlock(&iothread->init_done_lock); - while (!atomic_read(&iothread->stopping)) { + while (iothread->running) { aio_poll(iothread->ctx, true); if (atomic_read(&iothread->worker_context)) { @@ -XXX,XX +XXX,XX @@ static void *iothread_run(void *opaque) return NULL; } +/* Runs in iothread_run() thread */ +static void iothread_stop_bh(void *opaque) +{ + IOThread *iothread = opaque; + + iothread->running = false; /* stop iothread_run() */ + + if (iothread->main_loop) { + g_main_loop_quit(iothread->main_loop); + } +} + void iothread_stop(IOThread *iothread) { if (!iothread->ctx || iothread->stopping) { return; } iothread->stopping = true; - aio_notify(iothread->ctx); - if (atomic_read(&iothread->main_loop)) { - g_main_loop_quit(iothread->main_loop); - } + aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread); qemu_thread_join(&iothread->thread); } @@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp) char *name, *thread_name; iothread->stopping = false; + iothread->running = true; iothread->thread_id = -1; iothread->ctx = aio_context_new(&local_error); if (!iothread->ctx) { -- 2.14.3 This test case will prevent future regressions with savevm and IOThreads. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-id: 20171207201320.19284-7-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tests/qemu-iotests/203 | 59 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/203.out | 6 +++++ tests/qemu-iotests/group | 1 + 3 files changed, 66 insertions(+) create mode 100755 tests/qemu-iotests/203 create mode 100644 tests/qemu-iotests/203.out diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203 new file mode 100755 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/203 @@ -XXX,XX +XXX,XX @@ +#!/usr/bin/env python +# +# 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: Stefan Hajnoczi <stefanha@redhat.com> +# +# Check that QMP 'migrate' with multiple drives on a single IOThread completes +# successfully. This particular command triggered a hang in the source QEMU +# process due to recursive AioContext locking in bdrv_invalidate_all() and +# BDRV_POLL_WHILE(). + +import iotests + +iotests.verify_image_format(supported_fmts=['qcow2']) +iotests.verify_platform(['linux']) + +with iotests.FilePath('disk0.img') as disk0_img_path, \ + iotests.FilePath('disk1.img') as disk1_img_path, \ + iotests.VM() as vm: + + img_size = '10M' + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size) + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size) + + iotests.log('Launching VM...') + (vm.add_object('iothread,id=iothread0') + .add_drive(disk0_img_path, 'node-name=drive0-node', interface='none') + .add_drive(disk1_img_path, 'node-name=drive1-node', interface='none') + .launch()) + + iotests.log('Setting IOThreads...') + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='drive0-node', iothread='iothread0', + force=True)) + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='drive1-node', iothread='iothread0', + force=True)) + + iotests.log('Starting migration...') + iotests.log(vm.qmp('migrate', uri='exec:cat >/dev/null')) + while True: + vm.get_qmp_event(wait=60.0) + result = vm.qmp('query-migrate') + status = result.get('return', {}).get('status', None) + if status == 'completed': + break diff --git a/tests/qemu-iotests/203.out b/tests/qemu-iotests/203.out new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/qemu-iotests/203.out @@ -XXX,XX +XXX,XX @@ +Launching VM... +Setting IOThreads... +{u'return': {}} +{u'return': {}} +Starting migration... +{u'return': {}} 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 @@ 198 rw auto 200 rw auto 202 rw auto quick +203 rw auto -- 2.14.3