Re: [PATCH v5 13/45] block: Manipulate bs->file / bs->backing pointers in .attach/.detach

[Hanna Reitz]

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.

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 invalid.”

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

>    *
>    * Function doesn't update permissions, caller is responsible for this.
>    */