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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4359 bytes --]

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 <berto@igalia.com>
> ---
>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]