Skip to content

Commit

Permalink
block/qcow2: proper locking on bitmap add/remove paths
Browse files Browse the repository at this point in the history 
qmp_block_dirty_bitmap_add and do_block_dirty_bitmap_remove do acquire
aio context since 0a6c86d. But this is not enough: we also must
lock qcow2 mutex when access in-image metadata. Especially it concerns
freeing qcow2 clusters.

To achieve this, move qcow2_can_store_new_dirty_bitmap and
qcow2_remove_persistent_dirty_bitmap to coroutine context.

Since we work in coroutines in correct aio context, we don't need
context acquiring in blockdev.c anymore, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190920082543.23444-4-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
  • Loading branch information
@jnsnow
Vladimir Sementsov-Ogievskiy authored and jnsnow committed Oct 17, 2019
1 parent b56a1e3 commit d2c3080
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 48 deletions.
102 changes: 94 additions & 8 deletions block/dirty-bitmap.c
View file
Expand Up @@ -26,6 +26,7 @@
#include "trace.h"
#include "block/block_int.h"
#include "block/blockjob.h"
#include "qemu/main-loop.h"

struct BdrvDirtyBitmap {
QemuMutex *mutex;
Expand Down Expand Up @@ -455,18 +456,59 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
* not fail.
* This function doesn't release corresponding BdrvDirtyBitmap.
*/
int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
static int coroutine_fn
bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
{
if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
if (bs->drv && bs->drv->bdrv_co_remove_persistent_dirty_bitmap) {
return bs->drv->bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
}

return 0;
}

bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp)
typedef struct BdrvRemovePersistentDirtyBitmapCo {
BlockDriverState *bs;
const char *name;
Error **errp;
int ret;
} BdrvRemovePersistentDirtyBitmapCo;

static void coroutine_fn
bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
{
BdrvRemovePersistentDirtyBitmapCo *s = opaque;

s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
aio_wait_kick();
}

int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
{
if (qemu_in_coroutine()) {
return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
} else {
Coroutine *co;
BdrvRemovePersistentDirtyBitmapCo s = {
.bs = bs,
.name = name,
.errp = errp,
.ret = -EINPROGRESS,
};

co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry,
&s);
bdrv_coroutine_enter(bs, co);
BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS);

return s.ret;
}
}

static bool coroutine_fn
bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp)
{
BlockDriver *drv = bs->drv;

Expand All @@ -477,14 +519,58 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
return false;
}

if (!drv->bdrv_can_store_new_dirty_bitmap) {
if (!drv->bdrv_co_can_store_new_dirty_bitmap) {
error_setg_errno(errp, ENOTSUP,
"Can't store persistent bitmaps to %s",
bdrv_get_device_or_node_name(bs));
return false;
}

return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
return drv->bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
}

typedef struct BdrvCanStoreNewDirtyBitmapCo {
BlockDriverState *bs;
const char *name;
uint32_t granularity;
Error **errp;
bool ret;

bool in_progress;
} BdrvCanStoreNewDirtyBitmapCo;

static void coroutine_fn bdrv_co_can_store_new_dirty_bitmap_entry(void *opaque)
{
BdrvCanStoreNewDirtyBitmapCo *s = opaque;

s->ret = bdrv_co_can_store_new_dirty_bitmap(s->bs, s->name, s->granularity,
s->errp);
s->in_progress = false;
aio_wait_kick();
}

bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp)
{
if (qemu_in_coroutine()) {
return bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
} else {
Coroutine *co;
BdrvCanStoreNewDirtyBitmapCo s = {
.bs = bs,
.name = name,
.granularity = granularity,
.errp = errp,
.in_progress = true,
};

co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry,
&s);
bdrv_coroutine_enter(bs, co);
BDRV_POLL_WHILE(bs, s.in_progress);

return s.ret;
}
}

void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
Expand Down
24 changes: 16 additions & 8 deletions block/qcow2-bitmap.c
View file
Expand Up @@ -1404,12 +1404,13 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
return NULL;
}

int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp)
{
int ret;
BDRVQcow2State *s = bs->opaque;
Qcow2Bitmap *bm;
Qcow2Bitmap *bm = NULL;
Qcow2BitmapList *bm_list;

if (s->nb_bitmaps == 0) {
Expand All @@ -1418,10 +1419,13 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
return 0;
}

qemu_co_mutex_lock(&s->lock);

bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
if (bm_list == NULL) {
return -EIO;
ret = -EIO;
goto out;
}

bm = find_bitmap_by_name(bm_list, name);
Expand All @@ -1441,6 +1445,8 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
free_bitmap_clusters(bs, &bm->table);

out:
qemu_co_mutex_unlock(&s->lock);

bitmap_free(bm);
bitmap_list_free(bm_list);

Expand Down Expand Up @@ -1615,10 +1621,10 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
return 0;
}

bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
Error **errp)
bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
Error **errp)
{
BDRVQcow2State *s = bs->opaque;
bool found;
Expand Down Expand Up @@ -1655,8 +1661,10 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
goto fail;
}

qemu_co_mutex_lock(&s->lock);
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
qemu_co_mutex_unlock(&s->lock);
if (bm_list == NULL) {
goto fail;
}
Expand Down
5 changes: 3 additions & 2 deletions block/qcow2.c
View file
Expand Up @@ -5407,8 +5407,9 @@ BlockDriver bdrv_qcow2 = {
.bdrv_attach_aio_context = qcow2_attach_aio_context,

.bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
.bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
.bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
.bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap,
.bdrv_co_remove_persistent_dirty_bitmap =
qcow2_co_remove_persistent_dirty_bitmap,
};

static void bdrv_qcow2_init(void)
Expand Down
11 changes: 6 additions & 5 deletions block/qcow2.h
View file
Expand Up @@ -746,12 +746,13 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
Error **errp);
int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
Error **errp);
int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);

ssize_t coroutine_fn
qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
Expand Down
27 changes: 7 additions & 20 deletions blockdev.c
View file
Expand Up @@ -2898,16 +2898,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
disabled = false;
}

if (persistent) {
AioContext *aio_context = bdrv_get_aio_context(bs);
bool ok;

aio_context_acquire(aio_context);
ok = bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
aio_context_release(aio_context);
if (!ok) {
return;
}
if (persistent &&
!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
{
return;
}

bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
Expand Down Expand Up @@ -2939,17 +2933,10 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
return NULL;
}

if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
int ret;
AioContext *aio_context = bdrv_get_aio_context(bs);

aio_context_acquire(aio_context);
ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
aio_context_release(aio_context);

if (ret < 0) {
if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
{
return NULL;
}
}

if (release) {
Expand Down
10 changes: 5 additions & 5 deletions include/block/block_int.h
View file
Expand Up @@ -553,13 +553,13 @@ struct BlockDriver {
* field of BlockDirtyBitmap's in case of success.
*/
int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
uint32_t granularity,
Error **errp);
int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
uint32_t granularity,
Error **errp);
int (*bdrv_co_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
const char *name,
Error **errp);

/**
* Register/unregister a buffer for I/O. For example, when the driver is
Expand Down

0 comments on commit d2c3080

Please sign in to comment.