[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 05/11] block: allow block jobs in any arbitra
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/11] block: allow block jobs in any arbitrary node |
Date: |
Wed, 27 Apr 2016 14:30:55 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 |
On 04.04.2016 15:43, Alberto Garcia wrote:
> Currently, block jobs can only be owned by root nodes. This patch
> allows block jobs to be in any arbitrary node, by making the following
> changes:
>
> - Block jobs can now be identified by the node name of their
> BlockDriverState in addition to the device name. Since both device
> and node names live in the same namespace there's no ambiguity.
>
> - The "device" parameter used by all commands that operate on block
> jobs can also be a node name now.
>
> - The node name is used as a fallback to fill in the BlockJobInfo
> structure and all BLOCK_JOB_* events if there is no device name for
> that job.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
> blockdev.c | 18 ++++++++++--------
> blockjob.c | 5 +++--
> docs/qmp-events.txt | 8 ++++----
> qapi/block-core.json | 20 ++++++++++----------
> 4 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index edbcc19..d1f5dfb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3685,8 +3685,10 @@ void qmp_blockdev_mirror(const char *device, const
> char *target,
> aio_context_release(aio_context);
> }
>
> -/* Get the block job for a given device name and acquire its AioContext */
> -static BlockJob *find_block_job(const char *device, AioContext **aio_context,
> +/* Get the block job for a given device or node name
> + * and acquire its AioContext */
> +static BlockJob *find_block_job(const char *device_or_node,
> + AioContext **aio_context,
> Error **errp)
> {
> BlockBackend *blk;
> @@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device,
> AioContext **aio_context,
>
> *aio_context = NULL;
>
> - blk = blk_by_name(device);
> - if (!blk) {
> + bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);
> + if (!bs) {
If we get here, *errp is already set... [1]
> goto notfound;
> }
>
> - *aio_context = blk_get_aio_context(blk);
> + *aio_context = bdrv_get_aio_context(bs);
> aio_context_acquire(*aio_context);
>
> - if (!blk_is_available(blk)) {
> + blk = blk_by_name(device_or_node);
> + if (blk && !blk_is_available(blk)) {
I'd just drop this. The reason blk_is_available() was added here because
it became possible for BBs not to have a BDS.
Now that you get the BDS directly through bdrv_lookup_bs(), it's no
longer necessary.
> goto notfound;
> }
> - bs = blk_bs(blk);
>
> if (!bs->job) {
> goto notfound;
> @@ -3715,7 +3717,7 @@ static BlockJob *find_block_job(const char *device,
> AioContext **aio_context,
>
> notfound:
> error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> - "No active block job on device '%s'", device);
> + "No active block job on node '%s'", device_or_node);
[1] ...and then we'll get a failed assertion here (through error_setv()).
> if (*aio_context) {
> aio_context_release(*aio_context);
> *aio_context = NULL;
> diff --git a/blockjob.c b/blockjob.c
> index 3557048..2ab4794 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -67,7 +67,8 @@ void *block_job_create(const BlockJobDriver *driver,
> BlockDriverState *bs,
> BlockJob *job;
>
> if (bs->job) {
> - error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> + error_setg(errp, "Node '%s' is in use",
> + bdrv_get_device_or_node_name(bs));
> return NULL;
> }
> bdrv_ref(bs);
> @@ -78,7 +79,7 @@ void *block_job_create(const BlockJobDriver *driver,
> BlockDriverState *bs,
> bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
>
> job->driver = driver;
> - job->id = g_strdup(bdrv_get_device_name(bs));
> + job->id = g_strdup(bdrv_get_device_or_node_name(bs));
Hm, since this is only used for events, it's not too bad that a block
job will have a different name if it was created on a root BDS by
specifying its node name. But it still is kind of strange.
But as I said, it should be fine as long as one can still use the node
name for controlling it (which is the case).
Max
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v9 01/11] block: keep a list of block jobs, (continued)
- [Qemu-devel] [PATCH v9 10/11] qemu-iotests: test overlapping block-stream operations, Alberto Garcia, 2016/04/04
- [Qemu-devel] [PATCH v9 08/11] docs: Document how to stream to an intermediate layer, Alberto Garcia, 2016/04/04
- [Qemu-devel] [PATCH v9 04/11] block: use the block job list in bdrv_close(), Alberto Garcia, 2016/04/04
- [Qemu-devel] [PATCH v9 05/11] block: allow block jobs in any arbitrary node, Alberto Garcia, 2016/04/04
- [Qemu-devel] [PATCH v9 03/11] block: use the block job list in qmp_query_block_jobs(), Alberto Garcia, 2016/04/04
- [Qemu-devel] [PATCH v9 07/11] block: Add QMP support for streaming to an intermediate layer, Alberto Garcia, 2016/04/04