From patchwork Tue Mar 22 19:36:31 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 8644791 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id D6AADC0553 for ; Tue, 22 Mar 2016 19:39:57 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C825920212 for ; Tue, 22 Mar 2016 19:39:56 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5A5A020204 for ; Tue, 22 Mar 2016 19:39:55 +0000 (UTC) Received: from localhost ([::1]:39257 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiS9i-0006EA-QE for patchwork-qemu-devel@patchwork.kernel.org; Tue, 22 Mar 2016 15:39:54 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiS6o-0000qD-OS for qemu-devel@nongnu.org; Tue, 22 Mar 2016 15:36:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiS6j-0003B7-Rn for qemu-devel@nongnu.org; Tue, 22 Mar 2016 15:36:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37449) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiS6f-00037k-PJ; Tue, 22 Mar 2016 15:36:45 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 67DF1711D9; Tue, 22 Mar 2016 19:36:45 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-53.ams2.redhat.com [10.36.116.53]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2MJadsc002983; Tue, 22 Mar 2016 15:36:44 -0400 From: Kevin Wolf To: qemu-block@nongnu.org Date: Tue, 22 Mar 2016 20:36:31 +0100 Message-Id: <1458675397-24956-4-git-send-email-kwolf@redhat.com> In-Reply-To: <1458675397-24956-1-git-send-email-kwolf@redhat.com> References: <1458675397-24956-1-git-send-email-kwolf@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]); Tue, 22 Mar 2016 19:36:45 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com Subject: [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- 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(-) diff --git a/block/backup.c b/block/backup.c index 10397e2..016741e 100644 --- a/block/backup.c +++ b/block/backup.c @@ -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; } diff --git a/block/block-backend.c b/block/block-backend.c index c4c0dc0..03e68a7 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -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, }; /* diff --git a/block/commit.c b/block/commit.c index 446a3ae..5e45195 100644 --- a/block/commit.c +++ b/block/commit.c @@ -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; } diff --git a/block/mirror.c b/block/mirror.c index da18c0b..b9a93fd 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -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); diff --git a/block/stream.c b/block/stream.c index cafaa07..d74f97f 100644 --- a/block/stream.c +++ b/block/stream.c @@ -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; } diff --git a/blockjob.c b/blockjob.c index 9fc37ca..e6edd81 100644 --- a/blockjob.c +++ b/blockjob.c @@ -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; diff --git a/include/block/block_int.h b/include/block/block_int.h index 195abe8..9358949 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -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; diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 8bedc49..33a42c3 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -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