* [Qemu-devel] [PATCH 1/7] block: Add BlockOpType enum
2014-05-23 3:23 [Qemu-devel] [PATCH 0/7] block: Drop in_use with op blocker Fam Zheng
@ 2014-05-23 3:23 ` Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 2/7] block: Introduce op_blockers to BlockDriverState Fam Zheng
` (5 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Fam Zheng @ 2014-05-23 3:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Benoit Canet, jcody, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
This adds the enum of all the operations that can be taken on a block
device.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/block/block.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/include/block/block.h b/include/block/block.h
index 59be83f..cc4cc16 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -162,6 +162,25 @@ typedef struct BDRVReopenState {
void *opaque;
} BDRVReopenState;
+/*
+ * Block operation types
+ */
+typedef enum BlockOpType {
+ BLOCK_OP_TYPE_BACKUP_SOURCE,
+ BLOCK_OP_TYPE_BACKUP_TARGET,
+ BLOCK_OP_TYPE_CHANGE,
+ BLOCK_OP_TYPE_COMMIT,
+ BLOCK_OP_TYPE_DATAPLANE,
+ BLOCK_OP_TYPE_DRIVE_DEL,
+ BLOCK_OP_TYPE_EJECT,
+ BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+ BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
+ BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
+ BLOCK_OP_TYPE_MIRROR,
+ BLOCK_OP_TYPE_RESIZE,
+ BLOCK_OP_TYPE_STREAM,
+ BLOCK_OP_TYPE_MAX,
+} BlockOpType;
void bdrv_iostatus_enable(BlockDriverState *bs);
void bdrv_iostatus_reset(BlockDriverState *bs);
--
1.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 2/7] block: Introduce op_blockers to BlockDriverState
2014-05-23 3:23 [Qemu-devel] [PATCH 0/7] block: Drop in_use with op blocker Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 1/7] block: Add BlockOpType enum Fam Zheng
@ 2014-05-23 3:23 ` Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 3/7] block: Replace in_use with operation blocker Fam Zheng
` (4 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Fam Zheng @ 2014-05-23 3:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Benoit Canet, jcody, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
BlockDriverState.op_blockers is an array of lists with BLOCK_OP_TYPE_MAX
elements. Each list is a list of blockers of an operation type
(BlockOpType), that marks this BDS as currently blocked for a certain
type of operation with reason errors stored in the list. The rule of
usage is:
* BDS user who wants to take an operation should check if there's any
blocker of the type with bdrv_op_is_blocked().
* BDS user who wants to block certain types of operation, should call
bdrv_op_block (or bdrv_op_block_all to block all types of operations,
which is similar to the existing bdrv_set_in_use()).
* A blocker is only referenced by op_blockers, so the lifecycle is
managed by caller, and shouldn't be lost until unblock, so typically
a caller does these:
- Allocate a blocker with error_setg or similar, call bdrv_op_block()
to block some operations.
- Hold the blocker, do his job.
- Unblock operations that it blocked, with the same reason pointer
passed to bdrv_op_unblock().
- Release the blocker with error_free().
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 7 +++++
include/block/block_int.h | 5 ++++
3 files changed, 88 insertions(+)
diff --git a/block.c b/block.c
index 40c5e1a..589c6e3 100644
--- a/block.c
+++ b/block.c
@@ -335,6 +335,7 @@ void bdrv_register(BlockDriver *bdrv)
BlockDriverState *bdrv_new(const char *device_name, Error **errp)
{
BlockDriverState *bs;
+ int i;
if (bdrv_find(device_name)) {
error_setg(errp, "Device with id '%s' already exists",
@@ -353,6 +354,9 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp)
if (device_name[0] != '\0') {
QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
}
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ QLIST_INIT(&bs->op_blockers[i]);
+ }
bdrv_iostatus_disable(bs);
notifier_list_init(&bs->close_notifiers);
notifier_with_return_list_init(&bs->before_write_notifiers);
@@ -1952,6 +1956,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
bs_src->device_name);
bs_dest->device_list = bs_src->device_list;
+ memcpy(bs_dest->op_blockers, bs_src->op_blockers,
+ sizeof(bs_dest->op_blockers));
}
/*
@@ -5325,6 +5331,76 @@ void bdrv_unref(BlockDriverState *bs)
}
}
+struct BdrvOpBlocker {
+ Error *reason;
+ QLIST_ENTRY(BdrvOpBlocker) list;
+};
+
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
+{
+ BdrvOpBlocker *blocker;
+ assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+ if (!QLIST_EMPTY(&bs->op_blockers[op])) {
+ blocker = QLIST_FIRST(&bs->op_blockers[op]);
+ if (errp) {
+ error_setg(errp, "Device '%s' is busy: %s",
+ bs->device_name, error_get_pretty(blocker->reason));
+ }
+ return true;
+ }
+ return false;
+}
+
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+ BdrvOpBlocker *blocker;
+ assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+
+ blocker = g_malloc0(sizeof(BdrvOpBlocker));
+ blocker->reason = reason;
+ QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
+}
+
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+ BdrvOpBlocker *blocker, *next;
+ assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+ QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
+ if (blocker->reason == reason) {
+ QLIST_REMOVE(blocker, list);
+ g_free(blocker);
+ }
+ }
+}
+
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason)
+{
+ int i;
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ bdrv_op_block(bs, i, reason);
+ }
+}
+
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason)
+{
+ int i;
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ bdrv_op_unblock(bs, i, reason);
+ }
+}
+
+bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
+{
+ int i;
+
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ if (!QLIST_EMPTY(&bs->op_blockers[i])) {
+ return false;
+ }
+ }
+ return true;
+}
+
void bdrv_set_in_use(BlockDriverState *bs, int in_use)
{
assert(bs->in_use != in_use);
diff --git a/include/block/block.h b/include/block/block.h
index cc4cc16..7f0448c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -475,6 +475,13 @@ void bdrv_unref(BlockDriverState *bs);
void bdrv_set_in_use(BlockDriverState *bs, int in_use);
int bdrv_in_use(BlockDriverState *bs);
+bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason);
+void bdrv_op_block_all(BlockDriverState *bs, Error *reason);
+void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason);
+bool bdrv_op_blocker_is_empty(BlockDriverState *bs);
+
#ifdef CONFIG_LINUX_AIO
int raw_get_aio_fd(BlockDriverState *bs);
#else
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b8cc926..fdf2a7d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -270,6 +270,8 @@ typedef struct BlockLimits {
size_t opt_mem_alignment;
} BlockLimits;
+typedef struct BdrvOpBlocker BdrvOpBlocker;
+
/*
* Note: the function bdrv_append() copies and swaps contents of
* BlockDriverStates, so if you add new fields to this struct, please
@@ -360,6 +362,9 @@ struct BlockDriverState {
QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
+ /* operation blockers */
+ QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX];
+
/* long-running background operation */
BlockJob *job;
--
1.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 3/7] block: Replace in_use with operation blocker
2014-05-23 3:23 [Qemu-devel] [PATCH 0/7] block: Drop in_use with op blocker Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 1/7] block: Add BlockOpType enum Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 2/7] block: Introduce op_blockers to BlockDriverState Fam Zheng
@ 2014-05-23 3:23 ` Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 4/7] block: Move op_blocker check from block_job_create to its caller Fam Zheng
` (3 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Fam Zheng @ 2014-05-23 3:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Benoit Canet, jcody, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
This drops BlockDriverState.in_use with op_blockers:
- Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1).
- Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0).
- Check bdrv_op_is_blocked() in place of bdrv_in_use(bs).
The specific types are used, e.g. in place of starting block backup,
bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...).
There is one exception in block_job_create, where
bdrv_op_blocker_is_empty() is used, because we don't know the operation
type here. This doesn't matter because in a few commits away we will drop
the check and move it to callers that _do_ know the type.
- Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).
Note: there is only bdrv_op_block_all and bdrv_op_unblock_all callers at
this moment. So although the checks are specific to op types, this
changes can still be seen as identical logic with previously with
in_use. The difference is error message are improved because of blocker
error info.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block-migration.c | 7 +++++--
block.c | 24 +++++++-----------------
blockdev.c | 19 +++++++++----------
blockjob.c | 14 +++++++++-----
hw/block/dataplane/virtio-blk.c | 18 ++++++++++++------
include/block/block.h | 2 --
include/block/block_int.h | 1 -
include/block/blockjob.h | 3 +++
8 files changed, 45 insertions(+), 43 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index 56951e0..1656270 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -59,6 +59,7 @@ typedef struct BlkMigDevState {
unsigned long *aio_bitmap;
int64_t completed_sectors;
BdrvDirtyBitmap *dirty_bitmap;
+ Error *blocker;
} BlkMigDevState;
typedef struct BlkMigBlock {
@@ -361,7 +362,8 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
bmds->completed_sectors = 0;
bmds->shared_base = block_mig_state.shared_base;
alloc_aio_bitmap(bmds);
- bdrv_set_in_use(bs, 1);
+ error_setg(&bmds->blocker, "block device is in use by migration");
+ bdrv_op_block_all(bs, bmds->blocker);
bdrv_ref(bs);
block_mig_state.total_sector_sum += sectors;
@@ -599,7 +601,8 @@ static void blk_mig_cleanup(void)
blk_mig_lock();
while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
- bdrv_set_in_use(bmds->bs, 0);
+ bdrv_op_unblock_all(bmds->bs, bmds->blocker);
+ error_free(bmds->blocker);
bdrv_unref(bmds->bs);
g_free(bmds->aio_bitmap);
g_free(bmds);
diff --git a/block.c b/block.c
index 589c6e3..64c48a5 100644
--- a/block.c
+++ b/block.c
@@ -1949,7 +1949,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
bs_dest->refcnt = bs_src->refcnt;
/* job */
- bs_dest->in_use = bs_src->in_use;
bs_dest->job = bs_src->job;
/* keep the same entry in bdrv_states */
@@ -1992,7 +1991,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
assert(bs_new->job == NULL);
assert(bs_new->dev == NULL);
- assert(bs_new->in_use == 0);
+ assert(bdrv_op_blocker_is_empty(bs_new));
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
@@ -2011,7 +2010,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
/* Check a few fields that should remain attached to the device */
assert(bs_new->dev == NULL);
assert(bs_new->job == NULL);
- assert(bs_new->in_use == 0);
+ assert(bdrv_op_blocker_is_empty(bs_new));
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
@@ -2056,7 +2055,7 @@ static void bdrv_delete(BlockDriverState *bs)
{
assert(!bs->dev);
assert(!bs->job);
- assert(!bs->in_use);
+ assert(bdrv_op_blocker_is_empty(bs));
assert(!bs->refcnt);
assert(QLIST_EMPTY(&bs->dirty_bitmaps));
@@ -2238,7 +2237,8 @@ int bdrv_commit(BlockDriverState *bs)
return -ENOTSUP;
}
- if (bdrv_in_use(bs) || bdrv_in_use(bs->backing_hd)) {
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, NULL) ||
+ bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, NULL)) {
return -EBUSY;
}
@@ -3500,8 +3500,9 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
return -ENOTSUP;
if (bs->read_only)
return -EACCES;
- if (bdrv_in_use(bs))
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
return -EBUSY;
+ }
ret = drv->bdrv_truncate(bs, offset);
if (ret == 0) {
ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
@@ -5401,17 +5402,6 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
return true;
}
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
-{
- assert(bs->in_use != in_use);
- bs->in_use = in_use;
-}
-
-int bdrv_in_use(BlockDriverState *bs)
-{
- return bs->in_use;
-}
-
void bdrv_iostatus_enable(BlockDriverState *bs)
{
bs->iostatus_enabled = true;
diff --git a/blockdev.c b/blockdev.c
index 1cbcc1c..0a51902 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1334,8 +1334,8 @@ static void external_snapshot_prepare(BlkTransactionState *common,
return;
}
- if (bdrv_in_use(state->old_bs)) {
- error_set(errp, QERR_DEVICE_IN_USE, device);
+ if (bdrv_op_is_blocked(state->old_bs,
+ BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
return;
}
@@ -1557,8 +1557,7 @@ exit:
static void eject_device(BlockDriverState *bs, int force, Error **errp)
{
- if (bdrv_in_use(bs)) {
- error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
return;
}
if (!bdrv_dev_has_removable_media(bs)) {
@@ -1760,14 +1759,16 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
BlockDriverState *bs;
+ Error *local_err = NULL;
bs = bdrv_find(id);
if (!bs) {
qerror_report(QERR_DEVICE_NOT_FOUND, id);
return -1;
}
- if (bdrv_in_use(bs)) {
- qerror_report(QERR_DEVICE_IN_USE, id);
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
+ error_report("%s", error_get_pretty(local_err));
+ error_free(local_err);
return -1;
}
@@ -2023,8 +2024,7 @@ void qmp_drive_backup(const char *device, const char *target,
}
}
- if (bdrv_in_use(bs)) {
- error_set(errp, QERR_DEVICE_IN_USE, device);
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
return;
}
@@ -2157,8 +2157,7 @@ void qmp_drive_mirror(const char *device, const char *target,
}
}
- if (bdrv_in_use(bs)) {
- error_set(errp, QERR_DEVICE_IN_USE, device);
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) {
return;
}
diff --git a/blockjob.c b/blockjob.c
index cd4784f..60e72f5 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,14 +41,16 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
{
BlockJob *job;
- if (bs->job || bdrv_in_use(bs)) {
+ if (bs->job || !bdrv_op_blocker_is_empty(bs)) {
error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
return NULL;
}
bdrv_ref(bs);
- bdrv_set_in_use(bs, 1);
-
job = g_malloc0(driver->instance_size);
+ error_setg(&job->blocker, "block device is in use by block job: %s",
+ BlockJobType_lookup[driver->job_type]);
+ bdrv_op_block_all(bs, job->blocker);
+
job->driver = driver;
job->bs = bs;
job->cb = cb;
@@ -63,8 +65,9 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
block_job_set_speed(job, speed, &local_err);
if (local_err) {
bs->job = NULL;
+ bdrv_op_unblock_all(bs, job->blocker);
+ error_free(job->blocker);
g_free(job);
- bdrv_set_in_use(bs, 0);
error_propagate(errp, local_err);
return NULL;
}
@@ -79,8 +82,9 @@ void block_job_completed(BlockJob *job, int ret)
assert(bs->job == job);
job->cb(job->opaque, ret);
bs->job = NULL;
+ bdrv_op_unblock_all(bs, job->blocker);
+ error_free(job->blocker);
g_free(job);
- bdrv_set_in_use(bs, 0);
}
void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 70b8a5a..e49c253 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -70,6 +70,9 @@ struct VirtIOBlockDataPlane {
queue */
unsigned int num_reqs;
+
+ /* Operation blocker on BDS */
+ Error *blocker;
};
/* Raise an interrupt to signal guest, if necessary */
@@ -350,6 +353,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
{
VirtIOBlockDataPlane *s;
int fd;
+ Error *local_err = NULL;
*dataplane = NULL;
@@ -372,9 +376,10 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
/* If dataplane is (re-)enabled while the guest is running there could be
* block jobs that can conflict.
*/
- if (bdrv_in_use(blk->conf.bs)) {
- error_setg(errp,
- "cannot start dataplane thread while device is in use");
+ if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &local_err)) {
+ error_report("cannot start dataplane thread: %s",
+ error_get_pretty(local_err));
+ error_free(local_err);
return;
}
@@ -406,8 +411,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
}
s->ctx = iothread_get_aio_context(s->iothread);
- /* Prevent block operations that conflict with data plane thread */
- bdrv_set_in_use(blk->conf.bs, 1);
+ error_setg(&s->blocker, "block device is in use by data plane");
+ bdrv_op_block_all(blk->conf.bs, s->blocker);
*dataplane = s;
}
@@ -420,7 +425,8 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
}
virtio_blk_data_plane_stop(s);
- bdrv_set_in_use(s->blk->conf.bs, 0);
+ bdrv_op_unblock_all(s->blk->conf.bs, s->blocker);
+ error_free(s->blocker);
object_unref(OBJECT(s->iothread));
g_free(s);
}
diff --git a/include/block/block.h b/include/block/block.h
index 7f0448c..22935f1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -472,8 +472,6 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
void bdrv_ref(BlockDriverState *bs);
void bdrv_unref(BlockDriverState *bs);
-void bdrv_set_in_use(BlockDriverState *bs, int in_use);
-int bdrv_in_use(BlockDriverState *bs);
bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fdf2a7d..7b1c013 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -358,7 +358,6 @@ struct BlockDriverState {
QTAILQ_ENTRY(BlockDriverState) device_list;
QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
int refcnt;
- int in_use; /* users other than guest access, eg. block migration */
QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d76de62..c0a7875 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -106,6 +106,9 @@ struct BlockJob {
/** The completion function that will be called when the job completes. */
BlockDriverCompletionFunc *cb;
+ /** Block other operations when block job is running */
+ Error *blocker;
+
/** The opaque value that is passed to the completion function. */
void *opaque;
};
--
1.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 4/7] block: Move op_blocker check from block_job_create to its caller
2014-05-23 3:23 [Qemu-devel] [PATCH 0/7] block: Drop in_use with op blocker Fam Zheng
` (2 preceding siblings ...)
2014-05-23 3:23 ` [Qemu-devel] [PATCH 3/7] block: Replace in_use with operation blocker Fam Zheng
@ 2014-05-23 3:23 ` Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 5/7] block: Add bdrv_set_backing_hd() Fam Zheng
` (2 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Fam Zheng @ 2014-05-23 3:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Benoit Canet, jcody, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
It makes no sense to check for "any" blocker on bs, we are here only
because of the mechanical conversion from in_use to op_blockers. Remove
it now, and let the callers check specific operation types. Backup and
mirror already have it, add checker to stream and commit.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
blockdev.c | 8 ++++++++
blockjob.c | 2 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 0a51902..9a9bdec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1889,6 +1889,10 @@ void qmp_block_stream(const char *device, bool has_base,
return;
}
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
+ return;
+ }
+
if (base) {
base_bs = bdrv_find_backing_image(bs, base);
if (base_bs == NULL) {
@@ -1933,6 +1937,10 @@ void qmp_block_commit(const char *device,
return;
}
+ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
+ return;
+ }
+
/* default top_bs is the active layer */
top_bs = bs;
diff --git a/blockjob.c b/blockjob.c
index 60e72f5..7d84ca1 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,7 +41,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
{
BlockJob *job;
- if (bs->job || !bdrv_op_blocker_is_empty(bs)) {
+ if (bs->job) {
error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
return NULL;
}
--
1.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 5/7] block: Add bdrv_set_backing_hd()
2014-05-23 3:23 [Qemu-devel] [PATCH 0/7] block: Drop in_use with op blocker Fam Zheng
` (3 preceding siblings ...)
2014-05-23 3:23 ` [Qemu-devel] [PATCH 4/7] block: Move op_blocker check from block_job_create to its caller Fam Zheng
@ 2014-05-23 3:23 ` Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 6/7] block: Use bdrv_set_backing_hd everywhere Fam Zheng
2014-05-23 3:23 ` [Qemu-devel] [PATCH 7/7] block: Add backing_blocker in BlockDriverState Fam Zheng
6 siblings, 0 replies; 11+ messages in thread
From: Fam Zheng @ 2014-05-23 3:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Benoit Canet, jcody, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
This is the common but non-trivial steps to assign or change the
backing_hd of BDS.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
block.c | 36 +++++++++++++++++++++++-------------
include/block/block.h | 1 +
2 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index 64c48a5..911ba68 100644
--- a/block.c
+++ b/block.c
@@ -1094,6 +1094,21 @@ fail:
return ret;
}
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
+{
+
+ bs->backing_hd = backing_hd;
+ if (!backing_hd) {
+ goto out;
+ }
+ bs->open_flags &= ~BDRV_O_NO_BACKING;
+ pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
+ pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+ backing_hd->drv ? backing_hd->drv->format_name : "");
+out:
+ bdrv_refresh_limits(bs);
+}
+
/*
* Opens the backing file for a BlockDriverState if not yet open
*
@@ -1107,6 +1122,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
char *backing_filename = g_malloc0(PATH_MAX);
int ret = 0;
BlockDriver *back_drv = NULL;
+ BlockDriverState *backing_hd;
Error *local_err = NULL;
if (bs->backing_hd != NULL) {
@@ -1129,27 +1145,26 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
}
+ backing_hd = bdrv_new("", errp);
+
if (bs->backing_format[0] != '\0') {
back_drv = bdrv_find_format(bs->backing_format);
}
assert(bs->backing_hd == NULL);
- ret = bdrv_open(&bs->backing_hd,
+ ret = bdrv_open(&backing_hd,
*backing_filename ? backing_filename : NULL, NULL, options,
bdrv_backing_flags(bs->open_flags), back_drv, &local_err);
if (ret < 0) {
- bs->backing_hd = NULL;
+ bdrv_unref(backing_hd);
+ backing_hd = NULL;
bs->open_flags |= BDRV_O_NO_BACKING;
error_setg(errp, "Could not open backing file: %s",
error_get_pretty(local_err));
error_free(local_err);
goto free_exit;
}
-
- if (bs->backing_hd->file) {
- pstrcpy(bs->backing_file, sizeof(bs->backing_file),
- bs->backing_hd->file->filename);
- }
+ bdrv_set_backing_hd(bs, backing_hd);
/* Recalculate the BlockLimits with the backing file */
bdrv_refresh_limits(bs);
@@ -2043,12 +2058,7 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
/* The contents of 'tmp' will become bs_top, as we are
* swapping bs_new and bs_top contents. */
- bs_top->backing_hd = bs_new;
- bs_top->open_flags &= ~BDRV_O_NO_BACKING;
- pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
- bs_new->filename);
- pstrcpy(bs_top->backing_format, sizeof(bs_top->backing_format),
- bs_new->drv ? bs_new->drv->format_name : "");
+ bdrv_set_backing_hd(bs_top, bs_new);
}
static void bdrv_delete(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 22935f1..faee3aa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -216,6 +216,7 @@ int bdrv_parse_discard_flags(const char *mode, int *flags);
int bdrv_open_image(BlockDriverState **pbs, const char *filename,
QDict *options, const char *bdref_key, int flags,
bool allow_none, Error **errp);
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
int bdrv_open(BlockDriverState **pbs, const char *filename,
--
1.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 6/7] block: Use bdrv_set_backing_hd everywhere
2014-05-23 3:23 [Qemu-devel] [PATCH 0/7] block: Drop in_use with op blocker Fam Zheng
` (4 preceding siblings ...)
2014-05-23 3:23 ` [Qemu-devel] [PATCH 5/7] block: Add bdrv_set_backing_hd() Fam Zheng
@ 2014-05-23 3:23 ` Fam Zheng
2014-05-23 12:47 ` Stefan Hajnoczi
2014-05-23 12:48 ` Stefan Hajnoczi
2014-05-23 3:23 ` [Qemu-devel] [PATCH 7/7] block: Add backing_blocker in BlockDriverState Fam Zheng
6 siblings, 2 replies; 11+ messages in thread
From: Fam Zheng @ 2014-05-23 3:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Benoit Canet, jcody, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
We need to handle the coming backing_blocker properly, so don't open
code the assignment, instead, call bdrv_set_backing_hd to change
backing_hd.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 4 ++--
block/stream.c | 4 ++--
block/vvfat.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 911ba68..91c866c 100644
--- a/block.c
+++ b/block.c
@@ -2652,13 +2652,13 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
if (ret) {
goto exit;
}
- new_top_bs->backing_hd = base_bs;
+ bdrv_set_backing_hd(new_top_bs, base_bs);
bdrv_refresh_limits(new_top_bs);
QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
/* so that bdrv_close() does not recursively close the chain */
- intermediate_state->bs->backing_hd = NULL;
+ bdrv_set_backing_hd(intermediate_state->bs, NULL);
bdrv_unref(intermediate_state->bs);
}
ret = 0;
diff --git a/block/stream.c b/block/stream.c
index dd0b4ac..91d18a2 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -60,7 +60,7 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
/* Must assign before bdrv_delete() to prevent traversing dangling pointer
* while we delete backing image instances.
*/
- top->backing_hd = base;
+ bdrv_set_backing_hd(top, base);
while (intermediate) {
BlockDriverState *unused;
@@ -72,7 +72,7 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
unused = intermediate;
intermediate = intermediate->backing_hd;
- unused->backing_hd = NULL;
+ bdrv_set_backing_hd(unused, NULL);
bdrv_unref(unused);
}
diff --git a/block/vvfat.c b/block/vvfat.c
index c3af7ff..417e96f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2947,7 +2947,7 @@ static int enable_write_target(BDRVVVFATState *s)
unlink(s->qcow_filename);
#endif
- s->bs->backing_hd = bdrv_new("", &error_abort);
+ bdrv_set_backing_hd(s->bs, bdrv_new("", &error_abort));
s->bs->backing_hd->drv = &vvfat_write_target;
s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
*(void**)s->bs->backing_hd->opaque = s;
--
1.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] block: Use bdrv_set_backing_hd everywhere
2014-05-23 3:23 ` [Qemu-devel] [PATCH 6/7] block: Use bdrv_set_backing_hd everywhere Fam Zheng
@ 2014-05-23 12:47 ` Stefan Hajnoczi
2014-05-23 12:48 ` Stefan Hajnoczi
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-05-23 12:47 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Benoit Canet, jcody, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini
On Fri, May 23, 2014 at 11:23:22AM +0800, Fam Zheng wrote:
> We need to handle the coming backing_blocker properly, so don't open
> code the assignment, instead, call bdrv_set_backing_hd to change
> backing_hd.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 4 ++--
> block/stream.c | 4 ++--
> block/vvfat.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 911ba68..91c866c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2652,13 +2652,13 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
> if (ret) {
> goto exit;
> }
> - new_top_bs->backing_hd = base_bs;
> + bdrv_set_backing_hd(new_top_bs, base_bs);
>
> bdrv_refresh_limits(new_top_bs);
bdrv_set_backing_hd() already does bdrv_refresh_limits() when backing_hd
!= NULL (and base_bs is always non-NULL in this code path). The
bdrv_refresh_limits() can be dropped.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] block: Use bdrv_set_backing_hd everywhere
2014-05-23 3:23 ` [Qemu-devel] [PATCH 6/7] block: Use bdrv_set_backing_hd everywhere Fam Zheng
2014-05-23 12:47 ` Stefan Hajnoczi
@ 2014-05-23 12:48 ` Stefan Hajnoczi
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-05-23 12:48 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Benoit Canet, jcody, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini
On Fri, May 23, 2014 at 11:23:22AM +0800, Fam Zheng wrote:
> We need to handle the coming backing_blocker properly, so don't open
> code the assignment, instead, call bdrv_set_backing_hd to change
> backing_hd.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 4 ++--
> block/stream.c | 4 ++--
> block/vvfat.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 911ba68..91c866c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2652,13 +2652,13 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
> if (ret) {
> goto exit;
> }
> - new_top_bs->backing_hd = base_bs;
> + bdrv_set_backing_hd(new_top_bs, base_bs);
>
> bdrv_refresh_limits(new_top_bs);
Please ignore what I said about backing_hd and base_bs being non-NULL,
it doesn't matter. bdrv_set_backing_hd() always calls
bdrv_refresh_limits() so we definitely don't need to call it again here.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 7/7] block: Add backing_blocker in BlockDriverState
2014-05-23 3:23 [Qemu-devel] [PATCH 0/7] block: Drop in_use with op blocker Fam Zheng
` (5 preceding siblings ...)
2014-05-23 3:23 ` [Qemu-devel] [PATCH 6/7] block: Use bdrv_set_backing_hd everywhere Fam Zheng
@ 2014-05-23 3:23 ` Fam Zheng
2014-05-23 12:46 ` Stefan Hajnoczi
6 siblings, 1 reply; 11+ messages in thread
From: Fam Zheng @ 2014-05-23 3:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Benoit Canet, jcody, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
This makes use of op_blocker and blocks all the operations except for
commit target, on each BlockDriverState->backing_hd.
The asserts for op_blocker in bdrv_swap are removed because with this
change, the target of block commit has at least the backing blocker of
its child, so the assertion is not true. Callers should do their check.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 22 ++++++++++++++++++----
block/mirror.c | 2 +-
include/block/block_int.h | 3 +++
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 91c866c..b5aeca2 100644
--- a/block.c
+++ b/block.c
@@ -1097,14 +1097,29 @@ fail:
void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
{
+ if (bs->backing_hd) {
+ assert(bs->backing_blocker);
+ bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
+ } else if (backing_hd) {
+ error_setg(&bs->backing_blocker,
+ "device is used as backing hd of '%s'",
+ bs->device_name);
+ }
+
bs->backing_hd = backing_hd;
if (!backing_hd) {
+ error_free(bs->backing_blocker);
goto out;
}
bs->open_flags &= ~BDRV_O_NO_BACKING;
pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
pstrcpy(bs->backing_format, sizeof(bs->backing_format),
backing_hd->drv ? backing_hd->drv->format_name : "");
+
+ bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
+ /* Otherwise we won't be able to commit due to check in bdrv_commit */
+ bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
+ bs->backing_blocker);
out:
bdrv_refresh_limits(bs);
}
@@ -1803,8 +1818,9 @@ void bdrv_close(BlockDriverState *bs)
if (bs->drv) {
if (bs->backing_hd) {
- bdrv_unref(bs->backing_hd);
- bs->backing_hd = NULL;
+ BlockDriverState *backing_hd = bs->backing_hd;
+ bdrv_set_backing_hd(bs, NULL);
+ bdrv_unref(backing_hd);
}
bs->drv->bdrv_close(bs);
g_free(bs->opaque);
@@ -2006,7 +2022,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
assert(bs_new->job == NULL);
assert(bs_new->dev == NULL);
- assert(bdrv_op_blocker_is_empty(bs_new));
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
@@ -2025,7 +2040,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
/* Check a few fields that should remain attached to the device */
assert(bs_new->dev == NULL);
assert(bs_new->job == NULL);
- assert(bdrv_op_blocker_is_empty(bs_new));
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
diff --git a/block/mirror.c b/block/mirror.c
index 1c38aa8..94c8661 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -498,7 +498,7 @@ immediate_exit:
/* drop the bs loop chain formed by the swap: break the loop then
* trigger the unref from the top one */
BlockDriverState *p = s->base->backing_hd;
- s->base->backing_hd = NULL;
+ bdrv_set_backing_hd(s->base, NULL);
bdrv_unref(p);
}
}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7b1c013..f2e753f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -369,6 +369,9 @@ struct BlockDriverState {
QDict *options;
BlockdevDetectZeroesOptions detect_zeroes;
+
+ /* The error object in use for blocking operations on backing_hd */
+ Error *backing_blocker;
};
int get_tmp_filename(char *filename, int size);
--
1.9.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] block: Add backing_blocker in BlockDriverState
2014-05-23 3:23 ` [Qemu-devel] [PATCH 7/7] block: Add backing_blocker in BlockDriverState Fam Zheng
@ 2014-05-23 12:46 ` Stefan Hajnoczi
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-05-23 12:46 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Benoit Canet, jcody, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini
On Fri, May 23, 2014 at 11:23:23AM +0800, Fam Zheng wrote:
> This makes use of op_blocker and blocks all the operations except for
> commit target, on each BlockDriverState->backing_hd.
>
> The asserts for op_blocker in bdrv_swap are removed because with this
> change, the target of block commit has at least the backing blocker of
> its child, so the assertion is not true. Callers should do their check.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 22 ++++++++++++++++++----
> block/mirror.c | 2 +-
> include/block/block_int.h | 3 +++
> 3 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 91c866c..b5aeca2 100644
> --- a/block.c
> +++ b/block.c
> @@ -1097,14 +1097,29 @@ fail:
> void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> {
>
> + if (bs->backing_hd) {
> + assert(bs->backing_blocker);
> + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> + } else if (backing_hd) {
> + error_setg(&bs->backing_blocker,
> + "device is used as backing hd of '%s'",
> + bs->device_name);
> + }
> +
> bs->backing_hd = backing_hd;
> if (!backing_hd) {
> + error_free(bs->backing_blocker);
bs->backing_block = NULL;
...to make the assert(bs->backing_blocker) above more useful.
^ permalink raw reply [flat|nested] 11+ messages in thread