On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
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.
I think this was reproducible (rarely) with 030, but I can’t reproduce it now.
Oh well.
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
Sounds correct.
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 still keep BdrvChild** argument to return the child and int return
value, and not move to simply returning BdrvChild*, as we don't want to
lose int return values.
However we don't require *@child to be NULL anymore, and even allow
@child to be NULL, if caller don't need the new child pointer.
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\>'
Awesome.
block/snapshot-access.c needs a touchup, but other than that, this still seems
to hold.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
block.c | 156 ++++++++++++++-----------------
block/raw-format.c | 4 +-
block/snapshot.c | 1 -
include/block/block_int-common.h | 15 ++-
tests/unit/test-bdrv-drain.c | 10 +-
5 files changed, 89 insertions(+), 97 deletions(-)
diff --git a/block.c b/block.c
index 8e8ed639fe..6b43e101a1 100644
--- a/block.c
+++ b/block.c
@@ -1438,9 +1438,33 @@ 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)) {
Should be `||`.
+ /*
+ * Here we handle filters and block/raw-format.c when it behave like
+ * filter.
I’d like this comment to expand on how they are handled.
For example, that they generally have a single PRIMARY child, which is also the FILTERED
child, and that they may have multiple more children, but none of them will be a COW
child. 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. (Which is
why we ignore all other children.)
+ */
+ assert(!(child->role & BDRV_CHILD_COW));
+ if (child->role & (BDRV_CHILD_PRIMARY | BDRV_CHILD_FILTERED)) {
Why do we check for FILTERED here? It appears to me that PRIMARY is the flag that
tells us to put this child into bs->file (but for filters, sometimes we have to
make an exception and put it into bs->backing).
Is the check for FILTERED just a safeguard, so that filter drivers always set the two
in tandem? If so, I’d make the condition just `role & PRIMARY` and then in an
`else` path assert that `!(role & FILTERED)`.
+ assert(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;
+ }
+ }
[...]
@@ -2897,11 +2925,11 @@ 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.
+ * If @child is not NULL, it's set to new created child. Note, that @child
+ * pointer is stored in the transaction and therefore not cleared on abort.
I can’t quite parse this comment. It doesn’t look like `child` is stored in
the transaction. I mean, `new_child` is, which is what `*child` is, but
there’s a difference between `@child` and `*child` (or `*@child`) after all.
Or is there a “not” missing, i.e. “that the @child pointer is not stored in the
transaction”? That would also make more sense, why it isn’t cleared on abort.
+ * Consider @child as part of return value: we may change the return value of
+ * the function to BdrvChild* and return child directly, but this way we lose
+ * different return codes.
I mean, do we even care about return codes? I hope not, but maybe we do? We
do have `errp` for a description, and I think the only distinction we make in
the block layer based on error codes is ENOSPC vs. anything else. I hope this
function never returns ENOSPC, so I think the return value shouldn’t matter.
(I can understand that it seems like a loss if we can no longer decide between
e.g. EINVAL and EPERM, but I don’t think it really is. We could just make it
EINVAL always and it shouldn’t matter.)