Comparing changes

We are going to add more block-graph modifying transaction actions, and block-graph modifying functions are already based on Transaction API. Next, we'll need to separately update permissions after several graph-modifying actions, and this would be simple with help of Transaction API. So, now let's just transform what we have into new-style transaction actions. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-2-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Look at qmp_transaction(): dev_list is not obvious name for list of actions. Let's look at qapi spec, this argument is "actions". Let's follow the common practice of using same argument names in qapi scheme and code. To be honest, rename props to properties for same reason. Next, we have to rename global map of actions, to not conflict with new name for function argument. Rename also dev_entry loop variable accordingly to new name of the list. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510150624.310640-3-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510150624.310640-4-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Only backup supports GROUPED mode. Make this logic more clear. And avoid passing extra thing to each action. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-5-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Other bitmap related actions use the .bitmap pointer in .abort action, let's do same here: 1. It helps further refactoring, as bitmap-add is the only bitmap action that uses state.action in .abort 2. It must be safe: transaction actions rely on the fact that on .abort() the state is the same as at the end of .prepare(), so that in .abort() we could precisely rollback the changes done by .prepare(). The only way to remove the bitmap during transaction should be block-dirty-bitmap-remove action, but it postpones actual removal to .commit(), so we are OK on any rollback path. (Note also that bitmap-remove is the only bitmap action that has .commit() phase, except for simple g_free the state on .clean()) 3. Again, other bitmap actions behave this way: keep the bitmap pointer during the transaction. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-6-vsementsov@yandex-team.ru> [kwolf: Also remove the now unused BlockDirtyBitmapState.prepared] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Let's simplify things: First, actions generally don't need access to common BlkActionState structure. The only exclusion are backup actions that need block_job_txn. Next, for transaction actions of Transaction API is more native to allocated state structure in the action itself. So, do the following transformation: 1. Let all actions be represented by a function with corresponding structure as arguments. 2. Instead of array-map marshaller, let's make a function, that calls corresponding action directly. 3. BlkActionOps and BlkActionState structures become unused Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20230510150624.310640-7-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

"zlib" clusters are actually raw deflate (RFC1951) clusters without zlib headers. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp> Message-Id: <168424874322.11954.1340942046351859521-0@git.sr.ht> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

These are functions that modify the graph, so they must be able to take a writer lock. This is impossible if they already hold the reader lock. If they need a reader lock for some of their operations, they should take it internally. Many of them go through blk_*(), which will always take the lock itself. Direct calls of bdrv_*() need to take the reader lock. Note that while locking for bdrv_co_*() calls is checked by TSA, this is not the case for the mixed_coroutine_fns bdrv_*(). Holding the lock is still required when they are called from coroutine context like here! This effectively reverts 4ec8df0, but adds some internal locking instead. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-2-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

There are some error paths in blk_exp_add() that jump to 'fail:' before 'exp' is even created. So we can't just unconditionally access exp->blk. Add a NULL check, and switch from exp->blk to blk, which is available earlier, just to be extra sure that we really cover all cases where BlockDevOps could have been set for it (in practice, this only happens in drv->create() today, so this part of the change isn't strictly necessary). Fixes: Coverity CID 1509238 Fixes: de79b52 Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-3-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Tested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

qcow2_do_open() calls a few no_co_wrappers that wrap functions taking the graph lock internally as a writer. Therefore, it can't hold the reader lock across these calls, it causes deadlocks. Drop the lock temporarily around the calls. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-4-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

If we take a reader lock, we can't call any functions that take a writer lock internally without causing deadlocks once the reader lock is actually enforced in the main thread, too. Take the reader lock only where it is actually needed. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-5-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

If we take a reader lock, we can't call any functions that take a writer lock internally without causing deadlocks once the reader lock is actually enforced in the main thread, too. Take the reader lock only where it is actually needed. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-6-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

bdrv_unref() is a no_coroutine_fn, so calling it from coroutine context is invalid. Use bdrv_co_unref() instead. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-7-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

When jobs are sleeping, for example to enforce a given rate limit, they can be reentered early, in particular in order to get paused, to update the rate limit or to get cancelled. Before this patch, they behave in this case as if they had fully completed their rate limiting delay. This means that requests are sped up beyond their limit, violating the constraints that the user gave us. Change the block jobs to sleep in a loop until the necessary delay is completed, while still allowing cancelling them immediately as well pausing (handled by the pause point in job_sleep_ns()) and updating the rate limit. This change is also motivated by iotests cases being prone to fail because drain operations pause and unpause them so often that block jobs complete earlier than they are supposed to. In particular, the next commit would fail iotests 030 without this change. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-8-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

There are some conditions under which we don't actually need to do anything for taking a reader lock: Writing the graph is only possible from the main context while holding the BQL. So if a reader is running in the main context under the BQL and knows that it won't be interrupted until the next writer runs, we don't actually need to do anything. This is the case if the reader code neither has a nested event loop (this is forbidden anyway while you hold the lock) nor is a coroutine (because a writer could run when the coroutine has yielded). These conditions are exactly what bdrv_graph_rdlock_main_loop() asserts. They are not fulfilled in bdrv_graph_co_rdlock(), which always runs in a coroutine. This deletes the shortcuts in bdrv_graph_co_rdlock() that skip taking the reader lock in the main thread. Reported-by: Fiona Ebner <f.ebner@proxmox.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230510203601.418015-9-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Skip TestBlockdevReopen.test_insert_compress_filter() if the 'compress' driver isn't available. In order to make the test succeed when the case is skipped, we also need to remove any output from it (which would be missing in the case where we skip it). This is done by replacing qemu_io_log() with qemu_io(). In case of failure, qemu_io() raises an exception with the output of the qemu-io binary in its message, so we don't actually lose anything. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230511143801.255021-1-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

QEMU's event loop supports nesting, which means that event handler functions may themselves call aio_poll(). The condition that triggered a handler must be reset before the nested aio_poll() call, otherwise the same handler will be called and immediately re-enter aio_poll. This leads to an infinite loop and stack exhaustion. Poll handlers are especially prone to this issue, because they typically reset their condition by finishing the processing of pending work. Unfortunately it is during the processing of pending work that nested aio_poll() calls typically occur and the condition has not yet been reset. Disable a poll handler during ->io_poll_ready() so that a nested aio_poll() call cannot invoke ->io_poll_ready() again. As a result, the disabled poll handler and its associated fd handler do not run during the nested aio_poll(). Calling aio_set_fd_handler() from inside nested aio_poll() could cause it to run again. If the fd handler is pending inside nested aio_poll(), then it will also run again. In theory fd handlers can be affected by the same issue, but they are more likely to reset the condition before calling nested aio_poll(). This is a special case and it's somewhat complex, but I don't see a way around it as long as nested aio_poll() is supported. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2186181 Fixes: c382706 ("block: Mark bdrv_co_io_(un)plug() and callers GRAPH_RDLOCK") Cc: Kevin Wolf <kwolf@redhat.com> Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20230502184134.534703-2-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20230502184134.534703-3-stefanha@redhat.com> Tested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Two type hints fail centos-stream-8-x86_64 CI. They are actually broken. Changing them to Optional[re.Match[str]] fixes them locally for me, but then CI fails differently. Drop them for now. Fixes: 3e32dca (qapi: Rewrite parsing of doc comment section symbols and tags) Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20230517061600.1782455-1-armbru@redhat.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Tested-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Block layer patches - qcow2 spec: Rename "zlib" compression to "deflate" - Honour graph read lock even in the main thread + prerequisite fixes - aio-posix: do not nest poll handlers (fixes infinite recursion) - Refactor QMP blockdev transactions - iotests/245: Check if 'compress' driver is available # -----BEGIN PGP SIGNATURE----- # # iQJFBAABCAAvFiEE3D3rFZqa+V09dFb+fwmycsiPL9YFAmRlBcwRHGt3b2xmQHJl # ZGhhdC5jb20ACgkQfwmycsiPL9Z3MRAAuILiH3EV3Dz8L0tgacdMT2cinfdBJYjj # LMJrrxeaIgHGb3p80nnLRPE3JvT1+DwfCDr5Gi+T9BE7Qz2rnQ+SvnxBWa0Fns2i # E54amK6aOqFnqfAVLLHbe4Xc+SY91dWIZf3XDsEhhbWnsnAMauehkM9+W52XkFjQ # apzuoATZD4FeCZRX9PoSFSnTL88ZcA4PGdglr+fjPO4yWY3Hs9RhS9t35iY+zdFD # JqYwW8Os2kHO7nzpjdXkwz51mxKFcUqafb2Lbt6Fnhv8Q+rdAiSs5pxT/KSuwmCE # vOw6EKjz6XKpqwktKz0PhW/Hxee+eU1G3UOqYuV9v9gJhoji32pQD+PoadJ0chhk # xdcve9axWuwG4UYSSv4p/XxMMuuBdbWore/8mLazUoOjFWE1+wbZHmGsG/xQ8Pi7 # A+5rCL0iXPkf1C39CYLGyabc7TtQNooMkrJq7eNCTKuzx1/Y150itqU8Azn9bVe0 # bBGuuu1EvS9vwqMpijFEwnOpFjj38M7hTQayqS1/5PDxrms1ZEBL4TYwm296xwVZ # cmRvmDXUu6O6ss8oM8Kq+6gHzV7IJT41fvnHui7Re9uUC8qbZ0xxvrI5LBFzt63u # lfiv+nf8zp5abAfEaaMUakNmRSqlR4kjWB+bknMcdE6myOipZXKD66UbE4qFsHNt # oH07ljzq/k8= # =UqzX # -----END PGP SIGNATURE----- # gpg: Signature made Wed 17 May 2023 09:50:20 AM PDT # gpg: using RSA key DC3DEB159A9AF95D3D7456FE7F09B272C88F2FD6 # gpg: issuer "kwolf@redhat.com" # gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>" [full] * tag 'for-upstream' of https://repo.or.cz/qemu/kevin: tested: add test for nested aio_poll() in poll handlers aio-posix: do not nest poll handlers iotests/245: Check if 'compress' driver is available graph-lock: Honour read locks even in the main thread blockjob: Adhere to rate limit even when reentered early test-bdrv-drain: Call bdrv_co_unref() in coroutine context test-bdrv-drain: Take graph lock more selectively qemu-img: Take graph lock more selectively qcow2: Unlock the graph in qcow2_do_open() where necessary block/export: Fix null pointer dereference in error path block: Call .bdrv_co_create(_opts) unlocked docs/interop/qcow2.txt: fix description about "zlib" clusters blockdev: qmp_transaction: drop extra generic layer blockdev: use state.bitmap in block-dirty-bitmap-add action blockdev: transaction: refactor handling transaction properties blockdev: qmp_transaction: refactor loop to classic for blockdev: transactions: rename some things blockdev: refactor transaction to use Transaction API Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comparing changes

Open a pull request

Commits on May 17, 2023

This comparison is taking too long to generate.