On 2013年12月12日 21:46, Markus Armbruster wrote:
Fam Zheng <f...@redhat.com> writes:
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, ...).
- Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use).
Note: there is no block for only specific types for now, i.e. a caller
blocks all or none. So although the checks are type specific, the above
changes can still be seen as identical in logic.
PATCH 3/10 adds a new blocker that doesn't block all or none. It's not
related to existing in_use, though, and therefore doesn't contradict
your "no functional change" claim.
I've always hated in_use. Its meaning is ill-defined, and its use is
confusing. Three cheers for getting rid of it!
Markus,
Thank you very much for the thorough and nice review on this. The commit
message apparently went out dated, because I reordered the patch and did
numbers of rebase during my self reviewing and testing.
I meant to make it mechanism, and will fix toward that as you suggested.
So I'll put it back to the first part of series in the next revision, to
avoid preceding patches introducing blocker (like you pointed out).
Signed-off-by: Fam Zheng <f...@redhat.com>
---
block-migration.c | 7 +++++--
block.c | 32 +++++++++++++++++++-------------
blockdev.c | 29 +++++++++++++++++++----------
blockjob.c | 12 +++++++-----
hw/block/dataplane/virtio-blk.c | 16 ++++++++++------
include/block/block.h | 2 --
include/block/block_int.h | 1 -
include/block/blockjob.h | 3 +++
8 files changed, 63 insertions(+), 39 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index 897fdba..bf9a25f 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 {
@@ -346,7 +347,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;
@@ -584,7 +586,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 681d3be..f59f398 100644
--- a/block.c
+++ b/block.c
@@ -1652,15 +1652,17 @@ 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 */
pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
bs_src->device_name);
+ memcpy(bs_dest->op_blockers, bs_src->op_blockers,
+ sizeof(bs_dest->op_blockers));
bs_dest->list = bs_src->list;
}
Doesn't the new memcpy() belong to PATCH 02/10?
Yes, thanks.
+static bool bdrv_op_blocker_is_empty(BlockDriverState *bs);
/*
* Swap bs contents for two image chains while they are live,
* while keeping required fields on the BlockDriverState that is
@@ -1682,7 +1684,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(bs_new->in_use == 0);
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
Why can you drop the !in_use assertion rather than replacing it by a
bdrv_op_blocker_is_empty() assertion like you do elsewhere?
I shouldn't, will fix.
@@ -1701,7 +1702,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(bs_new->in_use == 0);
assert(bs_new->io_limits_enabled == false);
assert(!throttle_have_timer(&bs_new->throttle_state));
Same question.
Will fix too.
@@ -1738,7 +1738,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));
@@ -1912,6 +1912,7 @@ int bdrv_commit(BlockDriverState *bs)
int ret = 0;
uint8_t *buf;
char filename[PATH_MAX];
+ Error *local_err;
Recommend to always initialize local_err = NULL on general principles.
Your code doesn't strictly require it, because you use it only after
bdrv_op_is_blocked() set it. Initializing it is more obviously correct,
and more robust against change.
But: since your code doesn't do anything with local_err, simply drop it,
and pass NULL to brdv_op_is_blocked().
Good suggestion, thanks.
if (!drv)
return -ENOMEDIUM;
@@ -1920,7 +1921,9 @@ 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, &local_err) ||
+ bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, &local_err)) {
+ error_free(local_err);
return -EBUSY;
}
@@ -2935,6 +2938,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState
*bs,
int bdrv_truncate(BlockDriverState *bs, int64_t offset)
{
BlockDriver *drv = bs->drv;
+ Error *local_err;
int ret;
if (!drv)
return -ENOMEDIUM;
@@ -2942,8 +2946,10 @@ 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, &local_err)) {
+ error_free(local_err);
return -EBUSY;
+ }
ret = drv->bdrv_truncate(bs, offset);
if (ret == 0) {
ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
Likewise.
Yes.
@@ -4711,15 +4717,15 @@ void bdrv_op_unblock_all(BlockDriverState *bs, Error
*reason)
}
}
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
+static bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
I'd call it bdrv_op_is_any_blocked(), for symmetry with
bdrv_op_is_any_blocked(). Your choice.
I'd like to keep it as is for now. Renaming is easy while there are
non-trivial fixes needed for the series.
{
- assert(bs->in_use != in_use);
- bs->in_use = in_use;
-}
+ bool ret = true;
+ int i;
-int bdrv_in_use(BlockDriverState *bs)
-{
- return bs->in_use;
+ for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
+ ret = ret && QLIST_EMPTY(&bs->op_blockers[i]);
+ }
+ return ret;
I find this code simpler:
int i;
for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
if (!QLIST_EMPTY(&bs->op_blockers[i])) {
return false;
}
}
return true;
Taken, thanks.
}
void bdrv_iostatus_enable(BlockDriverState *bs)
diff --git a/blockdev.c b/blockdev.c
index 44755e1..369d8da 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1224,8 +1224,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;
}
This improves the error reported back to the caller, contradicting the
commit message's claim "no functional change". I don't mind, I just
want it noted in the commit message.
OK.
@@ -1441,10 +1441,10 @@ 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)) {
error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
return;
Likewise.
@@ -1637,14 +1637,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;
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);
Likewise.
Caveat, emptor: even I am unsure about which of the many error reporting
interfaces is to be used in which context, but here goes anyway: This
isn't wrong, because do_drive_del() isn't reachable from QMP. Still,
qerror_report_err() would be more robust against change.
Maybe Luiz can give more definite advice.
I'll keep the original error message and drop local_err.
return -1;
}
@@ -1755,6 +1757,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) {
Oh, here you *add* a blocker check! Why shouldn't this be a separate
patch?
@@ -1795,6 +1801,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;
Likewise.
These two are part of replacement for in_use check in
block_job_create(), where checking for bdrv_op_blocker_is_empty()
doesn't sound perfect, but works. Let's convert to that weird check
intermediately and improve through a separate patch.
@@ -1881,8 +1891,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;
}
Another error message improvement.
@@ -2011,11 +2020,11 @@ 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)) {
Likewise.
return;
}
+
Spurious whitespace change, please drop.
OK.
flags = bs->open_flags | BDRV_O_RDWR;
source = bs->backing_hd;
if (!source && sync == MIRROR_SYNC_MODE_TOP) {
diff --git a/blockjob.c b/blockjob.c
index 9e5fd5c..e198cb2 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *driver,
BlockDriverState *bs,
{
BlockJob *job;
- if (bs->job || bdrv_in_use(bs)) {
+ if (bs->job) {
error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
return NULL;
}
Why can you drop bdrv_in_use() without replacement here?
As explained above.
bdrv_ref(bs);
- bdrv_set_in_use(bs, 1);
-
job = g_malloc0(driver->instance_size);
job->driver = driver;
job->bs = bs;
@@ -64,11 +62,14 @@ void *block_job_create(const BlockJobDriver *driver,
BlockDriverState *bs,
if (error_is_set(&local_err)) {
bs->job = NULL;
g_free(job);
- bdrv_set_in_use(bs, 0);
error_propagate(errp, local_err);
return NULL;
}
}
+ 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);
+
return job;
}
The replacement for bdrv_set_in_use() is in a different place. Makes
sense to me, but it should be a separate patch, to keep this one as
mechanical as possible.
OK, makes sense.
@@ -79,8 +80,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 f2d7350..0a7b759 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -69,6 +69,9 @@ struct VirtIOBlockDataPlane {
queue */
unsigned int num_reqs;
+
+ /* Operation blocker on BDS */
+ Error *blocker;
};
/* Raise an interrupt to signal guest, if necessary */
@@ -385,6 +388,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *blk,
{
VirtIOBlockDataPlane *s;
int fd;
+ Error *local_err = NULL;
*dataplane = NULL;
@@ -406,8 +410,9 @@ bool 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_report("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("%s", error_get_pretty(local_err));
+ error_free(local_err);
return false;
}
Error message change. The new one tells us why in more detail, but is
doesn't tell us what failed. Have you reproduced the error to make sure
the message still makes sense?
No, I'll concatenate the original test with local_err.
@@ -422,9 +427,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *blk,
s->vdev = vdev;
s->fd = fd;
s->blk = blk;
-
- /* 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;
return true;
@@ -437,7 +441,7 @@ 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);
Don't you want to error_free(s->blocker)?
Good catch. Thanks.
Fam
