Skip to content

Commit

Permalink
block: Introduce bdrv_co_change_backing_file()
Browse files Browse the repository at this point in the history 
bdrv_change_backing_file() is called both inside and outside coroutine
context. This makes it difficult for it to take the graph lock
internally. It also means that driver implementations need to be able to
run outside of coroutines, too. Switch it to the usual model with a
coroutine based implementation and a co_wrapper instead. The new
function is marked GRAPH_RDLOCK.

As the co_wrapper now runs the function in the AioContext of the node
(as it should always have done), this is not GLOBAL_STATE_CODE() any
more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-20-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
  • Loading branch information
@kevmw
kevmw committed Nov 8, 2023
1 parent 244b26d commit e2dd273
Show file tree
Hide file tree
Showing 7 changed files with 120 additions and 107 deletions.
11 changes: 6 additions & 5 deletions block.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5764,13 +5764,14 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
* image file header
* -ENOTSUP - format driver doesn't support changing the backing file
*/
int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt, bool require)
int coroutine_fn
bdrv_co_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt, bool require)
{
BlockDriver *drv = bs->drv;
int ret;

GLOBAL_STATE_CODE();
IO_CODE();

if (!drv) {
return -ENOMEDIUM;
Expand All @@ -5785,8 +5786,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
return -EINVAL;
}

if (drv->bdrv_change_backing_file != NULL) {
ret = drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
if (drv->bdrv_co_change_backing_file != NULL) {
ret = drv->bdrv_co_change_backing_file(bs, backing_file, backing_fmt);
} else {
ret = -ENOTSUP;
}
Expand Down
128 changes: 66 additions & 62 deletions block/qcow2.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3155,8 +3155,9 @@ int qcow2_update_header(BlockDriverState *bs)
return ret;
}

static int qcow2_change_backing_file(BlockDriverState *bs,
const char *backing_file, const char *backing_fmt)
static int coroutine_fn GRAPH_RDLOCK
qcow2_co_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt)
{
BDRVQcow2State *s = bs->opaque;

Expand Down Expand Up @@ -3816,8 +3817,11 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
backing_format = BlockdevDriver_str(qcow2_opts->backing_fmt);
}

ret = bdrv_change_backing_file(blk_bs(blk), qcow2_opts->backing_file,
backing_format, false);
bdrv_graph_co_rdlock();
ret = bdrv_co_change_backing_file(blk_bs(blk), qcow2_opts->backing_file,
backing_format, false);
bdrv_graph_co_rdunlock();

if (ret < 0) {
error_setg_errno(errp, -ret, "Could not assign backing file '%s' "
"with format '%s'", qcow2_opts->backing_file,
Expand Down Expand Up @@ -6115,64 +6119,64 @@ static const char *const qcow2_strong_runtime_opts[] = {
};

BlockDriver bdrv_qcow2 = {
.format_name = "qcow2",
.instance_size = sizeof(BDRVQcow2State),
.bdrv_probe = qcow2_probe,
.bdrv_open = qcow2_open,
.bdrv_close = qcow2_close,
.bdrv_reopen_prepare = qcow2_reopen_prepare,
.bdrv_reopen_commit = qcow2_reopen_commit,
.bdrv_reopen_commit_post = qcow2_reopen_commit_post,
.bdrv_reopen_abort = qcow2_reopen_abort,
.bdrv_join_options = qcow2_join_options,
.bdrv_child_perm = bdrv_default_perms,
.bdrv_co_create_opts = qcow2_co_create_opts,
.bdrv_co_create = qcow2_co_create,
.bdrv_has_zero_init = qcow2_has_zero_init,
.bdrv_co_block_status = qcow2_co_block_status,

.bdrv_co_preadv_part = qcow2_co_preadv_part,
.bdrv_co_pwritev_part = qcow2_co_pwritev_part,
.bdrv_co_flush_to_os = qcow2_co_flush_to_os,

.bdrv_co_pwrite_zeroes = qcow2_co_pwrite_zeroes,
.bdrv_co_pdiscard = qcow2_co_pdiscard,
.bdrv_co_copy_range_from = qcow2_co_copy_range_from,
.bdrv_co_copy_range_to = qcow2_co_copy_range_to,
.bdrv_co_truncate = qcow2_co_truncate,
.bdrv_co_pwritev_compressed_part = qcow2_co_pwritev_compressed_part,
.bdrv_make_empty = qcow2_make_empty,

.bdrv_snapshot_create = qcow2_snapshot_create,
.bdrv_snapshot_goto = qcow2_snapshot_goto,
.bdrv_snapshot_delete = qcow2_snapshot_delete,
.bdrv_snapshot_list = qcow2_snapshot_list,
.bdrv_snapshot_load_tmp = qcow2_snapshot_load_tmp,
.bdrv_measure = qcow2_measure,
.bdrv_co_get_info = qcow2_co_get_info,
.bdrv_get_specific_info = qcow2_get_specific_info,

.bdrv_co_save_vmstate = qcow2_co_save_vmstate,
.bdrv_co_load_vmstate = qcow2_co_load_vmstate,

.is_format = true,
.supports_backing = true,
.bdrv_change_backing_file = qcow2_change_backing_file,

.bdrv_refresh_limits = qcow2_refresh_limits,
.bdrv_co_invalidate_cache = qcow2_co_invalidate_cache,
.bdrv_inactivate = qcow2_inactivate,

.create_opts = &qcow2_create_opts,
.amend_opts = &qcow2_amend_opts,
.strong_runtime_opts = qcow2_strong_runtime_opts,
.mutable_opts = mutable_opts,
.bdrv_co_check = qcow2_co_check,
.bdrv_amend_options = qcow2_amend_options,
.bdrv_co_amend = qcow2_co_amend,

.bdrv_detach_aio_context = qcow2_detach_aio_context,
.bdrv_attach_aio_context = qcow2_attach_aio_context,
.format_name = "qcow2",
.instance_size = sizeof(BDRVQcow2State),
.bdrv_probe = qcow2_probe,
.bdrv_open = qcow2_open,
.bdrv_close = qcow2_close,
.bdrv_reopen_prepare = qcow2_reopen_prepare,
.bdrv_reopen_commit = qcow2_reopen_commit,
.bdrv_reopen_commit_post = qcow2_reopen_commit_post,
.bdrv_reopen_abort = qcow2_reopen_abort,
.bdrv_join_options = qcow2_join_options,
.bdrv_child_perm = bdrv_default_perms,
.bdrv_co_create_opts = qcow2_co_create_opts,
.bdrv_co_create = qcow2_co_create,
.bdrv_has_zero_init = qcow2_has_zero_init,
.bdrv_co_block_status = qcow2_co_block_status,

.bdrv_co_preadv_part = qcow2_co_preadv_part,
.bdrv_co_pwritev_part = qcow2_co_pwritev_part,
.bdrv_co_flush_to_os = qcow2_co_flush_to_os,

.bdrv_co_pwrite_zeroes = qcow2_co_pwrite_zeroes,
.bdrv_co_pdiscard = qcow2_co_pdiscard,
.bdrv_co_copy_range_from = qcow2_co_copy_range_from,
.bdrv_co_copy_range_to = qcow2_co_copy_range_to,
.bdrv_co_truncate = qcow2_co_truncate,
.bdrv_co_pwritev_compressed_part = qcow2_co_pwritev_compressed_part,
.bdrv_make_empty = qcow2_make_empty,

.bdrv_snapshot_create = qcow2_snapshot_create,
.bdrv_snapshot_goto = qcow2_snapshot_goto,
.bdrv_snapshot_delete = qcow2_snapshot_delete,
.bdrv_snapshot_list = qcow2_snapshot_list,
.bdrv_snapshot_load_tmp = qcow2_snapshot_load_tmp,
.bdrv_measure = qcow2_measure,
.bdrv_co_get_info = qcow2_co_get_info,
.bdrv_get_specific_info = qcow2_get_specific_info,

.bdrv_co_save_vmstate = qcow2_co_save_vmstate,
.bdrv_co_load_vmstate = qcow2_co_load_vmstate,

.is_format = true,
.supports_backing = true,
.bdrv_co_change_backing_file = qcow2_co_change_backing_file,

.bdrv_refresh_limits = qcow2_refresh_limits,
.bdrv_co_invalidate_cache = qcow2_co_invalidate_cache,
.bdrv_inactivate = qcow2_inactivate,

.create_opts = &qcow2_create_opts,
.amend_opts = &qcow2_amend_opts,
.strong_runtime_opts = qcow2_strong_runtime_opts,
.mutable_opts = mutable_opts,
.bdrv_co_check = qcow2_co_check,
.bdrv_amend_options = qcow2_amend_options,
.bdrv_co_amend = qcow2_co_amend,

.bdrv_detach_aio_context = qcow2_detach_aio_context,
.bdrv_attach_aio_context = qcow2_attach_aio_context,

.bdrv_supports_persistent_dirty_bitmap =
qcow2_supports_persistent_dirty_bitmap,
Expand Down
64 changes: 32 additions & 32 deletions block/qed.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1498,9 +1498,9 @@ bdrv_qed_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}

static int bdrv_qed_change_backing_file(BlockDriverState *bs,
const char *backing_file,
const char *backing_fmt)
static int coroutine_fn GRAPH_RDLOCK
bdrv_qed_co_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt)
{
BDRVQEDState *s = bs->opaque;
QEDHeader new_header, le_header;
Expand Down Expand Up @@ -1562,7 +1562,7 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs,
}

/* Write new header */
ret = bdrv_pwrite_sync(bs->file, 0, buffer_len, buffer, 0);
ret = bdrv_co_pwrite_sync(bs->file, 0, buffer_len, buffer, 0);
g_free(buffer);
if (ret == 0) {
memcpy(&s->header, &new_header, sizeof(new_header));
Expand Down Expand Up @@ -1636,34 +1636,34 @@ static QemuOptsList qed_create_opts = {
};

static BlockDriver bdrv_qed = {
.format_name = "qed",
.instance_size = sizeof(BDRVQEDState),
.create_opts = &qed_create_opts,
.is_format = true,
.supports_backing = true,

.bdrv_probe = bdrv_qed_probe,
.bdrv_open = bdrv_qed_open,
.bdrv_close = bdrv_qed_close,
.bdrv_reopen_prepare = bdrv_qed_reopen_prepare,
.bdrv_child_perm = bdrv_default_perms,
.bdrv_co_create = bdrv_qed_co_create,
.bdrv_co_create_opts = bdrv_qed_co_create_opts,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_block_status = bdrv_qed_co_block_status,
.bdrv_co_readv = bdrv_qed_co_readv,
.bdrv_co_writev = bdrv_qed_co_writev,
.bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes,
.bdrv_co_truncate = bdrv_qed_co_truncate,
.bdrv_co_getlength = bdrv_qed_co_getlength,
.bdrv_co_get_info = bdrv_qed_co_get_info,
.bdrv_refresh_limits = bdrv_qed_refresh_limits,
.bdrv_change_backing_file = bdrv_qed_change_backing_file,
.bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache,
.bdrv_co_check = bdrv_qed_co_check,
.bdrv_detach_aio_context = bdrv_qed_detach_aio_context,
.bdrv_attach_aio_context = bdrv_qed_attach_aio_context,
.bdrv_drain_begin = bdrv_qed_drain_begin,
.format_name = "qed",
.instance_size = sizeof(BDRVQEDState),
.create_opts = &qed_create_opts,
.is_format = true,
.supports_backing = true,

.bdrv_probe = bdrv_qed_probe,
.bdrv_open = bdrv_qed_open,
.bdrv_close = bdrv_qed_close,
.bdrv_reopen_prepare = bdrv_qed_reopen_prepare,
.bdrv_child_perm = bdrv_default_perms,
.bdrv_co_create = bdrv_qed_co_create,
.bdrv_co_create_opts = bdrv_qed_co_create_opts,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_block_status = bdrv_qed_co_block_status,
.bdrv_co_readv = bdrv_qed_co_readv,
.bdrv_co_writev = bdrv_qed_co_writev,
.bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes,
.bdrv_co_truncate = bdrv_qed_co_truncate,
.bdrv_co_getlength = bdrv_qed_co_getlength,
.bdrv_co_get_info = bdrv_qed_co_get_info,
.bdrv_refresh_limits = bdrv_qed_refresh_limits,
.bdrv_co_change_backing_file = bdrv_qed_co_change_backing_file,
.bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache,
.bdrv_co_check = bdrv_qed_co_check,
.bdrv_detach_aio_context = bdrv_qed_detach_aio_context,
.bdrv_attach_aio_context = bdrv_qed_attach_aio_context,
.bdrv_drain_begin = bdrv_qed_drain_begin,
};

static void bdrv_qed_init(void)
Expand Down
3 changes: 1 addition & 2 deletions include/block/block-global-state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp);

int bdrv_commit(BlockDriverState *bs);
int GRAPH_RDLOCK bdrv_make_empty(BdrvChild *c, Error **errp);
int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt, bool warn);

void bdrv_register(BlockDriver *bdrv);
int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
const char *backing_file_str);
Expand Down
8 changes: 8 additions & 0 deletions include/block/block-io.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,14 @@ void bdrv_round_to_subclusters(BlockDriverState *bs,
void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);

int coroutine_fn GRAPH_RDLOCK
bdrv_co_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt, bool warn);

int co_wrapper_bdrv_rdlock
bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt, bool warn);

int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
int64_t pos, int size);

Expand Down
5 changes: 3 additions & 2 deletions include/block/block_int-common.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,9 @@ struct BlockDriver {
const char *name,
Error **errp);

int (*bdrv_change_backing_file)(BlockDriverState *bs,
const char *backing_file, const char *backing_fmt);
int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_change_backing_file)(
BlockDriverState *bs, const char *backing_file,
const char *backing_fmt);

/* TODO Better pass a option string/QDict/QemuOpts to add any rule? */
int (*bdrv_debug_breakpoint)(BlockDriverState *bs, const char *event,
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/test-bdrv-drain.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs,
return 0;
}

static int bdrv_test_change_backing_file(BlockDriverState *bs,
const char *backing_file,
const char *backing_fmt)
static int bdrv_test_co_change_backing_file(BlockDriverState *bs,
const char *backing_file,
const char *backing_fmt)
{
return 0;
}
Expand All @@ -116,7 +116,7 @@ static BlockDriver bdrv_test = {

.bdrv_child_perm = bdrv_default_perms,

.bdrv_change_backing_file = bdrv_test_change_backing_file,
.bdrv_co_change_backing_file = bdrv_test_co_change_backing_file,
};

static void aio_ret_cb(void *opaque, int ret)
Expand Down

0 comments on commit e2dd273

Please sign in to comment.