Re: [Qemu-block] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

28.12.2017 08:24, Fam Zheng wrote:
> On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   include/block/dirty-bitmap.h |  3 +++
> >   block/dirty-bitmap.c         | 25 ++++++++++++++++---------
> >   2 files changed, 19 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> > index 93d4336505..caf1f3d861 100644
> > --- a/include/block/dirty-bitmap.h
> > +++ b/include/block/dirty-bitmap.h
> > @@ -92,5 +92,8 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
> > *bs);
> >   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> >                                           BdrvDirtyBitmap *bitmap);
> >   char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
> > +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> > +                                                  BdrvDirtyBitmap *bitmap,
> > +                                                  Error **errp);
> >   
> >   #endif
> > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > index d83da077d5..fe27ddfb83 100644
> > --- a/block/dirty-bitmap.c
> > +++ b/block/dirty-bitmap.c
> > @@ -327,15 +327,11 @@ BdrvDirtyBitmap 
> > *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> >    * The merged parent will be un-frozen, but not explicitly re-enabled.
> >    * Called with BQL taken.
> Maybe update the comment as
>
> s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/
>
> and ...

we have the following comment:

    /* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
     * Reading from the list can be done with either the BQL or the
     * dirty_bitmap_mutex.  Modifying a bitmap only requires
     * dirty_bitmap_mutex. */
    QemuMutex dirty_bitmap_mutex;

(I don't understand well the logic, why is it so. Paolo introduced the 
lock, but didn't update some functions..)

so, actually, here we need both BQL and mutex.

> >    */
> > -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> > -                                           BdrvDirtyBitmap *parent,
> > -                                           Error **errp)
> > +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> > +                                                  BdrvDirtyBitmap *parent,
> > +                                                  Error **errp)
> >   {
> > -    BdrvDirtyBitmap *successor;
> > -
> > -    qemu_mutex_lock(parent->mutex);
> > -
> > -    successor = parent->successor;
> > +    BdrvDirtyBitmap *successor = parent->successor;
> >   
> >       if (!successor) {
> >           error_setg(errp, "Cannot reclaim a successor when none is present");
> > @@ -349,9 +345,20 @@ BdrvDirtyBitmap 
> > *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> >       bdrv_release_dirty_bitmap_locked(bs, successor);
> >       parent->successor = NULL;
> >   
> > +    return parent;
> > +}
> > +
> ... move the "Called with BQL taken" comment here?

and here BQL.

Ok, I'll add/fix comments.

> > +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> > +                                           BdrvDirtyBitmap *parent,
> > +                                           Error **errp)
> > +{
> > +    BdrvDirtyBitmap *ret;
> > +
> > +    qemu_mutex_lock(parent->mutex);
> > +    ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
> >       qemu_mutex_unlock(parent->mutex);
> >   
> > -    return parent;
> > +    return ret;
> >   }
> >   
> >   /**
> > --
> > 2.11.1


--
Best regards,
Vladimir