Re: [Qemu-devel] [PATCH 09/16] block: Split bdrv_move_feature_fields()

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]

Am 23.09.2015 um 18:36 hat Max Reitz geschrieben:
> On 17.09.2015 15:48, Kevin Wolf wrote:
> > After bdrv_swap(), some fields must be moved back to their original BDS
> > to compensate for the effects that a swap of the contents of the objects
> > has while keeping the old addresses. Other fields must be moved back
> > because they should logically be moved and must stay on top
> > 
> > When replacing bdrv_swap() with operations changing the pointers in the
> > parents, we only need the latter and must avoid swapping the former.
> > Split the function accordingly.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 743f82e..7930f3c 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1984,6 +1984,8 @@ static void bdrv_rebind(BlockDriverState *bs)
> >      }
> >  }
> >  
> > +/* Fields that need to stay with the top-level BDS, no matter whether the
> > + * address of the top-level BDS stays the same or not. */
> >  static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
> >                                       BlockDriverState *bs_src)
> >  {
> > @@ -2019,7 +2021,13 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
> >  
> >      /* dirty bitmap */
> >      bs_dest->dirty_bitmaps      = bs_src->dirty_bitmaps;
> > +}
> >  
> > +/* Fields that only need to be swapped if the contents of BDSes is swapped
> > + * rather than pointers being changed in the parents. */
> > +static void bdrv_move_reference_fields(BlockDriverState *bs_dest,
> > +                                       BlockDriverState *bs_src)
> > +{
> >      /* reference count */
> >      bs_dest->refcnt             = bs_src->refcnt;
> >  
> 
> I'm not sure whether the op blockers should be moved in this function...
> I think they should be moved in bdrv_move_feasture_fields(), because
> they generally depend on the position within the node graph and not on
> the BDS itself, don't they?

Op blockers are a mess. I believe we have both kinds. In any case, we
should be relatively safe here because the operation involving something
like bdrv_append() or bdrv_replace_in_backing_chain() would have been
blocked in the first place if there were an op blocker that needs to be
moved.

I seem to remember that there were two reasons for not moving op
blockers: The first one is that the next action the block jobs want to
do is to unblock (because the job has completed) and it's nicer to do
this on the same BDS as they blocked initially. The other one is that
bs->backing_blocker is already managed by bdrv_set_backing_hd() and
swapping in addition broke things.

Oh, and the third one is that swapping is plain wrong. We're not really
performing a symmetrical swap any more. If BDS A is replaced by BDS B
(or B is appended to a chain that had A as it's root before), all
parents of _both_ A and B point to B afterwards. So swapping would move
some op blockers to now completely unrelated BDSes.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]