| Message ID | 20170605123908.18777-17-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
HI! One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under lock too? 05.06.2017 15:39, Paolo Bonzini wrote: > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/dirty-bitmap.c | 68 ++++++++++++++++++++++++++++++++++++++------ > block/mirror.c | 11 +++++-- > include/block/block_int.h | 4 +-- > include/block/dirty-bitmap.h | 25 +++++++++++----- > migration/block.c | 10 ++++--- > 5 files changed, 94 insertions(+), 24 deletions(-)
On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: > HI! > > One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under > lock too? Any call to dirty bitmap functions between bdrv_clear_dirty_bitmap and bdrv_undo_clear_dirty_bitmap is problematic anyway, so bdrv_clear_dirty_bitmap really only needs the lock in the !out case; bdrv_undo_clear_dirty_bitmap is only called when out != NULL. However, I agree it would be cleaner to add the lock there, too. Paolo
26.06.2017 19:54, Paolo Bonzini wrote: > > On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: >> HI! >> >> One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under >> lock too? > Any call to dirty bitmap functions between bdrv_clear_dirty_bitmap and > bdrv_undo_clear_dirty_bitmap is problematic anyway, so > bdrv_clear_dirty_bitmap really only needs the lock in the !out case; > bdrv_undo_clear_dirty_bitmap is only called when out != NULL. > > However, I agree it would be cleaner to add the lock there, too. > > Paolo Also, you've added comment "Called with BQL taken" both to functions that calls bdrv_dirty_bitmaps_lock and that do not... What is BQL?
On 27/06/2017 11:07, Vladimir Sementsov-Ogievskiy wrote: > 26.06.2017 19:54, Paolo Bonzini wrote: >> >> On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: >>> HI! >>> >>> One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under >>> lock too? >> Any call to dirty bitmap functions between bdrv_clear_dirty_bitmap and >> bdrv_undo_clear_dirty_bitmap is problematic anyway, so >> bdrv_clear_dirty_bitmap really only needs the lock in the !out case; >> bdrv_undo_clear_dirty_bitmap is only called when out != NULL. >> >> However, I agree it would be cleaner to add the lock there, too. > > Also, you've added comment "Called with BQL taken" both to functions > that calls bdrv_dirty_bitmaps_lock and that do not... What is BQL? It's the "big QEMU lock", also known as "iothread lock". The locking policy is documented in block_int.h: /* 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; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; and the comments in block/dirty-bitmap.c reflect the above comment. Paolo
27.06.2017 12:27, Paolo Bonzini wrote: > > On 27/06/2017 11:07, Vladimir Sementsov-Ogievskiy wrote: >> 26.06.2017 19:54, Paolo Bonzini wrote: >>> On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: >>>> HI! >>>> >>>> One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under >>>> lock too? >>> Any call to dirty bitmap functions between bdrv_clear_dirty_bitmap and >>> bdrv_undo_clear_dirty_bitmap is problematic anyway, so >>> bdrv_clear_dirty_bitmap really only needs the lock in the !out case; >>> bdrv_undo_clear_dirty_bitmap is only called when out != NULL. >>> >>> However, I agree it would be cleaner to add the lock there, too. >> Also, you've added comment "Called with BQL taken" both to functions >> that calls bdrv_dirty_bitmaps_lock and that do not... What is BQL? > It's the "big QEMU lock", also known as "iothread lock". The locking policy > is documented in block_int.h: > > /* 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; > QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; > > and the comments in block/dirty-bitmap.c reflect the above comment. > > Paolo bdrv_enable_dirty_bitmap - writes to the list, it changes 'disabled' field. So it requires both BQL and dirty_bitmap_mutex? But the comment says only about BQL. Also, for example, if I want to create a new bitmap and than somehow change it, should I do it like this: bdrv_create_dirty_bitmap(...) bdrv_dirty_bitmaps_lock(bs) bitmap = bdrv_find_dirty_bitmap(bs, name) <some changes> bdrv_dirty_bitmaps_unlock(bs) - because we can't now trust the pointer returned by bdrv_create_dirty_bitmap, as it releases bitmap lock before return.
On 27/06/2017 11:47, Vladimir Sementsov-Ogievskiy wrote: > bdrv_enable_dirty_bitmap - writes to the list, it changes 'disabled' > field. So it requires both BQL and dirty_bitmap_mutex? But the comment > says only about BQL. This one is interesting. There could be a concurrent call to bdrv_set_dirty, indeed. I'll send a patch. > Also, for example, if I want to create a new bitmap and than somehow > change it, should I do it like this: > > bdrv_create_dirty_bitmap(...) > > bdrv_dirty_bitmaps_lock(bs) > > bitmap = bdrv_find_dirty_bitmap(bs, name) > > <some changes> > > bdrv_dirty_bitmaps_unlock(bs) > > - because we can't now trust the pointer returned by > bdrv_create_dirty_bitmap, as it releases bitmap lock before return. If you have the big QEMU lock (you do if you are in the QEMU monitor), you are protected from changes to the list of bitmaps. Paolo
27.06.2017 15:52, Paolo Bonzini wrote: > > On 27/06/2017 11:47, Vladimir Sementsov-Ogievskiy wrote: >> bdrv_enable_dirty_bitmap - writes to the list, it changes 'disabled' >> field. So it requires both BQL and dirty_bitmap_mutex? But the comment >> says only about BQL. > This one is interesting. There could be a concurrent call to > bdrv_set_dirty, indeed. I'll send a patch. > >> Also, for example, if I want to create a new bitmap and than somehow >> change it, should I do it like this: >> >> bdrv_create_dirty_bitmap(...) >> >> bdrv_dirty_bitmaps_lock(bs) >> >> bitmap = bdrv_find_dirty_bitmap(bs, name) >> >> <some changes> >> >> bdrv_dirty_bitmaps_unlock(bs) >> >> - because we can't now trust the pointer returned by >> bdrv_create_dirty_bitmap, as it releases bitmap lock before return. > If you have the big QEMU lock (you do if you are in the QEMU monitor), > you are protected from changes to the list of bitmaps. > > Paolo but you wrote "Writing to the list requires the BQL _and_ the dirty_bitmap_mutex". should it be BQL only?
On 27/06/2017 15:51, Vladimir Sementsov-Ogievskiy wrote: >>> >>> bdrv_create_dirty_bitmap(...) >>> >>> bdrv_dirty_bitmaps_lock(bs) >>> >>> bitmap = bdrv_find_dirty_bitmap(bs, name) >>> >>> <some changes> >>> >>> bdrv_dirty_bitmaps_unlock(bs) >>> >>> - because we can't now trust the pointer returned by >>> bdrv_create_dirty_bitmap, as it releases bitmap lock before return. >> If you have the big QEMU lock (you do if you are in the QEMU monitor), >> you are protected from changes to the list of bitmaps. >> >> Paolo > > but you wrote "Writing to the list requires the BQL _and_ the > dirty_bitmap_mutex". > > should it be BQL only? bdrv_dirty_bitmaps_lock/unlock is (should be) called by the functions that write to the list. I hope I can post the patch today already. Paolo
27.06.2017 16:58, Paolo Bonzini wrote: > > On 27/06/2017 15:51, Vladimir Sementsov-Ogievskiy wrote: >>>> bdrv_create_dirty_bitmap(...) >>>> >>>> bdrv_dirty_bitmaps_lock(bs) >>>> >>>> bitmap = bdrv_find_dirty_bitmap(bs, name) >>>> >>>> <some changes> >>>> >>>> bdrv_dirty_bitmaps_unlock(bs) >>>> >>>> - because we can't now trust the pointer returned by >>>> bdrv_create_dirty_bitmap, as it releases bitmap lock before return. >>> If you have the big QEMU lock (you do if you are in the QEMU monitor), >>> you are protected from changes to the list of bitmaps. >>> >>> Paolo >> but you wrote "Writing to the list requires the BQL _and_ the >> dirty_bitmap_mutex". >> >> should it be BQL only? > bdrv_dirty_bitmaps_lock/unlock is (should be) called by the functions > that write to the list. I hope I can post the patch today already. > > Paolo I'm likely not right, but for me introducing this mutex looks like dirty bitmaps may be accessed from concurrent threads. So for me it looks strange that only accessors are protected by the mutex and not the whole transactions. One example I've wrote above, other examples from code: are qmp_block_dirty_bitmap** functions: bdrv_create_dirty_bitmap() { bdrv_find_dirty_bitmap() .... bdrv_dirty_bitmaps_lock(bs); QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); bdrv_dirty_bitmaps_unlock(bs); } - we protect inserting into list from other threads, but what prevent creating bitmap with the same name from other thread after bdrv_find_dirty_bitmap() and before bdrv_dirty_bitmaps_lock() ? ----- qmp_block_dirty_bitmap_clear() { bitmap = block_dirty_bitmap_lookup() .... bdrv_clear_dirty_bitmap() } - bdrv_clear_dirty_bitmap is protected by lock, but what prevent deleting this bitmap by other thread between block_dirty_bitmap_lookup() and bdrv_clear_dirty_bitmap() ? --- it is not the whole list ---
On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: > I'm likely not right, but for me introducing this mutex looks like dirty > bitmaps may be accessed from concurrent threads. So for me it looks > strange that only accessors are protected by the mutex and not the whole > transactions. There must be something else protecting the whole transaction. > One example I've wrote above, other examples from code: are > qmp_block_dirty_bitmap** functions: > > bdrv_create_dirty_bitmap() { > > bdrv_find_dirty_bitmap() > > .... > > bdrv_dirty_bitmaps_lock(bs); > QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); > bdrv_dirty_bitmaps_unlock(bs); > > } > > - we protect inserting into list from other threads, but what prevent > creating bitmap with the same name from other thread after > bdrv_find_dirty_bitmap() and before bdrv_dirty_bitmaps_lock() ? It's like a read-write lock. The write side is invoked under the 'big QEMU lock' so there cannot be two concurrent writes. A bitmap can be written to after bdrv_find_dirty_bitmap returns, but only if _you_ tell another thread about the bitmap you've just created. If that doesn't happen, the bitmap cannot change. And it can also disappear because _your_ thread is the one with the big QEMU lock. Paolo
27.06.2017 17:26, Paolo Bonzini wrote: > > On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: >> I'm likely not right, but for me introducing this mutex looks like dirty >> bitmaps may be accessed from concurrent threads. So for me it looks >> strange that only accessors are protected by the mutex and not the whole >> transactions. > There must be something else protecting the whole transaction. But it doesn't? Before it (qmp_block_dirty_bitmap*) was protected by aio context acquire/release... > >> One example I've wrote above, other examples from code: are >> qmp_block_dirty_bitmap** functions: >> >> bdrv_create_dirty_bitmap() { >> >> bdrv_find_dirty_bitmap() >> >> .... >> >> bdrv_dirty_bitmaps_lock(bs); >> QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); >> bdrv_dirty_bitmaps_unlock(bs); >> >> } >> >> - we protect inserting into list from other threads, but what prevent >> creating bitmap with the same name from other thread after >> bdrv_find_dirty_bitmap() and before bdrv_dirty_bitmaps_lock() ? > It's like a read-write lock. > > The write side is invoked under the 'big QEMU lock' so there cannot be > two concurrent writes. > > A bitmap can be written to after bdrv_find_dirty_bitmap returns, but > only if _you_ tell another thread about the bitmap you've just created. no, I'm not about touching just created bitmap. I'm about creating bitmap with the same name by other thread (unlikely case, but possible). > If that doesn't happen, the bitmap cannot change. And it can also > disappear because _your_ thread is the one with the big QEMU lock. So, if I under BQL, I don't need dirty_bitmap_lock? > > Paolo
On 27/06/2017 16:43, Vladimir Sementsov-Ogievskiy wrote: >> The write side is invoked under the 'big QEMU lock' so there cannot be >> two concurrent writes. >> >> A bitmap can be written to after bdrv_find_dirty_bitmap returns, but >> only if _you_ tell another thread about the bitmap you've just created. > > no, I'm not about touching just created bitmap. I'm about creating > bitmap with the same name by other thread (unlikely case, but possible). You can't unless you drop the BQL. >> If that doesn't happen, the bitmap cannot change. And it can also >> disappear because _your_ thread is the one with the big QEMU lock. > > So, if I under BQL, I don't need dirty_bitmap_lock? Writing to the list requires _both_ BQL and dirty_bitmap_lock. Write functions actually have "called with BQL taken" in dirty-bitmap.c, because dirty-bitmap.c will call dirty_bitmap_lock/unlock itself. Reading from the list requires one of the two locks. Such functions have "called with BQL or dirty_bitmap lock taken". For reading/writing to the bitmap itself, you need dirty_bitmap_lock. dirty-bitmap.c can take the lock itself but, there are also functions named *_locked where the caller takes the lock. Paolo
27.06.2017 17:26, Paolo Bonzini wrote: > > On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: >> I'm likely not right, but for me introducing this mutex looks like dirty >> bitmaps may be accessed from concurrent threads. So for me it looks >> strange that only accessors are protected by the mutex and not the whole >> transactions. > There must be something else protecting the whole transaction. > >> One example I've wrote above, other examples from code: are >> qmp_block_dirty_bitmap** functions: >> >> bdrv_create_dirty_bitmap() { >> >> bdrv_find_dirty_bitmap() >> >> .... >> >> bdrv_dirty_bitmaps_lock(bs); >> QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); >> bdrv_dirty_bitmaps_unlock(bs); >> >> } >> >> - we protect inserting into list from other threads, but what prevent >> creating bitmap with the same name from other thread after >> bdrv_find_dirty_bitmap() and before bdrv_dirty_bitmaps_lock() ? > It's like a read-write lock. Ok, finally I understand (not all, but the main idea=), sorry for annoying. > > The write side is invoked under the 'big QEMU lock' so there cannot be > two concurrent writes. > > A bitmap can be written to after bdrv_find_dirty_bitmap returns, but > only if _you_ tell another thread about the bitmap you've just created. > If that doesn't happen, the bitmap cannot change. And it can also > disappear because _your_ thread is the one with the big QEMU lock. > > Paolo
27.06.2017 18:31, Vladimir Sementsov-Ogievskiy wrote: > 27.06.2017 17:26, Paolo Bonzini wrote: >> >> On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: >>> I'm likely not right, but for me introducing this mutex looks like >>> dirty >>> bitmaps may be accessed from concurrent threads. So for me it looks >>> strange that only accessors are protected by the mutex and not the >>> whole >>> transactions. >> There must be something else protecting the whole transaction. >> >>> One example I've wrote above, other examples from code: are >>> qmp_block_dirty_bitmap** functions: >>> >>> bdrv_create_dirty_bitmap() { >>> >>> bdrv_find_dirty_bitmap() >>> >>> .... >>> >>> bdrv_dirty_bitmaps_lock(bs); >>> QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); >>> bdrv_dirty_bitmaps_unlock(bs); >>> >>> } >>> >>> - we protect inserting into list from other threads, but what prevent >>> creating bitmap with the same name from other thread after >>> bdrv_find_dirty_bitmap() and before bdrv_dirty_bitmaps_lock() ? >> It's like a read-write lock. > > Ok, finally I understand (not all, but the main idea=), sorry for > annoying. Is there any example of accessing dirty bitmaps for read not under BQL but only under dirty bitmap lock? > >> >> The write side is invoked under the 'big QEMU lock' so there cannot be >> two concurrent writes. >> >> A bitmap can be written to after bdrv_find_dirty_bitmap returns, but >> only if _you_ tell another thread about the bitmap you've just created. >> If that doesn't happen, the bitmap cannot change. And it can also >> disappear because _your_ thread is the one with the big QEMU lock. >> >> Paolo > >
On 27/06/2017 17:32, Vladimir Sementsov-Ogievskiy wrote: >>>> >>>> >>>> - we protect inserting into list from other threads, but what prevent >>>> creating bitmap with the same name from other thread after >>>> bdrv_find_dirty_bitmap() and before bdrv_dirty_bitmaps_lock() ? >>> It's like a read-write lock. >> >> Ok, finally I understand (not all, but the main idea=), sorry for >> annoying. > > Is there any example of accessing dirty bitmaps for read not under BQL > but only under dirty bitmap lock? Yes, search for bdrv_dirty_bitmap_lock...unlock in block/mirror.c. There is also block/backup.c, which operates on a frozen bitmap. Paolo
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index fa78109365..a04c6e4154 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 */ @@ -62,6 +63,16 @@ static inline void bdrv_dirty_bitmaps_unlock(BlockDriverState *bs) qemu_mutex_unlock(&bs->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) { @@ -109,6 +120,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); @@ -134,20 +146,24 @@ 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, - 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) { uint64_t i; int sectors_per_bit = 1 << hbitmap_granularity(bitmap->meta); @@ -162,11 +178,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) @@ -393,8 +424,9 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) return list; } -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, - int64_t sector) +/* Called within bdrv_dirty_bitmap_lock..unlock */ +int bdrv_get_dirty_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, + int64_t sector) { if (bitmap) { return hbitmap_get(bitmap->bitmap, sector); @@ -467,23 +499,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 { @@ -492,6 +543,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 88ae882c46..19afcc6f1a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -342,6 +342,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) int max_io_sectors = MAX((s->buf_size >> BDRV_SECTOR_BITS) / MAX_IN_FLIGHT, MAX_IO_SECTORS); + bdrv_dirty_bitmap_lock(s->dirty_bitmap); sector_num = bdrv_dirty_iter_next(s->dbi); if (sector_num < 0) { bdrv_set_dirty_iter(s->dbi, 0); @@ -349,6 +350,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) 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; while (test_bit(first_chunk, s->in_flight_bitmap)) { @@ -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 c834b866a7..15c656d1df 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -611,8 +611,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 9dea14ba03..ad6558af56 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -36,8 +36,6 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap); const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap); int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap); DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap); -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, - int64_t sector); void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t cur_sector, int64_t nr_sectors); void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, @@ -45,6 +43,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 +53,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 +68,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 diff --git a/migration/block.c b/migration/block.c index 8fe484e8b4..d102c840f6 100644 --- a/migration/block.c +++ b/migration/block.c @@ -529,14 +529,15 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds, } else { blk_mig_unlock(); } - if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector)) { - + bdrv_dirty_bitmap_lock(bmds->dirty_bitmap); + if (bdrv_get_dirty_locked(bs, bmds->dirty_bitmap, sector)) { if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) { nr_sectors = total_sectors - sector; } else { nr_sectors = BDRV_SECTORS_PER_DIRTY_CHUNK; } - bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors); + bdrv_reset_dirty_bitmap_locked(bmds->dirty_bitmap, sector, nr_sectors); + bdrv_dirty_bitmap_unlock(bmds->dirty_bitmap); blk = g_new(BlkMigBlock, 1); blk->buf = g_malloc(BLOCK_SIZE); @@ -572,9 +573,10 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds, sector += nr_sectors; bmds->cur_dirty = sector; - break; } + + bdrv_dirty_bitmap_unlock(bmds->dirty_bitmap); sector += BDRV_SECTORS_PER_DIRTY_CHUNK; bmds->cur_dirty = sector; }