aboutsummaryrefslogtreecommitdiff
path: root/block.c
AgeCommit message (Collapse)AuthorFilesLines
2022-11-10block: Make bdrv_child_get_parent_aio_context I/OHanna Reitz1-1/+1
We want to use bdrv_child_get_parent_aio_context() from bdrv_parent_drained_{begin,end}_single(), both of which are "I/O or GS" functions. Prior to 3ed4f708fe1, all the implementations were I/O code anyway. 3ed4f708fe1 has put block jobs' AioContext field under the job mutex, so to make child_job_get_parent_aio_context() work in an I/O context, we need to take that lock there. Furthermore, blk_root_get_parent_aio_context() is not marked as anything, but is safe to run in an I/O context, so mark it that way now. (blk_get_aio_context() is an I/O code function.) With that done, all implementations explicitly are I/O code, so we can mark bdrv_child_get_parent_aio_context() as I/O code, too, so callers know it is safe to run from both GS and I/O contexts. Signed-off-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20221107151321.211175-2-hreitz@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-11-06module: add Error arguments to module_load and module_load_qomClaudio Fontana1-5/+15
improve error handling during module load, by changing: bool module_load(const char *prefix, const char *lib_name); void module_load_qom(const char *type); to: int module_load(const char *prefix, const char *name, Error **errp); int module_load_qom(const char *type, Error **errp); where the return value is: -1 on module load error, and errp is set with the error 0 on module or one of its dependencies are not installed 1 on module load success 2 on module load success (module already loaded or built-in) module_load_qom_one has been introduced in: commit 28457744c345 ("module: qom module support"), which built on top of module_load_one, but discarded the bool return value. Restore it. Adapt all callers to emit errors, or ignore them, or fail hard, as appropriate in each context. Replace the previous emission of errors via fprintf in _some_ error conditions with Error and error_report, so as to emit to the appropriate target. A memory leak is also fixed as part of the module_load changes. audio: when attempting to load an audio module, report module load errors. Note that still for some callers, a single issue may generate multiple error reports, and this could be improved further. Regarding the audio code itself, audio_add() seems to ignore errors, and this should probably be improved. block: when attempting to load a block module, report module load errors. For the code paths that already use the Error API, take advantage of those to report module load errors into the Error parameter. For the other code paths, we currently emit the error, but this could be improved further by adding Error parameters to all possible code paths. console: when attempting to load a display module, report module load errors. qdev: when creating a new qdev Device object (DeviceState), report load errors. If a module cannot be loaded to create that device, now abort execution (if no CONFIG_MODULE) or exit (if CONFIG_MODULE). qom/object.c: when initializing a QOM object, or looking up class_by_name, report module load errors. qtest: when processing the "module_load" qtest command, report errors in the load of the module. Signed-off-by: Claudio Fontana <cfontana@suse.de> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20220929093035.4231-4-cfontana@suse.de> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2022-11-06module: rename module_load_one to module_loadClaudio Fontana1-2/+2
Signed-off-by: Claudio Fontana <cfontana@suse.de> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20220929093035.4231-3-cfontana@suse.de> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2022-10-30Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into stagingStefan Hajnoczi1-475/+380
Block layer patches - Cleanup bs->backing and bs->file handling - Refactor bdrv_try_set_aio_context using transactions - Changes for improved coroutine_fn consistency - vhost-user-blk: fix the resize crash - io_uring: Use of io_uring_register_ring_fd() led to breakage, revert - vvfat: Fix some problems with r/w mode - Code cleanup - MAINTAINERS: Fold "Block QAPI, monitor, ..." into "Block layer core" # -----BEGIN PGP SIGNATURE----- # # iQJFBAABCAAvFiEE3D3rFZqa+V09dFb+fwmycsiPL9YFAmNazhIRHGt3b2xmQHJl # ZGhhdC5jb20ACgkQfwmycsiPL9ZyTw/8Dfck/SuxfyeLlnQItkjaV4cnqWOU8vHs # 9x0KhlptCs+HXdF/3iicpA0lHojn7mNnbdFGjPRY4E0LriQv91TQ5ycdEmrseFPf # sgeQlgdKCVU/pHjZ2wYarm2pE43Cx85a5xuufmw+7w49dNNZn14l4t+DgviuClVM # nuVaogfZFbYyetre+Qd2TgLl+gJ+0d4o7Zs5lSWLrT8t0L9AGkcWPA7Nrbl6loIE # dOautV4G7jLjuMiCeJZOGcnuRVe3gCQ5rCGBFzzH4DUtz4BmiYx4hd3LMEsP0PMM # CrsfDZS04Ztybl9M7TmJuwkAm1gx1JDMOuJuh18lbJocIOBvhkKKxY2wI5LIdZVI # ZntmU36RowkX+GGu/PYpYyMjBDClJppZCl7vnjyLYsVt6r0Vu6SmlHpJhcRYabhe # 96Kv1LXH9A6+ogKPU3Layw6JGjg01GNr1ALuT7PO3pGto/JshmOuBEJJDucoF84M # 5AfxFCohMROVldwblA6M0eKnlQBgtr5BvtgbV54BBo88VlFJgDJFQn7R09cTFUEo # UwaJoS+nIaiZ0bQQVZhZloVppUaTdVJojzfVRCZZctga96/tu1HSFnGLnbEFpUN3 # KOf+XnVNS6Ro+nPSDf9bMjbIom2JicGFfV+6yMgIoxY/d5UA2dTZfefil4TAlSod # 6PsTgg+jrm8= # =/Fw0 # -----END PGP SIGNATURE----- # gpg: Signature made Thu 27 Oct 2022 14:29:38 EDT # gpg: using RSA key DC3DEB159A9AF95D3D7456FE7F09B272C88F2FD6 # gpg: issuer "kwolf@redhat.com" # gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>" [full] # Primary key fingerprint: DC3D EB15 9A9A F95D 3D74 56FE 7F09 B272 C88F 2FD6 * tag 'for-upstream' of https://repo.or.cz/qemu/kevin: (58 commits) block/block-backend: blk_set_enable_write_cache is IO_CODE monitor: switch to *_co_* functions vmdk: switch to *_co_* functions vhdx: switch to *_co_* functions vdi: switch to *_co_* functions qed: switch to *_co_* functions qcow2: switch to *_co_* functions qcow: switch to *_co_* functions parallels: switch to *_co_* functions mirror: switch to *_co_* functions block: switch to *_co_* functions commit: switch to *_co_* functions vmdk: manually add more coroutine_fn annotations qcow2: manually add more coroutine_fn annotations qcow: manually add more coroutine_fn annotations blkdebug: add missing coroutine_fn annotation for indirect-called functions qcow2: add coroutine_fn annotation for indirect-called functions block: add missing coroutine_fn annotation to BlockDriverState callbacks coroutine-io: add missing coroutine_fn annotation to prototypes coroutine-lock: add missing coroutine_fn annotation to prototypes ... Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2022-10-27block: switch to *_co_* functionsAlberto Faria1-1/+1
Signed-off-by: Alberto Faria <afaria@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20221013123711.620631-16-pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: remove bdrv_try_set_aio_context and replace it with ↵Emanuele Giuseppe Esposito1-10/+4
bdrv_try_change_aio_context No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20221025084952.2139888-11-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: rename bdrv_child_try_change_aio_context in bdrv_try_change_aio_contextEmanuele Giuseppe Esposito1-3/+3
No functional changes intended. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20221025084952.2139888-10-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacksEmanuele Giuseppe Esposito1-196/+0
Together with all _can_set_ and _set_ APIs, as they are not needed anymore. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20221025084952.2139888-9-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: use the new _change_ API instead of _can_set_ and _set_Emanuele Giuseppe Esposito1-19/+25
Replace all direct usage of ->can_set_aio_ctx and ->set_aio_ctx, and call bdrv_child_try_change_aio_context() in bdrv_try_set_aio_context(), the main function called through the whole block layer. From this point onwards, ->can_set_aio_ctx and ->set_aio_ctx won't be used anymore. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20221025084952.2139888-8-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: implement .change_aio_ctx in child_of_bdsEmanuele Giuseppe Esposito1-0/+9
bdrv_child_cb_change_aio_ctx() is identical to bdrv_child_cb_can_set_aio_ctx(), as we only need to recursively go on the parent bs. Note: bdrv_child_try_change_aio_context() is not called by anyone at this point. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20221025084952.2139888-6-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27bdrv_change_aio_context: use hash table instead of list of visited nodesEmanuele Giuseppe Esposito1-12/+16
Minor performance improvement, but given that we have hash tables available, avoid iterating in the visited nodes list every time just to check if a node has been already visited. The data structure is not actually a proper hash map, but an hash set, as we are just adding nodes and not key,value pairs. Suggested-by: Hanna Reitz <hreitz@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20221025084952.2139888-4-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: use transactions as a replacement of ->{can_}set_aio_context()Emanuele Giuseppe Esposito1-1/+219
Simplify the way the aiocontext can be changed in a BDS graph. There are currently two problems in bdrv_try_set_aio_context: - There is a confusion of AioContext locks taken and released, because we assume that old aiocontext is always taken and new one is taken inside. - It doesn't look very safe to call bdrv_drained_begin while some nodes have already switched to the new aiocontext and others haven't. This could be especially dangerous because bdrv_drained_begin polls, so something else could be executed while graph is in an inconsistent state. Additional minor nitpick: can_set and set_ callbacks both traverse the graph, both using the ignored list of visited nodes in a different way. Therefore, get rid of all of this and introduce a new callback, change_aio_context, that uses transactions to efficiently, cleanly and most importantly safely change the aiocontext of a graph. This new callback is a "merge" of the two previous ones: - Just like can_set_aio_context, recursively traverses the graph. Marks all nodes that are visited using a GList, and checks if they *could* change the aio_context. - For each node that passes the above check, drain it and add a new transaction that implements a callback that effectively changes the aiocontext. - Once done, the recursive function returns if *all* nodes can change the AioContext. If so, commit the above transactions. Regardless of the outcome, call transaction.clean() to undo all drains done in the recursion. - The transaction list is scanned only after all nodes are being drained, so we are sure that they all are in the same context, and then we switch their AioContext, concluding the drain only after all nodes switched to the new AioContext. In this way we make sure that bdrv_drained_begin() is always called under the old AioContext, and bdrv_drained_end() under the new one. - Because of the above, we don't need to release and re-acquire the old AioContext every time, as everything is done once (and not per-node drain and aiocontext change). Note that the "change" API is not yet invoked anywhere. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20221025084952.2139888-3-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block.c: assert bs->aio_context is written under BQL and drainsEmanuele Giuseppe Esposito1-0/+2
Also here ->aio_context is read by I/O threads and written under BQL. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20221025084952.2139888-2-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: refactor bdrv_remove_file_or_backing_child to bdrv_remove_childVladimir Sementsov-Ogievskiy1-18/+9
Now the function can remove any child, so give it more common name. Drop assertions and drop bs argument which becomes unused. Function would be reused in a further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220726201134.924743-16-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: Manipulate bs->file / bs->backing pointers in .attach/.detachVladimir Sementsov-Ogievskiy1-129/+105
bs->file and bs->backing are a kind of duplication of part of bs->children. But very useful diplication, so let's not drop them at all:) We should manage bs->file and bs->backing in same place, where we manage bs->children, to keep them in sync. Moreover, generic io paths are unprepared to BdrvChild without a bs, so it's double good to clear bs->file / bs->backing when we detach the child. Detach is simple: if we detach bs->file or bs->backing child, just set corresponding field to NULL. Attach is a bit more complicated. But we still can precisely detect should we set one of bs->file / bs->backing or not: - if role is BDRV_CHILD_COW, we definitely deal with bs->backing - else, if role is BDRV_CHILD_FILTERED (it must be also BDRV_CHILD_PRIMARY), it's a filtered child. Use bs->drv->filtered_child_is_backing to chose the pointer field to modify. - else, if role is BDRV_CHILD_PRIMARY, we deal with bs->file - in all other cases, it's neither bs->backing nor bs->file. It's some other child and we shouldn't care OK. This change brings one more good thing: we can (and should) get rid of all indirect pointers in the block-graph-change transactions: bdrv_attach_child_common() stores BdrvChild** into transaction to clear it on abort. bdrv_attach_child_common() has two callers: bdrv_attach_child_noperm() just pass-through this feature, bdrv_root_attach_child() doesn't need the feature. Look at bdrv_attach_child_noperm() callers: - bdrv_attach_child() doesn't need the feature - bdrv_set_file_or_backing_noperm() uses the feature to manage bs->file and bs->backing, we don't want it anymore - bdrv_append() uses the feature to manage bs->backing, again we don't want it anymore So, we should drop this stuff! Great! We could probably keep BdrvChild** argument to keep the int return value, but it seems not worth the complexity. Finally, we now set .file / .backing automatically in generic code and want to restring setting them by hand outside of .attach/.detach. So, this patch cleanups all remaining places where they were set. To find such places I use: git grep '\->file =' git grep '\->backing =' git grep '&.*\<backing\>' git grep '&.*\<file\>' Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220726201134.924743-14-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27Revert "block: Pass BdrvChild ** to replace_child_noperm"Vladimir Sementsov-Ogievskiy1-12/+11
That's a preparation to previously reverted "block: Let replace_child_noperm free children". Drop it too, we don't need it for a new approach. This reverts commit be64bbb0149748f3999c49b13976aafb8330ea86. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220726201134.924743-13-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27Revert "block: Restructure remove_file_or_backing_child()"Vladimir Sementsov-Ogievskiy1-12/+9
That's a preparation to previously reverted "block: Let replace_child_noperm free children". Drop it too, we don't need it for a new approach. This reverts commit 562bda8bb41879eeda0bd484dd3d55134579b28e. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220726201134.924743-12-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27Revert "block: Let replace_child_tran keep indirect pointer"Vladimir Sementsov-Ogievskiy1-71/+10
That's a preparation to previously reverted "block: Let replace_child_noperm free children". Drop it too, we don't need it for a new approach. This reverts commit 82b54cf51656bf3cd5ed1ac549e8a1085a0e3290. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220726201134.924743-11-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27Revert "block: Let replace_child_noperm free children"Vladimir Sementsov-Ogievskiy1-79/+23
We are going to reimplement this behavior (clear bs->file / bs->backing pointers automatically when child->bs is cleared) in a nicer way, see further commit "block: Manipulate bs->file / bs->backing pointers in .attach/.detach". With this revert we bring back a problem that was fixed by b0a9f6fed3d8. Still the problem was mostly theoretical, we don't have concrete bugs fixed by b0a9f6fed3d8, we don't have a specific test. Probably some accidental failures of iotests are related. Alternatively, we may merge this and following three reverts into final "block: Manipulate ..." to avoid any kind of regression. But seems that in this case having separate clear revert commits is better. This reverts commit b0a9f6fed3d80de610dcd04a7e66f9f30a04174f. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220726201134.924743-10-vsementsov@yandex-team.ru> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: introduce bdrv_open_file_child() helperVladimir Sementsov-Ogievskiy1-0/+21
Almost all drivers call bdrv_open_child() similarly. Let's create a helper for this. The only not updated drivers that call bdrv_open_child() to set bs->file are raw-format and snapshot-access: raw-format sometimes want to have filtered child but don't set drv->is_filter to true. snapshot-access wants only DATA | PRIMARY Possibly we should implement drv->is_filter_func() handler, to consider raw-format as filter when it works as filter.. But it's another story. Note also, that we decrease assignments to bs->file in code: it helps us restrict modifying this field in further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220726201134.924743-3-vsementsov@yandex-team.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: Refactor get_tmp_filename()Bin Meng1-26/+30
At present there are two callers of get_tmp_filename() and they are inconsistent. One does: /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ char *tmp_filename = g_malloc0(PATH_MAX + 1); ... ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); while the other does: s->qcow_filename = g_malloc(PATH_MAX); ret = get_tmp_filename(s->qcow_filename, PATH_MAX); As we can see different 'size' arguments are passed. There are also platform specific implementations inside the function, and the use of snprintf is really undesirable. The function name is also misleading. It creates a temporary file, not just a filename. Refactor this routine by changing its name and signature to: char *create_tmp_file(Error **errp) and use g_get_tmp_dir() / g_mkstemp() for a consistent implementation. While we are here, add some comments to mention that /var/tmp is preferred over /tmp on non-win32 hosts. Signed-off-by: Bin Meng <bin.meng@windriver.com> Message-Id: <20221010040432.3380478-2-bin.meng@windriver.com> [kwolf: Fixed incorrect errno negation and iotest 051] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-27block: Ignore close() failure in get_tmp_filename()Bin Meng1-4/+1
The temporary file has been created and is ready for use. Checking return value of close() does not seem useful. The file descriptor is almost certainly closed; see close(2) under "Dealing with error returns from close()". Let's simply ignore close() failure here. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Bin Meng <bin.meng@windriver.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20221010040432.3380478-1-bin.meng@windriver.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-26block: add BDRV_REQ_REGISTERED_BUF request flagStefan Hajnoczi1-0/+14
Block drivers may optimize I/O requests accessing buffers previously registered with bdrv_register_buf(). Checking whether all elements of a request's QEMUIOVector are within previously registered buffers is expensive, so we need a hint from the user to avoid costly checks. Add a BDRV_REQ_REGISTERED_BUF request flag to indicate that all QEMUIOVector elements in an I/O request are known to be within previously registered buffers. Always pass the flag through to driver read/write functions. There is little harm in passing the flag to a driver that does not use it. Passing the flag to drivers avoids changes across many block drivers. Filter drivers would need to explicitly support the flag and pass through to their children when the children support it. That's a lot of code changes and it's hard to remember to do that everywhere, leading to silent reduced performance when the flag is accidentally dropped. The only problematic scenario with the approach in this patch is when a driver passes the flag through to internal I/O requests that don't use the same I/O buffer. In that case the hint may be set when it should actually be clear. This is a rare case though so the risk is low. Some drivers have assert(!flags), which no longer works when BDRV_REQ_REGISTERED_BUF is passed in. These assertions aren't very useful anyway since the functions are called almost exclusively by bdrv_driver_preadv/pwritev() so if we get flags handling right there then the assertion is not needed. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-id: 20221013185908.1297568-7-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
2022-10-07jobs: group together API calls under the same job lockEmanuele Giuseppe Esposito1-7/+10
Now that the API offers also _locked() functions, take advantage of it and give also the caller control to take the lock and call _locked functions. This makes sense especially when we have for loops, because it makes no sense to have: for(job = job_next(); ...) where each job_next() takes the lock internally. Instead we want JOB_LOCK_GUARD(); for(job = job_next_locked(); ...) In addition, protect also direct field accesses, by either creating a new critical section or widening the existing ones. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-Id: <20220926093214.506243-12-eesposit@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-10-07block: add missing coroutine_fn annotationsPaolo Bonzini1-3/+4
Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <20220922084924.201610-3-pbonzini@redhat.com> [kwolf: Fixed up coding style] Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-07-12block: Change blk_{pread,pwrite}() param orderAlberto Faria1-1/+1
Swap 'buf' and 'bytes' around for consistency with blk_co_{pread,pwrite}(), and in preparation to implement these functions using generated_co_wrapper. Callers were updated using this Coccinelle script: @@ expression blk, offset, buf, bytes, flags; @@ - blk_pread(blk, offset, buf, bytes, flags) + blk_pread(blk, offset, bytes, buf, flags) @@ expression blk, offset, buf, bytes, flags; @@ - blk_pwrite(blk, offset, buf, bytes, flags) + blk_pwrite(blk, offset, bytes, buf, flags) It had no effect on hw/block/nand.c, presumably due to the #if, so that file was updated manually. Overly-long lines were then fixed by hand. Signed-off-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220705161527.1054072-4-afaria@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
2022-07-12block: Add a 'flags' param to blk_pread()Alberto Faria1-1/+1
For consistency with other I/O functions, and in preparation to implement it using generated_co_wrapper. Callers were updated using this Coccinelle script: @@ expression blk, offset, buf, bytes; @@ - blk_pread(blk, offset, buf, bytes) + blk_pread(blk, offset, buf, bytes, 0) It had no effect on hw/block/nand.c, presumably due to the #if, so that file was updated manually. Overly-long lines were then fixed by hand. Signed-off-by: Alberto Faria <afaria@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Greg Kurz <groug@kaod.org> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220705161527.1054072-3-afaria@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
2022-07-12block: Make blk_{pread,pwrite}() return 0 on successAlberto Faria1-3/+5
They currently return the value of their 'bytes' parameter on success. Make them return 0 instead, for consistency with other I/O functions and in preparation to implement them using generated_co_wrapper. This also makes it clear that short reads/writes are not possible. Signed-off-by: Alberto Faria <afaria@redhat.com> Message-Id: <20220705161527.1054072-2-afaria@redhat.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
2022-05-04block: Classify bdrv_get_flags() as I/O functionHanna Reitz1-1/+1
This function is safe to call in an I/O context, and qcow2_do_open() does so (invoked in an I/O context by qcow2_co_invalidate_cache()). Signed-off-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220427114057.36651-2-hreitz@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-04-06Replace qemu_real_host_page variables with inlined functionsMarc-André Lureau1-2/+2
Replace the global variables with inlined helper functions. getpagesize() is very likely annotated with a "const" function attribute (at least with glibc), and thus optimization should apply even better. This avoids the need for a constructor initialization too. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20220323155743.1585078-12-marcandre.lureau@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2022-03-04block_int-common.h: assertions in the callers of BdrvChildClass function ↵Emanuele Giuseppe Esposito1-1/+9
pointers Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-29-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block_int-common.h: assertions in the callers of BlockDriver function pointersEmanuele Giuseppe Esposito1-0/+17
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-27-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block/coroutines: I/O and "I/O or GS" APIEmanuele Giuseppe Esposito1-0/+2
block coroutines functions run in different aiocontext, and are not protected by the BQL. Therefore are I/O. On the other side, generated_co_wrapper functions use BDRV_POLL_WHILE, meaning the caller can either be the main loop or a specific iothread. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-25-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block.c: add assertions to static functionsEmanuele Giuseppe Esposito1-1/+45
Following the assertion derived from the API split, propagate the assertion also in the static functions. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-18-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block: introduce assert_bdrv_graph_writableEmanuele Giuseppe Esposito1-0/+4
We want to be sure that the functions that write the child and parent list of a bs are under BQL and drain. BQL prevents from concurrent writings from the GS API, while drains protect from I/O. TODO: drains are missing in some functions using this assert. Therefore a proper assertion will fail. Because adding drains requires additional discussions, they will be added in future series. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-15-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04IO_CODE and IO_OR_GS_CODE for block_int I/O APIEmanuele Giuseppe Esposito1-1/+13
Mark all I/O functions with IO_CODE, and all "I/O OR GS" with IO_OR_GS_CODE. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-14-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04assertions for block_int global state APIEmanuele Giuseppe Esposito1-0/+15
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-13-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block.c: assertions to the block layer permissions APIEmanuele Giuseppe Esposito1-0/+12
Now that we "covered" the three main cases where the permission API was being used under BQL (fuse, amend and invalidate_cache), we can safely assert for the permission functions implemented in block.c Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-11-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04IO_CODE and IO_OR_GS_CODE for block I/O APIEmanuele Giuseppe Esposito1-1/+36
Mark all I/O functions with IO_CODE, and all "I/O OR GS" with IO_OR_GS_CODE. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-6-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04assertions for block global state APIEmanuele Giuseppe Esposito1-2/+123
All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-5-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04include/block/block: split header into I/O and global state APIEmanuele Giuseppe Esposito1-0/+3
block.h currently contains a mix of functions: some of them run under the BQL and modify the block layer graph, others are instead thread-safe and perform I/O in iothreads. Some others can only be called by either the main loop or the iothread running the AioContext (and not other iothreads), and using them in another thread would cause deadlocks, and therefore it is not ideal to define them as I/O. It is not easy to understand which function is part of which group (I/O vs GS vs "I/O or GS"), and this patch aims to clarify it. The "GS" functions need the BQL, and often use aio_context_acquire/release and/or drain to be sure they can modify the graph safely. The I/O function are instead thread safe, and can run in any AioContext. "I/O or GS" functions run instead in the main loop or in a single iothread, and use BDRV_POLL_WHILE(). By splitting the header in two files, block-io.h and block-global-state.h we have a clearer view on what needs what kind of protection. block-common.h contains common structures shared by both headers. block.h is left there for legacy and to avoid changing all includes in all c files that use the block APIs. Assertions are added in the next patch. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220303151616.325444-4-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activateEmanuele Giuseppe Esposito1-14/+23
Split bdrv_co_invalidate cache in two: the Global State (under BQL) code that takes care of permissions and running GS callbacks, and leave only the I/O code (->bdrv_co_invalidate_cache) running in the I/O coroutine. The only side effect is that bdrv_co_invalidate_cache is not recursive anymore, and so is every direct call to bdrv_invalidate_cache(). Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Message-Id: <20220209105452.1694545-6-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block: rename bdrv_invalidate_cache_all, blk_invalidate_cache and ↵Emanuele Giuseppe Esposito1-1/+1
test_sync_op_invalidate_cache Following the bdrv_activate renaming, change also the name of the respective callers. bdrv_invalidate_cache_all -> bdrv_activate_all blk_invalidate_cache -> blk_activate test_sync_op_invalidate_cache -> test_sync_op_activate No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220209105452.1694545-5-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-03-04block: introduce bdrv_activateEmanuele Giuseppe Esposito1-1/+6
This function is currently just a wrapper for bdrv_invalidate_cache(), but in future will contain the code of bdrv_co_invalidate_cache() that has to always be protected by BQL, and leave the rest in the I/O coroutine. Replace all bdrv_invalidate_cache() invokations with bdrv_activate(). Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20220209105452.1694545-4-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-02-01block: bdrv_set_backing_hd(): use drained sectionVladimir Sementsov-Ogievskiy1-0/+4
Graph modifications should be done in drained section. stream_prepare() handler of block stream job call bdrv_set_backing_hd() without using drained section and it's theoretically possible that some IO request will interleave with graph modification and will use outdated pointers to removed block nodes. Some other callers use bdrv_set_backing_hd() not caring about drained sections too. So it seems good to make a drained section exactly in bdrv_set_backing_hd(). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20220124173741.2984056-1-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-01-14block: drop BLK_PERM_GRAPH_MODVladimir Sementsov-Ogievskiy1-6/+1
First, this permission never protected a node from being changed, as generic child-replacing functions don't check it. Second, it's a strange thing: it presents a permission of parent node to change its child. But generally, children are replaced by different mechanisms, like jobs or qmp commands, not by nodes. Graph-mod permission is hard to understand. All other permissions describe operations which done by parent node on its child: read, write, resize. Graph modification operations are something completely different. The only place where BLK_PERM_GRAPH_MOD is used as "perm" (not shared perm) is mirror_start_job, for s->target. Still modern code should use bdrv_freeze_backing_chain() to protect from graph modification, if we don't do it somewhere it may be considered as a bug. So, it's a bit risky to drop GRAPH_MOD, and analyzing of possible loss of protection is hard. But one day we should do it, let's do it now. One more bit of information is that locking the corresponding byte in file-posix doesn't make sense at all. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Message-Id: <20210902093754.2352-1-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2022-01-14block_int: make bdrv_backing_overridden staticEmanuele Giuseppe Esposito1-1/+3
bdrv_backing_overridden is only used in block.c, so there is no need to leave it in block_int.h Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20211215121140.456939-2-eesposit@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2021-11-16block: Let replace_child_noperm free childrenHanna Reitz1-23/+79
In most of the block layer, especially when traversing down from other BlockDriverStates, we assume that BdrvChild.bs can never be NULL. When it becomes NULL, it is expected that the corresponding BdrvChild pointer also becomes NULL and the BdrvChild object is freed. Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs pointer to NULL, it should also immediately set the corresponding BdrvChild pointer (like bs->file or bs->backing) to NULL. In that context, it also makes sense for this function to free the child. Sometimes we cannot do so, though, because it is called in a transactional context where the caller might still want to reinstate the child in the abort branch (and free it only on commit), so this behavior has to remain optional. In bdrv_replace_child_tran()'s abort handler, we now rely on the fact that the BdrvChild passed to bdrv_replace_child_tran() must have had a non-NULL .bs pointer initially. Make a note of that and assert it. Signed-off-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20211111120829.81329-10-hreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20211115145409.176785-10-kwolf@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
2021-11-16block: Let replace_child_tran keep indirect pointerHanna Reitz1-10/+73
As of a future commit, bdrv_replace_child_noperm() will clear the indirect BdrvChild pointer passed to it if the new child BDS is NULL. bdrv_replace_child_tran() will want to let it do that, but revert this change in its abort handler. For that, we need to have it receive a BdrvChild ** pointer, too, and keep it stored in the BdrvReplaceChildState object that we attach to the transaction. Note that we do not need to store it in the BdrvReplaceChildState when new_bs is not NULL, because then there is nothing to revert. This is important so that bdrv_replace_node_noperm() can pass a pointer to a loop-local variable to bdrv_replace_child_tran() without worrying that this pointer will outlive one loop iteration. (Of course, for that to work, bdrv_replace_node_noperm() and in turn bdrv_replace_node() and its relatives may not be called with a NULL @to node. Luckily, they already are not, but now we should assert this.) bdrv_remove_file_or_backing_child() on the other hand needs to ensure that the indirect pointer it passes will stay valid for the duration of the transaction. Ensure this by keeping a strong reference to the BDS whose &bs->backing or &bs->file it passes to bdrv_replace_child_tran(), and giving up that reference only in the transaction .clean() handler. Signed-off-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20211111120829.81329-9-hreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20211115145409.176785-9-kwolf@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
2021-11-16block: Restructure remove_file_or_backing_child()Hanna Reitz1-9/+12
As of a future patch, bdrv_replace_child_tran() will take a BdrvChild ** pointer. Prepare for that by getting such a pointer and using it where applicable, and (dereferenced) as a parameter for bdrv_replace_child_tran(). Signed-off-by: Hanna Reitz <hreitz@redhat.com> Message-Id: <20211111120829.81329-7-hreitz@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20211115145409.176785-7-kwolf@redhat.com> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
generated by cgit v1.1 at 2024-12-25 02:54:46 +0000