@@ -220,9 +220,8 @@ static void backup_iostatus_reset(BlockJob *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common);
- if (s->target->blk) {
- blk_iostatus_reset(s->target->blk);
- }
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_reset(s->target);
}
static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
@@ -402,10 +401,9 @@ static void coroutine_fn backup_run(void *opaque)
job->done_bitmap = bitmap_new(end);
- if (target->blk) {
- blk_set_on_error(target->blk, on_target_error, on_target_error);
- blk_iostatus_enable(target->blk);
- }
+ /* FIXME Only touch on_error and iostatus of a job-owned BB */
+ bdrv_set_on_error(target, on_target_error, on_target_error);
+ bdrv_iostatus_enable(target);
bdrv_add_before_write_notifier(bs, &before_write);
@@ -482,9 +480,9 @@ static void coroutine_fn backup_run(void *opaque)
qemu_co_rwlock_unlock(&job->flush_rwlock);
g_free(job->done_bitmap);
- if (target->blk) {
- blk_iostatus_disable(target->blk);
- }
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_disable(target);
+
bdrv_op_unblock_all(target, job->common.blocker);
data = g_malloc(sizeof(*data));
@@ -515,7 +513,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error");
return;
}
@@ -100,6 +100,39 @@ static const char *blk_root_get_name(BdrvChild *child)
return blk_name(child->opaque);
}
+static void blk_root_iostatus_reset(BdrvChild *child)
+{
+ return blk_iostatus_reset(child->opaque);
+}
+
+static void blk_root_iostatus_enable(BdrvChild *child)
+{
+ return blk_iostatus_enable(child->opaque);
+}
+
+static void blk_root_iostatus_disable(BdrvChild *child)
+{
+ return blk_iostatus_disable(child->opaque);
+}
+
+static bool blk_root_iostatus_is_enabled(BdrvChild *child)
+{
+ return blk_iostatus_is_enabled(child->opaque);
+}
+
+static void blk_root_iostatus_set_err(BdrvChild *child, int error)
+{
+ return blk_iostatus_set_err(child->opaque, error);
+}
+
+static void blk_root_set_on_error(BdrvChild *child,
+ BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error)
+{
+ return blk_set_on_error(child->opaque, on_read_error, on_write_error);
+}
+
+
static const BdrvChildRole child_root = {
.inherit_options = blk_root_inherit_options,
@@ -108,6 +141,15 @@ static const BdrvChildRole child_root = {
.get_name = blk_root_get_name,
.drain_queue = blk_drain_throttling_queue,
+
+ /* FIXME Remove these callbacks. Children setting the iostatus are wrong,
+ * it should be controlled by the parents. */
+ .iostatus_reset = blk_root_iostatus_reset,
+ .iostatus_enable = blk_root_iostatus_enable,
+ .iostatus_disable = blk_root_iostatus_disable,
+ .iostatus_is_enabled = blk_root_iostatus_is_enabled,
+ .iostatus_set_err = blk_root_iostatus_set_err,
+ .set_on_error = blk_root_set_on_error,
};
/*
@@ -215,7 +215,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, "Invalid parameter combination");
return;
}
@@ -689,9 +689,9 @@ immediate_exit:
g_free(s->cow_bitmap);
g_free(s->in_flight_bitmap);
bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
- if (s->target->blk) {
- blk_iostatus_disable(s->target->blk);
- }
+
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_disable(s->target);
data = g_malloc(sizeof(*data));
data->ret = ret;
@@ -716,9 +716,8 @@ static void mirror_iostatus_reset(BlockJob *job)
{
MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
- if (s->target->blk) {
- blk_iostatus_reset(s->target->blk);
- }
+ /* FIXME Only touch iostatus of a job-owned BB */
+ bdrv_iostatus_reset(s->target);
}
static void mirror_complete(BlockJob *job, Error **errp)
@@ -802,7 +801,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error");
return;
}
@@ -855,10 +854,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
bdrv_op_block_all(s->target, s->common.blocker);
- if (s->target->blk) {
- blk_set_on_error(s->target->blk, on_target_error, on_target_error);
- blk_iostatus_enable(s->target->blk);
- }
+ /* FIXME Only touch on_error and iostatus of a job-owned BB */
+ bdrv_set_on_error(s->target, on_target_error, on_target_error);
+ bdrv_iostatus_enable(s->target);
+
s->common.co = qemu_coroutine_create(mirror_run);
trace_mirror_start(bs, s, s->common.co, opaque);
qemu_coroutine_enter(s->common.co, s);
@@ -224,7 +224,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
- (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
+ !bdrv_iostatus_is_enabled(bs)) {
error_setg(errp, QERR_INVALID_PARAMETER, "on-error");
return;
}
@@ -50,6 +50,87 @@ struct BlockJobTxn {
int refcnt;
};
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_reset(BlockDriverState *bs)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_reset) {
+ c->role->iostatus_reset(c);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_enable(BlockDriverState *bs)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_enable) {
+ c->role->iostatus_enable(c);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_disable(BlockDriverState *bs)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_disable) {
+ c->role->iostatus_disable(c);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+bool bdrv_iostatus_is_enabled(BlockDriverState *bs)
+{
+ BdrvChild *c;
+ bool ret = false;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_is_enabled) {
+ ret = c->role->iostatus_is_enabled(c);
+ if (!ret) {
+ return false;
+ }
+ }
+ }
+
+ /* Return false if no BB exists; true if BBs exist and all of them have
+ * iostatus enabled */
+ return ret;
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->iostatus_set_err) {
+ c->role->iostatus_set_err(c, error);
+ }
+ }
+}
+
+/* TODO This is a function that shouldn't exist. Remove all of its users. */
+void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error)
+{
+ BdrvChild *c;
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->set_on_error) {
+ c->role->set_on_error(c, on_read_error, on_write_error);
+ }
+ }
+}
+
void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
int64_t speed, BlockCompletionFunc *cb,
void *opaque, Error **errp)
@@ -443,8 +524,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
job->user_paused = true;
block_job_pause(job);
block_job_iostatus_set_err(job, error);
- if (bs->blk && bs != job->bs) {
- blk_iostatus_set_err(bs->blk, error);
+ if (bs != job->bs) {
+ bdrv_iostatus_set_err(bs, error);
}
}
return action;
@@ -361,6 +361,16 @@ struct BdrvChildRole {
const char* (*get_name)(BdrvChild *child);
bool (*drain_queue)(BdrvChild *child);
+
+ /* FIXME Remove these callbacks. Children setting the iostatus are wrong,
+ * it should be controlled by the parents. */
+ void (*iostatus_reset)(BdrvChild *child);
+ void (*iostatus_enable)(BdrvChild *child);
+ void (*iostatus_disable)(BdrvChild *child);
+ bool (*iostatus_is_enabled)(BdrvChild *child);
+ void (*iostatus_set_err)(BdrvChild *child, int error);
+ void (*set_on_error)(BdrvChild *child, BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error);
};
extern const BdrvChildRole child_file;
@@ -448,4 +448,13 @@ void block_job_txn_unref(BlockJobTxn *txn);
*/
void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job);
+/* TODO These are functions that shouldn't exist. Remove all of their users. */
+void bdrv_iostatus_reset(BlockDriverState *bs);
+void bdrv_iostatus_enable(BlockDriverState *bs);
+void bdrv_iostatus_disable(BlockDriverState *bs);
+bool bdrv_iostatus_is_enabled(BlockDriverState *bs);
+void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
+void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
+ BlockdevOnError on_write_error);
+
#endif
The block jobs currently modify the target BB's error handling options and require that the source BB's iostatus is enabled in order to implement the per-job error options. It's obvious that this is something between ugly, adventurous and plain wrong, so we should ideally fix this instead of thinking about how to handle multiple BBs in this case. Unfortunately we have a chicken-and-egg problem here: In order to fix the problem, the block jobs need their own BBs. This requires multiple BBs per BDS, which in turn means that BDS.blk must be removed. Removing BDS.blk means that the jobs already need their own BB. The only other options is that we lift the iostatus operations to the BdrvChild level and deal with multiple BBs now. So even though we don't want to have these callbacks in the end (because they indicate broken logic), this patch converts the iostatus operations for block jobs targets to BdrvChild callbacks; if more than one parent implements iostatus operations, they are called for each of them. Once the conversion is completed, block jobs will get their own internal BB and the callbacks can be removed again. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/backup.c | 20 +++++------ block/block-backend.c | 42 +++++++++++++++++++++++ block/commit.c | 2 +- block/mirror.c | 21 ++++++------ block/stream.c | 2 +- blockjob.c | 85 +++++++++++++++++++++++++++++++++++++++++++++-- include/block/block_int.h | 10 ++++++ include/block/blockjob.h | 9 +++++ 8 files changed, 165 insertions(+), 26 deletions(-)