[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 13/45] block: Manipulate bs->file / bs->backing pointers i

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v5 13/45] block: Manipulate bs->file / bs->backing pointers in .attach/.detach
Date: Thu, 9 Jun 2022 16:40:05 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1

On 6/7/22 18:55, Hanna Reitz wrote:
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

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

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
- 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\>'


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)
      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 
- * 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.

Yes, "not" is missing, sorry)

I’d also like to ask for this to be even more clear, e.g. by adding a sentence 
“When this transaction is aborted, the pointer stored in *@child becomes 


+ * 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.)

Hmm. Seems reasonable. I'll check if we can move to simply return the child.

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]