Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/dirty-bitmap.c | 74 +++++++++++++++++++++++++++++++++++++++++---
block/mirror.c | 11 +++++--
include/block/block_int.h | 4 +--
include/block/dirty-bitmap.h | 23 +++++++++++---
4 files changed, 97 insertions(+), 15 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index e13718e..b854077 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -37,6 +37,7 @@
* or enabled. A frozen bitmap can only abdicate() or reclaim().
*/
struct BdrvDirtyBitmap {
+ QemuMutex *mutex;
HBitmap *bitmap; /* Dirty sector bitmap implementation */
HBitmap *meta; /* Meta dirty bitmap */
BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
@@ -69,6 +70,16 @@ static inline void bdrv_dirty_bitmaps_unlock(BlockDriverState *bs)
qemu_mutex_unlock(&dirty_bitmap_mutex);
}
+void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap)
+{
+ qemu_mutex_lock(bitmap->mutex);
+}
+
+void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap)
+{
+ qemu_mutex_unlock(bitmap->mutex);
+}
+
/* Called with BQL or dirty_bitmap lock taken. */
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
{
@@ -116,6 +127,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return NULL;
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
+ bitmap->mutex = &bs->dirty_bitmap_mutex;
bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
bitmap->size = bitmap_size;
bitmap->name = g_strdup(name);
@@ -141,18 +153,22 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int chunk_size)
{
assert(!bitmap->meta);
+ qemu_mutex_lock(bitmap->mutex);
bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
chunk_size * BITS_PER_BYTE);
+ qemu_mutex_unlock(bitmap->mutex);
}
void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
assert(bitmap->meta);
+ qemu_mutex_lock(bitmap->mutex);
hbitmap_free_meta(bitmap->bitmap);
bitmap->meta = NULL;
+ qemu_mutex_unlock(bitmap->mutex);
}
-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
+int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, int64_t sector,
int nb_sectors)
{
@@ -169,11 +185,26 @@ int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
return false;
}
+int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, int64_t sector,
+ int nb_sectors)
+{
+ bool dirty;
+
+ qemu_mutex_lock(bitmap->mutex);
+ dirty = bdrv_dirty_bitmap_get_meta_locked(bs, bitmap, sector, nb_sectors);
+ qemu_mutex_unlock(bitmap->mutex);
+
+ return dirty;
+}
+
void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, int64_t sector,
int nb_sectors)
{
+ qemu_mutex_lock(bitmap->mutex);
hbitmap_reset(bitmap->meta, sector, nb_sectors);
+ qemu_mutex_unlock(bitmap->mutex);
}
int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
@@ -400,7 +431,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
return list;
}
-int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t sector)
{
if (bitmap) {
@@ -410,6 +442,18 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
}
}
+int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t sector)
+{
+ bool dirty;
+
+ bdrv_dirty_bitmap_lock(bitmap);
+ dirty = bdrv_get_dirty_locked(bs, bitmap, sector);
+ bdrv_dirty_bitmap_unlock(bitmap);
+
+ return dirty;
+}
+
/**
* Chooses a default granularity based on the existing cluster size,
* but clamped between [4K, 64K]. Defaults to 64K in the case that there
@@ -474,23 +518,42 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
return hbitmap_iter_next(&iter->hbi);
}
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int64_t nr_sectors)
+{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+}
+
void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors)
{
+ bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_set_dirty_bitmap_locked(bitmap, cur_sector, nr_sectors);
+ bdrv_dirty_bitmap_unlock(bitmap);
+}
+
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int64_t nr_sectors)
+{
assert(bdrv_dirty_bitmap_enabled(bitmap));
- hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+ hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int64_t nr_sectors)
{
- assert(bdrv_dirty_bitmap_enabled(bitmap));
- hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+ bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_reset_dirty_bitmap_locked(bitmap, cur_sector, nr_sectors);
+ bdrv_dirty_bitmap_unlock(bitmap);
}
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
{
assert(bdrv_dirty_bitmap_enabled(bitmap));
+ bdrv_dirty_bitmap_lock(bitmap);
if (!out) {
hbitmap_reset_all(bitmap->bitmap);
} else {
@@ -499,6 +562,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
hbitmap_granularity(backup));
*out = backup;
}
+ bdrv_dirty_bitmap_unlock(bitmap);
}
void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
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_set_dirty_iter(s->dbi, 0);
sector_num = bdrv_dirty_iter_next(s->dbi);
trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
assert(sector_num >= 0);
+ bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
}
first_chunk = sector_num / sectors_per_chunk;
@@ -360,12 +362,13 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
/* Find the number of consective dirty chunks following the first dirty
* one, and wait for in flight requests in them. */
+ bdrv_dirty_bitmap_lock(s->dirty_bitmap);
while (nb_chunks * sectors_per_chunk < (s->buf_size >> BDRV_SECTOR_BITS)) {
int64_t next_dirty;
int64_t next_sector = sector_num + nb_chunks * sectors_per_chunk;
int64_t next_chunk = next_sector / sectors_per_chunk;
if (next_sector >= end ||
- !bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) {
+ !bdrv_get_dirty_locked(source, s->dirty_bitmap, next_sector)) {
break;
}
if (test_bit(next_chunk, s->in_flight_bitmap)) {
@@ -386,8 +389,10 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
* calling bdrv_get_block_status_above could yield - if some blocks are
* marked dirty in this window, we need to know.
*/
- bdrv_reset_dirty_bitmap(s->dirty_bitmap, sector_num,
- nb_chunks * sectors_per_chunk);
+ bdrv_reset_dirty_bitmap_locked(s->dirty_bitmap, sector_num,
+ nb_chunks * sectors_per_chunk);
+ bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
+
bitmap_set(s->in_flight_bitmap, sector_num / sectors_per_chunk, nb_chunks);
while (nb_chunks > 0 && sector_num < end) {
int64_t ret;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 03db2cf..c264ead 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -598,8 +598,8 @@ struct BlockDriverState {
/* 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 requires the AioContext
- * lock. */
+ * dirty_bitmap_mutex. Modifying a bitmap only requires
+ * dirty_bitmap_mutex. */
QemuMutex dirty_bitmap_mutex;
QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9dea14b..b6fc35b 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -45,6 +45,9 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, int64_t sector,
int nb_sectors);
+int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, int64_t sector,
+ int nb_sectors);
void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, int64_t sector,
int nb_sectors);
@@ -52,11 +55,6 @@ BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
uint64_t first_sector);
void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
-int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
-void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
-int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
-void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
uint64_t start, uint64_t count);
@@ -72,4 +70,19 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
bool finish);
void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
+/* Functions that require manual locking. */
+void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap);
+int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t sector);
+void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int64_t nr_sectors);
+void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int64_t nr_sectors);
+int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
+int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
+
#endif
--
2.9.3
On 04/20/2017 07:00 AM, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/dirty-bitmap.c | 74 +++++++++++++++++++++++++++++++++++++++++--- > block/mirror.c | 11 +++++-- > include/block/block_int.h | 4 +-- > include/block/dirty-bitmap.h | 23 +++++++++++--- > 4 files changed, 97 insertions(+), 15 deletions(-) Fun conflicts with my pending patches to switch dirty-bitmap to be byte-based instead of sector-based. I doubt the computer will be able to resolve nicely, but I think rebasing by hand should be pretty doable, no matter which of our patches lands first. https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02163.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, 04/20 14:00, Paolo Bonzini wrote:
> @@ -400,7 +431,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> return list;
> }
>
> -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +/* Called within bdrv_dirty_bitmap_lock..unlock */
> +int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> int64_t sector)
Parameter indentation is off now.
> {
> if (bitmap) {
> @@ -410,6 +442,18 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> }
> }
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 03db2cf..c264ead 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -598,8 +598,8 @@ struct BlockDriverState {
>
> /* 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 requires the AioContext
> - * lock. */
> + * dirty_bitmap_mutex. Modifying a bitmap only requires
> + * dirty_bitmap_mutex. */
I'm confused by this comment. What's added in this patch is bitmap->mutex, not
dirty_bitmap_mutex. Is it a mistake?
> QemuMutex dirty_bitmap_mutex;
> QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
>
Fam
On 04/05/2017 10:05, Fam Zheng wrote: >> /* 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 requires the AioContext >> - * lock. */ >> + * dirty_bitmap_mutex. Modifying a bitmap only requires >> + * dirty_bitmap_mutex. */ > I'm confused by this comment. What's added in this patch is bitmap->mutex, not > dirty_bitmap_mutex. Is it a mistake? bitmap->mutex is a pointer that points to the (single) per-BDS mutex. This patch changes the modification of bitmaps to require bitmap->mutex (which points to bs->dirty_bitmap_mutex) instead of the AioContext lock. Paolo >> QemuMutex dirty_bitmap_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.
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
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
© 2016 - 2026 Red Hat, Inc.