From patchwork Thu Apr 20 12:00:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 752773 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3w7yPc72bnz9s2x for ; Thu, 20 Apr 2017 22:10:44 +1000 (AEST) Received: from localhost ([::1]:53570 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d1Av4-0005ws-6l for incoming@patchwork.ozlabs.org; Thu, 20 Apr 2017 08:10:42 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38745) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d1Amq-0007ga-4n for qemu-devel@nongnu.org; Thu, 20 Apr 2017 08:02:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d1Ami-00034a-PY for qemu-devel@nongnu.org; Thu, 20 Apr 2017 08:02:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41376) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1d1Am3-0002Ym-8L; Thu, 20 Apr 2017 08:01:23 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2B43767EDD; Thu, 20 Apr 2017 12:01:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2B43767EDD Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=pbonzini@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 2B43767EDD Received: from donizetti.redhat.com (ovpn-117-9.ams2.redhat.com [10.36.117.9]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v3KC0w45002037; Thu, 20 Apr 2017 08:01:21 -0400 From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Thu, 20 Apr 2017 14:00:56 +0200 Message-Id: <20170420120058.28404-16-pbonzini@redhat.com> In-Reply-To: <20170420120058.28404-1-pbonzini@redhat.com> References: <20170420120058.28404-1-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 20 Apr 2017 12:01:22 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 15/17] block: introduce dirty_bitmap_mutex X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: qemu-block@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" It protects only the list of dirty bitmaps; in the next patch we will also protect their content. Signed-off-by: Paolo Bonzini --- block/dirty-bitmap.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++- block/mirror.c | 3 ++- blockdev.c | 44 +++++++--------------------------------- include/block/block_int.h | 5 +++++ migration/block.c | 6 ------ 5 files changed, 64 insertions(+), 45 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 519737c..e13718e 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -52,6 +52,24 @@ struct BdrvDirtyBitmapIter { BdrvDirtyBitmap *bitmap; }; +static QemuMutex dirty_bitmap_mutex; + +static void __attribute__((__constructor__)) bdrv_dirty_bitmaps_init_lock(void) +{ + qemu_mutex_init(&dirty_bitmap_mutex); +} + +static inline void bdrv_dirty_bitmaps_lock(BlockDriverState *bs) +{ + qemu_mutex_lock(&dirty_bitmap_mutex); +} + +static inline void bdrv_dirty_bitmaps_unlock(BlockDriverState *bs) +{ + qemu_mutex_unlock(&dirty_bitmap_mutex); +} + +/* Called with BQL or dirty_bitmap lock taken. */ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name) { BdrvDirtyBitmap *bm; @@ -65,6 +83,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name) return NULL; } +/* Called with BQL taken. */ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap) { assert(!bdrv_dirty_bitmap_frozen(bitmap)); @@ -72,6 +91,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap) bitmap->name = NULL; } +/* Called with BQL taken. */ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, uint32_t granularity, const char *name, @@ -100,7 +120,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, bitmap->size = bitmap_size; bitmap->name = g_strdup(name); bitmap->disabled = false; + bdrv_dirty_bitmaps_lock(bs); QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); + bdrv_dirty_bitmaps_unlock(bs); return bitmap; } @@ -164,16 +186,19 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) return bitmap->name; } +/* Called with BQL taken. */ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) { return bitmap->successor; } +/* Called with BQL taken. */ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) { return !(bitmap->disabled || bitmap->successor); } +/* Called with BQL taken. */ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) { if (bdrv_dirty_bitmap_frozen(bitmap)) { @@ -188,6 +213,7 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) /** * Create a successor bitmap destined to replace this bitmap after an operation. * Requires that the bitmap is not frozen and has no successor. + * Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp) @@ -220,6 +246,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, /** * For a bitmap with a successor, yield our name to the successor, * delete the old bitmap, and return a handle to the new bitmap. + * Called with BQL taken. */ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, @@ -247,6 +274,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, * In cases of failure where we can no longer safely delete the parent, * we may wish to re-join the parent and child/successor. * The merged parent will be un-frozen, but not explicitly re-enabled. + * Called with BQL taken. */ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *parent, @@ -271,25 +299,30 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, /** * Truncates _all_ bitmaps attached to a BDS. + * Called with BQL taken. */ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) { BdrvDirtyBitmap *bitmap; uint64_t size = bdrv_nb_sectors(bs); + bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { assert(!bdrv_dirty_bitmap_frozen(bitmap)); assert(!bitmap->active_iterators); hbitmap_truncate(bitmap->bitmap, size); bitmap->size = size; } + bdrv_dirty_bitmaps_unlock(bs); } +/* Called with BQL taken. */ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, bool only_named) { BdrvDirtyBitmap *bm, *next; + bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) { assert(!bm->active_iterators); @@ -301,15 +334,19 @@ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, g_free(bm); if (bitmap) { - return; + goto out; } } } if (bitmap) { abort(); } + +out: + bdrv_dirty_bitmaps_unlock(bs); } +/* Called with BQL taken. */ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) { bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false); @@ -318,18 +355,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) /** * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). * There must not be any frozen bitmaps attached. + * Called with BQL taken. */ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) { bdrv_do_release_matching_dirty_bitmap(bs, NULL, true); } +/* Called with BQL taken. */ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { assert(!bdrv_dirty_bitmap_frozen(bitmap)); bitmap->disabled = true; } +/* Called with BQL taken. */ void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { assert(!bdrv_dirty_bitmap_frozen(bitmap)); @@ -342,6 +382,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) BlockDirtyInfoList *list = NULL; BlockDirtyInfoList **plist = &list; + bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1); BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1); @@ -354,6 +395,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) *plist = entry; plist = &entry->next; } + bdrv_dirty_bitmaps_unlock(bs); return list; } @@ -508,12 +550,19 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int64_t nr_sectors) { BdrvDirtyBitmap *bitmap; + + if (QLIST_EMPTY(&bs->dirty_bitmaps)) { + return; + } + + bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { if (!bdrv_dirty_bitmap_enabled(bitmap)) { continue; } hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); } + bdrv_dirty_bitmaps_unlock(bs); } /** diff --git a/block/mirror.c b/block/mirror.c index 2e05dac..dc227a2 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -506,6 +506,8 @@ static void mirror_exit(BlockJob *job, void *opaque) BlockDriverState *mirror_top_bs = s->mirror_top_bs; Error *local_err = NULL; + bdrv_release_dirty_bitmap(src, s->dirty_bitmap); + /* Make sure that the source BDS doesn't go away before we called * block_job_completed(). */ bdrv_ref(src); @@ -899,7 +901,6 @@ immediate_exit: g_free(s->cow_bitmap); g_free(s->in_flight_bitmap); bdrv_dirty_iter_free(s->dbi); - bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); data = g_malloc(sizeof(*data)); data->ret = ret; diff --git a/blockdev.c b/blockdev.c index e9b5717..09325f2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1364,12 +1364,10 @@ out_aio_context: static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node, const char *name, BlockDriverState **pbs, - AioContext **paio, Error **errp) { BlockDriverState *bs; BdrvDirtyBitmap *bitmap; - AioContext *aio_context; if (!node) { error_setg(errp, "Node cannot be NULL"); @@ -1385,29 +1383,17 @@ static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node, return NULL; } - aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(aio_context); - bitmap = bdrv_find_dirty_bitmap(bs, name); if (!bitmap) { error_setg(errp, "Dirty bitmap '%s' not found", name); - goto fail; + return NULL; } if (pbs) { *pbs = bs; } - if (paio) { - *paio = aio_context; - } else { - aio_context_release(aio_context); - } return bitmap; - - fail: - aio_context_release(aio_context); - return NULL; } /* New and old BlockDriverState structs for atomic group operations */ @@ -2024,7 +2010,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, &state->bs, - &state->aio_context, errp); if (!state->bitmap) { return; @@ -2733,7 +2718,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, bool has_granularity, uint32_t granularity, Error **errp) { - AioContext *aio_context; BlockDriverState *bs; if (!name || name[0] == '\0') { @@ -2746,14 +2730,11 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, return; } - aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(aio_context); - if (has_granularity) { if (granularity < 512 || !is_power_of_2(granularity)) { error_setg(errp, "Granularity must be power of 2 " "and at least 512"); - goto out; + return; } } else { /* Default to cluster size, if available: */ @@ -2761,19 +2742,15 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, } bdrv_create_dirty_bitmap(bs, granularity, name, errp); - - out: - aio_context_release(aio_context); } void qmp_block_dirty_bitmap_remove(const char *node, const char *name, Error **errp) { - AioContext *aio_context; BlockDriverState *bs; BdrvDirtyBitmap *bitmap; - bitmap = block_dirty_bitmap_lookup(node, name, &bs, &aio_context, errp); + bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp); if (!bitmap || !bs) { return; } @@ -2782,13 +2759,10 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, error_setg(errp, "Bitmap '%s' is currently frozen and cannot be removed", name); - goto out; + return; } bdrv_dirty_bitmap_make_anon(bitmap); bdrv_release_dirty_bitmap(bs, bitmap); - - out: - aio_context_release(aio_context); } /** @@ -2798,11 +2772,10 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, void qmp_block_dirty_bitmap_clear(const char *node, const char *name, Error **errp) { - AioContext *aio_context; BdrvDirtyBitmap *bitmap; BlockDriverState *bs; - bitmap = block_dirty_bitmap_lookup(node, name, &bs, &aio_context, errp); + bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp); if (!bitmap || !bs) { return; } @@ -2811,18 +2784,15 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, error_setg(errp, "Bitmap '%s' is currently frozen and cannot be modified", name); - goto out; + return; } else if (!bdrv_dirty_bitmap_enabled(bitmap)) { error_setg(errp, "Bitmap '%s' is currently disabled and cannot be cleared", name); - goto out; + return; } bdrv_clear_dirty_bitmap(bitmap, NULL); - - out: - aio_context_release(aio_context); } void hmp_drive_del(Monitor *mon, const QDict *qdict) diff --git a/include/block/block_int.h b/include/block/block_int.h index b298de8..03db2cf 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -596,6 +596,11 @@ struct BlockDriverState { uint64_t write_threshold_offset; NotifierWithReturn write_threshold_notifier; + /* 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. */ + QemuMutex dirty_bitmap_mutex; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; /* Offset after the highest byte written to */ diff --git a/migration/block.c b/migration/block.c index 7734ff7..c33b522 100644 --- a/migration/block.c +++ b/migration/block.c @@ -346,10 +346,8 @@ static int set_dirty_tracking(void) int ret; QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { - aio_context_acquire(blk_get_aio_context(bmds->blk)); bmds->dirty_bitmap = bdrv_create_dirty_bitmap(blk_bs(bmds->blk), BLOCK_SIZE, NULL, NULL); - aio_context_release(blk_get_aio_context(bmds->blk)); if (!bmds->dirty_bitmap) { ret = -errno; goto fail; @@ -360,9 +358,7 @@ static int set_dirty_tracking(void) fail: QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { if (bmds->dirty_bitmap) { - aio_context_acquire(blk_get_aio_context(bmds->blk)); bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap); - aio_context_release(blk_get_aio_context(bmds->blk)); } } return ret; @@ -375,9 +371,7 @@ static void unset_dirty_tracking(void) BlkMigDevState *bmds; QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { - aio_context_acquire(blk_get_aio_context(bmds->blk)); bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap); - aio_context_release(blk_get_aio_context(bmds->blk)); } }