Re: [Qemu-devel] [PATCH v9 05/11] block: allow block jobs in any arbitrary node

[Kevin Wolf]

Am 04.04.2016 um 15:43 hat Alberto Garcia geschrieben:
> 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.

Careful, we know this is a part of our API that we have already messed
up and we don't want to make things worse while adding new things before
we've cleaned it up.

Let's keep in mind where we are planning to go with block jobs: They
should become background jobs, not tied to any block device. The close
connection to a single BDS already doesn't make a lot of sense today
because most block jobs involve at least two BDSes.

In the final state, we will have a job ID that uniquely identifies the
job, and each command that starts a job will take an ID from the client.
For compatibility, we'll use the block device name as the job ID when
using old commands that don't get an explicit ID yet.

In the existing qemu version, you can't start two block jobs on the same
device, and in future versions, you're supposed to specify an ID each
time. This is why the default can always be supposed to work without
conflicts. If in newer versions, the client mixes both ways (which it
shouldn't do), starting a new block job may fail because the device name
is already in use as an ID for another job.

Now we can probably make the same argument for node names, so we can
extend the interface and still keep it compatible.

Where we need to be careful is that with device names and node names, we
have potentially two names describing the same BDS and therefore the
same job. This must not happen, because we won't be able to represent
that in the generic background job API. Any job has just a single ID
there.

> - The "device" parameter used by all commands that operate on block
>   jobs can also be a node name now.

So I think the least we need to do is change this one to resolve the
block job by its ID (job->id) rather than device or node names.

> - 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);

Specifically, this one is bad. It allows two different ways to specify
the same job.

The other thing about this patch is that I'm not sure how badly it
conflicts with my series to convert block jobs to BlockBackend. It seems
that you switch everything from blk to bs here, and I'll have to switch
back to blk (once job->blk exists).

Kevin