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 <kw...@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
pgpeiz79vcoYs.pgp
Description: PGP signature
