diff --git a/block/mirror.c b/block/mirror.c index efec2c7674..959e3dfbd6 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -771,13 +771,6 @@ static int mirror_exit_common(Job *job) block_job_remove_all_bdrv(bjob); bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_abort); - /* We just changed the BDS the job BB refers to (with either or both of the - * bdrv_replace_node() calls), so switch the BB back so the cleanup does - * the right thing. We don't need any permissions any more now. */ - blk_remove_bs(bjob->blk); - blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort); - blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort); - bs_opaque->job = NULL; bdrv_drained_end(src); diff --git a/block/stream.c b/block/stream.c index e45113aed6..7c6b173ddd 100644 --- a/block/stream.c +++ b/block/stream.c @@ -33,6 +33,7 @@ enum { typedef struct StreamBlockJob { BlockJob common; + BlockBackend *blk; BlockDriverState *base_overlay; /* COW overlay (stream from this) */ BlockDriverState *above_base; /* Node directly above the base */ BlockDriverState *cor_filter_bs; @@ -88,17 +89,18 @@ static int stream_prepare(Job *job) static void stream_clean(Job *job) { StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); - BlockJob *bjob = &s->common; if (s->cor_filter_bs) { bdrv_cor_filter_drop(s->cor_filter_bs); s->cor_filter_bs = NULL; } + blk_unref(s->blk); + s->blk = NULL; + /* Reopen the image back in read-only mode if necessary */ if (s->bs_read_only) { /* Give up write permissions before making it read-only */ - blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort); bdrv_reopen_set_read_only(s->target_bs, true, NULL); } @@ -108,7 +110,6 @@ static void stream_clean(Job *job) static int coroutine_fn stream_run(Job *job, Error **errp) { StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); - BlockBackend *blk = s->common.blk; BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs); int64_t len; int64_t offset = 0; @@ -159,7 +160,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) } trace_stream_one_iteration(s, offset, n, ret); if (copy) { - ret = stream_populate(blk, offset, n); + ret = stream_populate(s->blk, offset, n); } if (ret < 0) { BlockErrorAction action = @@ -294,13 +295,24 @@ void stream_start(const char *job_id, BlockDriverState *bs, } s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs, - BLK_PERM_CONSISTENT_READ, - basic_flags | BLK_PERM_WRITE, + 0, BLK_PERM_ALL, speed, creation_flags, NULL, NULL, errp); if (!s) { goto fail; } + s->blk = blk_new_with_bs(cor_filter_bs, BLK_PERM_CONSISTENT_READ, + basic_flags | BLK_PERM_WRITE, errp); + if (!s->blk) { + goto fail; + } + /* + * Disable request queuing in the BlockBackend to avoid deadlocks on drain: + * The job reports that it's busy until it reaches a pause point. + */ + blk_set_disable_request_queuing(s->blk, true); + blk_set_allow_aio_context_change(s->blk, true); + /* * Prevent concurrent jobs trying to modify the graph structure here, we * already have our own plans. Also don't allow resize as the image size is diff --git a/blockdev.c b/blockdev.c index 0eb2823b1b..b5ff9b854e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3315,7 +3315,7 @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, return NULL; } - *aio_context = blk_get_aio_context(job->blk); + *aio_context = block_job_get_aio_context(job); aio_context_acquire(*aio_context); return job; @@ -3420,7 +3420,7 @@ void qmp_block_job_finalize(const char *id, Error **errp) * automatically acquires the new one), so make sure we release the correct * one. */ - aio_context = blk_get_aio_context(job->blk); + aio_context = block_job_get_aio_context(job); job_unref(&job->job); aio_context_release(aio_context); } @@ -3711,7 +3711,7 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) if (block_job_is_internal(job)) { continue; } - aio_context = blk_get_aio_context(job->blk); + aio_context = block_job_get_aio_context(job); aio_context_acquire(aio_context); value = block_job_query(job, errp); aio_context_release(aio_context); diff --git a/blockjob.c b/blockjob.c index 4bad1408cb..10815a89fe 100644 --- a/blockjob.c +++ b/blockjob.c @@ -86,7 +86,6 @@ void block_job_free(Job *job) BlockJob *bjob = container_of(job, BlockJob, job); block_job_remove_all_bdrv(bjob); - blk_unref(bjob->blk); ratelimit_destroy(&bjob->limit); error_free(bjob->blocker); } @@ -433,22 +432,16 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, uint64_t shared_perm, int64_t speed, int flags, BlockCompletionFunc *cb, void *opaque, Error **errp) { - BlockBackend *blk; BlockJob *job; + int ret; if (job_id == NULL && !(flags & JOB_INTERNAL)) { job_id = bdrv_get_device_name(bs); } - blk = blk_new_with_bs(bs, perm, shared_perm, errp); - if (!blk) { - return NULL; - } - - job = job_create(job_id, &driver->job_driver, txn, blk_get_aio_context(blk), + job = job_create(job_id, &driver->job_driver, txn, bdrv_get_aio_context(bs), flags, cb, opaque, errp); if (job == NULL) { - blk_unref(blk); return NULL; } @@ -458,8 +451,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, ratelimit_init(&job->limit); - job->blk = blk; - job->finalize_cancelled_notifier.notify = block_job_event_cancelled; job->finalize_completed_notifier.notify = block_job_event_completed; job->pending_notifier.notify = block_job_event_pending; @@ -476,21 +467,23 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, error_setg(&job->blocker, "block device is in use by block job: %s", job_type_str(&job->job)); - block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort); + + ret = block_job_add_bdrv(job, "main node", bs, perm, shared_perm, errp); + if (ret < 0) { + goto fail; + } bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker); - /* Disable request queuing in the BlockBackend to avoid deadlocks on drain: - * The job reports that it's busy until it reaches a pause point. */ - blk_set_disable_request_queuing(blk, true); - blk_set_allow_aio_context_change(blk, true); - if (!block_job_set_speed(job, speed, errp)) { - job_early_fail(&job->job); - return NULL; + goto fail; } return job; + +fail: + job_early_fail(&job->job); + return NULL; } void block_job_iostatus_reset(BlockJob *job) @@ -547,3 +540,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, } return action; } + +AioContext *block_job_get_aio_context(BlockJob *job) +{ + return job->job.aio_context; +} diff --git a/include/block/blockjob.h b/include/block/blockjob.h index d200f33c10..87fbb3985f 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -43,9 +43,6 @@ typedef struct BlockJob { /** Data belonging to the generic Job infrastructure */ Job job; - /** The block device on which the job is operating. */ - BlockBackend *blk; - /** Status that is published by the query-block-jobs QMP API */ BlockDeviceIoStatus iostatus; @@ -173,4 +170,11 @@ bool block_job_is_internal(BlockJob *job); */ const BlockJobDriver *block_job_driver(BlockJob *job); +/* + * block_job_get_aio_context: + * + * Returns aio context associated with a block job. + */ +AioContext *block_job_get_aio_context(BlockJob *job); + #endif diff --git a/job.c b/job.c index dbfa67bb0a..54db80df66 100644 --- a/job.c +++ b/job.c @@ -352,6 +352,7 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, notifier_list_init(&job->on_finalize_completed); notifier_list_init(&job->on_pending); notifier_list_init(&job->on_ready); + notifier_list_init(&job->on_idle); job_state_transition(job, JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, diff --git a/qemu-img.c b/qemu-img.c index f036a1d428..21ba1e6800 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -902,7 +902,7 @@ static void common_block_job_cb(void *opaque, int ret) static void run_block_job(BlockJob *job, Error **errp) { uint64_t progress_current, progress_total; - AioContext *aio_context = blk_get_aio_context(job->blk); + AioContext *aio_context = block_job_get_aio_context(job); int ret = 0; aio_context_acquire(aio_context); diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out index c4c15fb275..63203d9944 100644 --- a/tests/qemu-iotests/141.out +++ b/tests/qemu-iotests/141.out @@ -132,7 +132,7 @@ wrote 1048576/1048576 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}} {'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}} -{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}} {'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}} {"return": {}} diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283 index a09e0183ae..5defe48e97 100755 --- a/tests/qemu-iotests/283 +++ b/tests/qemu-iotests/283 @@ -93,7 +93,8 @@ vm.qmp_log('blockdev-add', **{ 'take-child-perms': ['write'] }) -vm.qmp_log('blockdev-backup', sync='full', device='source', target='target') +vm.qmp_log('blockdev-backup', sync='full', device='source', target='target', + job_id="backup0") vm.shutdown() diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index 5bb75952ef..5e4f456db5 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -4,7 +4,7 @@ {"return": {}} {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} {"return": {}} -{"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} +{"execute": "blockdev-backup", "arguments": {"device": "source", "job-id": "backup0", "sync": "full", "target": "target"}} {"error": {"class": "GenericError", "desc": "Permission conflict on node 'base': permissions 'write' are both required by node 'other' (uses node 'base' as 'image' child) and unshared by node 'source' (uses node 'base' as 'image' child)."}} === copy-before-write filter should be gone after job-finalize === diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 2d3c17e566..36be84ae55 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -772,6 +772,7 @@ static void test_iothread_drain_subtree(void) typedef struct TestBlockJob { BlockJob common; + BlockDriverState *bs; int run_ret; int prepare_ret; bool running; @@ -783,7 +784,7 @@ static int test_job_prepare(Job *job) TestBlockJob *s = container_of(job, TestBlockJob, common.job); /* Provoke an AIO_WAIT_WHILE() call to verify there is no deadlock */ - blk_flush(s->common.blk); + bdrv_flush(s->bs); return s->prepare_ret; } @@ -792,7 +793,7 @@ static void test_job_commit(Job *job) TestBlockJob *s = container_of(job, TestBlockJob, common.job); /* Provoke an AIO_WAIT_WHILE() call to verify there is no deadlock */ - blk_flush(s->common.blk); + bdrv_flush(s->bs); } static void test_job_abort(Job *job) @@ -800,7 +801,7 @@ static void test_job_abort(Job *job) TestBlockJob *s = container_of(job, TestBlockJob, common.job); /* Provoke an AIO_WAIT_WHILE() call to verify there is no deadlock */ - blk_flush(s->common.blk); + bdrv_flush(s->bs); } static int coroutine_fn test_job_run(Job *job, Error **errp) @@ -915,6 +916,7 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, tjob = block_job_create("job0", &test_job_driver, NULL, src, 0, BLK_PERM_ALL, 0, 0, NULL, NULL, &error_abort); + tjob->bs = src; job = &tjob->common; block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort); @@ -1538,6 +1540,7 @@ typedef struct TestDropBackingBlockJob { bool should_complete; bool *did_complete; BlockDriverState *detach_also; + BlockDriverState *bs; } TestDropBackingBlockJob; static int coroutine_fn test_drop_backing_job_run(Job *job, Error **errp) @@ -1557,7 +1560,7 @@ static void test_drop_backing_job_commit(Job *job) TestDropBackingBlockJob *s = container_of(job, TestDropBackingBlockJob, common.job); - bdrv_set_backing_hd(blk_bs(s->common.blk), NULL, &error_abort); + bdrv_set_backing_hd(s->bs, NULL, &error_abort); bdrv_set_backing_hd(s->detach_also, NULL, &error_abort); *s->did_complete = true; @@ -1657,6 +1660,7 @@ static void test_blockjob_commit_by_drained_end(void) job = block_job_create("job", &test_drop_backing_job_driver, NULL, bs_parents[2], 0, BLK_PERM_ALL, 0, 0, NULL, NULL, &error_abort); + job->bs = bs_parents[2]; job->detach_also = bs_parents[0]; job->did_complete = &job_has_completed; diff --git a/tests/unit/test-blockjob-txn.c b/tests/unit/test-blockjob-txn.c index 8bd13b9949..c69028b450 100644 --- a/tests/unit/test-blockjob-txn.c +++ b/tests/unit/test-blockjob-txn.c @@ -25,14 +25,6 @@ typedef struct { int *result; } TestBlockJob; -static void test_block_job_clean(Job *job) -{ - BlockJob *bjob = container_of(job, BlockJob, job); - BlockDriverState *bs = blk_bs(bjob->blk); - - bdrv_unref(bs); -} - static int coroutine_fn test_block_job_run(Job *job, Error **errp) { TestBlockJob *s = container_of(job, TestBlockJob, common.job); @@ -73,7 +65,6 @@ static const BlockJobDriver test_block_job_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .run = test_block_job_run, - .clean = test_block_job_clean, }, }; @@ -105,6 +96,7 @@ static BlockJob *test_block_job_start(unsigned int iterations, s = block_job_create(job_id, &test_block_job_driver, txn, bs, 0, BLK_PERM_ALL, 0, JOB_DEFAULT, test_block_job_cb, data, &error_abort); + bdrv_unref(bs); /* referenced by job now */ s->iterations = iterations; s->use_timer = use_timer; s->rc = rc;