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

[Fam Zheng]

On Thu, 12/28 14:16, Vladimir Sementsov-Ogievskiy wrote:
> 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.

Yes, because of that comment my interpretion has been both "BQL and the mutex"
whereever we say "within bdrv_dirty_bitmap_lock..unlock", as to some other
places in this file.

Fam