OpenE2K/qemu-e2k
13
4
Fork 0
Code Issues 4 Pull Requests Projects 2 Releases Activity

qcow2: Do not reopen data_file in invalidate_cache

Browse Source 
qcow2_co_invalidate_cache() closes and opens the qcow2 file, by calling
qcow2_close() and qcow2_do_open().  These two functions must thus be
usable from both a global-state and an I/O context.

As they are, they are not safe to call in an I/O context, because they
use bdrv_unref_child() and bdrv_open_child() to close/open the data_file
child, respectively, both of which are global-state functions.  When
used from qcow2_co_invalidate_cache(), we do not need to close/open the
data_file child, though (we do not do this for bs->file or bs->backing
either), and so we should skip it in the qcow2_co_invalidate_cache()
path.

To do so, add a parameter to qcow2_do_open() and qcow2_close() to make
them skip handling s->data_file, and have qcow2_co_invalidate_cache()
exempt it from the memset() on the BDRVQcow2State.

(Note that the QED driver similarly closes/opens the QED image by
invoking bdrv_qed_close()+bdrv_qed_do_open(), but both functions seem
safe to use in an I/O context.)

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/945
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20220427114057.36651-3-hreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This commit is contained in:
Hanna Reitz 2022-04-27 13:40:55 +02:00 committed by Kevin Wolf
parent 15aee7ac95
commit 06e9cd19a4
1 changed files with 62 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

104
block/qcow2.c
View File

@ -1296,7 +1296,8 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)
/* Called with s->lock held. */ /* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp) int flags, bool open_data_file,
Error **errp)
{ {
ERRP_GUARD(); ERRP_GUARD();
BDRVQcow2State *s = bs->opaque; BDRVQcow2State *s = bs->opaque;
@ -1614,50 +1615,52 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
goto fail; goto fail;
} }
/* Open external data file */ if (open_data_file) {
s->data_file = bdrv_open_child(NULL, options, "data-file", bs, /* Open external data file */
&child_of_bds, BDRV_CHILD_DATA, s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
true, errp); &child_of_bds, BDRV_CHILD_DATA,
if (*errp) { true, errp);
ret = -EINVAL; if (*errp) {
goto fail; ret = -EINVAL;
} goto fail;
}
if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) { if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
if (!s->data_file && s->image_data_file) { if (!s->data_file && s->image_data_file) {
s->data_file = bdrv_open_child(s->image_data_file, options, s->data_file = bdrv_open_child(s->image_data_file, options,
"data-file", bs, &child_of_bds, "data-file", bs, &child_of_bds,
BDRV_CHILD_DATA, false, errp); BDRV_CHILD_DATA, false, errp);
if (!s->data_file) {
ret = -EINVAL;
goto fail;
}
}
if (!s->data_file) { if (!s->data_file) {
error_setg(errp, "'data-file' is required for this image");
ret = -EINVAL; ret = -EINVAL;
goto fail; goto fail;
} }
}
if (!s->data_file) {
error_setg(errp, "'data-file' is required for this image");
ret = -EINVAL;
goto fail;
}
/* No data here */ /* No data here */
bs->file->role &= ~BDRV_CHILD_DATA; bs->file->role &= ~BDRV_CHILD_DATA;
/* Must succeed because we have given up permissions if anything */ /* Must succeed because we have given up permissions if anything */
bdrv_child_refresh_perms(bs, bs->file, &error_abort); bdrv_child_refresh_perms(bs, bs->file, &error_abort);
} else { } else {
if (s->data_file) { if (s->data_file) {
error_setg(errp, "'data-file' can only be set for images with an " error_setg(errp, "'data-file' can only be set for images with "
"external data file"); "an external data file");
ret = -EINVAL; ret = -EINVAL;
goto fail; goto fail;
} }
s->data_file = bs->file; s->data_file = bs->file;
if (data_file_is_raw(bs)) { if (data_file_is_raw(bs)) {
error_setg(errp, "data-file-raw requires a data file"); error_setg(errp, "data-file-raw requires a data file");
ret = -EINVAL; ret = -EINVAL;
goto fail; goto fail;
}
} }
} }
@ -1839,7 +1842,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
fail: fail:
g_free(s->image_data_file); g_free(s->image_data_file);
if (has_data_file(bs)) { if (open_data_file && has_data_file(bs)) {
bdrv_unref_child(bs, s->data_file); bdrv_unref_child(bs, s->data_file);
s->data_file = NULL; s->data_file = NULL;
} }
@ -1876,7 +1879,8 @@ static void coroutine_fn qcow2_open_entry(void *opaque)
BDRVQcow2State *s = qoc->bs->opaque; BDRVQcow2State *s = qoc->bs->opaque;
qemu_co_mutex_lock(&s->lock); qemu_co_mutex_lock(&s->lock);
qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp); qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, true,
qoc->errp);
qemu_co_mutex_unlock(&s->lock); qemu_co_mutex_unlock(&s->lock);
} }
@ -2714,7 +2718,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
return result; return result;
} }
static void qcow2_close(BlockDriverState *bs) static void qcow2_do_close(BlockDriverState *bs, bool close_data_file)
{ {
BDRVQcow2State *s = bs->opaque; BDRVQcow2State *s = bs->opaque;
qemu_vfree(s->l1_table); qemu_vfree(s->l1_table);
@ -2740,7 +2744,7 @@ static void qcow2_close(BlockDriverState *bs)
g_free(s->image_backing_file); g_free(s->image_backing_file);
g_free(s->image_backing_format); g_free(s->image_backing_format);
if (has_data_file(bs)) { if (close_data_file && has_data_file(bs)) {
bdrv_unref_child(bs, s->data_file); bdrv_unref_child(bs, s->data_file);
s->data_file = NULL; s->data_file = NULL;
} }
@ -2749,11 +2753,17 @@ static void qcow2_close(BlockDriverState *bs)
qcow2_free_snapshots(bs); qcow2_free_snapshots(bs);
} }
static void qcow2_close(BlockDriverState *bs)
{
qcow2_do_close(bs, true);
}
static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
Error **errp) Error **errp)
{ {
ERRP_GUARD(); ERRP_GUARD();
BDRVQcow2State *s = bs->opaque; BDRVQcow2State *s = bs->opaque;
BdrvChild *data_file;
int flags = s->flags; int flags = s->flags;
QCryptoBlock *crypto = NULL; QCryptoBlock *crypto = NULL;
QDict *options; QDict *options;
@ -2767,14 +2777,24 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
crypto = s->crypto; crypto = s->crypto;
s->crypto = NULL; s->crypto = NULL;
qcow2_close(bs); /*
* Do not reopen s->data_file (i.e., have qcow2_do_close() not close it,
* and then prevent qcow2_do_open() from opening it), because this function
* runs in the I/O path and as such we must not invoke global-state
* functions like bdrv_unref_child() and bdrv_open_child().
*/
qcow2_do_close(bs, false);
data_file = s->data_file;
memset(s, 0, sizeof(BDRVQcow2State)); memset(s, 0, sizeof(BDRVQcow2State));
s->data_file = data_file;
options = qdict_clone_shallow(bs->options); options = qdict_clone_shallow(bs->options);
flags &= ~BDRV_O_INACTIVE; flags &= ~BDRV_O_INACTIVE;
qemu_co_mutex_lock(&s->lock); qemu_co_mutex_lock(&s->lock);
ret = qcow2_do_open(bs, options, flags, errp); ret = qcow2_do_open(bs, options, flags, false, errp);
qemu_co_mutex_unlock(&s->lock); qemu_co_mutex_unlock(&s->lock);
qobject_unref(options); qobject_unref(options);
if (ret < 0) { if (ret < 0) {