| Message ID | 75bbfc268498ae3733d7ffb896ab0852527d5b11.1348589526.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/25/2012 10:29 AM, Jeff Cody wrote: > The command for live block commit is added, which has the following > arguments: > > device: the block device to perform the commit on (mandatory) > base: the base image to commit into; optional (if not specified, > it is the underlying original image) > top: the top image of the commit - all data from inside top down > to base will be committed into base. optional (if not specified, > it is one below the active image) - see note below This says that for 'base' <- 'mid' <- 'active', omitting 'top' is the same as specifying 'top':'mid'. > speed: maximum speed, in bytes/sec > > note: eventually this will support merging down the active layer, > but that code is not yet complete. If the active layer is passed > in currently as top, or top is left to the default, then an error > will be returned. This says that for 'base' <- 'mid' <- 'active', omitting 'top' is an error (because it would be the same as an explicit 'top:'active'). Let's check the code... > + if (top && has_top) { > + /* if we want to allow the active layer, > + * use 'bdrv_find_image()' here */ > + top_bs = bdrv_find_backing_image(bs, top); > + } else { > + /* default is one below the active layer, unless that is > + * the base */ > + top_bs = bs->backing_hd; Aha - the former is correct as coded (you default to 'top':'mid' in my example), so the 'note' in your commit message needs editing. On the other hand, when we ever DO get around to adding live commit, which is the saner default? That is, am I more likely to want to do live commit from 'active', or more likely to do commit of the layer below 'active'? If live commit is the more desirable case, then that argues that THIS commit should always error out if !has_top, and that the future patch will default top_bs = bs, and the rest of your commit message and documentation would need tweaking accordingly. I don't have a preference either way (libvirt can arrange to always pass the top argument, and thus avoid the confusion when top is omitted), but if anyone else cares, now is the time to get the API right before we are locked in to it. > +++ b/qapi-schema.json > @@ -1468,6 +1468,41 @@ > 'returns': 'str' } > > ## > +# @block-commit > +# > +# Live commit of data from overlay image nodes into backing nodes - i.e., > +# writes data between 'top' and 'base' into 'base'. > +# > +# @device: the name of the device > +# > +# @base: #optional The file name of the backing image to write data into. > +# If not specified, this is the deepest backing image > +# > +# @top: #optional The file name of the backing image within the image chain, > +# which contains the topmost data to be committed down. > +# If not specified, this is one layer below the active > +# layer (i.e. active->backing_hd). > +# > +# If top == base, that is an error. > +# This documentation of @top matches the first paragraph of your commit message. > +# > +# @speed: #optional the maximum speed, in bytes per second > +# > +# Returns: Nothing on success > +# If commit or stream is already active on this device, DeviceInUse > +# If @device does not exist, DeviceNotFound > +# If image commit is not supported by this device, NotSupported > +# If @base does not exist, a generic error is returned > +# If @top does not exist, a generic error is returned These two lines could be merged, or even made more generic (for example, based on the rest of this thread, you will also be erroring out if base and top exist, but appear as swapped arguments): If @base or @top is invalid, a generic error is returned
On 09/25/2012 03:42 PM, Eric Blake wrote: > On 09/25/2012 10:29 AM, Jeff Cody wrote: >> The command for live block commit is added, which has the following >> arguments: >> >> device: the block device to perform the commit on (mandatory) >> base: the base image to commit into; optional (if not specified, >> it is the underlying original image) >> top: the top image of the commit - all data from inside top down >> to base will be committed into base. optional (if not specified, >> it is one below the active image) - see note below > > This says that for 'base' <- 'mid' <- 'active', omitting 'top' is the > same as specifying 'top':'mid'. > That is the intended default. >> speed: maximum speed, in bytes/sec >> >> note: eventually this will support merging down the active layer, >> but that code is not yet complete. If the active layer is passed >> in currently as top, or top is left to the default, then an error >> will be returned. > > This says that for 'base' <- 'mid' <- 'active', omitting 'top' is an > error (because it would be the same as an explicit 'top:'active'). This is how I originally had it, and then I changed it to default to top->backing_hd. I apparently did not remember to catch the note, however. > > Let's check the code... > >> + if (top && has_top) { >> + /* if we want to allow the active layer, >> + * use 'bdrv_find_image()' here */ >> + top_bs = bdrv_find_backing_image(bs, top); >> + } else { >> + /* default is one below the active layer, unless that is >> + * the base */ >> + top_bs = bs->backing_hd; > > Aha - the former is correct as coded (you default to 'top':'mid' in my > example), so the 'note' in your commit message needs editing. > > On the other hand, when we ever DO get around to adding live commit, > which is the saner default? That is, am I more likely to want to do > live commit from 'active', or more likely to do commit of the layer > below 'active'? If live commit is the more desirable case, then that > argues that THIS commit should always error out if !has_top, and that > the future patch will default top_bs = bs, and the rest of your commit > message and documentation would need tweaking accordingly. > > I don't have a preference either way (libvirt can arrange to always pass > the top argument, and thus avoid the confusion when top is omitted), but > if anyone else cares, now is the time to get the API right before we are > locked in to it. > I guess I don't have a strong preference either - I originally had it the other way, but then that meant the default in the current implementation was actually an error. Also, I assumed (danger!) that the most common use of commit would be a snapshot, followed by a commit of active->backing_hd. With that assumption, it seemed like a sane default. >> +++ b/qapi-schema.json >> @@ -1468,6 +1468,41 @@ >> 'returns': 'str' } >> >> ## >> +# @block-commit >> +# >> +# Live commit of data from overlay image nodes into backing nodes - i.e., >> +# writes data between 'top' and 'base' into 'base'. >> +# >> +# @device: the name of the device >> +# >> +# @base: #optional The file name of the backing image to write data into. >> +# If not specified, this is the deepest backing image >> +# >> +# @top: #optional The file name of the backing image within the image chain, >> +# which contains the topmost data to be committed down. >> +# If not specified, this is one layer below the active >> +# layer (i.e. active->backing_hd). >> +# >> +# If top == base, that is an error. >> +# > > This documentation of @top matches the first paragraph of your commit > message. > >> +# >> +# @speed: #optional the maximum speed, in bytes per second >> +# >> +# Returns: Nothing on success >> +# If commit or stream is already active on this device, DeviceInUse >> +# If @device does not exist, DeviceNotFound >> +# If image commit is not supported by this device, NotSupported >> +# If @base does not exist, a generic error is returned >> +# If @top does not exist, a generic error is returned > > These two lines could be merged, or even made more generic (for example, > based on the rest of this thread, you will also be erroring out if base > and top exist, but appear as swapped arguments): > > If @base or @top is invalid, a generic error is returned > OK
Am 25.09.2012 18:29, schrieb Jeff Cody: > The command for live block commit is added, which has the following > arguments: > > device: the block device to perform the commit on (mandatory) > base: the base image to commit into; optional (if not specified, > it is the underlying original image) > top: the top image of the commit - all data from inside top down > to base will be committed into base. optional (if not specified, > it is one below the active image) - see note below > speed: maximum speed, in bytes/sec > > note: eventually this will support merging down the active layer, > but that code is not yet complete. If the active layer is passed > in currently as top, or top is left to the default, then an error > will be returned. > > The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will > be emitted. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > diff --git a/qapi-schema.json b/qapi-schema.json > index 14e4419..e614453 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1468,6 +1468,41 @@ > 'returns': 'str' } > > ## > +# @block-commit > +# > +# Live commit of data from overlay image nodes into backing nodes - i.e., > +# writes data between 'top' and 'base' into 'base'. > +# > +# @device: the name of the device > +# > +# @base: #optional The file name of the backing image to write data into. > +# If not specified, this is the deepest backing image > +# > +# @top: #optional The file name of the backing image within the image chain, > +# which contains the topmost data to be committed down. > +# If not specified, this is one layer below the active > +# layer (i.e. active->backing_hd). Why isn't active the default any more? I know, we don't support it yet, but long term this is what makes most sense as a default. > +# > +# If top == base, that is an error. > +# > +# > +# @speed: #optional the maximum speed, in bytes per second > +# > +# Returns: Nothing on success > +# If commit or stream is already active on this device, DeviceInUse > +# If @device does not exist, DeviceNotFound > +# If image commit is not supported by this device, NotSupported > +# If @base does not exist, a generic error is returned > +# If @top does not exist, a generic error is returned > +# If @speed is invalid, InvalidParameter > +# > +# Since: 1.3 > +# > +## > +{ 'command': 'block-commit', > + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', > + '*speed': 'int' } } Kevin
On 09/26/2012 10:13 AM, Kevin Wolf wrote: > Am 25.09.2012 18:29, schrieb Jeff Cody: >> The command for live block commit is added, which has the following >> arguments: >> >> device: the block device to perform the commit on (mandatory) >> base: the base image to commit into; optional (if not specified, >> it is the underlying original image) >> top: the top image of the commit - all data from inside top down >> to base will be committed into base. optional (if not specified, >> it is one below the active image) - see note below >> speed: maximum speed, in bytes/sec >> >> note: eventually this will support merging down the active layer, >> but that code is not yet complete. If the active layer is passed >> in currently as top, or top is left to the default, then an error >> will be returned. >> >> The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will >> be emitted. >> >> Signed-off-by: Jeff Cody <jcody@redhat.com> > >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 14e4419..e614453 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -1468,6 +1468,41 @@ >> 'returns': 'str' } >> >> ## >> +# @block-commit >> +# >> +# Live commit of data from overlay image nodes into backing nodes - i.e., >> +# writes data between 'top' and 'base' into 'base'. >> +# >> +# @device: the name of the device >> +# >> +# @base: #optional The file name of the backing image to write data into. >> +# If not specified, this is the deepest backing image >> +# >> +# @top: #optional The file name of the backing image within the image chain, >> +# which contains the topmost data to be committed down. >> +# If not specified, this is one layer below the active >> +# layer (i.e. active->backing_hd). > > Why isn't active the default any more? I know, we don't support it yet, > but long term this is what makes most sense as a default. > Eric had a similar question, and asked if anyone had any preference - this was my response: --- I guess I don't have a strong preference either - I originally had it the other way, but then that meant the default in the current implementation was actually an error. Also, I assumed (danger!) that the most common use of commit would be a snapshot, followed by a commit of active->backing_hd. With that assumption, it seemed like a sane default. --- I can certainly revert back to having the active layer be the top, if that is the preference. >> +# >> +# If top == base, that is an error. >> +# >> +# >> +# @speed: #optional the maximum speed, in bytes per second >> +# >> +# Returns: Nothing on success >> +# If commit or stream is already active on this device, DeviceInUse >> +# If @device does not exist, DeviceNotFound >> +# If image commit is not supported by this device, NotSupported >> +# If @base does not exist, a generic error is returned >> +# If @top does not exist, a generic error is returned >> +# If @speed is invalid, InvalidParameter >> +# >> +# Since: 1.3 >> +# >> +## >> +{ 'command': 'block-commit', >> + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', >> + '*speed': 'int' } } > > Kevin >
Am 26.09.2012 16:25, schrieb Jeff Cody: > On 09/26/2012 10:13 AM, Kevin Wolf wrote: >> Am 25.09.2012 18:29, schrieb Jeff Cody: >>> The command for live block commit is added, which has the following >>> arguments: >>> >>> device: the block device to perform the commit on (mandatory) >>> base: the base image to commit into; optional (if not specified, >>> it is the underlying original image) >>> top: the top image of the commit - all data from inside top down >>> to base will be committed into base. optional (if not specified, >>> it is one below the active image) - see note below >>> speed: maximum speed, in bytes/sec >>> >>> note: eventually this will support merging down the active layer, >>> but that code is not yet complete. If the active layer is passed >>> in currently as top, or top is left to the default, then an error >>> will be returned. >>> >>> The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will >>> be emitted. >>> >>> Signed-off-by: Jeff Cody <jcody@redhat.com> >> >>> diff --git a/qapi-schema.json b/qapi-schema.json >>> index 14e4419..e614453 100644 >>> --- a/qapi-schema.json >>> +++ b/qapi-schema.json >>> @@ -1468,6 +1468,41 @@ >>> 'returns': 'str' } >>> >>> ## >>> +# @block-commit >>> +# >>> +# Live commit of data from overlay image nodes into backing nodes - i.e., >>> +# writes data between 'top' and 'base' into 'base'. >>> +# >>> +# @device: the name of the device >>> +# >>> +# @base: #optional The file name of the backing image to write data into. >>> +# If not specified, this is the deepest backing image >>> +# >>> +# @top: #optional The file name of the backing image within the image chain, >>> +# which contains the topmost data to be committed down. >>> +# If not specified, this is one layer below the active >>> +# layer (i.e. active->backing_hd). >> >> Why isn't active the default any more? I know, we don't support it yet, >> but long term this is what makes most sense as a default. >> > > Eric had a similar question, and asked if anyone had any preference - > this was my response: > > --- > > I guess I don't have a strong preference either - I originally had it > the other way, but then that meant the default in the current > implementation was actually an error. We can make it non-optional for now and use active as the default once we introduce support for committing the active layer. > Also, I assumed (danger!) that the most common use of commit would be a > snapshot, followed by a commit of active->backing_hd. With that > assumption, it seemed like a sane default. > > --- > > I can certainly revert back to having the active layer be the top, if > that is the preference. I think it is, if nothing else for consistency with the existing synchronous 'commit' command. Kevin
On 09/26/2012 08:25 AM, Jeff Cody wrote: >>> +# @top: #optional The file name of the backing image within the image chain, >>> +# which contains the topmost data to be committed down. >>> +# If not specified, this is one layer below the active >>> +# layer (i.e. active->backing_hd). >> >> Why isn't active the default any more? I know, we don't support it yet, >> but long term this is what makes most sense as a default. >> > > Eric had a similar question, and asked if anyone had any preference - > this was my response: > > --- > > I guess I don't have a strong preference either - I originally had it > the other way, but then that meant the default in the current > implementation was actually an error. > > Also, I assumed (danger!) that the most common use of commit would be a > snapshot, followed by a commit of active->backing_hd. With that > assumption, it seemed like a sane default. Actually, I envision my personal use case to be: Take a snapshot, do an experiment in the guest (such as install a questionable package), and then either roll back to the snapshot (experiment failed) or commit the active (experiment worked, no need to have a snapshot any more); either way, taking the snapshot created a new temporary file name, and after I make my decision on whether to commit or discard the snapshot, I want to get back to the original file name. Since the snapshot effectively created a temporary file name, I'd rather not have to know the name of that temporary file just to pass it to an explicit 'top' argument when committing the active layer. Having to specify 'top' to avoid an error when not committing the active layer is not as bad as getting the defaults wrong for when we do add support for committing the active layer. > --- > > I can certainly revert back to having the active layer be the top, if > that is the preference. Given that it has been asked again, I'd say yes, go ahead and revert to the behavior of defaulting 'top' to the active layer.
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index 2878058..4491020 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -50,7 +50,8 @@ Emitted when a block job has been cancelled. Data: -- "type": Job type ("stream" for image streaming, json-string) +- "type": Job type (json-string; "stream" for image streaming + "commit" for block commit) - "device": Device name (json-string) - "len": Maximum progress value (json-int) - "offset": Current progress value (json-int) @@ -73,7 +74,8 @@ Emitted when a block job has completed. Data: -- "type": Job type ("stream" for image streaming, json-string) +- "type": Job type (json-string; "stream" for image streaming + "commit" for block commit) - "device": Device name (json-string) - "len": Maximum progress value (json-int) - "offset": Current progress value (json-int) diff --git a/blockdev.c b/blockdev.c index 9caa16f..6f1080c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1124,6 +1124,70 @@ void qmp_block_stream(const char *device, bool has_base, trace_qmp_block_stream(bs, bs->job); } +void qmp_block_commit(const char *device, + bool has_base, const char *base, + bool has_top, const char *top, + bool has_speed, int64_t speed, + Error **errp) +{ + BlockDriverState *bs; + BlockDriverState *base_bs, *top_bs; + Error *local_err = NULL; + /* This will be part of the QMP command, if/when the + * BlockdevOnError change for blkmirror makes it in + */ + BlockErrorAction on_error = BLOCK_ERR_REPORT; + + /* drain all i/o before commits */ + bdrv_drain_all(); + + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + if (base && has_base) { + base_bs = bdrv_find_backing_image(bs, base); + } else { + base_bs = bdrv_find_base(bs); + } + + if (base_bs == NULL) { + error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL"); + return; + } + + if (top && has_top) { + /* if we want to allow the active layer, + * use 'bdrv_find_image()' here */ + top_bs = bdrv_find_backing_image(bs, top); + } else { + /* default is one below the active layer, unless that is + * the base */ + top_bs = bs->backing_hd; + if (top_bs == base_bs) { + error_setg(errp, + "Invalid files for merge: top and base are the same"); + return; + } + } + if (top_bs == NULL) { + error_setg(errp, "Top image file %s not found", top ? top : "NULL"); + return; + } + + commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs, + &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); + return; + } + /* Grab a reference so hotplug does not delete the BlockDriverState from + * underneath us. + */ + drive_get_ref(drive_get_by_blockdev(bs)); +} + static BlockJob *find_block_job(const char *device) { BlockDriverState *bs; diff --git a/qapi-schema.json b/qapi-schema.json index 14e4419..e614453 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1468,6 +1468,41 @@ 'returns': 'str' } ## +# @block-commit +# +# Live commit of data from overlay image nodes into backing nodes - i.e., +# writes data between 'top' and 'base' into 'base'. +# +# @device: the name of the device +# +# @base: #optional The file name of the backing image to write data into. +# If not specified, this is the deepest backing image +# +# @top: #optional The file name of the backing image within the image chain, +# which contains the topmost data to be committed down. +# If not specified, this is one layer below the active +# layer (i.e. active->backing_hd). +# +# If top == base, that is an error. +# +# +# @speed: #optional the maximum speed, in bytes per second +# +# Returns: Nothing on success +# If commit or stream is already active on this device, DeviceInUse +# If @device does not exist, DeviceNotFound +# If image commit is not supported by this device, NotSupported +# If @base does not exist, a generic error is returned +# If @top does not exist, a generic error is returned +# If @speed is invalid, InvalidParameter +# +# Since: 1.3 +# +## +{ 'command': 'block-commit', + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', + '*speed': 'int' } } + # @migrate_cancel # # Cancel the current executing migration process. diff --git a/qmp-commands.hx b/qmp-commands.hx index 6e21ddb..e244763 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -792,6 +792,12 @@ EQMP }, { + .name = "block-commit", + .args_type = "device:B,base:s?,top:s?,speed:o?", + .mhandler.cmd_new = qmp_marshal_input_block_commit, + }, + + { .name = "block-job-set-speed", .args_type = "device:B,speed:o", .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,
The command for live block commit is added, which has the following arguments: device: the block device to perform the commit on (mandatory) base: the base image to commit into; optional (if not specified, it is the underlying original image) top: the top image of the commit - all data from inside top down to base will be committed into base. optional (if not specified, it is one below the active image) - see note below speed: maximum speed, in bytes/sec note: eventually this will support merging down the active layer, but that code is not yet complete. If the active layer is passed in currently as top, or top is left to the default, then an error will be returned. The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will be emitted. Signed-off-by: Jeff Cody <jcody@redhat.com> --- QMP/qmp-events.txt | 6 +++-- blockdev.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 35 +++++++++++++++++++++++++++++ qmp-commands.hx | 6 +++++ 4 files changed, 109 insertions(+), 2 deletions(-)