Re: [Qemu-devel] [Qemu-block] [PATCH 2/6] block: allow block jobs in any arbitrary node

[Max Reitz <mreitz@redhat.com>]

On 08.04.2015 16: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>
> ---
>   block/mirror.c            |  5 +++--
>   blockdev.c                | 14 +++++++-------
>   blockjob.c                | 17 +++++++++--------
>   docs/qmp/qmp-events.txt   |  8 ++++----
>   include/qapi/qmp/qerror.h |  3 ---
>   qapi/block-core.json      | 18 +++++++++---------
>   6 files changed, 32 insertions(+), 33 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 4056164..189e8f8 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -607,8 +607,9 @@ static void mirror_complete(BlockJob *job, Error **errp)
>           return;
>       }
>       if (!s->synced) {
> -        error_set(errp, QERR_BLOCK_JOB_NOT_READY,
> -                  bdrv_get_device_name(job->bs));
> +        error_setg(errp,
> +                   "The active block job for device '%s' cannot be completed",
> +                   bdrv_get_device_name(job->bs));

I know there is no way right now that bdrv_get_device_name() returns an 
empty string, but somehow I'd feel more consistent for this to be 
bdrv_get_device_or_node_name() and the string saying "node" instead of 
"device". Your choice, though, since it's completely correct for now.

>           return;
>       }
>   
> diff --git a/blockdev.c b/blockdev.c
> index 567d5e3..dc5d931 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2643,18 +2643,18 @@ out:
>       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;
>       BlockDriverState *bs;
>   
> -    blk = blk_by_name(device);
> -    if (!blk) {
> +    bs = bdrv_lookup_bs(device_or_node, device_or_node, NULL);
> +    if (!bs) {
>           goto notfound;

It would be possible to pass errp to bdrv_lookup_bs() and move the 
error_set() under notfound to the if (!bs->job) block (I'd find the 
error message confusing if there just is no node with that name; but on 
the other hand, it wouldn't be a regression).

>       }
> -    bs = blk_bs(blk);
>   
>       *aio_context = bdrv_get_aio_context(bs);
>       aio_context_acquire(*aio_context);
> @@ -2668,7 +2668,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);
>       *aio_context = NULL;
>       return NULL;
>   }
> diff --git a/blockjob.c b/blockjob.c
> index 562362b..b2b6cc9 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -52,7 +52,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>       BlockJob *job;
>   
>       if (bs->job) {
> -        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> +        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_or_node_name(bs));

Hm, the error message only mentions "device" now... Not too nice.

>           return NULL;
>       }
>       bdrv_ref(bs);
> @@ -121,8 +121,9 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>   void block_job_complete(BlockJob *job, Error **errp)
>   {
>       if (job->paused || job->cancelled || !job->driver->complete) {
> -        error_set(errp, QERR_BLOCK_JOB_NOT_READY,
> -                  bdrv_get_device_name(job->bs));
> +        error_setg(errp,
> +                   "The active block job for node '%s' cannot be completed",
> +                   bdrv_get_device_or_node_name(job->bs));
>           return;
>       }
>   
> @@ -268,7 +269,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
>   {
>       BlockJobInfo *info = g_new0(BlockJobInfo, 1);
>       info->type      = g_strdup(BlockJobType_lookup[job->driver->job_type]);
> -    info->device    = g_strdup(bdrv_get_device_name(job->bs));
> +    info->device    = g_strdup(bdrv_get_device_or_node_name(job->bs));
>       info->len       = job->len;
>       info->busy      = job->busy;
>       info->paused    = job->paused;
> @@ -290,7 +291,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)
>   void block_job_event_cancelled(BlockJob *job)
>   {
>       qapi_event_send_block_job_cancelled(job->driver->job_type,
> -                                        bdrv_get_device_name(job->bs),
> +                                        bdrv_get_device_or_node_name(job->bs),
>                                           job->len,
>                                           job->offset,
>                                           job->speed,
> @@ -300,7 +301,7 @@ void block_job_event_cancelled(BlockJob *job)
>   void block_job_event_completed(BlockJob *job, const char *msg)
>   {
>       qapi_event_send_block_job_completed(job->driver->job_type,
> -                                        bdrv_get_device_name(job->bs),
> +                                        bdrv_get_device_or_node_name(job->bs),
>                                           job->len,
>                                           job->offset,
>                                           job->speed,
> @@ -314,7 +315,7 @@ void block_job_event_ready(BlockJob *job)
>       job->ready = true;
>   
>       qapi_event_send_block_job_ready(job->driver->job_type,
> -                                    bdrv_get_device_name(job->bs),
> +                                    bdrv_get_device_or_node_name(job->bs),
>                                       job->len,
>                                       job->offset,
>                                       job->speed, &error_abort);
> @@ -343,7 +344,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
>       default:
>           abort();
>       }
> -    qapi_event_send_block_job_error(bdrv_get_device_name(job->bs),
> +    qapi_event_send_block_job_error(bdrv_get_device_or_node_name(job->bs),
>                                       is_read ? IO_OPERATION_TYPE_READ :
>                                       IO_OPERATION_TYPE_WRITE,
>                                       action, &error_abort);
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index b19e490..db90c61 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -89,7 +89,7 @@ Data:
>   
>   - "type":     Job type (json-string; "stream" for image streaming
>                                        "commit" for block commit)
> -- "device":   Device name (json-string)
> +- "device":   Device name, or node name if not present (json-string)
>   - "len":      Maximum progress value (json-int)
>   - "offset":   Current progress value (json-int)
>                 On success this is equal to len.
> @@ -113,7 +113,7 @@ Data:
>   
>   - "type":     Job type (json-string; "stream" for image streaming
>                                        "commit" for block commit)
> -- "device":   Device name (json-string)
> +- "device":   Device name, or node name if not present (json-string)
>   - "len":      Maximum progress value (json-int)
>   - "offset":   Current progress value (json-int)
>                 On success this is equal to len.
> @@ -140,7 +140,7 @@ Emitted when a block job encounters an error.
>   
>   Data:
>   
> -- "device": device name (json-string)
> +- "device": device name, or node name if not present (json-string)
>   - "operation": I/O operation (json-string, "read" or "write")
>   - "action": action that has been taken, it's one of the following (json-string):
>       "ignore": error has been ignored, the job may fail later
> @@ -164,7 +164,7 @@ Data:
>   
>   - "type":     Job type (json-string; "stream" for image streaming
>                                        "commit" for block commit)
> -- "device":   Device name (json-string)
> +- "device":   Device name, or node name if not present (json-string)
>   - "len":      Maximum progress value (json-int)
>   - "offset":   Current progress value (json-int)
>                 On success this is equal to len.
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index e567339..4ba442e 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -37,9 +37,6 @@ void qerror_report_err(Error *err);
>   #define QERR_BASE_NOT_FOUND \
>       ERROR_CLASS_GENERIC_ERROR, "Base '%s' not found"
>   
> -#define QERR_BLOCK_JOB_NOT_READY \
> -    ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be completed"
> -
>   #define QERR_BUS_NO_HOTPLUG \
>       ERROR_CLASS_GENERIC_ERROR, "Bus '%s' does not support hotplugging"
>   
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 21e6cb5..3639454 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1065,7 +1065,7 @@
>   #
>   # Throttling can be disabled by setting the speed to 0.
>   #
> -# @device: the device name
> +# @device: the device or node name of the owner of the block job.
>   #
>   # @speed:  the maximum speed, in bytes per second, or 0 for unlimited.
>   #          Defaults to 0.
> @@ -1096,7 +1096,7 @@
>   # operation can be started at a later time to finish copying all data from the
>   # backing file.
>   #
> -# @device: the device name
> +# @device: the device or node name of the owner of the block job.
>   #
>   # @force: #optional whether to allow cancellation of a paused job (default
>   #         false).  Since 1.3.
> @@ -1122,7 +1122,7 @@
>   # the operation is actually paused.  Cancelling a paused job automatically
>   # resumes it.
>   #
> -# @device: the device name
> +# @device: the device or node name of the owner of the block job.
>   #
>   # Returns: Nothing on success
>   #          If no background operation is active on this device, DeviceNotActive
> @@ -1142,7 +1142,7 @@
>   #
>   # This command also clears the error status of the job.
>   #
> -# @device: the device name
> +# @device: the device or node name of the owner of the block job.
>   #
>   # Returns: Nothing on success
>   #          If no background operation is active on this device, DeviceNotActive
> @@ -1168,7 +1168,7 @@
>   #
>   # A cancelled or paused job cannot be completed.
>   #
> -# @device: the device name
> +# @device: the device or node name of the owner of the block job.
>   #
>   # Returns: Nothing on success
>   #          If no background operation is active on this device, DeviceNotActive
> @@ -1821,7 +1821,7 @@
>   #
>   # @type: job type
>   #
> -# @device: device name
> +# @device: device name, or node name if not present
>   #
>   # @len: maximum progress value
>   #
> @@ -1852,7 +1852,7 @@
>   #
>   # @type: job type
>   #
> -# @device: device name
> +# @device: device name, or node name if not present
>   #
>   # @len: maximum progress value
>   #
> @@ -1875,7 +1875,7 @@
>   #
>   # Emitted when a block job encounters an error
>   #
> -# @device: device name
> +# @device: device name, or node name if not present
>   #
>   # @operation: I/O operation
>   #
> @@ -1895,7 +1895,7 @@
>   #
>   # @type: job type
>   #
> -# @device: device name
> +# @device: device name, or node name if not present
>   #
>   # @len: maximum progress value
>   #

I think you need to apply the same treatment for the comment of 
BlockJobInfo, too.

That last thing is the only thing preventing me from giving an R-b, the 
rest is optional.

Max