* [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration
@ 2014-11-27 9:40 Vladimir Sementsov-Ogievskiy
2014-12-15 8:45 ` Vladimir Sementsov-Ogievskiy
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2014-11-27 9:40 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Vladimir Sementsov-Ogievskiy,
Stefan Hajnoczi, Denis V. Lunev, John Snow
Mirror and migration use dirty bitmaps for their purposes, and since
commit [block: per caller dirty bitmap] they use their own bitmaps, not
the global one. But they use old functions bdrv_set_dirty and
bdrv_reset_dirty, which change all dirty bitmaps.
Named dirty bitmaps series by Fam and Snow are affected: mirroring and
migration will spoil all (not related to this mirroring or migration)
named dirty bitmaps.
This patch fixes this by adding bdrv_set_dirty_bitmap and
bdrv_reset_dirty_bitmap, which change concrete bitmap. Also, to prevent
such mistakes in future, old functions bdrv_(set,reset)_dirty are made
static, for internal block usage.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
CC: John Snow <jsnow@redhat.com>
CC: Fam Zheng <famz@redhat.com>
CC: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
This patch conflicts with
[PATCH v8 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
by John Snow, which introduces bdrv_reset_dirty_bitmap too, but it resets the whole
bitmap, not specified sectors range.
block-migration.c | 5 +++--
block.c | 23 ++++++++++++++++++++---
block/mirror.c | 11 +++++++----
include/block/block.h | 6 ++++--
4 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index 08db01a..5866987 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -303,7 +303,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
nr_sectors, blk_mig_read_cb, blk);
- bdrv_reset_dirty(bs, cur_sector, nr_sectors);
+ bdrv_reset_dirty_bitmap(bs, bmds->dirty_bitmap, cur_sector, nr_sectors);
qemu_mutex_unlock_iothread();
bmds->cur_sector = cur_sector + nr_sectors;
@@ -496,7 +496,8 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
g_free(blk);
}
- bdrv_reset_dirty(bmds->bs, sector, nr_sectors);
+ bdrv_reset_dirty_bitmap(bmds->bs, bmds->dirty_bitmap, sector,
+ nr_sectors);
break;
}
sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
diff --git a/block.c b/block.c
index a612594..4d12c0d 100644
--- a/block.c
+++ b/block.c
@@ -97,6 +97,10 @@ static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
static QLIST_HEAD(, BlockDriver) bdrv_drivers =
QLIST_HEAD_INITIALIZER(bdrv_drivers);
+static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
+ int nr_sectors);
+static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
+ int nr_sectors);
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
@@ -5361,8 +5365,20 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
hbitmap_iter_init(hbi, bitmap->bitmap, 0);
}
-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
- int nr_sectors)
+void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors)
+{
+ hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+}
+
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors)
+{
+ hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+}
+
+static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
+ int nr_sectors)
{
BdrvDirtyBitmap *bitmap;
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
@@ -5370,7 +5386,8 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
}
}
-void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
+static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
+ int nr_sectors)
{
BdrvDirtyBitmap *bitmap;
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
diff --git a/block/mirror.c b/block/mirror.c
index 2c6dd2a..9019d1b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -128,7 +128,8 @@ static void mirror_write_complete(void *opaque, int ret)
BlockDriverState *source = s->common.bs;
BlockErrorAction action;
- bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
+ bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
+ op->nb_sectors);
action = mirror_error_action(s, false, -ret);
if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
s->ret = ret;
@@ -145,7 +146,8 @@ static void mirror_read_complete(void *opaque, int ret)
BlockDriverState *source = s->common.bs;
BlockErrorAction action;
- bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
+ bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
+ op->nb_sectors);
action = mirror_error_action(s, true, -ret);
if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
s->ret = ret;
@@ -286,7 +288,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
next_sector += sectors_per_chunk;
}
- bdrv_reset_dirty(source, sector_num, nb_sectors);
+ bdrv_reset_dirty_bitmap(source, s->dirty_bitmap, sector_num,
+ nb_sectors);
/* Copy the dirty cluster. */
s->in_flight++;
@@ -442,7 +445,7 @@ static void coroutine_fn mirror_run(void *opaque)
assert(n > 0);
if (ret == 1) {
- bdrv_set_dirty(bs, sector_num, n);
+ bdrv_set_dirty_bitmap(bs, s->dirty_bitmap, sector_num, n);
sector_num = next;
} else {
sector_num += n;
diff --git a/include/block/block.h b/include/block/block.h
index 5450610..237ddb0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -433,8 +433,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
-void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
+void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors);
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors);
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration
2014-11-27 9:40 [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration Vladimir Sementsov-Ogievskiy
@ 2014-12-15 8:45 ` Vladimir Sementsov-Ogievskiy
2014-12-15 19:30 ` John Snow
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2014-12-15 8:45 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, John Snow, Stefan Hajnoczi, Denis V. Lunev
ping
v2 just fixes over 80 characters lines mentioned by Fam Zheng in
previous version of this patch:
[PATCH RFC] block: fix spoiling all dirty bitmaps by mirror and migration
Best regards,
Vladimir
On 27.11.2014 12:40, Vladimir Sementsov-Ogievskiy wrote:
> Mirror and migration use dirty bitmaps for their purposes, and since
> commit [block: per caller dirty bitmap] they use their own bitmaps, not
> the global one. But they use old functions bdrv_set_dirty and
> bdrv_reset_dirty, which change all dirty bitmaps.
>
> Named dirty bitmaps series by Fam and Snow are affected: mirroring and
> migration will spoil all (not related to this mirroring or migration)
> named dirty bitmaps.
>
> This patch fixes this by adding bdrv_set_dirty_bitmap and
> bdrv_reset_dirty_bitmap, which change concrete bitmap. Also, to prevent
> such mistakes in future, old functions bdrv_(set,reset)_dirty are made
> static, for internal block usage.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>
> This patch conflicts with
> [PATCH v8 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
> by John Snow, which introduces bdrv_reset_dirty_bitmap too, but it resets the whole
> bitmap, not specified sectors range.
>
> block-migration.c | 5 +++--
> block.c | 23 ++++++++++++++++++++---
> block/mirror.c | 11 +++++++----
> include/block/block.h | 6 ++++--
> 4 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/block-migration.c b/block-migration.c
> index 08db01a..5866987 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -303,7 +303,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
> nr_sectors, blk_mig_read_cb, blk);
>
> - bdrv_reset_dirty(bs, cur_sector, nr_sectors);
> + bdrv_reset_dirty_bitmap(bs, bmds->dirty_bitmap, cur_sector, nr_sectors);
> qemu_mutex_unlock_iothread();
>
> bmds->cur_sector = cur_sector + nr_sectors;
> @@ -496,7 +496,8 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> g_free(blk);
> }
>
> - bdrv_reset_dirty(bmds->bs, sector, nr_sectors);
> + bdrv_reset_dirty_bitmap(bmds->bs, bmds->dirty_bitmap, sector,
> + nr_sectors);
> break;
> }
> sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> diff --git a/block.c b/block.c
> index a612594..4d12c0d 100644
> --- a/block.c
> +++ b/block.c
> @@ -97,6 +97,10 @@ static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
> static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> QLIST_HEAD_INITIALIZER(bdrv_drivers);
>
> +static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors);
> +static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors);
> /* If non-zero, use only whitelisted block drivers */
> static int use_bdrv_whitelist;
>
> @@ -5361,8 +5365,20 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
> hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> }
>
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> - int nr_sectors)
> +void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors)
> {
> BdrvDirtyBitmap *bitmap;
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> @@ -5370,7 +5386,8 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> }
> }
>
> -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
> +static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors)
> {
> BdrvDirtyBitmap *bitmap;
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> diff --git a/block/mirror.c b/block/mirror.c
> index 2c6dd2a..9019d1b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -128,7 +128,8 @@ static void mirror_write_complete(void *opaque, int ret)
> BlockDriverState *source = s->common.bs;
> BlockErrorAction action;
>
> - bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
> + bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
> + op->nb_sectors);
> action = mirror_error_action(s, false, -ret);
> if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
> s->ret = ret;
> @@ -145,7 +146,8 @@ static void mirror_read_complete(void *opaque, int ret)
> BlockDriverState *source = s->common.bs;
> BlockErrorAction action;
>
> - bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
> + bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
> + op->nb_sectors);
> action = mirror_error_action(s, true, -ret);
> if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
> s->ret = ret;
> @@ -286,7 +288,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> next_sector += sectors_per_chunk;
> }
>
> - bdrv_reset_dirty(source, sector_num, nb_sectors);
> + bdrv_reset_dirty_bitmap(source, s->dirty_bitmap, sector_num,
> + nb_sectors);
>
> /* Copy the dirty cluster. */
> s->in_flight++;
> @@ -442,7 +445,7 @@ static void coroutine_fn mirror_run(void *opaque)
>
> assert(n > 0);
> if (ret == 1) {
> - bdrv_set_dirty(bs, sector_num, n);
> + bdrv_set_dirty_bitmap(bs, s->dirty_bitmap, sector_num, n);
> sector_num = next;
> } else {
> sector_num += n;
> diff --git a/include/block/block.h b/include/block/block.h
> index 5450610..237ddb0 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -433,8 +433,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> +void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> void bdrv_dirty_iter_init(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration
2014-11-27 9:40 [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration Vladimir Sementsov-Ogievskiy
2014-12-15 8:45 ` Vladimir Sementsov-Ogievskiy
@ 2014-12-15 19:30 ` John Snow
2014-12-16 0:57 ` Fam Zheng
2014-12-16 9:40 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: John Snow @ 2014-12-15 19:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: Kevin Wolf, Denis V. Lunev, Fam Zheng, Stefan Hajnoczi
On 11/27/2014 04:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> Mirror and migration use dirty bitmaps for their purposes, and since
> commit [block: per caller dirty bitmap] they use their own bitmaps, not
> the global one. But they use old functions bdrv_set_dirty and
> bdrv_reset_dirty, which change all dirty bitmaps.
>
> Named dirty bitmaps series by Fam and Snow are affected: mirroring and
> migration will spoil all (not related to this mirroring or migration)
> named dirty bitmaps.
>
> This patch fixes this by adding bdrv_set_dirty_bitmap and
> bdrv_reset_dirty_bitmap, which change concrete bitmap. Also, to prevent
> such mistakes in future, old functions bdrv_(set,reset)_dirty are made
> static, for internal block usage.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>
> This patch conflicts with
> [PATCH v8 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
> by John Snow, which introduces bdrv_reset_dirty_bitmap too, but it resets the whole
> bitmap, not specified sectors range.
>
> block-migration.c | 5 +++--
> block.c | 23 ++++++++++++++++++++---
> block/mirror.c | 11 +++++++----
> include/block/block.h | 6 ++++--
> 4 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/block-migration.c b/block-migration.c
> index 08db01a..5866987 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -303,7 +303,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
> nr_sectors, blk_mig_read_cb, blk);
>
> - bdrv_reset_dirty(bs, cur_sector, nr_sectors);
> + bdrv_reset_dirty_bitmap(bs, bmds->dirty_bitmap, cur_sector, nr_sectors);
> qemu_mutex_unlock_iothread();
>
> bmds->cur_sector = cur_sector + nr_sectors;
> @@ -496,7 +496,8 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> g_free(blk);
> }
>
> - bdrv_reset_dirty(bmds->bs, sector, nr_sectors);
> + bdrv_reset_dirty_bitmap(bmds->bs, bmds->dirty_bitmap, sector,
> + nr_sectors);
> break;
> }
> sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> diff --git a/block.c b/block.c
> index a612594..4d12c0d 100644
> --- a/block.c
> +++ b/block.c
> @@ -97,6 +97,10 @@ static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
> static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> QLIST_HEAD_INITIALIZER(bdrv_drivers);
>
> +static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors);
> +static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors);
> /* If non-zero, use only whitelisted block drivers */
> static int use_bdrv_whitelist;
>
> @@ -5361,8 +5365,20 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
> hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> }
>
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> - int nr_sectors)
> +void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors)
> {
> BdrvDirtyBitmap *bitmap;
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> @@ -5370,7 +5386,8 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> }
> }
>
> -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
> +static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors)
> {
> BdrvDirtyBitmap *bitmap;
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> diff --git a/block/mirror.c b/block/mirror.c
> index 2c6dd2a..9019d1b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -128,7 +128,8 @@ static void mirror_write_complete(void *opaque, int ret)
> BlockDriverState *source = s->common.bs;
> BlockErrorAction action;
>
> - bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
> + bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
> + op->nb_sectors);
> action = mirror_error_action(s, false, -ret);
> if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
> s->ret = ret;
> @@ -145,7 +146,8 @@ static void mirror_read_complete(void *opaque, int ret)
> BlockDriverState *source = s->common.bs;
> BlockErrorAction action;
>
> - bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
> + bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
> + op->nb_sectors);
> action = mirror_error_action(s, true, -ret);
> if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
> s->ret = ret;
> @@ -286,7 +288,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> next_sector += sectors_per_chunk;
> }
>
> - bdrv_reset_dirty(source, sector_num, nb_sectors);
> + bdrv_reset_dirty_bitmap(source, s->dirty_bitmap, sector_num,
> + nb_sectors);
>
> /* Copy the dirty cluster. */
> s->in_flight++;
> @@ -442,7 +445,7 @@ static void coroutine_fn mirror_run(void *opaque)
>
> assert(n > 0);
> if (ret == 1) {
> - bdrv_set_dirty(bs, sector_num, n);
> + bdrv_set_dirty_bitmap(bs, s->dirty_bitmap, sector_num, n);
> sector_num = next;
> } else {
> sector_num += n;
> diff --git a/include/block/block.h b/include/block/block.h
> index 5450610..237ddb0 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -433,8 +433,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> +void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> void bdrv_dirty_iter_init(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>
Oh, good catch. I'll update my series accordingly to keep consistent naming.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration
2014-11-27 9:40 [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration Vladimir Sementsov-Ogievskiy
2014-12-15 8:45 ` Vladimir Sementsov-Ogievskiy
2014-12-15 19:30 ` John Snow
@ 2014-12-16 0:57 ` Fam Zheng
2014-12-16 9:40 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Fam Zheng @ 2014-12-16 0:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, Denis V. Lunev, John Snow, qemu-devel, Stefan Hajnoczi
On Thu, 11/27 12:40, Vladimir Sementsov-Ogievskiy wrote:
> Mirror and migration use dirty bitmaps for their purposes, and since
> commit [block: per caller dirty bitmap] they use their own bitmaps, not
> the global one. But they use old functions bdrv_set_dirty and
> bdrv_reset_dirty, which change all dirty bitmaps.
>
> Named dirty bitmaps series by Fam and Snow are affected: mirroring and
> migration will spoil all (not related to this mirroring or migration)
> named dirty bitmaps.
>
> This patch fixes this by adding bdrv_set_dirty_bitmap and
> bdrv_reset_dirty_bitmap, which change concrete bitmap. Also, to prevent
> such mistakes in future, old functions bdrv_(set,reset)_dirty are made
> static, for internal block usage.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>
> This patch conflicts with
> [PATCH v8 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
> by John Snow, which introduces bdrv_reset_dirty_bitmap too, but it resets the whole
> bitmap, not specified sectors range.
>
> block-migration.c | 5 +++--
> block.c | 23 ++++++++++++++++++++---
> block/mirror.c | 11 +++++++----
> include/block/block.h | 6 ++++--
> 4 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/block-migration.c b/block-migration.c
> index 08db01a..5866987 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -303,7 +303,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
> nr_sectors, blk_mig_read_cb, blk);
>
> - bdrv_reset_dirty(bs, cur_sector, nr_sectors);
> + bdrv_reset_dirty_bitmap(bs, bmds->dirty_bitmap, cur_sector, nr_sectors);
> qemu_mutex_unlock_iothread();
>
> bmds->cur_sector = cur_sector + nr_sectors;
> @@ -496,7 +496,8 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> g_free(blk);
> }
>
> - bdrv_reset_dirty(bmds->bs, sector, nr_sectors);
> + bdrv_reset_dirty_bitmap(bmds->bs, bmds->dirty_bitmap, sector,
> + nr_sectors);
> break;
> }
> sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
> diff --git a/block.c b/block.c
> index a612594..4d12c0d 100644
> --- a/block.c
> +++ b/block.c
> @@ -97,6 +97,10 @@ static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
> static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> QLIST_HEAD_INITIALIZER(bdrv_drivers);
>
> +static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors);
> +static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors);
> /* If non-zero, use only whitelisted block drivers */
> static int use_bdrv_whitelist;
>
> @@ -5361,8 +5365,20 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
> hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> }
>
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> - int nr_sectors)
> +void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors)
> {
> BdrvDirtyBitmap *bitmap;
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> @@ -5370,7 +5386,8 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> }
> }
>
> -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
> +static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors)
> {
> BdrvDirtyBitmap *bitmap;
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> diff --git a/block/mirror.c b/block/mirror.c
> index 2c6dd2a..9019d1b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -128,7 +128,8 @@ static void mirror_write_complete(void *opaque, int ret)
> BlockDriverState *source = s->common.bs;
> BlockErrorAction action;
>
> - bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
> + bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
> + op->nb_sectors);
> action = mirror_error_action(s, false, -ret);
> if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
> s->ret = ret;
> @@ -145,7 +146,8 @@ static void mirror_read_complete(void *opaque, int ret)
> BlockDriverState *source = s->common.bs;
> BlockErrorAction action;
>
> - bdrv_set_dirty(source, op->sector_num, op->nb_sectors);
> + bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
> + op->nb_sectors);
> action = mirror_error_action(s, true, -ret);
> if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
> s->ret = ret;
> @@ -286,7 +288,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> next_sector += sectors_per_chunk;
> }
>
> - bdrv_reset_dirty(source, sector_num, nb_sectors);
> + bdrv_reset_dirty_bitmap(source, s->dirty_bitmap, sector_num,
> + nb_sectors);
>
> /* Copy the dirty cluster. */
> s->in_flight++;
> @@ -442,7 +445,7 @@ static void coroutine_fn mirror_run(void *opaque)
>
> assert(n > 0);
> if (ret == 1) {
> - bdrv_set_dirty(bs, sector_num, n);
> + bdrv_set_dirty_bitmap(bs, s->dirty_bitmap, sector_num, n);
> sector_num = next;
> } else {
> sector_num += n;
> diff --git a/include/block/block.h b/include/block/block.h
> index 5450610..237ddb0 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -433,8 +433,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> +void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> void bdrv_dirty_iter_init(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> --
> 1.9.1
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration
2014-11-27 9:40 [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2014-12-16 0:57 ` Fam Zheng
@ 2014-12-16 9:40 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2014-12-16 9:40 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: Kevin Wolf, Denis V. Lunev, John Snow, Fam Zheng, Stefan Hajnoczi
On 2014-11-27 at 10:40, Vladimir Sementsov-Ogievskiy wrote:
> Mirror and migration use dirty bitmaps for their purposes, and since
> commit [block: per caller dirty bitmap] they use their own bitmaps, not
> the global one. But they use old functions bdrv_set_dirty and
> bdrv_reset_dirty, which change all dirty bitmaps.
>
> Named dirty bitmaps series by Fam and Snow are affected: mirroring and
> migration will spoil all (not related to this mirroring or migration)
> named dirty bitmaps.
>
> This patch fixes this by adding bdrv_set_dirty_bitmap and
> bdrv_reset_dirty_bitmap, which change concrete bitmap. Also, to prevent
> such mistakes in future, old functions bdrv_(set,reset)_dirty are made
> static, for internal block usage.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>
> This patch conflicts with
> [PATCH v8 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
> by John Snow, which introduces bdrv_reset_dirty_bitmap too, but it resets the whole
> bitmap, not specified sectors range.
>
> block-migration.c | 5 +++--
> block.c | 23 ++++++++++++++++++++---
> block/mirror.c | 11 +++++++----
> include/block/block.h | 6 ++++--
> 4 files changed, 34 insertions(+), 11 deletions(-)
Thanks, applied to my block tree:
https://github.com/XanClic/qemu/commits/block
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-12-16 9:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-27 9:40 [Qemu-devel] [PATCH v2] block: fix spoiling all dirty bitmaps by mirror and migration Vladimir Sementsov-Ogievskiy
2014-12-15 8:45 ` Vladimir Sementsov-Ogievskiy
2014-12-15 19:30 ` John Snow
2014-12-16 0:57 ` Fam Zheng
2014-12-16 9:40 ` Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.