Reopen isn't easy with respect to locking because many of its functions
need to iterate the graph, some change it, and then you get some drains
in the middle where you can't hold any locks.
Therefore just documents most of the functions to be unlocked, and take
locks internally before accessing the graph.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <
20230929145157.45443-9-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
/*
* Returns true if @child can be reached recursively from @bs
*/
/*
* Returns true if @child can be reached recursively from @bs
*/
-static bool bdrv_recurse_has_child(BlockDriverState *bs,
- BlockDriverState *child)
+static bool GRAPH_RDLOCK
+bdrv_recurse_has_child(BlockDriverState *bs, BlockDriverState *child)
*
* To be called with bs->aio_context locked.
*/
*
* To be called with bs->aio_context locked.
*/
-static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
- BlockDriverState *bs,
- QDict *options,
- const BdrvChildClass *klass,
- BdrvChildRole role,
- bool parent_is_format,
- QDict *parent_options,
- int parent_flags,
- bool keep_old_opts)
+static BlockReopenQueue * GRAPH_RDLOCK
+bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
+ QDict *options, const BdrvChildClass *klass,
+ BdrvChildRole role, bool parent_is_format,
+ QDict *parent_options, int parent_flags,
+ bool keep_old_opts)
+ /*
+ * Strictly speaking, draining is illegal under GRAPH_RDLOCK. We know that
+ * we've been called with bdrv_graph_rdlock_main_loop(), though, so it's ok
+ * in practice.
+ */
bdrv_drained_begin(bs);
if (bs_queue == NULL) {
bdrv_drained_begin(bs);
if (bs_queue == NULL) {
QDict *options, bool keep_old_opts)
{
GLOBAL_STATE_CODE();
QDict *options, bool keep_old_opts)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, 0, false,
NULL, 0, keep_old_opts);
return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, 0, false,
NULL, 0, keep_old_opts);
* Callers must make sure that their AioContext locking is still correct after
* this.
*/
* Callers must make sure that their AioContext locking is still correct after
* this.
*/
-static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
- bool is_backing, Transaction *tran,
- Error **errp)
+static int GRAPH_UNLOCKED
+bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
+ bool is_backing, Transaction *tran,
+ Error **errp)
{
BlockDriverState *bs = reopen_state->bs;
BlockDriverState *new_child_bs;
{
BlockDriverState *bs = reopen_state->bs;
BlockDriverState *new_child_bs;
QObject *value;
const char *str;
AioContext *ctx, *old_ctx;
QObject *value;
const char *str;
AioContext *ctx, *old_ctx;
int ret;
GLOBAL_STATE_CODE();
int ret;
GLOBAL_STATE_CODE();
new_child_bs = bdrv_lookup_bs(NULL, str, errp);
if (new_child_bs == NULL) {
return -EINVAL;
new_child_bs = bdrv_lookup_bs(NULL, str, errp);
if (new_child_bs == NULL) {
return -EINVAL;
- } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
+ }
+
+ bdrv_graph_rdlock_main_loop();
+ has_child = bdrv_recurse_has_child(new_child_bs, bs);
+ bdrv_graph_rdunlock_main_loop();
+
+ if (has_child) {
error_setg(errp, "Making '%s' a %s child of '%s' would create a "
"cycle", str, child_name, bs->node_name);
return -EINVAL;
error_setg(errp, "Making '%s' a %s child of '%s' would create a "
"cycle", str, child_name, bs->node_name);
return -EINVAL;
* After calling this function, the transaction @change_child_tran may only be
* completed while holding a writer lock for the graph.
*/
* After calling this function, the transaction @change_child_tran may only be
* completed while holding a writer lock for the graph.
*/
-static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
- BlockReopenQueue *queue,
- Transaction *change_child_tran, Error **errp)
+static int GRAPH_UNLOCKED
+bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
+ Transaction *change_child_tran, Error **errp)
{
int ret = -1;
int old_flags;
{
int ret = -1;
int old_flags;
if (qdict_size(reopen_state->options)) {
const QDictEntry *entry = qdict_first(reopen_state->options);
if (qdict_size(reopen_state->options)) {
const QDictEntry *entry = qdict_first(reopen_state->options);
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
do {
QObject *new = entry->value;
QObject *old = qdict_get(reopen_state->bs->options, entry->key);
do {
QObject *new = entry->value;
QObject *old = qdict_get(reopen_state->bs->options, entry->key);
* makes them final by swapping the staging BlockDriverState contents into
* the active BlockDriverState contents.
*/
* makes them final by swapping the staging BlockDriverState contents into
* the active BlockDriverState contents.
*/
-static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
+static void GRAPH_UNLOCKED bdrv_reopen_commit(BDRVReopenState *reopen_state)
{
BlockDriver *drv;
BlockDriverState *bs;
{
BlockDriver *drv;
BlockDriverState *bs;
drv->bdrv_reopen_commit(reopen_state);
}
drv->bdrv_reopen_commit(reopen_state);
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/* set BDS specific flags now */
qobject_unref(bs->explicit_options);
qobject_unref(bs->options);
/* set BDS specific flags now */
qobject_unref(bs->explicit_options);
qobject_unref(bs->options);
qdict_del(bs->explicit_options, "backing");
qdict_del(bs->options, "backing");
qdict_del(bs->explicit_options, "backing");
qdict_del(bs->options, "backing");
- bdrv_graph_rdlock_main_loop();
bdrv_refresh_limits(bs, NULL, NULL);
bdrv_refresh_limits(bs, NULL, NULL);
- bdrv_graph_rdunlock_main_loop();
bdrv_refresh_total_sectors(bs, bs->total_sectors);
}
bdrv_refresh_total_sectors(bs, bs->total_sectors);
}
* Abort the reopen, and delete and free the staged changes in
* reopen_state
*/
* Abort the reopen, and delete and free the staged changes in
* reopen_state
*/
-static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
+static void GRAPH_UNLOCKED bdrv_reopen_abort(BDRVReopenState *reopen_state)
Error **errp);
/* For handling image reopen for split or non-split files. */
Error **errp);
/* For handling image reopen for split or non-split files. */
- int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
- BlockReopenQueue *queue, Error **errp);
- void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
- void (*bdrv_reopen_commit_post)(BDRVReopenState *reopen_state);
- void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
+ int GRAPH_UNLOCKED_PTR (*bdrv_reopen_prepare)(
+ BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp);
+ void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit)(
+ BDRVReopenState *reopen_state);
+ void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)(
+ BDRVReopenState *reopen_state);
+ void GRAPH_UNLOCKED_PTR (*bdrv_reopen_abort)(
+ BDRVReopenState *reopen_state);
void (*bdrv_join_options)(QDict *options, QDict *old_options);
int GRAPH_UNLOCKED_PTR (*bdrv_open)(
void (*bdrv_join_options)(QDict *options, QDict *old_options);
int GRAPH_UNLOCKED_PTR (*bdrv_open)(