| Message ID | 09630807f1f0b683168820e53c678d5e12a08342.1428503789.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
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
On Wed 15 Apr 2015 05:22:13 PM CEST, Max Reitz <mreitz@redhat.com> wrote: >> 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. Yeah, I decided to leave it like that while the mirror operation can only work on root nodes. In general in all these patches I'm only using bdrv_get_device_or_node_name() in the cases where it's actually possible that the device name is empty. >> +/* 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). Sounds reasonable, I'll change that. >> 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. Errr... I overlooked that, I'll fix it. >> @@ -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. You're right again :) Berto
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)); 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; } - 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)); 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 #
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(-)