Re: [PATCH v2 12/36] block: refactor bdrv_child* permission functions

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: qemu-block@nongnu.org, armbru@redhat.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v2 12/36] block: refactor bdrv_child* permission functions
Date: Wed, 20 Jan 2021 10:09:54 +0100	[thread overview]
Message-ID: <20210120090954.GB5521@merkur.fritz.box> (raw)
In-Reply-To: <0b634f73-b997-d72d-e7ab-3a1dcdfeb509@virtuozzo.com>

Am 19.01.2021 um 19:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 19.01.2021 21:09, Kevin Wolf wrote:
> > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Split out non-recursive parts, and refactor as block graph transaction
> > > action.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > >   block.c | 79 ++++++++++++++++++++++++++++++++++++++++++---------------
> > >   1 file changed, 59 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index a756f3e8ad..7267b4a3e9 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -48,6 +48,7 @@
> > >   #include "qemu/timer.h"
> > >   #include "qemu/cutils.h"
> > >   #include "qemu/id.h"
> > > +#include "qemu/transactions.h"
> > >   #include "block/coroutines.h"
> > >   #ifdef CONFIG_BSD
> > > @@ -2033,6 +2034,61 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
> > >       }
> > >   }
> > > +static void bdrv_child_set_perm_commit(void *opaque)
> > > +{
> > > +    BdrvChild *c = opaque;
> > > +
> > > +    c->has_backup_perm = false;
> > > +}
> > > +
> > > +static void bdrv_child_set_perm_abort(void *opaque)
> > > +{
> > > +    BdrvChild *c = opaque;
> > > +    /*
> > > +     * We may have child->has_backup_perm unset at this point, as in case of
> > > +     * _check_ stage of permission update failure we may _check_ not the whole
> > > +     * subtree.  Still, _abort_ is called on the whole subtree anyway.
> > > +     */
> > > +    if (c->has_backup_perm) {
> > > +        c->perm = c->backup_perm;
> > > +        c->shared_perm = c->backup_shared_perm;
> > > +        c->has_backup_perm = false;
> > > +    }
> > > +}
> > > +
> > > +static TransactionActionDrv bdrv_child_set_pem_drv = {
> > > +    .abort = bdrv_child_set_perm_abort,
> > > +    .commit = bdrv_child_set_perm_commit,
> > > +};
> > > +
> > > +/*
> > > + * With tran=NULL needs to be followed by direct call to either
> > > + * bdrv_child_set_perm_commit() or bdrv_child_set_perm_abort().
> > > + *
> > > + * With non-NUll tran needs to be followed by tran_abort() or tran_commit()
> > 
> > s/NUll/NULL/
> > 
> > > + * instead.
> > > + */
> > > +static void bdrv_child_set_perm_safe(BdrvChild *c, uint64_t perm,
> > > +                                     uint64_t shared, GSList **tran)
> > > +{
> > > +    if (!c->has_backup_perm) {
> > > +        c->has_backup_perm = true;
> > > +        c->backup_perm = c->perm;
> > > +        c->backup_shared_perm = c->shared_perm;
> > > +    }
> > 
> > This is the obvious refactoring at this point, and it's fine as such.
> > 
> > However, when you start to actually use tran (and in new places), it
> > means that I have to check that we can never end up here recursively
> > with a different tran.
> 
> I don't follow.. Which different tran do you mean?

Any really. At this point in the series, nothing passes a non-NULL tran
yet. When you add the first user, it is only a local transaction for a
single node. If something else called bdrv_child_set_perm_safe() on the
same node before the transaction has completed, the result would be
broken.

So reviewing/understanding this change (and actually developing it in
the first place) means going through all the code that ends up inside
the transaction and making sure that we never try to change permissions
for the same node a second time in any context.

> > 
> > It would probably be much cleaner if the next patch moved the backup
> > state into the opaque struct for BdrvAction.
> 
> But old code which calls the function with tran=NULL can't use it..
> Hmm, we can probably support both ways: c->backup_perm for callers
> with tran=NULL and opaque struct for new style callers.

Hm, you're right about that... Maybe that's too much complication then.

What happens in the next patch is still understandable enough with the
way you currently have it. Let's see what it looks like with the rest.

Kevin