From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42290) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YiP9d-0005T2-Jk for qemu-devel@nongnu.org; Wed, 15 Apr 2015 11:23:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YiP9b-0001e1-MA for qemu-devel@nongnu.org; Wed, 15 Apr 2015 11:23:05 -0400 Message-ID: <552E8225.1070806@redhat.com> Date: Wed, 15 Apr 2015 17:22:13 +0200 From: Max Reitz MIME-Version: 1.0 References: <09630807f1f0b683168820e53c678d5e12a08342.1428503789.git.berto@igalia.com> In-Reply-To: <09630807f1f0b683168820e53c678d5e12a08342.1428503789.git.berto@igalia.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 2/6] block: allow block jobs in any arbitrary node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: Stefan Hajnoczi , qemu-block@nongnu.org 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 > --- > 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