| Message ID | 6034f1747fb838368a057df8463084b6a2326fe4.1466598035.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 22.06.2016 um 14:24 hat Alberto Garcia geschrieben: > Block jobs are identified by the name of the BlockBackend of the BDS > where the job was started. Let me rephrase that: Block jobs are identified by an ID which defaults to the name of the BlockBackend where the job was started and can't currently be set by the user. > We want block jobs to have unique, arbitrary identifiers that are not > tied to a block device, so this patch decouples the ID from the device > name in the BlockJob structure. > > The ID is generated automatically for the moment, in later patches > we'll allow the user to set it. This approach requires us to keep track of both a device name and an ID in order to maintain compatibility, and we always need to check both when a job is referenced. This model is already a pain with node-name and BB name, I wouldn't want to introduce more instances. Why don't we go the easy route and make the ID configurable, but still default to the device name? > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > /** > + * BlockBackend name of the BDS owning the job at the time when > + * the job is started. For compatibility with clients that don't > + * support the ID field. > + */ > + char *device; This in particular feels like a step backwards. This addition is tightening the coupling between jobs and devices instead of removing it. > @@ -464,7 +468,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(job->id); > + info->device = g_strdup(job->device); > info->len = job->len; > info->busy = job->busy; > info->paused = job->pause_count > 0; > @@ -486,7 +490,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, > - job->id, > + job->device, > job->len, > job->offset, > job->speed, > @@ -496,7 +500,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, > - job->id, > + job->device, > job->len, > job->offset, > job->speed, > @@ -510,7 +514,7 @@ void block_job_event_ready(BlockJob *job) > job->ready = true; > > qapi_event_send_block_job_ready(job->driver->job_type, > - job->id, > + job->device, > job->len, > job->offset, > job->speed, &error_abort); > @@ -538,7 +542,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, > default: > abort(); > } > - qapi_event_send_block_job_error(job->id, > + qapi_event_send_block_job_error(job->device, > is_read ? IO_OPERATION_TYPE_READ : > IO_OPERATION_TYPE_WRITE, > action, &error_abort); These monitor related functions are the only users of the device name. Let's simply redefine the respective QMP fields to refer to the job ID rather than the device (because identifying the job is really what they are used for; once we allow more than one block job per device, putting the device there will be completely useless). All of these redefinitions are backwards compatible. Only clients which are new enough to set an individual ID can get something that isn't a device name here. And they will be prepared for it. Kevin
On Wed 22 Jun 2016 02:42:24 PM CEST, Kevin Wolf wrote: >> We want block jobs to have unique, arbitrary identifiers that are not >> tied to a block device, so this patch decouples the ID from the >> device name in the BlockJob structure. >> >> The ID is generated automatically for the moment, in later patches >> we'll allow the user to set it. > > This approach requires us to keep track of both a device name and an > ID in order to maintain compatibility, and we always need to check > both when a job is referenced. This model is already a pain with > node-name and BB name, I wouldn't want to introduce more instances. > > Why don't we go the easy route and make the ID configurable, but still > default to the device name? That was my first approach when I started to write the series, but I discarded it for several reasons: - More confusing API: we'd have many instances of fields and parameters named 'device' holding something that is not a device name or anything similar. We'd have e.g. block-commit(id, device), and the created job would have a 'device' that is not the user-specified 'device' but the user-specified 'id', which has no relation to devices. - More risk of collisions: with the current system if no ID is specified a new one is be generated. That's guaranteed to be valid and unique. If we default to the device name we have no such guarantee. - Potential compatibility break: there's a field that used to hold the name of the device but it no longer does. I don't think any of them is a showstopper, and as you said if a client is recent enough to understand job IDs it should also expect the device field to hold something different from a device name. But I'm not a big fan of this kind of scenarios where the semantics of a returned value depend on what we can assume that the caller is able to handle. I thought adding a new 'ID' field was simpler. The device name is still a device name (where it makes sense). The default ID is guaranteed to be valid and guaranteed not to clash with user-defined IDs. The API is (in my opinion) more clear. The only problems that I can think of: - BlockJobInfo and the events expose the 'device' field which is superfluous. - block-job-{pause,resume,...} can take an ID or a device name. All that said, I'm open to change the implementation, but I'd like to make sure that the problems of keeping the ID and device name merged are considered. Berto
Am 22.06.2016 um 16:42 hat Alberto Garcia geschrieben: > On Wed 22 Jun 2016 02:42:24 PM CEST, Kevin Wolf wrote: > >> We want block jobs to have unique, arbitrary identifiers that are not > >> tied to a block device, so this patch decouples the ID from the > >> device name in the BlockJob structure. > >> > >> The ID is generated automatically for the moment, in later patches > >> we'll allow the user to set it. > > > > This approach requires us to keep track of both a device name and an > > ID in order to maintain compatibility, and we always need to check > > both when a job is referenced. This model is already a pain with > > node-name and BB name, I wouldn't want to introduce more instances. > > > > Why don't we go the easy route and make the ID configurable, but still > > default to the device name? > > That was my first approach when I started to write the series, but I > discarded it for several reasons: > > - More confusing API: we'd have many instances of fields and parameters > named 'device' holding something that is not a device name or anything > similar. > > We'd have e.g. block-commit(id, device), and the created job would > have a 'device' that is not the user-specified 'device' but the > user-specified 'id', which has no relation to devices. Yes, the 'device' field ends up having the wrong name then. This is ugly, but I think having code to deal with two ways of addressing jobs, in two different namespaces, is a cost too high to be justifiable just for making things look nicer. I seem to remember that John is going to introduce a new set of job management functions anyway while he generalises block jobs to just background jobs, so in that case the ugliness would be confined to a deprecated legacy interface. I think that's tolerable. > - More risk of collisions: with the current system if no ID is specified > a new one is be generated. That's guaranteed to be valid and unique. > If we default to the device name we have no such guarantee. The expectation is that if a client passes an ID for one job, it will pass IDs for all jobs. As long as you never pass an ID, the interface looks exactly the same as it used to. If you pass it always, you're obviously responsible for choosing unique IDs. The only vaguely interesting case is if you mix both styles and then create a new job with an explicit ID that you already used for a block device. In this case you get an error, and I'd argue you deserve it. > - Potential compatibility break: there's a field that used to hold the > name of the device but it no longer does. Old clients never set an explicit ID, so what they get in this field is always the default, which is a device name. I don't see the compatibility problem? > I don't think any of them is a showstopper, and as you said if a client > is recent enough to understand job IDs it should also expect the device > field to hold something different from a device name. But I'm not a big > fan of this kind of scenarios where the semantics of a returned value > depend on what we can assume that the caller is able to handle. It's really just another case of making something configurable and letting it default to the previously hardcoded value. Pretty much a normal thing to do. > I thought adding a new 'ID' field was simpler. The device name is still > a device name (where it makes sense). The default ID is guaranteed to be > valid and guaranteed not to clash with user-defined IDs. The API is (in > my opinion) more clear. > > The only problems that I can think of: > > - BlockJobInfo and the events expose the 'device' field which is > superfluous. > - block-job-{pause,resume,...} can take an ID or a device name. Yes. There are two parts that I don't like about this. The first one is that we need additional code to keep track of the device name and to look it up. The other, more important one is that it couples block jobs more tightly with a BDS: * What do you with a background job that doesn't have a device name because it doesn't work on a BDS? Will 'device' become optional everywhere? But how is this less problematic for compatibility than returning non-device-name IDs? (To be clear, I don't think either is a real problem, but you can hardly dismiss one and accept the other.) * And what do you do once we allow more than one job per device? Then the device name isn't suitable for addressing the job any more. And letting the client use the device name after it started the first job, but not any more after it started the second one, feels wrong. Kevin
On Wed 22 Jun 2016 05:49:28 PM CEST, Kevin Wolf wrote: >> I thought adding a new 'ID' field was simpler. The device name is >> still a device name (where it makes sense). The default ID is >> guaranteed to be valid and guaranteed not to clash with user-defined >> IDs. The API is (in my opinion) more clear. >> >> The only problems that I can think of: >> >> - BlockJobInfo and the events expose the 'device' field which is >> superfluous. >> - block-job-{pause,resume,...} can take an ID or a device name. > > Yes. There are two parts that I don't like about this. > > The first one is that we need additional code to keep track of the > device name and to look it up. I think this part is negligible, but ok. > The other, more important one is that it couples block jobs more > tightly with a BDS: > > * What do you with a background job that doesn't have a device name > because it doesn't work on a BDS? Will 'device' become optional > everywhere? But how is this less problematic for compatibility than > returning non-device-name IDs? (To be clear, I don't think either is > a real problem, but you can hardly dismiss one and accept the > other.) > * And what do you do once we allow more than one job per device? Then > the device name isn't suitable for addressing the job any more. And > letting the client use the device name after it started the first > job, but not any more after it started the second one, feels wrong. Fair enough. Unless Max, Eric or someone else has something else to add I'll give it a try and see how it looks. Berto
On 23.06.2016 14:47, Alberto Garcia wrote: > On Wed 22 Jun 2016 05:49:28 PM CEST, Kevin Wolf wrote: >>> I thought adding a new 'ID' field was simpler. The device name is >>> still a device name (where it makes sense). The default ID is >>> guaranteed to be valid and guaranteed not to clash with user-defined >>> IDs. The API is (in my opinion) more clear. >>> >>> The only problems that I can think of: >>> >>> - BlockJobInfo and the events expose the 'device' field which is >>> superfluous. >>> - block-job-{pause,resume,...} can take an ID or a device name. >> >> Yes. There are two parts that I don't like about this. >> >> The first one is that we need additional code to keep track of the >> device name and to look it up. > > I think this part is negligible, but ok. > >> The other, more important one is that it couples block jobs more >> tightly with a BDS: >> >> * What do you with a background job that doesn't have a device name >> because it doesn't work on a BDS? Will 'device' become optional >> everywhere? But how is this less problematic for compatibility than >> returning non-device-name IDs? (To be clear, I don't think either is >> a real problem, but you can hardly dismiss one and accept the >> other.) > >> * And what do you do once we allow more than one job per device? Then >> the device name isn't suitable for addressing the job any more. And >> letting the client use the device name after it started the first >> job, but not any more after it started the second one, feels wrong. > > Fair enough. Unless Max, Eric or someone else has something else to add > I'll give it a try and see how it looks. Sorry for the late response, but then again I don't actually have an opinion either way. The thing I feel most strongly about is the issue of storing a general ID in a field named "device". However, as Kevin hinted at this becoming irrelevant with John's work on decoupling block jobs from the block layer. (And it probably will, because we don't want to call e.g. block-job-pause that way then anymore, so John's probably going to add newly named commands anyway, even if they do the same as the old ones. I don't know.) But this also means that I don't understand the first point of the second part of what you quoted from Kevin here. I think he's referring to what qemu returns e.g. in query-block-jobs. But why should query-block-jobs return non-*block*-jobs? I think it should always omit all background jobs that are not related to the block layer. Or at least that would make sense. The second point of the second part doesn't really strike with me either. If a client uses the device name to identify a block job, they should be used to a version of qemu that isn't capable of having more than one job per device anyway, so they shouldn't launch more than a single job per device to begin with. So I don't think any of the pros and cons is a very strong point. I personally feel like having a single ID field is more natural and making the device name its default value is very intuitive, but I take issue with its naming ("device"). So I don't care either way. (Maybe, aside from John's work, we could get the naming issue out of the way by introducing something like QMP aliases...?) Max
On Wed 29 Jun 2016 07:20:55 PM CEST, Max Reitz wrote: >>>> I thought adding a new 'ID' field was simpler. The device name is >>>> still a device name (where it makes sense). The default ID is >>>> guaranteed to be valid and guaranteed not to clash with >>>> user-defined IDs. The API is (in my opinion) more clear. >>>> >>>> The only problems that I can think of: >>>> >>>> - BlockJobInfo and the events expose the 'device' field which is >>>> superfluous. >>>> - block-job-{pause,resume,...} can take an ID or a device name. >>> >>> Yes. There are two parts that I don't like about this. >>> >>> The first one is that we need additional code to keep track of the >>> device name and to look it up. >> >> I think this part is negligible, but ok. >> >>> The other, more important one is that it couples block jobs more >>> tightly with a BDS: >>> >>> * What do you with a background job that doesn't have a device name >>> because it doesn't work on a BDS? Will 'device' become optional >>> everywhere? But how is this less problematic for compatibility than >>> returning non-device-name IDs? (To be clear, I don't think either is >>> a real problem, but you can hardly dismiss one and accept the >>> other.) >> >>> * And what do you do once we allow more than one job per device? Then >>> the device name isn't suitable for addressing the job any more. And >>> letting the client use the device name after it started the first >>> job, but not any more after it started the second one, feels wrong. >> >> Fair enough. Unless Max, Eric or someone else has something else to add >> I'll give it a try and see how it looks. > > Sorry for the late response, but then again I don't actually have an > opinion either way. > > The thing I feel most strongly about is the issue of storing a general > ID in a field named "device". However, as Kevin hinted at this > becoming irrelevant with John's work on decoupling block jobs from the > block layer. I actually forgot to Cc him, I'm doing it now. The idea is that I don't want to add anything now that is going to cause headaches later. Adding a new 'id' field to block jobs and keeping 'device' feels more natural to me, but reusing the 'device' field and allowing any ID set by the user requires less changes both to the code and the API. Berto
On 06/30/2016 09:03 AM, Alberto Garcia wrote: > On Wed 29 Jun 2016 07:20:55 PM CEST, Max Reitz wrote: >>>>> I thought adding a new 'ID' field was simpler. The device name is >>>>> still a device name (where it makes sense). The default ID is >>>>> guaranteed to be valid and guaranteed not to clash with >>>>> user-defined IDs. The API is (in my opinion) more clear. >>>>> >>>>> The only problems that I can think of: >>>>> >>>>> - BlockJobInfo and the events expose the 'device' field which is >>>>> superfluous. >>>>> - block-job-{pause,resume,...} can take an ID or a device name. >>>> >>>> Yes. There are two parts that I don't like about this. >>>> >>>> The first one is that we need additional code to keep track of the >>>> device name and to look it up. >>> >>> I think this part is negligible, but ok. >>> >>>> The other, more important one is that it couples block jobs more >>>> tightly with a BDS: >>>> >>>> * What do you with a background job that doesn't have a device name >>>> because it doesn't work on a BDS? Will 'device' become optional >>>> everywhere? But how is this less problematic for compatibility than >>>> returning non-device-name IDs? (To be clear, I don't think either is >>>> a real problem, but you can hardly dismiss one and accept the >>>> other.) >>> >>>> * And what do you do once we allow more than one job per device? Then >>>> the device name isn't suitable for addressing the job any more. And >>>> letting the client use the device name after it started the first >>>> job, but not any more after it started the second one, feels wrong. >>> >>> Fair enough. Unless Max, Eric or someone else has something else to add >>> I'll give it a try and see how it looks. >> >> Sorry for the late response, but then again I don't actually have an >> opinion either way. >> >> The thing I feel most strongly about is the issue of storing a general >> ID in a field named "device". However, as Kevin hinted at this >> becoming irrelevant with John's work on decoupling block jobs from the >> block layer. > > I actually forgot to Cc him, I'm doing it now. > > The idea is that I don't want to add anything now that is going to cause > headaches later. Adding a new 'id' field to block jobs and keeping > 'device' feels more natural to me, but reusing the 'device' field and > allowing any ID set by the user requires less changes both to the code > and the API. > > Berto > Reviewing everything now, sorry for being MIA, and thank you for keeping me in the loop. --js
diff --git a/block/mirror.c b/block/mirror.c index a04ed9c..bcb1999 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -755,7 +755,8 @@ static void mirror_complete(BlockJob *job, Error **errp) target = blk_bs(s->target); if (!s->synced) { - error_setg(errp, QERR_BLOCK_JOB_NOT_READY, job->id); + error_setg(errp, "The active block job '%s' cannot be completed", + job->id); return; } diff --git a/blockjob.c b/blockjob.c index 90c4e26..8bcb5ea 100644 --- a/blockjob.c +++ b/blockjob.c @@ -33,6 +33,7 @@ #include "qapi/qmp/qerror.h" #include "qapi/qmp/qjson.h" #include "qemu/coroutine.h" +#include "qemu/id.h" #include "qmp-commands.h" #include "qemu/timer.h" #include "qapi-event.h" @@ -125,7 +126,8 @@ 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->device = g_strdup(bdrv_get_device_name(bs)); + job->id = id_generate(ID_JOB); job->blk = blk; job->cb = cb; job->opaque = opaque; @@ -168,6 +170,7 @@ void block_job_unref(BlockJob *job) block_job_detach_aio_context, job); blk_unref(job->blk); error_free(job->blocker); + g_free(job->device); g_free(job->id); QLIST_REMOVE(job, job_list); g_free(job); @@ -289,7 +292,8 @@ 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_setg(errp, QERR_BLOCK_JOB_NOT_READY, job->id); + error_setg(errp, "The active block job '%s' cannot be completed", + job->id); return; } @@ -464,7 +468,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(job->id); + info->device = g_strdup(job->device); info->len = job->len; info->busy = job->busy; info->paused = job->pause_count > 0; @@ -486,7 +490,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, - job->id, + job->device, job->len, job->offset, job->speed, @@ -496,7 +500,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, - job->id, + job->device, job->len, job->offset, job->speed, @@ -510,7 +514,7 @@ void block_job_event_ready(BlockJob *job) job->ready = true; qapi_event_send_block_job_ready(job->driver->job_type, - job->id, + job->device, job->len, job->offset, job->speed, &error_abort); @@ -538,7 +542,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, default: abort(); } - qapi_event_send_block_job_error(job->id, + qapi_event_send_block_job_error(job->device, is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE, action, &error_abort); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 7dc720c..856486a 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -106,14 +106,18 @@ struct BlockJob { BlockBackend *blk; /** - * The ID of the block job. Currently the BlockBackend name of the BDS - * owning the job at the time when the job is started. - * - * TODO Decouple block job IDs from BlockBackend names + * The ID of the block job. */ char *id; /** + * BlockBackend name of the BDS owning the job at the time when + * the job is started. For compatibility with clients that don't + * support the ID field. + */ + char *device; + + /** * The coroutine that executes the job. If not NULL, it is * reentered when busy is false and the job is cancelled. */ diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index d08652a..6586c9f 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -19,9 +19,6 @@ #define QERR_BASE_NOT_FOUND \ "Base '%s' not found" -#define QERR_BLOCK_JOB_NOT_READY \ - "The active block job for device '%s' cannot be completed" - #define QERR_BUS_NO_HOTPLUG \ "Bus '%s' does not support hotplugging" diff --git a/include/qemu/id.h b/include/qemu/id.h index 7d90335..c6c73ca 100644 --- a/include/qemu/id.h +++ b/include/qemu/id.h @@ -4,6 +4,7 @@ typedef enum IdSubSystems { ID_QDEV, ID_BLOCK, + ID_JOB, ID_MAX /* last element, used as array size */ } IdSubSystems; diff --git a/util/id.c b/util/id.c index 6141352..eb5478b 100644 --- a/util/id.c +++ b/util/id.c @@ -34,6 +34,7 @@ bool id_wellformed(const char *id) static const char *const id_subsys_str[ID_MAX] = { [ID_QDEV] = "qdev", [ID_BLOCK] = "block", + [ID_JOB] = "job", }; /*
Block jobs are identified by the name of the BlockBackend of the BDS where the job was started. We want block jobs to have unique, arbitrary identifiers that are not tied to a block device, so this patch decouples the ID from the device name in the BlockJob structure. The ID is generated automatically for the moment, in later patches we'll allow the user to set it. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/mirror.c | 3 ++- blockjob.c | 18 +++++++++++------- include/block/blockjob.h | 12 ++++++++---- include/qapi/qmp/qerror.h | 3 --- include/qemu/id.h | 1 + util/id.c | 1 + 6 files changed, 23 insertions(+), 15 deletions(-)