Re: [Qemu-block] [PATCH 16/17] block: protect modification of dirty bitmaps with a mutex
On Fri, May 05, 2017 at 12:47:35PM +0200, Paolo Bonzini wrote:
>
>
> On 05/05/2017 12:36, Stefan Hajnoczi wrote:
> > On Thu, Apr 20, 2017 at 02:00:57PM +0200, Paolo Bonzini wrote:
> >> @@ -410,6 +442,18 @@ int bdrv_get_dirty(BlockDriverState *bs,
> >> BdrvDirtyBitmap *bitmap,
> >> }
> >> }
> >>
> >> +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> >> + int64_t sector)
> >
> > Is it a good idea to offer an unlocked bdrv_get_dirty() API? It
> > encourages non-atomic access to the bitmap, e.g.
> >
> > if (bdrv_get_dirty()) {
> > ...do something outside the lock...
> > bdrv_reset_dirty_bitmap();
> > }
> >
> > The unlocked API should be test-and-set/clear instead so that callers
> > automatically avoid race conditions.
>
> I'm not sure it's possible to implement atomic test and clear for
> HBitmap. But I can look into removing unlocked bdrv_get_dirty, the only
> user is block migration.
Removing unlocked bdrv_get_dirty() is good.
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH 16/17] block: protect modification of dirty bitmaps with a mutex
On 05/05/2017 12:36, Stefan Hajnoczi wrote:
> On Thu, Apr 20, 2017 at 02:00:57PM +0200, Paolo Bonzini wrote:
>> @@ -410,6 +442,18 @@ int bdrv_get_dirty(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap,
>> }
>> }
>>
>> +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>> + int64_t sector)
>
> Is it a good idea to offer an unlocked bdrv_get_dirty() API? It
> encourages non-atomic access to the bitmap, e.g.
>
> if (bdrv_get_dirty()) {
> ...do something outside the lock...
> bdrv_reset_dirty_bitmap();
> }
>
> The unlocked API should be test-and-set/clear instead so that callers
> automatically avoid race conditions.
I'm not sure it's possible to implement atomic test and clear for
HBitmap. But I can look into removing unlocked bdrv_get_dirty, the only
user is block migration.
>> diff --git a/block/mirror.c b/block/mirror.c
>> index dc227a2..6a5b0f8 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -344,10 +344,12 @@ static uint64_t coroutine_fn
>> mirror_iteration(MirrorBlockJob *s)
>>
>> sector_num = bdrv_dirty_iter_next(s->dbi);
>> if (sector_num < 0) {
>> +bdrv_dirty_bitmap_lock(s->dirty_bitmap);
>
> bdrv_dirty_iter_next() is listed under "functions that require manual
> locking" but it's being called outside of the lock.
Thanks, will fix.
Paolo
Re: [Qemu-block] [PATCH 16/17] block: protect modification of dirty bitmaps with a mutex
On Thu, Apr 20, 2017 at 02:00:57PM +0200, Paolo Bonzini wrote:
> @@ -410,6 +442,18 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap
> *bitmap,
> }
> }
>
> +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t sector)
Is it a good idea to offer an unlocked bdrv_get_dirty() API? It
encourages non-atomic access to the bitmap, e.g.
if (bdrv_get_dirty()) {
...do something outside the lock...
bdrv_reset_dirty_bitmap();
}
The unlocked API should be test-and-set/clear instead so that callers
automatically avoid race conditions.
> diff --git a/block/mirror.c b/block/mirror.c
> index dc227a2..6a5b0f8 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -344,10 +344,12 @@ static uint64_t coroutine_fn
> mirror_iteration(MirrorBlockJob *s)
>
> sector_num = bdrv_dirty_iter_next(s->dbi);
> if (sector_num < 0) {
> +bdrv_dirty_bitmap_lock(s->dirty_bitmap);
bdrv_dirty_iter_next() is listed under "functions that require manual
locking" but it's being called outside of the lock.
signature.asc
Description: PGP signature
