| Message ID | f93f935ae0912c7f22ff7e65214c184d53f3e997.1347629357.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/14/2012 07:41 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 the active image) - see note below > speed: maximum speed, in bytes/sec > on_error: action to take on error (optional - default is report) Shouldn't this be on-error, with a dash? Also, this doesn't match the actual commit, since you pulled it while waiting to rebase on Paolo's patches. > > 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 the error > QERR_TOP_NOT_FOUND will be returned. I don't think we need a new error category for this particular failure (and in 4/8, you labeled the error as generic); so it is sufficient to state that 'an error will be returned'. > > The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will > be emitted. > > +++ b/blockdev.c > @@ -825,7 +825,6 @@ exit: > return; > } > > - > static void eject_device(BlockDriverState *bs, int force, Error **errp) Spurious whitespace change. > +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(); Is this technically necessary for now? Since you are forbidding actions on the active image for now, and changing the active image (via snapshot or pull) already drains all I/O, there should be nothing remaining to drain for any of the backing files. Obviously, it will be necessary in the future when you do add support for committing the active layer. > +++ b/qapi-schema.json > @@ -1404,6 +1404,41 @@ > 'returns': 'str' } > > ## > +# @block-commit > +# > +# Live commit of data from child image nodes into parent nodes - i.e., > +# writes data between 'top' and 'base' into 'base'. > +# > +# @device: the name of the device > +# > +# @base: #optional The file name of the parent image of the device to write > +# data into. If not specified, this is the original parent > +# image. I though we wanted to fix the terminology to avoid 'parent'; how about: 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 child image, above which data will > +# not be committed down. If not specified, this is one > +# layer below the active layer (i.e. active->backing_hd). Again, how about: The file name of the backing image within the chain which contains the data to be committed down. If not specified... > +# > +# 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, BaseNotFound > +# If @top does not exist, TopNotFound BaseNotFound is a generic error, and I'm arguing that TopNotFound should be likewise.
On 09/14/2012 09:05 PM, Eric Blake wrote: > On 09/14/2012 07:41 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 the active image) - see note below >> speed: maximum speed, in bytes/sec >> on_error: action to take on error (optional - default is report) > > Shouldn't this be on-error, with a dash? Also, this doesn't match the > actual commit, since you pulled it while waiting to rebase on Paolo's > patches. > Yes, thanks, I need to update the commit message. >> >> 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 the error >> QERR_TOP_NOT_FOUND will be returned. > > I don't think we need a new error category for this particular failure > (and in 4/8, you labeled the error as generic); so it is sufficient to > state that 'an error will be returned'. > Yes, QERR_TOP_NOT_FOUND will be a generic error, so I will just note that an error will be returned, as you suggest. >> >> The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will >> be emitted. >> > >> +++ b/blockdev.c >> @@ -825,7 +825,6 @@ exit: >> return; >> } >> >> - >> static void eject_device(BlockDriverState *bs, int force, Error **errp) > > Spurious whitespace change. > Thanks > >> +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(); > > Is this technically necessary for now? Since you are forbidding actions > on the active image for now, and changing the active image (via snapshot > or pull) already drains all I/O, there should be nothing remaining to > drain for any of the backing files. Obviously, it will be necessary in > the future when you do add support for committing the active layer. > Technically, it is not needed for the current iteration, but since this is the command handler that will be the same as when we add the active layer, and I know we will eventually support it, I went ahead and added it now. It shouldn't hurt anything. >> +++ b/qapi-schema.json >> @@ -1404,6 +1404,41 @@ >> 'returns': 'str' } >> >> ## >> +# @block-commit >> +# >> +# Live commit of data from child image nodes into parent nodes - i.e., >> +# writes data between 'top' and 'base' into 'base'. >> +# >> +# @device: the name of the device >> +# >> +# @base: #optional The file name of the parent image of the device to write >> +# data into. If not specified, this is the original parent >> +# image. > > I though we wanted to fix the terminology to avoid 'parent'; how about: > > The file name of the backing image to write data into. If not > specified, this is the deepest backing image. > OK, sounds good. >> +# >> +# @top: #optional The file name of the child image, above which data will >> +# not be committed down. If not specified, this is one >> +# layer below the active layer (i.e. active->backing_hd). > > Again, how about: > > The file name of the backing image within the chain which contains the > data to be committed down. If not specified... > Hmm, how about a slight tweak: The file name of the backing image within the image chain, which contains the topmost data to be committed down. If not specified... Since it doesn't necessarily have _the_ data to commit down, but indicates the upper bounds of the data to commit down. >> +# >> +# 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, BaseNotFound >> +# If @top does not exist, TopNotFound > > BaseNotFound is a generic error, and I'm arguing that TopNotFound should > be likewise. > Agree - both TopNotFound and BaseNotFound are of type ERROR_CLASS_GENERIC_ERROR
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..785e999 100644 --- a/blockdev.c +++ b/blockdev.c @@ -825,7 +825,6 @@ exit: return; } - static void eject_device(BlockDriverState *bs, int force, Error **errp) { if (bdrv_in_use(bs)) { @@ -1124,6 +1123,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_set(errp, ERROR_CLASS_GENERIC_ERROR, + "Invalid files for merge: top and base are the same"); + return; + } + } + if (top_bs == NULL) { + error_set(errp, QERR_TOP_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 a9f465a..c99ccfa 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1404,6 +1404,41 @@ 'returns': 'str' } ## +# @block-commit +# +# Live commit of data from child image nodes into parent nodes - i.e., +# writes data between 'top' and 'base' into 'base'. +# +# @device: the name of the device +# +# @base: #optional The file name of the parent image of the device to write +# data into. If not specified, this is the original parent +# image. +# +# @top: #optional The file name of the child image, above which data will +# not 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, BaseNotFound +# If @top does not exist, TopNotFound +# 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 the active image) - see note below speed: maximum speed, in bytes/sec on_error: action to take on error (optional - default is report) 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 the error QERR_TOP_NOT_FOUND 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 | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++- qapi-schema.json | 35 +++++++++++++++++++++++++++++ qmp-commands.hx | 6 +++++ 4 files changed, 109 insertions(+), 3 deletions(-)