* [PATCH v6 01/15] block: BlockDriver: add .filtered_child_is_backing field
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 02/15] block: introduce bdrv_open_file_child() helper Vladimir Sementsov-Ogievskiy
` (14 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov, John Snow
Unfortunately not all filters use .file child as filtered child. Two
exclusions are mirror_top and commit_top. Happily they both are private
filters. Bad thing is that this inconsistency is observable through qmp
commands query-block / query-named-block-nodes. So, could we just
change mirror_top and commit_top to use file child as all other filter
driver is an open question. Probably, we could do that with some kind
of deprecation period, but how to warn users during it?
For now, let's just add a field so we can distinguish them in generic
code, it will be used in further commits.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
block/commit.c | 1 +
block/mirror.c | 1 +
include/block/block_int-common.h | 13 +++++++++++++
3 files changed, 15 insertions(+)
diff --git a/block/commit.c b/block/commit.c
index 851d1c557a..7722a392af 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -238,6 +238,7 @@ static BlockDriver bdrv_commit_top = {
.bdrv_child_perm = bdrv_commit_top_child_perm,
.is_filter = true,
+ .filtered_child_is_backing = true,
};
void commit_start(const char *job_id, BlockDriverState *bs,
diff --git a/block/mirror.c b/block/mirror.c
index d8ecb9efa2..824b273fc7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1578,6 +1578,7 @@ static BlockDriver bdrv_mirror_top = {
.bdrv_child_perm = bdrv_mirror_top_child_perm,
.is_filter = true,
+ .filtered_child_is_backing = true,
};
static BlockJob *mirror_start_job(
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..9d91ccbcbf 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -119,6 +119,19 @@ struct BlockDriver {
* (And this filtered child must then be bs->file or bs->backing.)
*/
bool is_filter;
+ /*
+ * Only make sense for filter drivers, for others must be false.
+ * If true, filtered child is bs->backing. Otherwise it's bs->file.
+ * Only two internal filters use bs->backing as filtered child and has this
+ * field set to true: mirror_top and commit_top.
+ *
+ * Never create any more such filters!
+ *
+ * TODO: imagine how to deprecate this behavior and make all filters work
+ * similarly using bs->file as filtered child.
+ */
+ bool filtered_child_is_backing;
+
/*
* Set to true if the BlockDriver is a format driver. Format nodes
* generally do not expect their children to be other format nodes
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 02/15] block: introduce bdrv_open_file_child() helper
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 01/15] block: BlockDriver: add .filtered_child_is_backing field Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 03/15] block/blklogwrites: don't care to remove bs->file child on failure Vladimir Sementsov-Ogievskiy
` (13 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, hreitz, vsementsov, Ari Sundholm,
Pavel Dovgalyuk, Paolo Bonzini, Stefan Hajnoczi, John Snow,
Denis V. Lunev, Wen Congyang, Xie Changlong, Stefan Weil,
Jeff Cody, Fam Zheng
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>
---
block.c | 21 +++++++++++++++++++++
block/blkdebug.c | 9 +++------
block/blklogwrites.c | 7 ++-----
block/blkreplay.c | 7 ++-----
block/blkverify.c | 9 +++------
block/bochs.c | 7 +++----
block/cloop.c | 7 +++----
block/copy-before-write.c | 9 ++++-----
block/copy-on-read.c | 9 ++++-----
block/crypto.c | 11 ++++++-----
block/dmg.c | 7 +++----
block/filter-compress.c | 8 +++-----
block/parallels.c | 7 +++----
block/preallocate.c | 9 ++++-----
block/qcow.c | 6 ++----
block/qcow2.c | 8 ++++----
block/qed.c | 8 ++++----
block/replication.c | 8 +++-----
block/throttle.c | 8 +++-----
block/vdi.c | 7 +++----
block/vhdx.c | 7 +++----
block/vmdk.c | 7 +++----
block/vpc.c | 7 +++----
include/block/block-global-state.h | 3 +++
24 files changed, 95 insertions(+), 101 deletions(-)
diff --git a/block.c b/block.c
index 2c00dddd80..883b1bb242 100644
--- a/block.c
+++ b/block.c
@@ -3666,6 +3666,27 @@ BdrvChild *bdrv_open_child(const char *filename,
errp);
}
+/*
+ * Wrapper on bdrv_open_child() for most popular case: open primary child of bs.
+ */
+int bdrv_open_file_child(const char *filename,
+ QDict *options, const char *bdref_key,
+ BlockDriverState *parent, Error **errp)
+{
+ BdrvChildRole role;
+
+ /* commit_top and mirror_top don't use this function */
+ assert(!parent->drv->filtered_child_is_backing);
+
+ role = parent->drv->is_filter ?
+ (BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY) : BDRV_CHILD_IMAGE;
+
+ parent->file = bdrv_open_child(filename, options, bdref_key, parent,
+ &child_of_bds, role, false, errp);
+
+ return parent->file ? 0 : -EINVAL;
+}
+
/*
* TODO Future callers may need to specify parent/child_class in order for
* option inheritance to work. Existing callers use it for the root node.
diff --git a/block/blkdebug.c b/block/blkdebug.c
index bbf2948703..5fcfc8ac6f 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -503,12 +503,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Open the image file */
- bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
- bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- ret = -EINVAL;
+ ret = bdrv_open_file_child(qemu_opt_get(opts, "x-image"), options, "image",
+ bs, errp);
+ if (ret < 0) {
goto out;
}
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index f7a251e91f..f66a617eb3 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -155,11 +155,8 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Open the file */
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false,
- errp);
- if (!bs->file) {
- ret = -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
goto fail;
}
diff --git a/block/blkreplay.c b/block/blkreplay.c
index dcbe780ddb..76a0b8d12a 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -26,11 +26,8 @@ static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
int ret;
/* Open the image file */
- bs->file = bdrv_open_child(NULL, options, "image", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- ret = -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "image", bs, errp);
+ if (ret < 0) {
goto fail;
}
diff --git a/block/blkverify.c b/block/blkverify.c
index e4a37af3b2..e4d40a63aa 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -122,12 +122,9 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Open the raw file */
- bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw",
- bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- ret = -EINVAL;
+ ret = bdrv_open_file_child(qemu_opt_get(opts, "x-raw"), options, "raw",
+ bs, errp);
+ if (ret < 0) {
goto fail;
}
diff --git a/block/bochs.c b/block/bochs.c
index 4d68658087..b2dc06bbfd 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -110,10 +110,9 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
diff --git a/block/cloop.c b/block/cloop.c
index b8c6d0eccd..bee87da173 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -71,10 +71,9 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
/* read header */
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index a8a06fdc09..4fad564691 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -376,12 +376,11 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
BDRVCopyBeforeWriteState *s = bs->opaque;
BdrvDirtyBitmap *bitmap = NULL;
int64_t cluster_size;
+ int ret;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
s->target = bdrv_open_child(NULL, options, "target", bs, &child_of_bds,
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 1fc7fb3333..815ac1d835 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -41,12 +41,11 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
BDRVStateCOR *state = bs->opaque;
/* Find a bottom node name, if any */
const char *bottom_node = qdict_get_try_str(options, "bottom");
+ int ret;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
bs->supported_read_flags = BDRV_REQ_PREFETCH;
diff --git a/block/crypto.c b/block/crypto.c
index 1ba82984ef..e165447a5b 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -261,15 +261,14 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
{
BlockCrypto *crypto = bs->opaque;
QemuOpts *opts = NULL;
- int ret = -EINVAL;
+ int ret;
QCryptoBlockOpenOptions *open_opts = NULL;
unsigned int cflags = 0;
QDict *cryptoopts = NULL;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
bs->supported_write_flags = BDRV_REQ_FUA &
@@ -277,6 +276,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
opts = qemu_opts_create(opts_spec, NULL, 0, &error_abort);
if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+ ret = -EINVAL;
goto cleanup;
}
@@ -285,6 +285,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
open_opts = block_crypto_open_opts_init(cryptoopts, errp);
if (!open_opts) {
+ ret = -EINVAL;
goto cleanup;
}
diff --git a/block/dmg.c b/block/dmg.c
index c626587f9c..f91c5d3980 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -440,10 +440,9 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
block_module_load_one("dmg-bz2");
diff --git a/block/filter-compress.c b/block/filter-compress.c
index d5be538619..305716c86c 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -30,11 +30,9 @@
static int compress_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- return -EINVAL;
+ int ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
if (!bs->file->bs->drv || !block_driver_can_compress(bs->file->bs->drv)) {
diff --git a/block/parallels.c b/block/parallels.c
index 8879b7027a..952ef86514 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -736,10 +736,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
Error *local_err = NULL;
char *buf;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
diff --git a/block/preallocate.c b/block/preallocate.c
index e15cb8c74a..d50ee7f49b 100644
--- a/block/preallocate.c
+++ b/block/preallocate.c
@@ -134,6 +134,7 @@ static int preallocate_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVPreallocateState *s = bs->opaque;
+ int ret;
/*
* s->data_end and friends should be initialized on permission update.
@@ -141,11 +142,9 @@ static int preallocate_open(BlockDriverState *bs, QDict *options, int flags,
*/
s->file_end = s->zero_start = s->data_end = -EINVAL;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
if (!preallocate_absorb_opts(&s->opts, options, bs->file->bs, errp)) {
diff --git a/block/qcow.c b/block/qcow.c
index 4fba1b9e36..b4033c0db9 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -121,10 +121,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
qdict_extract_subqdict(options, &encryptopts, "encrypt.");
encryptfmt = qdict_get_try_str(encryptopts, "format");
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- ret = -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 4f5e6440fb..90df4c4dd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1895,11 +1895,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
.errp = errp,
.ret = -EINPROGRESS
};
+ int ret;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
/* Initialise locks */
diff --git a/block/qed.c b/block/qed.c
index f34d9a3ac1..1ff024f16d 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -559,11 +559,11 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
.errp = errp,
.ret = -EINPROGRESS
};
+ int ret;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
bdrv_qed_init_state(bs);
diff --git a/block/replication.c b/block/replication.c
index 55c8f894aa..2f17397764 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -88,11 +88,9 @@ static int replication_open(BlockDriverState *bs, QDict *options,
const char *mode;
const char *top_id;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
ret = -EINVAL;
diff --git a/block/throttle.c b/block/throttle.c
index 6e8d52fa24..4fb5798c27 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -78,11 +78,9 @@ static int throttle_open(BlockDriverState *bs, QDict *options,
char *group;
int ret;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
bs->supported_write_flags = bs->file->bs->supported_write_flags |
BDRV_REQ_WRITE_UNCHANGED;
diff --git a/block/vdi.c b/block/vdi.c
index cca3a3a356..a539081138 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -377,10 +377,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
int ret;
QemuUUID uuid_link, uuid_parent;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
logout("\n");
diff --git a/block/vhdx.c b/block/vhdx.c
index 410c6f9610..994f8b91cc 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -997,10 +997,9 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
uint64_t signature;
Error *local_err = NULL;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
s->bat = NULL;
diff --git a/block/vmdk.c b/block/vmdk.c
index 38e5ab3806..7f23d5d86b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1315,10 +1315,9 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
BDRVVmdkState *s = bs->opaque;
uint32_t magic;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
buf = vmdk_read_desc(bs->file, 0, errp);
diff --git a/block/vpc.c b/block/vpc.c
index 4d8f16e199..cd82eb7f92 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -233,10 +233,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
int ret;
int64_t bs_size;
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_IMAGE, false, errp);
- if (!bs->file) {
- return -EINVAL;
+ ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
+ if (ret < 0) {
+ return ret;
}
opts = qemu_opts_create(&vpc_runtime_opts, NULL, 0, &error_abort);
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 21265e3966..29a38d7e18 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -76,6 +76,9 @@ BdrvChild *bdrv_open_child(const char *filename,
const BdrvChildClass *child_class,
BdrvChildRole child_role,
bool allow_none, Error **errp);
+int bdrv_open_file_child(const char *filename,
+ QDict *options, const char *bdref_key,
+ BlockDriverState *parent, Error **errp);
BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 03/15] block/blklogwrites: don't care to remove bs->file child on failure
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 01/15] block: BlockDriver: add .filtered_child_is_backing field Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 02/15] block: introduce bdrv_open_file_child() helper Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 04/15] test-bdrv-graph-mod: update test_parallel_perm_update test case Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov, Ari Sundholm
We don't need to remove bs->file, generic layer takes care of it. No
other driver cares to remove bs->file on failure by hand.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
block/blklogwrites.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index f66a617eb3..7d25df97cc 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -254,10 +254,6 @@ fail_log:
s->log_file = NULL;
}
fail:
- if (ret < 0) {
- bdrv_unref_child(bs, bs->file);
- bs->file = NULL;
- }
qemu_opts_del(opts);
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 04/15] test-bdrv-graph-mod: update test_parallel_perm_update test case
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 03/15] block/blklogwrites: don't care to remove bs->file child on failure Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 05/15] tests-bdrv-drain: bdrv_replace_test driver: declare supports_backing Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
test_parallel_perm_update() does two things that we are going to
restrict in the near future:
1. It updates bs->file field by hand. bs->file will be managed
automatically by generic code (together with bs->children list).
Let's better refactor our "tricky" bds to have own state where one
of children is linked as "selected".
This also looks less "tricky", so avoid using this word.
2. It create FILTERED children that are not PRIMARY. Except for tests
all FILTERED children in the Qemu block layer are always PRIMARY as
well. We are going to formalize this rule, so let's better use DATA
children here.
3. It creates more than one FILTERED child, which is already abandoned
in BDRV_CHILD_FILTERED's description.
While being here, update the picture to better correspond to the test
code.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
tests/unit/test-bdrv-graph-mod.c | 80 +++++++++++++++++++-------------
1 file changed, 49 insertions(+), 31 deletions(-)
diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
index a6e3bb79be..e2f1355af1 100644
--- a/tests/unit/test-bdrv-graph-mod.c
+++ b/tests/unit/test-bdrv-graph-mod.c
@@ -241,13 +241,26 @@ static void test_parallel_exclusive_write(void)
bdrv_unref(top);
}
-static void write_to_file_perms(BlockDriverState *bs, BdrvChild *c,
- BdrvChildRole role,
- BlockReopenQueue *reopen_queue,
- uint64_t perm, uint64_t shared,
- uint64_t *nperm, uint64_t *nshared)
+/*
+ * write-to-selected node may have several DATA children, one of them may be
+ * "selected". Exclusive write permission is taken on selected child.
+ *
+ * We don't realize write handler itself, as we need only to test how permission
+ * update works.
+ */
+typedef struct BDRVWriteToSelectedState {
+ BdrvChild *selected;
+} BDRVWriteToSelectedState;
+
+static void write_to_selected_perms(BlockDriverState *bs, BdrvChild *c,
+ BdrvChildRole role,
+ BlockReopenQueue *reopen_queue,
+ uint64_t perm, uint64_t shared,
+ uint64_t *nperm, uint64_t *nshared)
{
- if (bs->file && c == bs->file) {
+ BDRVWriteToSelectedState *s = bs->opaque;
+
+ if (s->selected && c == s->selected) {
*nperm = BLK_PERM_WRITE;
*nshared = BLK_PERM_ALL & ~BLK_PERM_WRITE;
} else {
@@ -256,9 +269,10 @@ static void write_to_file_perms(BlockDriverState *bs, BdrvChild *c,
}
}
-static BlockDriver bdrv_write_to_file = {
- .format_name = "tricky-perm",
- .bdrv_child_perm = write_to_file_perms,
+static BlockDriver bdrv_write_to_selected = {
+ .format_name = "write-to-selected",
+ .instance_size = sizeof(BDRVWriteToSelectedState),
+ .bdrv_child_perm = write_to_selected_perms,
};
@@ -266,15 +280,18 @@ static BlockDriver bdrv_write_to_file = {
* The following test shows that topological-sort order is required for
* permission update, simple DFS is not enough.
*
- * Consider the block driver which has two filter children: one active
- * with exclusive write access and one inactive with no specific
- * permissions.
+ * Consider the block driver (write-to-selected) which has two children: one is
+ * selected so we have exclusive write access to it and for the other one we
+ * don't need any specific permissions.
*
* And, these two children has a common base child, like this:
+ * (additional "top" on top is used in test just because the only public
+ * function to update permission should get a specific child to update.
+ * Making bdrv_refresh_perms() public just for this test isn't worth it)
*
- * ┌─────┐ ┌──────┐
- * │ fl2 │ ◀── │ top │
- * └─────┘ └──────┘
+ * ┌─────┐ ┌───────────────────┐ ┌─────┐
+ * │ fl2 │ ◀── │ write-to-selected │ ◀── │ top │
+ * └─────┘ └───────────────────┘ └─────┘
* │ │
* │ │ w
* │ ▼
@@ -290,14 +307,14 @@ static BlockDriver bdrv_write_to_file = {
*
* So, exclusive write is propagated.
*
- * Assume, we want to make fl2 active instead of fl1.
- * So, we set some option for top driver and do permission update.
+ * Assume, we want to select fl2 instead of fl1.
+ * So, we set some option for write-to-selected driver and do permission update.
*
* With simple DFS, if permission update goes first through
- * top->fl1->base branch it will succeed: it firstly drop exclusive write
- * permissions and than apply them for another BdrvChildren.
- * But if permission update goes first through top->fl2->base branch it
- * will fail, as when we try to update fl2->base child, old not yet
+ * write-to-selected -> fl1 -> base branch it will succeed: it firstly drop
+ * exclusive write permissions and than apply them for another BdrvChildren.
+ * But if permission update goes first through write-to-selected -> fl2 -> base
+ * branch it will fail, as when we try to update fl2->base child, old not yet
* updated fl1->base child will be in conflict.
*
* With topological-sort order we always update parents before children, so fl1
@@ -306,9 +323,10 @@ static BlockDriver bdrv_write_to_file = {
static void test_parallel_perm_update(void)
{
BlockDriverState *top = no_perm_node("top");
- BlockDriverState *tricky =
- bdrv_new_open_driver(&bdrv_write_to_file, "tricky", BDRV_O_RDWR,
+ BlockDriverState *ws =
+ bdrv_new_open_driver(&bdrv_write_to_selected, "ws", BDRV_O_RDWR,
&error_abort);
+ BDRVWriteToSelectedState *s = ws->opaque;
BlockDriverState *base = no_perm_node("base");
BlockDriverState *fl1 = pass_through_node("fl1");
BlockDriverState *fl2 = pass_through_node("fl2");
@@ -320,33 +338,33 @@ static void test_parallel_perm_update(void)
*/
bdrv_ref(base);
- bdrv_attach_child(top, tricky, "file", &child_of_bds, BDRV_CHILD_DATA,
+ bdrv_attach_child(top, ws, "file", &child_of_bds, BDRV_CHILD_DATA,
&error_abort);
- c_fl1 = bdrv_attach_child(tricky, fl1, "first", &child_of_bds,
- BDRV_CHILD_FILTERED, &error_abort);
- c_fl2 = bdrv_attach_child(tricky, fl2, "second", &child_of_bds,
- BDRV_CHILD_FILTERED, &error_abort);
+ c_fl1 = bdrv_attach_child(ws, fl1, "first", &child_of_bds,
+ BDRV_CHILD_DATA, &error_abort);
+ c_fl2 = bdrv_attach_child(ws, fl2, "second", &child_of_bds,
+ BDRV_CHILD_DATA, &error_abort);
bdrv_attach_child(fl1, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED,
&error_abort);
bdrv_attach_child(fl2, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED,
&error_abort);
/* Select fl1 as first child to be active */
- tricky->file = c_fl1;
+ s->selected = c_fl1;
bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort);
assert(c_fl1->perm & BLK_PERM_WRITE);
assert(!(c_fl2->perm & BLK_PERM_WRITE));
/* Now, try to switch active child and update permissions */
- tricky->file = c_fl2;
+ s->selected = c_fl2;
bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort);
assert(c_fl2->perm & BLK_PERM_WRITE);
assert(!(c_fl1->perm & BLK_PERM_WRITE));
/* Switch once more, to not care about real child order in the list */
- tricky->file = c_fl1;
+ s->selected = c_fl1;
bdrv_child_refresh_perms(top, top->children.lh_first, &error_abort);
assert(c_fl1->perm & BLK_PERM_WRITE);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 05/15] tests-bdrv-drain: bdrv_replace_test driver: declare supports_backing
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 04/15] test-bdrv-graph-mod: update test_parallel_perm_update test case Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 06/15] test-bdrv-graph-mod: fix filters to be filters Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, hreitz, vsementsov, Eric Blake, Nikita Lapshin
We do add COW child to the node. In future we are going to forbid
adding COW child to the node that doesn't support backing. So, fix it
here now.
Don't worry about setting bs->backing itself: in further commit we'll
update the block-layer to automatically set/unset this field in generic
code.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
tests/unit/test-bdrv-drain.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 36be84ae55..23d425a494 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1948,6 +1948,7 @@ static void coroutine_fn bdrv_replace_test_co_drain_end(BlockDriverState *bs)
static BlockDriver bdrv_replace_test = {
.format_name = "replace_test",
.instance_size = sizeof(BDRVReplaceTestState),
+ .supports_backing = true,
.bdrv_close = bdrv_replace_test_close,
.bdrv_co_preadv = bdrv_replace_test_co_preadv,
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 06/15] test-bdrv-graph-mod: fix filters to be filters
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 05/15] tests-bdrv-drain: bdrv_replace_test driver: declare supports_backing Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 07/15] block: document connection between child roles and bs->backing/bs->file Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
bdrv_pass_through is used as filter, even all node variables has
corresponding names. We want to append it, so it should be
backing-child-based filter like mirror_top.
So, in test_update_perm_tree, first child should be DATA, as we don't
want filters with two filtered children.
bdrv_exclusive_writer is used as a filter once. So it should be filter
anyway. We want to append it, so it should be backing-child-based
fitler too.
Make all FILTERED children to be PRIMARY as well. We are going to force
this rule by assertion soon.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
include/block/block_int-common.h | 5 +++--
tests/unit/test-bdrv-graph-mod.c | 24 +++++++++++++++++-------
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 9d91ccbcbf..d68adc6ff3 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -122,8 +122,9 @@ struct BlockDriver {
/*
* Only make sense for filter drivers, for others must be false.
* If true, filtered child is bs->backing. Otherwise it's bs->file.
- * Only two internal filters use bs->backing as filtered child and has this
- * field set to true: mirror_top and commit_top.
+ * Two internal filters use bs->backing as filtered child and has this
+ * field set to true: mirror_top and commit_top. There also two such test
+ * filters in tests/unit/test-bdrv-graph-mod.c.
*
* Never create any more such filters!
*
diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
index e2f1355af1..c522591531 100644
--- a/tests/unit/test-bdrv-graph-mod.c
+++ b/tests/unit/test-bdrv-graph-mod.c
@@ -26,6 +26,8 @@
static BlockDriver bdrv_pass_through = {
.format_name = "pass-through",
+ .is_filter = true,
+ .filtered_child_is_backing = true,
.bdrv_child_perm = bdrv_default_perms,
};
@@ -57,6 +59,8 @@ static void exclusive_write_perms(BlockDriverState *bs, BdrvChild *c,
static BlockDriver bdrv_exclusive_writer = {
.format_name = "exclusive-writer",
+ .is_filter = true,
+ .filtered_child_is_backing = true,
.bdrv_child_perm = exclusive_write_perms,
};
@@ -134,7 +138,7 @@ static void test_update_perm_tree(void)
blk_insert_bs(root, bs, &error_abort);
bdrv_attach_child(filter, bs, "child", &child_of_bds,
- BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
+ BDRV_CHILD_DATA, &error_abort);
ret = bdrv_append(filter, bs, NULL);
g_assert_cmpint(ret, <, 0);
@@ -228,11 +232,14 @@ static void test_parallel_exclusive_write(void)
*/
bdrv_ref(base);
- bdrv_attach_child(top, fl1, "backing", &child_of_bds, BDRV_CHILD_DATA,
+ bdrv_attach_child(top, fl1, "backing", &child_of_bds,
+ BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
- bdrv_attach_child(fl1, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED,
+ bdrv_attach_child(fl1, base, "backing", &child_of_bds,
+ BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
- bdrv_attach_child(fl2, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED,
+ bdrv_attach_child(fl2, base, "backing", &child_of_bds,
+ BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_replace_node(fl1, fl2, &error_abort);
@@ -344,9 +351,11 @@ static void test_parallel_perm_update(void)
BDRV_CHILD_DATA, &error_abort);
c_fl2 = bdrv_attach_child(ws, fl2, "second", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
- bdrv_attach_child(fl1, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED,
+ bdrv_attach_child(fl1, base, "backing", &child_of_bds,
+ BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
- bdrv_attach_child(fl2, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED,
+ bdrv_attach_child(fl2, base, "backing", &child_of_bds,
+ BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
/* Select fl1 as first child to be active */
@@ -397,7 +406,8 @@ static void test_append_greedy_filter(void)
BlockDriverState *base = no_perm_node("base");
BlockDriverState *fl = exclusive_writer_node("fl1");
- bdrv_attach_child(top, base, "backing", &child_of_bds, BDRV_CHILD_COW,
+ bdrv_attach_child(top, base, "backing", &child_of_bds,
+ BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_append(fl, base, &error_abort);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 07/15] block: document connection between child roles and bs->backing/bs->file
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 06/15] test-bdrv-graph-mod: fix filters to be filters Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 08/15] block/snapshot: stress that we fallback to primary child Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
Make the informal rules formal. In further commit we'll add
corresponding assertions.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
include/block/block-common.h | 39 ++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/include/block/block-common.h b/include/block/block-common.h
index fdb7306e78..fda67a7c38 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -313,6 +313,45 @@ enum {
*
* At least one of DATA, METADATA, FILTERED, or COW must be set for
* every child.
+ *
+ *
+ * = Connection with bs->children, bs->file and bs->backing fields =
+ *
+ * 1. Filters
+ *
+ * Filter drivers have drv->is_filter = true.
+ *
+ * Filter node has exactly one FILTERED|PRIMARY child, and may have other
+ * children which must not have these bits (one example is the
+ * copy-before-write filter, which also has its target DATA child).
+ *
+ * Filter nodes never have COW children.
+ *
+ * For most filters, the filtered child is linked in bs->file, bs->backing is
+ * NULL. For some filters (as an exception), it is the other way around; those
+ * drivers will have drv->filtered_child_is_backing set to true (see that
+ * field’s documentation for what drivers this concerns)
+ *
+ * 2. "raw" driver (block/raw-format.c)
+ *
+ * Formally it's not a filter (drv->is_filter = false)
+ *
+ * bs->backing is always NULL
+ *
+ * Only has one child, linked in bs->file. Its role is either FILTERED|PRIMARY
+ * (like filter) or DATA|PRIMARY depending on options.
+ *
+ * 3. Other drivers
+ *
+ * Don't have any FILTERED children.
+ *
+ * May have at most one COW child. In this case it's linked in bs->backing.
+ * Otherwise bs->backing is NULL. COW child is never PRIMARY.
+ *
+ * May have at most one PRIMARY child. In this case it's linked in bs->file.
+ * Otherwise bs->file is NULL.
+ *
+ * May also have some other children that don't have the PRIMARY or COW bit set.
*/
enum BdrvChildRoleBits {
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 08/15] block/snapshot: stress that we fallback to primary child
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 07/15] block: document connection between child roles and bs->backing/bs->file Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 09/15] Revert "block: Let replace_child_noperm free children" Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
Actually what we chose is a primary child. Let's stress it in the code.
We are going to drop indirect pointer logic here in future. Actually
this commit simplifies the future work: we drop use of indirection in
the assertion now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
block/snapshot.c | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index d6f53c3065..75e8d3a937 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -161,21 +161,14 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
{
BdrvChild **fallback;
- BdrvChild *child;
+ BdrvChild *child = bdrv_primary_child(bs);
- /*
- * The only BdrvChild pointers that are safe to modify (and which
- * we can thus return a reference to) are bs->file and
- * bs->backing.
- */
- fallback = &bs->file;
- if (!*fallback && bs->drv && bs->drv->is_filter) {
- fallback = &bs->backing;
- }
-
- if (!*fallback) {
+ /* We allow fallback only to primary child */
+ if (!child) {
return NULL;
}
+ fallback = (child == bs->file ? &bs->file : &bs->backing);
+ assert(*fallback == child);
/*
* Check that there are no other children that would need to be
@@ -309,15 +302,12 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
}
/*
- * fallback_ptr is &bs->file or &bs->backing. *fallback_ptr
- * was closed above and set to NULL, but the .bdrv_open() call
- * has opened it again, because we set the respective option
- * (with the qdict_put_str() call above).
- * Assert that .bdrv_open() has attached some child on
- * *fallback_ptr, and that it has attached the one we wanted
- * it to (i.e., fallback_bs).
+ * fallback was a primary child. It was closed above and set to NULL,
+ * but the .bdrv_open() call has opened it again, because we set the
+ * respective option (with the qdict_put_str() call above).
+ * Assert that .bdrv_open() has attached the right BDS as primary child.
*/
- assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
+ assert(bdrv_primary_bs(bs) == fallback_bs);
bdrv_unref(fallback_bs);
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 09/15] Revert "block: Let replace_child_noperm free children"
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 08/15] block/snapshot: stress that we fallback to primary child Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 10/15] Revert "block: Let replace_child_tran keep indirect pointer" Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
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>
---
block.c | 102 +++++++++++++-------------------------------------------
1 file changed, 23 insertions(+), 79 deletions(-)
diff --git a/block.c b/block.c
index 883b1bb242..34ca046470 100644
--- a/block.c
+++ b/block.c
@@ -90,10 +90,8 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
static bool bdrv_recurse_has_child(BlockDriverState *bs,
BlockDriverState *child);
-static void bdrv_child_free(BdrvChild *child);
static void bdrv_replace_child_noperm(BdrvChild **child,
- BlockDriverState *new_bs,
- bool free_empty_child);
+ BlockDriverState *new_bs);
static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
BdrvChild *child,
Transaction *tran);
@@ -2338,7 +2336,6 @@ typedef struct BdrvReplaceChildState {
BdrvChild *child;
BdrvChild **childp;
BlockDriverState *old_bs;
- bool free_empty_child;
} BdrvReplaceChildState;
static void bdrv_replace_child_commit(void *opaque)
@@ -2346,9 +2343,6 @@ static void bdrv_replace_child_commit(void *opaque)
BdrvReplaceChildState *s = opaque;
GLOBAL_STATE_CODE();
- if (s->free_empty_child && !s->child->bs) {
- bdrv_child_free(s->child);
- }
bdrv_unref(s->old_bs);
}
@@ -2366,26 +2360,22 @@ static void bdrv_replace_child_abort(void *opaque)
* modify the BdrvChild * pointer we indirectly pass to it, i.e. it
* will not modify s->child. From that perspective, it does not matter
* whether we pass s->childp or &s->child.
+ * (TODO: Right now, bdrv_replace_child_noperm() never modifies that
+ * pointer anyway (though it will in the future), so at this point it
+ * absolutely does not matter whether we pass s->childp or &s->child.)
* (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use
* it here.
* (3) If new_bs is NULL, *s->childp will have been NULLed by
* bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
* must not pass a NULL *s->childp here.
+ * (TODO: In its current state, bdrv_replace_child_noperm() will not
+ * have NULLed *s->childp, so this does not apply yet. It will in the
+ * future.)
*
* So whether new_bs was NULL or not, we cannot pass s->childp here; and in
* any case, there is no reason to pass it anyway.
*/
- bdrv_replace_child_noperm(&s->child, s->old_bs, true);
- /*
- * The child was pre-existing, so s->old_bs must be non-NULL, and
- * s->child thus must not have been freed
- */
- assert(s->child != NULL);
- if (!new_bs) {
- /* As described above, *s->childp was cleared, so restore it */
- assert(s->childp != NULL);
- *s->childp = s->child;
- }
+ bdrv_replace_child_noperm(&s->child, s->old_bs);
bdrv_unref(new_bs);
}
@@ -2402,44 +2392,30 @@ static TransactionActionDrv bdrv_replace_child_drv = {
*
* The function doesn't update permissions, caller is responsible for this.
*
- * (*childp)->bs must not be NULL.
- *
* Note that if new_bs == NULL, @childp is stored in a state object attached
* to @tran, so that the old child can be reinstated in the abort handler.
* Therefore, if @new_bs can be NULL, @childp must stay valid until the
* transaction is committed or aborted.
*
- * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
- * freed (on commit). @free_empty_child should only be false if the
- * caller will free the BDrvChild themselves (which may be important
- * if this is in turn called in another transactional context).
+ * (TODO: The reinstating does not happen yet, but it will once
+ * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
*/
static void bdrv_replace_child_tran(BdrvChild **childp,
BlockDriverState *new_bs,
- Transaction *tran,
- bool free_empty_child)
+ Transaction *tran)
{
BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
*s = (BdrvReplaceChildState) {
.child = *childp,
.childp = new_bs == NULL ? childp : NULL,
.old_bs = (*childp)->bs,
- .free_empty_child = free_empty_child,
};
tran_add(tran, &bdrv_replace_child_drv, s);
- /* The abort handler relies on this */
- assert(s->old_bs != NULL);
-
if (new_bs) {
bdrv_ref(new_bs);
}
- /*
- * Pass free_empty_child=false, we will free the child (if
- * necessary) in bdrv_replace_child_commit() (if our
- * @free_empty_child parameter was true).
- */
- bdrv_replace_child_noperm(childp, new_bs, false);
+ bdrv_replace_child_noperm(childp, new_bs);
/* old_bs reference is transparently moved from *childp to @s */
}
@@ -2821,22 +2797,8 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
return permissions[qapi_perm];
}
-/**
- * Replace (*childp)->bs by @new_bs.
- *
- * If @new_bs is NULL, *childp will be set to NULL, too: BDS parents
- * generally cannot handle a BdrvChild with .bs == NULL, so clearing
- * BdrvChild.bs should generally immediately be followed by the
- * BdrvChild pointer being cleared as well.
- *
- * If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
- * freed. @free_empty_child should only be false if the caller will
- * free the BdrvChild themselves (this may be important in a
- * transactional context, where it may only be freed on commit).
- */
static void bdrv_replace_child_noperm(BdrvChild **childp,
- BlockDriverState *new_bs,
- bool free_empty_child)
+ BlockDriverState *new_bs)
{
BdrvChild *child = *childp;
BlockDriverState *old_bs = child->bs;
@@ -2875,9 +2837,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
}
child->bs = new_bs;
- if (!new_bs) {
- *childp = NULL;
- }
if (new_bs) {
assert_bdrv_graph_writable(new_bs);
@@ -2908,10 +2867,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
bdrv_parent_drained_end_single(child);
drain_saldo++;
}
-
- if (free_empty_child && !child->bs) {
- bdrv_child_free(child);
- }
}
/**
@@ -2943,14 +2898,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
BlockDriverState *bs = child->bs;
GLOBAL_STATE_CODE();
- /*
- * Pass free_empty_child=false, because we still need the child
- * for the AioContext operations on the parent below; those
- * BdrvChildClass methods all work on a BdrvChild object, so we
- * need to keep it as an empty shell (after this function, it will
- * not be attached to any parent, and it will not have a .bs).
- */
- bdrv_replace_child_noperm(s->child, NULL, false);
+ bdrv_replace_child_noperm(s->child, NULL);
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -2972,6 +2920,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
bdrv_unref(bs);
bdrv_child_free(child);
+ *s->child = NULL;
}
static TransactionActionDrv bdrv_attach_child_common_drv = {
@@ -3050,9 +2999,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
}
bdrv_ref(child_bs);
- bdrv_replace_child_noperm(&new_child, child_bs, true);
- /* child_bs was non-NULL, so new_child must not have been freed */
- assert(new_child != NULL);
+ bdrv_replace_child_noperm(&new_child, child_bs);
*child = new_child;
@@ -3113,7 +3060,8 @@ static void bdrv_detach_child(BdrvChild **childp)
BlockDriverState *old_bs = (*childp)->bs;
GLOBAL_STATE_CODE();
- bdrv_replace_child_noperm(childp, NULL, true);
+ bdrv_replace_child_noperm(childp, NULL);
+ bdrv_child_free(*childp);
if (old_bs) {
/*
@@ -5167,11 +5115,7 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
}
if (child->bs) {
- /*
- * Pass free_empty_child=false, we will free the child in
- * bdrv_remove_filter_or_cow_child_commit()
- */
- bdrv_replace_child_tran(childp, NULL, tran, false);
+ bdrv_replace_child_tran(childp, NULL, tran);
}
s = g_new(BdrvRemoveFilterOrCowChild, 1);
@@ -5181,6 +5125,8 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
.is_backing = (childp == &bs->backing),
};
tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
+
+ *childp = NULL;
}
/*
@@ -5224,7 +5170,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
* Passing a pointer to the local variable @c is fine here, because
* @to is not NULL, and so &c will not be attached to the transaction.
*/
- bdrv_replace_child_tran(&c, to, tran, true);
+ bdrv_replace_child_tran(&c, to, tran);
}
return 0;
@@ -5389,9 +5335,7 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
bdrv_drained_begin(old_bs);
bdrv_drained_begin(new_bs);
- bdrv_replace_child_tran(&child, new_bs, tran, true);
- /* @new_bs must have been non-NULL, so @child must not have been freed */
- assert(child != NULL);
+ bdrv_replace_child_tran(&child, new_bs, tran);
found = g_hash_table_new(NULL, NULL);
refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 10/15] Revert "block: Let replace_child_tran keep indirect pointer"
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 09/15] Revert "block: Let replace_child_noperm free children" Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 11/15] Revert "block: Restructure remove_file_or_backing_child()" Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
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>
---
block.c | 81 +++++++--------------------------------------------------
1 file changed, 10 insertions(+), 71 deletions(-)
diff --git a/block.c b/block.c
index 34ca046470..a83845b120 100644
--- a/block.c
+++ b/block.c
@@ -2334,7 +2334,6 @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm,
typedef struct BdrvReplaceChildState {
BdrvChild *child;
- BdrvChild **childp;
BlockDriverState *old_bs;
} BdrvReplaceChildState;
@@ -2352,29 +2351,7 @@ static void bdrv_replace_child_abort(void *opaque)
BlockDriverState *new_bs = s->child->bs;
GLOBAL_STATE_CODE();
- /*
- * old_bs reference is transparently moved from @s to s->child.
- *
- * Pass &s->child here instead of s->childp, because:
- * (1) s->old_bs must be non-NULL, so bdrv_replace_child_noperm() will not
- * modify the BdrvChild * pointer we indirectly pass to it, i.e. it
- * will not modify s->child. From that perspective, it does not matter
- * whether we pass s->childp or &s->child.
- * (TODO: Right now, bdrv_replace_child_noperm() never modifies that
- * pointer anyway (though it will in the future), so at this point it
- * absolutely does not matter whether we pass s->childp or &s->child.)
- * (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use
- * it here.
- * (3) If new_bs is NULL, *s->childp will have been NULLed by
- * bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
- * must not pass a NULL *s->childp here.
- * (TODO: In its current state, bdrv_replace_child_noperm() will not
- * have NULLed *s->childp, so this does not apply yet. It will in the
- * future.)
- *
- * So whether new_bs was NULL or not, we cannot pass s->childp here; and in
- * any case, there is no reason to pass it anyway.
- */
+ /* old_bs reference is transparently moved from @s to @s->child */
bdrv_replace_child_noperm(&s->child, s->old_bs);
bdrv_unref(new_bs);
}
@@ -2391,32 +2368,22 @@ static TransactionActionDrv bdrv_replace_child_drv = {
* Note: real unref of old_bs is done only on commit.
*
* The function doesn't update permissions, caller is responsible for this.
- *
- * Note that if new_bs == NULL, @childp is stored in a state object attached
- * to @tran, so that the old child can be reinstated in the abort handler.
- * Therefore, if @new_bs can be NULL, @childp must stay valid until the
- * transaction is committed or aborted.
- *
- * (TODO: The reinstating does not happen yet, but it will once
- * bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
*/
-static void bdrv_replace_child_tran(BdrvChild **childp,
- BlockDriverState *new_bs,
+static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
Transaction *tran)
{
BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
*s = (BdrvReplaceChildState) {
- .child = *childp,
- .childp = new_bs == NULL ? childp : NULL,
- .old_bs = (*childp)->bs,
+ .child = child,
+ .old_bs = child->bs,
};
tran_add(tran, &bdrv_replace_child_drv, s);
if (new_bs) {
bdrv_ref(new_bs);
}
- bdrv_replace_child_noperm(childp, new_bs);
- /* old_bs reference is transparently moved from *childp to @s */
+ bdrv_replace_child_noperm(&child, new_bs);
+ /* old_bs reference is transparently moved from @child to @s */
}
/*
@@ -5041,7 +5008,6 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
typedef struct BdrvRemoveFilterOrCowChild {
BdrvChild *child;
- BlockDriverState *bs;
bool is_backing;
} BdrvRemoveFilterOrCowChild;
@@ -5071,19 +5037,10 @@ static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
bdrv_child_free(s->child);
}
-static void bdrv_remove_filter_or_cow_child_clean(void *opaque)
-{
- BdrvRemoveFilterOrCowChild *s = opaque;
-
- /* Drop the bs reference after the transaction is done */
- bdrv_unref(s->bs);
- g_free(s);
-}
-
static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
.abort = bdrv_remove_filter_or_cow_child_abort,
.commit = bdrv_remove_filter_or_cow_child_commit,
- .clean = bdrv_remove_filter_or_cow_child_clean,
+ .clean = g_free,
};
/*
@@ -5101,11 +5058,6 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
return;
}
- /*
- * Keep a reference to @bs so @childp will stay valid throughout the
- * transaction (required by bdrv_replace_child_tran())
- */
- bdrv_ref(bs);
if (child == bs->backing) {
childp = &bs->backing;
} else if (child == bs->file) {
@@ -5115,13 +5067,12 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
}
if (child->bs) {
- bdrv_replace_child_tran(childp, NULL, tran);
+ bdrv_replace_child_tran(*childp, NULL, tran);
}
s = g_new(BdrvRemoveFilterOrCowChild, 1);
*s = (BdrvRemoveFilterOrCowChild) {
.child = child,
- .bs = bs,
.is_backing = (childp == &bs->backing),
};
tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
@@ -5147,7 +5098,6 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
{
BdrvChild *c, *next;
- assert(to != NULL);
GLOBAL_STATE_CODE();
QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
@@ -5165,12 +5115,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
c->name, from->node_name);
return -EPERM;
}
-
- /*
- * Passing a pointer to the local variable @c is fine here, because
- * @to is not NULL, and so &c will not be attached to the transaction.
- */
- bdrv_replace_child_tran(&c, to, tran);
+ bdrv_replace_child_tran(c, to, tran);
}
return 0;
@@ -5185,8 +5130,6 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
*
* With @detach_subchain=true @to must be in a backing chain of @from. In this
* case backing link of the cow-parent of @to is removed.
- *
- * @to must not be NULL.
*/
static int bdrv_replace_node_common(BlockDriverState *from,
BlockDriverState *to,
@@ -5200,7 +5143,6 @@ static int bdrv_replace_node_common(BlockDriverState *from,
int ret;
GLOBAL_STATE_CODE();
- assert(to != NULL);
if (detach_subchain) {
assert(bdrv_chain_contains(from, to));
@@ -5257,9 +5199,6 @@ out:
return ret;
}
-/**
- * Replace node @from by @to (where neither may be NULL).
- */
int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
Error **errp)
{
@@ -5335,7 +5274,7 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
bdrv_drained_begin(old_bs);
bdrv_drained_begin(new_bs);
- bdrv_replace_child_tran(&child, new_bs, tran);
+ bdrv_replace_child_tran(child, new_bs, tran);
found = g_hash_table_new(NULL, NULL);
refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 11/15] Revert "block: Restructure remove_file_or_backing_child()"
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 10/15] Revert "block: Let replace_child_tran keep indirect pointer" Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 12/15] Revert "block: Pass BdrvChild ** to replace_child_noperm" Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
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>
---
block.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index a83845b120..3ac332a460 100644
--- a/block.c
+++ b/block.c
@@ -5051,33 +5051,30 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
BdrvChild *child,
Transaction *tran)
{
- BdrvChild **childp;
BdrvRemoveFilterOrCowChild *s;
+ assert(child == bs->backing || child == bs->file);
+
if (!child) {
return;
}
- if (child == bs->backing) {
- childp = &bs->backing;
- } else if (child == bs->file) {
- childp = &bs->file;
- } else {
- g_assert_not_reached();
- }
-
if (child->bs) {
- bdrv_replace_child_tran(*childp, NULL, tran);
+ bdrv_replace_child_tran(child, NULL, tran);
}
s = g_new(BdrvRemoveFilterOrCowChild, 1);
*s = (BdrvRemoveFilterOrCowChild) {
.child = child,
- .is_backing = (childp == &bs->backing),
+ .is_backing = (child == bs->backing),
};
tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
- *childp = NULL;
+ if (s->is_backing) {
+ bs->backing = NULL;
+ } else {
+ bs->file = NULL;
+ }
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 12/15] Revert "block: Pass BdrvChild ** to replace_child_noperm"
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 11/15] Revert "block: Restructure remove_file_or_backing_child()" Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 13/15] block: Manipulate bs->file / bs->backing pointers in .attach/.detach Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
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>
---
block.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index 3ac332a460..ca86cd86d3 100644
--- a/block.c
+++ b/block.c
@@ -90,7 +90,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
static bool bdrv_recurse_has_child(BlockDriverState *bs,
BlockDriverState *child);
-static void bdrv_replace_child_noperm(BdrvChild **child,
+static void bdrv_replace_child_noperm(BdrvChild *child,
BlockDriverState *new_bs);
static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
BdrvChild *child,
@@ -2352,7 +2352,7 @@ static void bdrv_replace_child_abort(void *opaque)
GLOBAL_STATE_CODE();
/* old_bs reference is transparently moved from @s to @s->child */
- bdrv_replace_child_noperm(&s->child, s->old_bs);
+ bdrv_replace_child_noperm(s->child, s->old_bs);
bdrv_unref(new_bs);
}
@@ -2382,7 +2382,7 @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
if (new_bs) {
bdrv_ref(new_bs);
}
- bdrv_replace_child_noperm(&child, new_bs);
+ bdrv_replace_child_noperm(child, new_bs);
/* old_bs reference is transparently moved from @child to @s */
}
@@ -2764,10 +2764,9 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
return permissions[qapi_perm];
}
-static void bdrv_replace_child_noperm(BdrvChild **childp,
+static void bdrv_replace_child_noperm(BdrvChild *child,
BlockDriverState *new_bs)
{
- BdrvChild *child = *childp;
BlockDriverState *old_bs = child->bs;
int new_bs_quiesce_counter;
int drain_saldo;
@@ -2865,7 +2864,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
BlockDriverState *bs = child->bs;
GLOBAL_STATE_CODE();
- bdrv_replace_child_noperm(s->child, NULL);
+ bdrv_replace_child_noperm(child, NULL);
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
@@ -2966,7 +2965,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
}
bdrv_ref(child_bs);
- bdrv_replace_child_noperm(&new_child, child_bs);
+ bdrv_replace_child_noperm(new_child, child_bs);
*child = new_child;
@@ -3022,13 +3021,13 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
return 0;
}
-static void bdrv_detach_child(BdrvChild **childp)
+static void bdrv_detach_child(BdrvChild *child)
{
- BlockDriverState *old_bs = (*childp)->bs;
+ BlockDriverState *old_bs = child->bs;
GLOBAL_STATE_CODE();
- bdrv_replace_child_noperm(childp, NULL);
- bdrv_child_free(*childp);
+ bdrv_replace_child_noperm(child, NULL);
+ bdrv_child_free(child);
if (old_bs) {
/*
@@ -3140,7 +3139,7 @@ void bdrv_root_unref_child(BdrvChild *child)
GLOBAL_STATE_CODE();
child_bs = child->bs;
- bdrv_detach_child(&child);
+ bdrv_detach_child(child);
bdrv_unref(child_bs);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 13/15] block: Manipulate bs->file / bs->backing pointers in .attach/.detach
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 12/15] Revert "block: Pass BdrvChild ** to replace_child_noperm" Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 14/15] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov, John Snow
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>
---
block.c | 234 ++++++++++++++-----------------
block/raw-format.c | 4 +-
block/snapshot-access.c | 6 +-
block/snapshot.c | 1 -
include/block/block_int-common.h | 15 +-
tests/unit/test-bdrv-drain.c | 10 +-
6 files changed, 126 insertions(+), 144 deletions(-)
diff --git a/block.c b/block.c
index ca86cd86d3..6b08d20d8c 100644
--- a/block.c
+++ b/block.c
@@ -1438,9 +1438,39 @@ static void bdrv_child_cb_attach(BdrvChild *child)
assert_bdrv_graph_writable(bs);
QLIST_INSERT_HEAD(&bs->children, child, next);
-
- if (child->role & BDRV_CHILD_COW) {
+ if (bs->drv->is_filter || (child->role & BDRV_CHILD_FILTERED)) {
+ /*
+ * Here we handle filters and block/raw-format.c when it behave like
+ * filter. They generally have a single PRIMARY child, which is also the
+ * FILTERED child, and that they may have multiple more children, which
+ * are neither PRIMARY nor FILTERED. And never we have a COW child here.
+ * So bs->file will be the PRIMARY child, unless the PRIMARY child goes
+ * into bs->backing on exceptional cases; and bs->backing will be
+ * nothing else.
+ */
+ assert(!(child->role & BDRV_CHILD_COW));
+ if (child->role & BDRV_CHILD_PRIMARY) {
+ assert(child->role & BDRV_CHILD_FILTERED);
+ assert(!bs->backing);
+ assert(!bs->file);
+
+ if (bs->drv->filtered_child_is_backing) {
+ bs->backing = child;
+ } else {
+ bs->file = child;
+ }
+ } else {
+ assert(!(child->role & BDRV_CHILD_FILTERED));
+ }
+ } else if (child->role & BDRV_CHILD_COW) {
+ assert(bs->drv->supports_backing);
+ assert(!(child->role & BDRV_CHILD_PRIMARY));
+ assert(!bs->backing);
+ bs->backing = child;
bdrv_backing_attach(child);
+ } else if (child->role & BDRV_CHILD_PRIMARY) {
+ assert(!bs->file);
+ bs->file = child;
}
bdrv_apply_subtree_drain(child, bs);
@@ -1458,6 +1488,12 @@ static void bdrv_child_cb_detach(BdrvChild *child)
assert_bdrv_graph_writable(bs);
QLIST_REMOVE(child, next);
+ if (child == bs->backing) {
+ assert(child != bs->file);
+ bs->backing = NULL;
+ } else if (child == bs->file) {
+ bs->file = NULL;
+ }
}
static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
@@ -1663,7 +1699,7 @@ open_failed:
bs->drv = NULL;
if (bs->file != NULL) {
bdrv_unref_child(bs, bs->file);
- bs->file = NULL;
+ assert(!bs->file);
}
g_free(bs->opaque);
bs->opaque = NULL;
@@ -2852,7 +2888,7 @@ static void bdrv_child_free(BdrvChild *child)
}
typedef struct BdrvAttachChildCommonState {
- BdrvChild **child;
+ BdrvChild *child;
AioContext *old_parent_ctx;
AioContext *old_child_ctx;
} BdrvAttachChildCommonState;
@@ -2860,33 +2896,31 @@ typedef struct BdrvAttachChildCommonState {
static void bdrv_attach_child_common_abort(void *opaque)
{
BdrvAttachChildCommonState *s = opaque;
- BdrvChild *child = *s->child;
- BlockDriverState *bs = child->bs;
+ BlockDriverState *bs = s->child->bs;
GLOBAL_STATE_CODE();
- bdrv_replace_child_noperm(child, NULL);
+ bdrv_replace_child_noperm(s->child, NULL);
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
}
- if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
+ if (bdrv_child_get_parent_aio_context(s->child) != s->old_parent_ctx) {
GSList *ignore;
/* No need to ignore `child`, because it has been detached already */
ignore = NULL;
- child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
- &error_abort);
+ s->child->klass->can_set_aio_ctx(s->child, s->old_parent_ctx, &ignore,
+ &error_abort);
g_slist_free(ignore);
ignore = NULL;
- child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
+ s->child->klass->set_aio_ctx(s->child, s->old_parent_ctx, &ignore);
g_slist_free(ignore);
}
bdrv_unref(bs);
- bdrv_child_free(child);
- *s->child = NULL;
+ bdrv_child_free(s->child);
}
static TransactionActionDrv bdrv_attach_child_common_drv = {
@@ -2897,28 +2931,22 @@ static TransactionActionDrv bdrv_attach_child_common_drv = {
/*
* Common part of attaching bdrv child to bs or to blk or to job
*
- * Resulting new child is returned through @child.
- * At start *@child must be NULL.
- * @child is saved to a new entry of @tran, so that *@child could be reverted to
- * NULL on abort(). So referenced variable must live at least until transaction
- * end.
- *
* Function doesn't update permissions, caller is responsible for this.
+ *
+ * Returns new created child.
*/
-static int bdrv_attach_child_common(BlockDriverState *child_bs,
- const char *child_name,
- const BdrvChildClass *child_class,
- BdrvChildRole child_role,
- uint64_t perm, uint64_t shared_perm,
- void *opaque, BdrvChild **child,
- Transaction *tran, Error **errp)
+static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
+ const char *child_name,
+ const BdrvChildClass *child_class,
+ BdrvChildRole child_role,
+ uint64_t perm, uint64_t shared_perm,
+ void *opaque,
+ Transaction *tran, Error **errp)
{
BdrvChild *new_child;
AioContext *parent_ctx;
AioContext *child_ctx = bdrv_get_aio_context(child_bs);
- assert(child);
- assert(*child == NULL);
assert(child_class->get_parent_desc);
GLOBAL_STATE_CODE();
@@ -2960,42 +2988,35 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
if (ret < 0) {
error_propagate(errp, local_err);
bdrv_child_free(new_child);
- return ret;
+ return NULL;
}
}
bdrv_ref(child_bs);
bdrv_replace_child_noperm(new_child, child_bs);
- *child = new_child;
-
BdrvAttachChildCommonState *s = g_new(BdrvAttachChildCommonState, 1);
*s = (BdrvAttachChildCommonState) {
- .child = child,
+ .child = new_child,
.old_parent_ctx = parent_ctx,
.old_child_ctx = child_ctx,
};
tran_add(tran, &bdrv_attach_child_common_drv, s);
- return 0;
+ return new_child;
}
/*
- * Variable referenced by @child must live at least until transaction end.
- * (see bdrv_attach_child_common() doc for details)
- *
* Function doesn't update permissions, caller is responsible for this.
*/
-static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
- BlockDriverState *child_bs,
- const char *child_name,
- const BdrvChildClass *child_class,
- BdrvChildRole child_role,
- BdrvChild **child,
- Transaction *tran,
- Error **errp)
+static BdrvChild *bdrv_attach_child_noperm(BlockDriverState *parent_bs,
+ BlockDriverState *child_bs,
+ const char *child_name,
+ const BdrvChildClass *child_class,
+ BdrvChildRole child_role,
+ Transaction *tran,
+ Error **errp)
{
- int ret;
uint64_t perm, shared_perm;
assert(parent_bs->drv);
@@ -3004,21 +3025,16 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
if (bdrv_recurse_has_child(child_bs, parent_bs)) {
error_setg(errp, "Making '%s' a %s child of '%s' would create a cycle",
child_bs->node_name, child_name, parent_bs->node_name);
- return -EINVAL;
+ return NULL;
}
bdrv_get_cumulative_perm(parent_bs, &perm, &shared_perm);
bdrv_child_perm(parent_bs, child_bs, NULL, child_role, NULL,
perm, shared_perm, &perm, &shared_perm);
- ret = bdrv_attach_child_common(child_bs, child_name, child_class,
- child_role, perm, shared_perm, parent_bs,
- child, tran, errp);
- if (ret < 0) {
- return ret;
- }
-
- return 0;
+ return bdrv_attach_child_common(child_bs, child_name, child_class,
+ child_role, perm, shared_perm, parent_bs,
+ tran, errp);
}
static void bdrv_detach_child(BdrvChild *child)
@@ -3063,15 +3079,16 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
void *opaque, Error **errp)
{
int ret;
- BdrvChild *child = NULL;
+ BdrvChild *child;
Transaction *tran = tran_new();
GLOBAL_STATE_CODE();
- ret = bdrv_attach_child_common(child_bs, child_name, child_class,
+ child = bdrv_attach_child_common(child_bs, child_name, child_class,
child_role, perm, shared_perm, opaque,
- &child, tran, errp);
- if (ret < 0) {
+ tran, errp);
+ if (!child) {
+ ret = -EINVAL;
goto out;
}
@@ -3079,11 +3096,10 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
out:
tran_finalize(tran, ret);
- /* child is unset on failure by bdrv_attach_child_common_abort() */
- assert((ret < 0) == !child);
bdrv_unref(child_bs);
- return child;
+
+ return ret < 0 ? NULL : child;
}
/*
@@ -3105,14 +3121,15 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
Error **errp)
{
int ret;
- BdrvChild *child = NULL;
+ BdrvChild *child;
Transaction *tran = tran_new();
GLOBAL_STATE_CODE();
- ret = bdrv_attach_child_noperm(parent_bs, child_bs, child_name, child_class,
- child_role, &child, tran, errp);
- if (ret < 0) {
+ child = bdrv_attach_child_noperm(parent_bs, child_bs, child_name,
+ child_class, child_role, tran, errp);
+ if (!child) {
+ ret = -EINVAL;
goto out;
}
@@ -3123,12 +3140,10 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
out:
tran_finalize(tran, ret);
- /* child is unset on failure by bdrv_attach_child_common_abort() */
- assert((ret < 0) == !child);
bdrv_unref(child_bs);
- return child;
+ return ret < 0 ? NULL : child;
}
/* Callers must ensure that child->frozen is false. */
@@ -3270,7 +3285,6 @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
bool is_backing,
Transaction *tran, Error **errp)
{
- int ret = 0;
bool update_inherits_from =
bdrv_inherits_from_recursive(child_bs, parent_bs);
BdrvChild *child = is_backing ? parent_bs->backing : parent_bs->file;
@@ -3328,14 +3342,12 @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
goto out;
}
- ret = bdrv_attach_child_noperm(parent_bs, child_bs,
- is_backing ? "backing" : "file",
- &child_of_bds, role,
- is_backing ? &parent_bs->backing :
- &parent_bs->file,
- tran, errp);
- if (ret < 0) {
- return ret;
+ child = bdrv_attach_child_noperm(parent_bs, child_bs,
+ is_backing ? "backing" : "file",
+ &child_of_bds, role,
+ tran, errp);
+ if (!child) {
+ return -EINVAL;
}
@@ -3591,14 +3603,16 @@ int bdrv_open_file_child(const char *filename,
/* commit_top and mirror_top don't use this function */
assert(!parent->drv->filtered_child_is_backing);
-
role = parent->drv->is_filter ?
(BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY) : BDRV_CHILD_IMAGE;
- parent->file = bdrv_open_child(filename, options, bdref_key, parent,
- &child_of_bds, role, false, errp);
+ if (!bdrv_open_child(filename, options, bdref_key, parent,
+ &child_of_bds, role, false, errp))
+ {
+ return -EINVAL;
+ }
- return parent->file ? 0 : -EINVAL;
+ return 0;
}
/*
@@ -4873,8 +4887,8 @@ static void bdrv_close(BlockDriverState *bs)
bdrv_unref_child(bs, child);
}
- bs->backing = NULL;
- bs->file = NULL;
+ assert(!bs->backing);
+ assert(!bs->file);
g_free(bs->opaque);
bs->opaque = NULL;
qatomic_set(&bs->copy_on_read, 0);
@@ -5005,41 +5019,14 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
return ret;
}
-typedef struct BdrvRemoveFilterOrCowChild {
- BdrvChild *child;
- bool is_backing;
-} BdrvRemoveFilterOrCowChild;
-
-static void bdrv_remove_filter_or_cow_child_abort(void *opaque)
-{
- BdrvRemoveFilterOrCowChild *s = opaque;
- BlockDriverState *parent_bs = s->child->opaque;
-
- if (s->is_backing) {
- parent_bs->backing = s->child;
- } else {
- parent_bs->file = s->child;
- }
-
- /*
- * We don't have to restore child->bs here to undo bdrv_replace_child_tran()
- * because that function is transactionable and it registered own completion
- * entries in @tran, so .abort() for bdrv_replace_child_safe() will be
- * called automatically.
- */
-}
-
static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
{
- BdrvRemoveFilterOrCowChild *s = opaque;
GLOBAL_STATE_CODE();
- bdrv_child_free(s->child);
+ bdrv_child_free(opaque);
}
static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
- .abort = bdrv_remove_filter_or_cow_child_abort,
.commit = bdrv_remove_filter_or_cow_child_commit,
- .clean = g_free,
};
/*
@@ -5050,8 +5037,6 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
BdrvChild *child,
Transaction *tran)
{
- BdrvRemoveFilterOrCowChild *s;
-
assert(child == bs->backing || child == bs->file);
if (!child) {
@@ -5062,18 +5047,7 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
bdrv_replace_child_tran(child, NULL, tran);
}
- s = g_new(BdrvRemoveFilterOrCowChild, 1);
- *s = (BdrvRemoveFilterOrCowChild) {
- .child = child,
- .is_backing = (child == bs->backing),
- };
- tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
-
- if (s->is_backing) {
- bs->backing = NULL;
- } else {
- bs->file = NULL;
- }
+ tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, child);
}
/*
@@ -5227,16 +5201,18 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
Error **errp)
{
int ret;
+ BdrvChild *child;
Transaction *tran = tran_new();
GLOBAL_STATE_CODE();
assert(!bs_new->backing);
- ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
- &child_of_bds, bdrv_backing_role(bs_new),
- &bs_new->backing, tran, errp);
- if (ret < 0) {
+ child = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
+ &child_of_bds, bdrv_backing_role(bs_new),
+ tran, errp);
+ if (!child) {
+ ret = -EINVAL;
goto out;
}
diff --git a/block/raw-format.c b/block/raw-format.c
index 69fd650eaf..d8ca8ee3a9 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -457,8 +457,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
file_role = BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY;
}
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- file_role, false, errp);
+ bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
+ file_role, false, errp);
if (!bs->file) {
return -EINVAL;
}
diff --git a/block/snapshot-access.c b/block/snapshot-access.c
index 77b87c1946..0a30ec6cd9 100644
--- a/block/snapshot-access.c
+++ b/block/snapshot-access.c
@@ -82,9 +82,9 @@ static void snapshot_access_refresh_filename(BlockDriverState *bs)
static int snapshot_access_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- BDRV_CHILD_DATA | BDRV_CHILD_PRIMARY,
- false, errp);
+ bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
+ BDRV_CHILD_DATA | BDRV_CHILD_PRIMARY,
+ false, errp);
if (!bs->file) {
return -EINVAL;
}
diff --git a/block/snapshot.c b/block/snapshot.c
index 75e8d3a937..f3971ac2bd 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -288,7 +288,6 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
/* .bdrv_open() will re-attach it */
bdrv_unref_child(bs, *fallback_ptr);
- *fallback_ptr = NULL;
ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index d68adc6ff3..c4d8b11dbb 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1056,9 +1056,6 @@ struct BlockDriverState {
QDict *full_open_options;
char exact_filename[PATH_MAX];
- BdrvChild *backing;
- BdrvChild *file;
-
/* I/O Limits */
BlockLimits bl;
@@ -1117,7 +1114,19 @@ struct BlockDriverState {
* parent node of this node.
*/
BlockDriverState *inherits_from;
+
+ /*
+ * @backing and @file are some of @children or NULL. All these three fields
+ * (@file, @backing and @children) are modified only in
+ * bdrv_child_cb_attach() and bdrv_child_cb_detach().
+ *
+ * See also comment in include/block/block.h, to learn how backing and file
+ * are connected with BdrvChildRole.
+ */
QLIST_HEAD(, BdrvChild) children;
+ BdrvChild *backing;
+ BdrvChild *file;
+
QLIST_HEAD(, BdrvChild) parents;
QDict *options;
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 23d425a494..4cf99edf5b 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1808,9 +1808,8 @@ static void test_drop_intermediate_poll(void)
for (i = 0; i < 3; i++) {
if (i) {
/* Takes the reference to chain[i - 1] */
- chain[i]->backing = bdrv_attach_child(chain[i], chain[i - 1],
- "chain", &chain_child_class,
- BDRV_CHILD_COW, &error_abort);
+ bdrv_attach_child(chain[i], chain[i - 1], "chain",
+ &chain_child_class, BDRV_CHILD_COW, &error_abort);
}
}
@@ -2028,9 +2027,8 @@ static void do_test_replace_child_mid_drain(int old_drain_count,
new_child_bs->total_sectors = 1;
bdrv_ref(old_child_bs);
- parent_bs->backing = bdrv_attach_child(parent_bs, old_child_bs, "child",
- &child_of_bds, BDRV_CHILD_COW,
- &error_abort);
+ bdrv_attach_child(parent_bs, old_child_bs, "child", &child_of_bds,
+ BDRV_CHILD_COW, &error_abort);
for (i = 0; i < old_drain_count; i++) {
bdrv_drained_begin(old_child_bs);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 14/15] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (12 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 13/15] block: Manipulate bs->file / bs->backing pointers in .attach/.detach Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-24 21:28 ` [PATCH v6 15/15] block: refactor bdrv_remove_file_or_backing_child to bdrv_remove_child Vladimir Sementsov-Ogievskiy
2022-06-30 13:18 ` [PATCH v6 00/15] block: cleanup backing and file handling Hanna Reitz
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
Now the indirection is not actually used, we can safely reduce it to
simple pointer. For consistency do a bit of refactoring to get rid of
_ptr suffixes that become meaningless.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/snapshot.c | 38 ++++++++++++++++----------------------
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index f3971ac2bd..e22ac3eac6 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -151,34 +151,29 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
}
/**
- * Return a pointer to the child BDS pointer to which we can fall
+ * Return a pointer to child of given BDS to which we can fall
* back if the given BDS does not support snapshots.
* Return NULL if there is no BDS to (safely) fall back to.
- *
- * We need to return an indirect pointer because bdrv_snapshot_goto()
- * has to modify the BdrvChild pointer.
*/
-static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
+static BdrvChild *bdrv_snapshot_fallback_child(BlockDriverState *bs)
{
- BdrvChild **fallback;
- BdrvChild *child = bdrv_primary_child(bs);
+ BdrvChild *fallback = bdrv_primary_child(bs);
+ BdrvChild *child;
/* We allow fallback only to primary child */
- if (!child) {
+ if (!fallback) {
return NULL;
}
- fallback = (child == bs->file ? &bs->file : &bs->backing);
- assert(*fallback == child);
/*
* Check that there are no other children that would need to be
* snapshotted. If there are, it is not safe to fall back to
- * *fallback.
+ * fallback.
*/
QLIST_FOREACH(child, &bs->children, next) {
if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
BDRV_CHILD_FILTERED) &&
- child != *fallback)
+ child != fallback)
{
return NULL;
}
@@ -189,8 +184,7 @@ static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
{
- BdrvChild **child_ptr = bdrv_snapshot_fallback_ptr(bs);
- return child_ptr ? (*child_ptr)->bs : NULL;
+ return child_bs(bdrv_snapshot_fallback_child(bs));
}
int bdrv_can_snapshot(BlockDriverState *bs)
@@ -237,7 +231,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
Error **errp)
{
BlockDriver *drv = bs->drv;
- BdrvChild **fallback_ptr;
+ BdrvChild *fallback;
int ret, open_ret;
GLOBAL_STATE_CODE();
@@ -260,13 +254,13 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret;
}
- fallback_ptr = bdrv_snapshot_fallback_ptr(bs);
- if (fallback_ptr) {
+ fallback = bdrv_snapshot_fallback_child(bs);
+ if (fallback) {
QDict *options;
QDict *file_options;
Error *local_err = NULL;
- BlockDriverState *fallback_bs = (*fallback_ptr)->bs;
- char *subqdict_prefix = g_strdup_printf("%s.", (*fallback_ptr)->name);
+ BlockDriverState *fallback_bs = fallback->bs;
+ char *subqdict_prefix = g_strdup_printf("%s.", fallback->name);
options = qdict_clone_shallow(bs->options);
@@ -277,8 +271,8 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
qobject_unref(file_options);
g_free(subqdict_prefix);
- /* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr */
- qdict_put_str(options, (*fallback_ptr)->name,
+ /* Force .bdrv_open() below to re-attach fallback_bs on fallback */
+ qdict_put_str(options, fallback->name,
bdrv_get_node_name(fallback_bs));
/* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
@@ -287,7 +281,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
}
/* .bdrv_open() will re-attach it */
- bdrv_unref_child(bs, *fallback_ptr);
+ bdrv_unref_child(bs, fallback);
ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 15/15] block: refactor bdrv_remove_file_or_backing_child to bdrv_remove_child
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (13 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 14/15] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr Vladimir Sementsov-Ogievskiy
@ 2022-06-24 21:28 ` Vladimir Sementsov-Ogievskiy
2022-06-30 13:18 ` [PATCH v6 00/15] block: cleanup backing and file handling Hanna Reitz
15 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-06-24 21:28 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, kwolf, hreitz, vsementsov
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>
---
block.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/block.c b/block.c
index 6b08d20d8c..ddd043f556 100644
--- a/block.c
+++ b/block.c
@@ -92,9 +92,7 @@ static bool bdrv_recurse_has_child(BlockDriverState *bs,
static void bdrv_replace_child_noperm(BdrvChild *child,
BlockDriverState *new_bs);
-static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
- BdrvChild *child,
- Transaction *tran);
+static void bdrv_remove_child(BdrvChild *child, Transaction *tran);
static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
Transaction *tran);
@@ -3335,7 +3333,7 @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
if (child) {
bdrv_unset_inherits_from(parent_bs, child, tran);
- bdrv_remove_file_or_backing_child(parent_bs, child, tran);
+ bdrv_remove_child(child, tran);
}
if (!child_bs) {
@@ -5019,26 +5017,19 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
return ret;
}
-static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
+static void bdrv_remove_child_commit(void *opaque)
{
GLOBAL_STATE_CODE();
bdrv_child_free(opaque);
}
-static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
- .commit = bdrv_remove_filter_or_cow_child_commit,
+static TransactionActionDrv bdrv_remove_child_drv = {
+ .commit = bdrv_remove_child_commit,
};
-/*
- * A function to remove backing or file child of @bs.
- * Function doesn't update permissions, caller is responsible for this.
- */
-static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
- BdrvChild *child,
- Transaction *tran)
+/* Function doesn't update permissions, caller is responsible for this. */
+static void bdrv_remove_child(BdrvChild *child, Transaction *tran)
{
- assert(child == bs->backing || child == bs->file);
-
if (!child) {
return;
}
@@ -5047,7 +5038,7 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
bdrv_replace_child_tran(child, NULL, tran);
}
- tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, child);
+ tran_add(tran, &bdrv_remove_child_drv, child);
}
/*
@@ -5058,7 +5049,7 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
Transaction *tran)
{
- bdrv_remove_file_or_backing_child(bs, bdrv_filter_or_cow_child(bs), tran);
+ bdrv_remove_child(bdrv_filter_or_cow_child(bs), tran);
}
static int bdrv_replace_node_noperm(BlockDriverState *from,
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 00/15] block: cleanup backing and file handling
2022-06-24 21:28 [PATCH v6 00/15] block: cleanup backing and file handling Vladimir Sementsov-Ogievskiy
` (14 preceding siblings ...)
2022-06-24 21:28 ` [PATCH v6 15/15] block: refactor bdrv_remove_file_or_backing_child to bdrv_remove_child Vladimir Sementsov-Ogievskiy
@ 2022-06-30 13:18 ` Hanna Reitz
15 siblings, 0 replies; 17+ messages in thread
From: Hanna Reitz @ 2022-06-30 13:18 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: qemu-devel, kwolf
On 24.06.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> That's the first part of
> "[PATCH v5 00/45] Transactional block-graph modifying API",
> updated and almost reviewed.
>
> On commit (15) is added to original scope of
> "block: cleanup backing and file handling", as it's related.
>
> 01: add Hanna's r-b
> 02: - mention snapshot-access in commit msg
> - return ret in compress_open instead of EINVAL
> - add Hanna's r-b
> 03: add Hanna's r-b
> 04: - add case in commit msg
> - fix comments
> 05: - fix type in commit msg
> - add Hanna's r-b
> 06: add Hanna's r-b
> 07: wording improvements
> 08: - fix wording
> - add Hanna's r-b
>
> 09: I add the description, whey we allow a degradation. Still,
> up to maintainers: it's OK to merge 09-13 into one bit commit
>
> 13: - fix s/|/||/
> - improve comment
> - more readable logic when handle filters in bdrv_child_cb_attach()
> - don't keep **child indirection, move to just returning a child ptr
> (honestly, I didn't analyze all the callers do they need this int value. Do you think it's needed?)
> - handle snapshot-access.c
> 14: get rid of _ptr
> 15: update comment
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Patch 2 needs to be rebased on 79ef0cebb5694411e7452f0cf15c4bd170c7f2d6,
but that should be straightforward.
^ permalink raw reply [flat|nested] 17+ messages in thread