| Message ID | 20190606154132.153330-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: drop bs->job | expand |
Am 06.06.2019 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben: > Drop remaining users of bs->job: > 1. assertions actually duplicated by assert(!bs->refcnt) > 2. trace-point seems not enough reason to change stream_start to return > BlockJob pointer > 3. Restricting creation of two jobs based on same bs is bad idea, as > 3.1 Some jobs creates filters to be their main node, so, this check > don't actually prevent creating second job on same real node (which > will create another filter node) (but I hope it is restricted by > other mechanisms) > 3.2 Even without bs->job we have two systems of permissions: > op-blockers and BLK_PERM > 3.3 We may want to run several jobs on one node one day This made make check break (test-blockjob tests that you can't create two block jobs on the same node). So I need to squash in the following, if you agree. Kevin diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c index 8c91980c70..b33f899873 100644 --- a/tests/test-blockjob.c +++ b/tests/test-blockjob.c @@ -122,8 +122,9 @@ static void test_job_ids(void) /* This one is valid */ job[0] = do_test_id(blk[0], "id0", true); - /* We cannot have two jobs in the same BDS */ - do_test_id(blk[0], "id1", false); + /* We can have two jobs in the same BDS */ + job[1] = do_test_id(blk[0], "id1", true); + job_early_fail(&job[1]->job); /* Duplicate job IDs are not allowed */ job[1] = do_test_id(blk[1], "id0", false);
17.06.2019 19:58, Kevin Wolf wrote: > Am 06.06.2019 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Drop remaining users of bs->job: >> 1. assertions actually duplicated by assert(!bs->refcnt) >> 2. trace-point seems not enough reason to change stream_start to return >> BlockJob pointer >> 3. Restricting creation of two jobs based on same bs is bad idea, as >> 3.1 Some jobs creates filters to be their main node, so, this check >> don't actually prevent creating second job on same real node (which >> will create another filter node) (but I hope it is restricted by >> other mechanisms) >> 3.2 Even without bs->job we have two systems of permissions: >> op-blockers and BLK_PERM >> 3.3 We may want to run several jobs on one node one day > > This made make check break (test-blockjob tests that you can't create > two block jobs on the same node). So I need to squash in the following, > if you agree. > > Kevin > > > diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c > index 8c91980c70..b33f899873 100644 > --- a/tests/test-blockjob.c > +++ b/tests/test-blockjob.c > @@ -122,8 +122,9 @@ static void test_job_ids(void) > /* This one is valid */ > job[0] = do_test_id(blk[0], "id0", true); > > - /* We cannot have two jobs in the same BDS */ > - do_test_id(blk[0], "id1", false); > + /* We can have two jobs in the same BDS */ > + job[1] = do_test_id(blk[0], "id1", true); > + job_early_fail(&job[1]->job); > > /* Duplicate job IDs are not allowed */ > job[1] = do_test_id(blk[1], "id0", false); > I agree, thanks. Sorry for me always running only iotests and ignoring make check :(
diff --git a/include/block/block_int.h b/include/block/block_int.h index 8bb1cfb80a..a498c2670b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -812,9 +812,6 @@ struct BlockDriverState { /* operation blockers */ QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX]; - /* long-running background operation */ - BlockJob *job; - /* The node that this node inherited default options from (and a reopen on * which can affect this node by changing these defaults). This is always a * parent node of this node. */ diff --git a/block.c b/block.c index e3e77feee0..ceb2ea23c5 100644 --- a/block.c +++ b/block.c @@ -3905,7 +3905,6 @@ static void bdrv_close(BlockDriverState *bs) BdrvAioNotifier *ban, *ban_next; BdrvChild *child, *next; - assert(!bs->job); assert(!bs->refcnt); bdrv_drained_begin(bs); /* complete I/O */ @@ -4146,7 +4145,6 @@ out: static void bdrv_delete(BlockDriverState *bs) { - assert(!bs->job); assert(bdrv_op_blocker_is_empty(bs)); assert(!bs->refcnt); diff --git a/blockdev.c b/blockdev.c index 58aa1369a4..51eb5e86d7 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3264,7 +3264,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, goto out; } - trace_qmp_block_stream(bs, bs->job); + trace_qmp_block_stream(bs); out: aio_context_release(aio_context); diff --git a/blockjob.c b/blockjob.c index 7b6737adde..d64ea2623a 100644 --- a/blockjob.c +++ b/blockjob.c @@ -84,9 +84,7 @@ BlockJob *block_job_get(const char *id) void block_job_free(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); - BlockDriverState *bs = blk_bs(bjob->blk); - bs->job = NULL; block_job_remove_all_bdrv(bjob); blk_unref(bjob->blk); error_free(bjob->blocker); @@ -403,11 +401,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, BlockJob *job; int ret; - if (bs->job) { - error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs)); - return NULL; - } - if (job_id == NULL && !(flags & JOB_INTERNAL)) { job_id = bdrv_get_device_name(bs); } @@ -450,7 +443,6 @@ 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); - bs->job = job; bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker); diff --git a/block/trace-events b/block/trace-events index eab51497fc..912f84af39 100644 --- a/block/trace-events +++ b/block/trace-events @@ -53,7 +53,7 @@ qmp_block_job_resume(void *job) "job %p" qmp_block_job_complete(void *job) "job %p" qmp_block_job_finalize(void *job) "job %p" qmp_block_job_dismiss(void *job) "job %p" -qmp_block_stream(void *bs, void *job) "bs %p job %p" +qmp_block_stream(void *bs) "bs %p" # file-posix.c # file-win32.c
Drop remaining users of bs->job: 1. assertions actually duplicated by assert(!bs->refcnt) 2. trace-point seems not enough reason to change stream_start to return BlockJob pointer 3. Restricting creation of two jobs based on same bs is bad idea, as 3.1 Some jobs creates filters to be their main node, so, this check don't actually prevent creating second job on same real node (which will create another filter node) (but I hope it is restricted by other mechanisms) 3.2 Even without bs->job we have two systems of permissions: op-blockers and BLK_PERM 3.3 We may want to run several jobs on one node one day And finally, drop bs->job pointer itself. Hurrah! Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block_int.h | 3 --- block.c | 2 -- blockdev.c | 2 +- blockjob.c | 8 -------- block/trace-events | 2 +- 5 files changed, 2 insertions(+), 15 deletions(-)