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

[Fam Zheng]

On Wed, 05/13 16:27, 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>
> Reviewed-by: Max Reitz <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>

Reviewed-by: Fam Zheng <address@hidden>

> ---
>  block/mirror.c            |  5 +++--
>  blockdev.c                | 16 ++++++++--------
>  blockjob.c                | 18 ++++++++++--------
>  docs/qmp/qmp-events.txt   |  8 ++++----
>  include/qapi/qmp/qerror.h |  3 ---
>  qapi/block-core.json      | 20 ++++++++++----------
>  6 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 58f391a..4b36e0b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -598,8 +598,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));
>          return;
>      }
>  
> diff --git a/blockdev.c b/blockdev.c
> index bf36a0e..1fcf466 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2813,32 +2813,32 @@ 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, errp);
> +    if (!bs) {
>          goto notfound;
>      }
> -    bs = blk_bs(blk);
>  
>      *aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(*aio_context);
>  
>      if (!bs->job) {
>          aio_context_release(*aio_context);
> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> +              "No active block job on node '%s'", device_or_node);
>          goto notfound;
>      }
>  
>      return bs->job;
>  
>  notfound:
> -    error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> -              "No active block job on device '%s'", device);
>      *aio_context = NULL;
>      return NULL;
>  }
> diff --git a/blockjob.c b/blockjob.c
> index c46984d..96471c6 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -52,7 +52,8 @@ 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_setg(errp, "Node '%s' is in use",
> +                   bdrv_get_device_or_node_name(bs));
>          return NULL;
>      }
>      bdrv_ref(bs);
> @@ -121,8 +122,9 @@ void block_job_set_speed(BlockJob *job, int64_t speed, 
> Error **errp)
>  void block_job_complete(BlockJob *job, Error **errp)
>  {
>      if (job->pause_count || 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;
>      }
>  
> @@ -277,7 +279,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->pause_count > 0;
> @@ -299,7 +301,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,
> @@ -309,7 +311,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,
> @@ -323,7 +325,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);
> @@ -352,7 +354,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 6dc2cca..791f9dd 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -90,7 +90,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.
> @@ -114,7 +114,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.
> @@ -141,7 +141,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
> @@ -165,7 +165,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 863ffea..9b5cddd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -547,7 +547,7 @@
>  #
>  # @type: the job type ('stream' for image streaming)
>  #
> -# @device: the block device name
> +# @device: the block device name, or node name if not present
>  #
>  # @len: the maximum progress value
>  #
> @@ -1146,7 +1146,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.
> @@ -1177,7 +1177,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.
> @@ -1203,7 +1203,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
> @@ -1223,7 +1223,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
> @@ -1249,7 +1249,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
> @@ -1903,7 +1903,7 @@
>  #
>  # @type: job type
>  #
> -# @device: device name
> +# @device: device name, or node name if not present
>  #
>  # @len: maximum progress value
>  #
> @@ -1934,7 +1934,7 @@
>  #
>  # @type: job type
>  #
> -# @device: device name
> +# @device: device name, or node name if not present
>  #
>  # @len: maximum progress value
>  #
> @@ -1957,7 +1957,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
>  #
> @@ -1977,7 +1977,7 @@
>  #
>  # @type: job type
>  #
> -# @device: device name
> +# @device: device name, or node name if not present
>  #
>  # @len: maximum progress value
>  #
> -- 
> 2.1.4
> 
>