Live block commit
v2 cover Live block commit
Live block commit.
I originally had intended for this RFC series to include the more complicated case of a live commit of the active layer, but removed it for this commit in the hopes of making it into the soft feature freeze for 1.2, so this series is the simpler case.
This series adds the basic case, of a live commit between two images below the active layer, e.g.:
[base] <--- [snp-1] <--- [snp-2] <--- [snp-3] <--- [active]
can be collapsed down via commit, into:
[base] <--- [active]
or,
[base] <--- [snp-1] <--- [active],
[base] <--- [snp-3] <--- [active],
etc..
TODO: * qemu-io tests (in progress)
* 'stage-2' of live commit functionality, to be able to push down the active layer. This structured something like mirroring, to allow for convergence.
Changes from the RFC v1 series:
* This patch series is not on top of Paolo's blk mirror series yet, to make it easier to apply independently if desired. This means some of what was in the previous RFC series is not in this one (BlockdevOnError, for instance), but that can be easily added in once Paolo's series are in.
* This patches series is dependent on the reopen() series with transactional reopen.
* The target release for this series is 1.3
* Found some mistakes in the reopen calls
* Dropped the BlockdevOnError argument (for now), will add in if rebasing on top of Paolo's series.
* Used the new qerror system
Jeff Cody (6):
1/6 block: add support functions for live commit, to find and delete
images.
2/6 block: add live block commit functionality 3/6 blockdev: rename block_stream_cb to a generic block_job_cb 4/6 qerror: new error for live block commit, QERR_TOP_NOT_FOUND 5/6 block: helper function, to find the base image of a chain 6/6 QAPI: add command for live block commit, 'block-commit'
block.c | 158 ++++++++++++++++++++++++++++++++++++++++ block.h | 6 +- block/Makefile.objs | 1 + block/commit.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block_int.h | 19 +++++ blockdev.c | 91 ++++++++++++++++++++++- qapi-schema.json | 30 ++++++++ qerror.h | 3 + qmp-commands.hx | 6 ++ trace-events | 4 +- 10 files changed, 515 insertions(+), 5 deletions(-) create mode 100644 block/commit.c
Signature · 1 line
1.7.11.2
RFC PATCH v2 1/6 block: add support functions for live commit, to find and delete images.
Add bdrv_find_child(), and bdrv_delete_intermediate().
bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain,
find the image that is the immediate top of 'bs'
bdrv_delete_intermediate():
Given 3 BDS (active, top, base), delete images above base up to and including top, and set base to be the parent of top's child node.
E.g., this converts:
bottom <- base <- intermediate <- top <- active
to
bottom <- base <- active
where top == active is permitted, although active will not be deleted.
--- block.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ block.h | 5 ++- 2 files changed, 146 insertions(+), 1 deletion(-)
block.c +142 -0
@@ -1752,6 +1752,148 @@ int bdrv_change_backing_file(BlockDriverState *bs,return ret; } +/* Finds the image layer immediately to the 'top' of bs. +* +* active is the current topmost image. +*/ +BlockDriverState *bdrv_find_child (BlockDriverState *active, +BlockDriverState *bs) +{ +BlockDriverState *child =NULL ; +BlockDriverState *intermediate; + +/* if the active bs layer is the same as the new top, then there +* is no image above the top, so it will be returned as the child +*/ +if (active == bs) { + child = active; + }else { + intermediate = active; +while (intermediate->backing_hd) { +if (intermediate->backing_hd == bs) { + child = intermediate; +break ; + } + intermediate = intermediate->backing_hd; + } + } + +return child; +} + +typedef struct BlkIntermediateStates { +BlockDriverState *bs; +QSIMPLEQ_ENTRY (BlkIntermediateStates) entry; +}BlkIntermediateStates ; + + +/* deletes images above 'base' up to and including 'top', and sets the image +* above 'top' to have base as its backing file. +* +* E.g., this will convert the following chain: +* bottom <- base <- intermediate <- top <- active +* +* to +* +* bottom <- base <- active +* +* It is allowed for bottom==base, in which case it converts: +* +* base <- intermediate <- top <- active +* +* to +* +* base <- active +* +* It is also allowed for top==active, except in that case active is not +* deleted: +* +* base <- intermediate <- top +* +* becomes +* +* base <- top +*/ +int bdrv_delete_intermediate (BlockDriverState *active,BlockDriverState *top, +BlockDriverState *base) +{ +BlockDriverState *intermediate; +BlockDriverState *base_bs =NULL ; +BlockDriverState *new_top_bs =NULL ; +BlkIntermediateStates *intermediate_state, *next; +int ret =-1 ; + + QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; +QSIMPLEQ_INIT (&states_to_delete); + +if (!top->drv || !base->drv) { + goto exit; + } + + new_top_bs =bdrv_find_child (active, top); + +/* special case of new_top_bs->backing_hd already pointing to base - nothing +* to do, no intermediate images +*/ +if (new_top_bs->backing_hd == base) { + ret =0 ; + goto exit; + } + +if (new_top_bs ==NULL ) { +/* we could not find the image above 'top', this is an error */ + goto exit; + } + +/* if the active and top image passed in are the same, then we +* can't delete the active, so we start one below +*/ + intermediate = (active == top) ? active->backing_hd : top; + +/* now we will go down through the list, and add each BDS we find +* into our deletion queue, until we hit the 'base' +*/ +while (intermediate) { + intermediate_state =g_malloc0 (sizeof (BlkIntermediateStates)); + intermediate_state->bs = intermediate; +QSIMPLEQ_INSERT_TAIL (&states_to_delete, intermediate_state, entry); + +if (intermediate->backing_hd == base) { + base_bs = intermediate->backing_hd; +break ; + } + intermediate = intermediate->backing_hd; + } +if (base_bs ==NULL ) { +/* something went wrong, we did not end at the base. safely +* unravel everything, and exit with error */ + goto exit; + } + +/* success - we can delete the intermediate states, and link top->base */ + ret =bdrv_change_backing_file (new_top_bs, base_bs->filename, + base_bs->drv ? base_bs->drv->format_name :"" ); +if (ret) { + goto exit; + } + new_top_bs->backing_hd = base_bs; + + + QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { +/* so that bdrv_close() does not recursively close the chain */ + intermediate_state->bs->backing_hd =NULL ; +bdrv_delete (intermediate_state->bs); + } + ret =0 ; + +exit: + QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { +g_free (intermediate_state); + } +return ret; +} + +static int bdrv_check_byte_request (BlockDriverState *bs,int64_t offset,size_t size) {
block.h +4 -1
@@ -201,7 +201,10 @@ int bdrv_commit_all(void);int bdrv_change_backing_file (BlockDriverState *bs,const char *backing_file,const char *backing_fmt);void bdrv_register (BlockDriver *bdrv); - +int bdrv_delete_intermediate (BlockDriverState *active,BlockDriverState *top, +BlockDriverState *base); +BlockDriverState *bdrv_find_child (BlockDriverState *active, +BlockDriverState *bs); typedef struct BdrvCheckResult { int corruptions;
Signature · 1 line
1.7.11.2
Re: [Qemu-devel] [RFC v2 PATCH 1/6] block: add support functions for live commit, to find and delete images.
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 14 lines
Add
bdrv_find_child(), andbdrv_delete_intermediate().
bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain,
find the image that is the immediate top of 'bs'
bdrv_delete_intermediate():
Given 3 BDS (active, top, base), delete images above base up to and including top, and set base to be the parent of top's child node.E.g., this converts:
bottom <- base <- intermediate <- top <- active
Quoted 8 lines
to
bottom <- base <- active
where top == active is permitted, although active will not be deleted.
Signed-off-by Jeff Cody <[email protected]>
At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.
Quoted 10 lines
--- block.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ block.h | 5 ++- 2 files changed, 146 insertions(+), 1 deletion(-)block.c +0
-0
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1758
Quoted 7 lines
block.c +4
-0@@ -1752,6 +1752,148 @@ int bdrv_change_backing_file(BlockDriverState *bs,return ret; } +/* Finds the image layer immediately to the 'top' of bs. +* +* active is the current topmost image. +*/
Most other function header comments in block.c have the /* on its own line, so it would be nice to stay consistent.
Without looking at the code yet, I'm not quite sure if I understand this correctly. Could use an example:
base <- a <- b <- c <- d
bdrv_find_child(d, a) would return b? Why is it called bdrv_find_child then, b is neither a (direct) child of d nor a.
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1767
Quoted 9 lines
block.c +9
-0+BlockDriverState *bdrv_find_child (BlockDriverState *active, +BlockDriverState *bs) +{ +BlockDriverState *child =NULL ; +BlockDriverState *intermediate; + +/* if the active bs layer is the same as the new top, then there +* is no image above the top, so it will be returned as the child +*/
"new top"?
And should this be mentioned in the header comment? Not sure how generally useful this behaviour is. If it's only the right thing for the only caller, maybe returning NULL and handling the case in the caller would be better.
Quoted 5 lines
block.c +5
-0+if (active == bs) { + child = active; + }else { + intermediate = active; +while (intermediate->backing_hd) {
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1782
Quoted 10 lines
block.c +10
-0+if (intermediate->backing_hd == bs) { + child = intermediate; +break ; + } + intermediate = intermediate->backing_hd; + } + } + +return child; +}
So it returns NULL when bs isn't in the backing file chain of active. Probably worth mentioning in the comment as well.
Quoted 17 lines
block.c +17
-0+ +typedef struct BlkIntermediateStates { +BlockDriverState *bs; +QSIMPLEQ_ENTRY (BlkIntermediateStates) entry; +}BlkIntermediateStates ; + + +/* deletes images above 'base' up to and including 'top', and sets the image +* above 'top' to have base as its backing file. +* +* E.g., this will convert the following chain: +* bottom <- base <- intermediate <- top <- active +* +* to +* +* bottom <- base <- active +*
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1809
Quoted 10 lines
block.c +10
-0+* It is allowed for bottom==base, in which case it converts: +* +* base <- intermediate <- top <- active +* +* to +* +* base <- active +* +* It is also allowed for top==active, except in that case active is not +* deleted:
Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing?
Quoted 14 lines
block.c +14
-0+* +* base <- intermediate <- top +* +* becomes +* +* base <- top +*/ +int bdrv_delete_intermediate (BlockDriverState *active,BlockDriverState *top, +BlockDriverState *base) +{ +BlockDriverState *intermediate; +BlockDriverState *base_bs =NULL ; +BlockDriverState *new_top_bs =NULL ; +BlkIntermediateStates *intermediate_state, *next;
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1833
Quoted 10 lines
block.c +10
-0+int ret =-1 ; + + QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; +QSIMPLEQ_INIT (&states_to_delete); + +if (!top->drv || !base->drv) { + goto exit; + } + + new_top_bs =bdrv_find_child (active, top);
new_top_bs is the first BDS that should not be removed from the chain.
Why do we pass top and then search for new_top instead of passing new_top directly?
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1843
Quoted 10 lines
block.c +10
-0+ +/* special case of new_top_bs->backing_hd already pointing to base - nothing +* to do, no intermediate images +*/ +if (new_top_bs->backing_hd == base) { + ret =0 ; + goto exit; + } + +if (new_top_bs ==NULL ) {
Never reached, we segfault before. (How about unit tests for this function, in the style of tests/check-q*.c?)
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1851
Quoted 8 lines
block.c +8
-0+/* we could not find the image above 'top', this is an error */ + goto exit; + } + +/* if the active and top image passed in are the same, then we +* can't delete the active, so we start one below +*/ + intermediate = (active == top) ? active->backing_hd : top;
Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted.
This is equivalent to an unconditional new_top_bs->backing_hd.
Quoted 14 lines
block.c +14
-0+ +/* now we will go down through the list, and add each BDS we find +* into our deletion queue, until we hit the 'base' +*/ +while (intermediate) { + intermediate_state =g_malloc0 (sizeof (BlkIntermediateStates)); + intermediate_state->bs = intermediate; +QSIMPLEQ_INSERT_TAIL (&states_to_delete, intermediate_state, entry); + +if (intermediate->backing_hd == base) { + base_bs = intermediate->backing_hd; +break ; + } + intermediate = intermediate->backing_hd;
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1875
Quoted 10 lines
block.c +10
-0+ } +if (base_bs ==NULL ) { +/* something went wrong, we did not end at the base. safely +* unravel everything, and exit with error */ + goto exit; + } + +/* success - we can delete the intermediate states, and link top->base */ + ret =bdrv_change_backing_file (new_top_bs, base_bs->filename, + base_bs->drv ? base_bs->drv->format_name :"" );
Requires that new_top_bs is opened r/w. Definitely worth documenting.
Kevin
Re: [Qemu-devel] [RFC v2 PATCH 1/6] block: add support functions for live commit, to find and delete images.
Quoted 22 lines · Kevin Wolf
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 21 lines
Add
bdrv_find_child(), andbdrv_delete_intermediate().
bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain,
find the image that is the immediate top of 'bs'
bdrv_delete_intermediate():
Given 3 BDS (active, top, base), delete images above base up to and including top, and set base to be the parent of top's child node.E.g., this converts:
bottom <- base <- intermediate <- top <- active
to
bottom <- base <- active
where top == active is permitted, although active will not be deleted.
Quoted 7 lines
Quoted 1 line
Signed-off-by Jeff Cody <[email protected]>At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.
Hmm, the naming seems consistent with bdrv_delete(), which does not actually delete the image files either (and, that is essentially what this does... calls bdrv_delete(), on the intermediate images).
However, here are some other name proposals:
* bdrv_disconnect_intermediate() * bdrv_drop_intermediate() * bdrv_shorten_chain()
Quoted 22 lines
Quoted 11 lines
--- block.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ block.h | 5 ++- 2 files changed, 146 insertions(+), 1 deletion(-)block.c +0
-0block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1758
Quoted 7 lines
block.c +4
-0@@ -1752,6 +1752,148 @@ int bdrv_change_backing_file(BlockDriverState *bs,return ret; } +/* Finds the image layer immediately to the 'top' of bs. +* +* active is the current topmost image. +*/ Most other function header comments in
block.chave the /* on its own line, so it would be nice to stay consistent.
Quoted 8 lines
Without looking at the code yet, I'm not quite sure if I understand this correctly. Could use an example:
base <- a <- b <- c <- d
bdrv_find_child(d, a)would return b? Why is it calledbdrv_find_childthen, b is neither a (direct) child of d nor a.
For completeness: as we discussed on IRC, this will be renamed to bdrv_find_overlay(), avoiding the parent/child terminology, which can be a bit ambiguous.
Quoted 12 lines
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1767
Quoted 9 lines
block.c +9
-0+BlockDriverState *bdrv_find_child (BlockDriverState *active, +BlockDriverState *bs) +{ +BlockDriverState *child =NULL ; +BlockDriverState *intermediate; + +/* if the active bs layer is the same as the new top, then there +* is no image above the top, so it will be returned as the child +*/ "new top"?
Quoted 5 lines
And should this be mentioned in the header comment? Not sure how generally useful this behaviour is. If it's only the right thing for the only caller, maybe returning NULL and handling the case in the caller would be better.
Yes, it should definitely be mentioned in the comment header. I initially was going to say that I don't think NULL is appropriate to return here, but on more reflection I think you correct. We shouldn't return non-NULL if it is not actually the overlay to bs. The caller can check on a NULL return, and determine what to do at that point.
Quoted 10 lines
Quoted 10 lines
block.c +10
-0+if (active == bs) { + child = active; + }else { + intermediate = active; +while (intermediate->backing_hd) { +if (intermediate->backing_hd == bs) { + child = intermediate; +break ; + } + intermediate = intermediate->backing_hd;
Quoted 8 lines
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1782
Quoted 5 lines
block.c +5
-0+ } + } + +return child; +}So it returns NULL when bs isn't in the backing file chain of active. Probably worth mentioning in the comment as well.
Agreed.
Quoted 23 lines
Quoted 23 lines
block.c +22
-0+ +typedef struct BlkIntermediateStates { +BlockDriverState *bs; +QSIMPLEQ_ENTRY (BlkIntermediateStates) entry; +}BlkIntermediateStates ; + + +/* deletes images above 'base' up to and including 'top', and sets the image +* above 'top' to have base as its backing file. +* +* E.g., this will convert the following chain: +* bottom <- base <- intermediate <- top <- active +* +* to +* +* bottom <- base <- active +* +* It is allowed for bottom==base, in which case it converts: +* +* base <- intermediate <- top <- active +* +* to
Quoted 9 lines
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1809
Quoted 5 lines
block.c +5
-0+* +* base <- active +* +* It is also allowed for top==active, except in that case active is not +* deleted: Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing?
To remain consistent, maybe we should define it as an error if top==active, and return error in that case? The caller can be responsible for checking for that - if the caller wants to merge down the active layer, there are additional steps to be taken anyway.
Quoted 27 lines
Quoted 14 lines
block.c +14
-0+* +* base <- intermediate <- top +* +* becomes +* +* base <- top +*/ +int bdrv_delete_intermediate (BlockDriverState *active,BlockDriverState *top, +BlockDriverState *base) +{ +BlockDriverState *intermediate; +BlockDriverState *base_bs =NULL ; +BlockDriverState *new_top_bs =NULL ; +BlkIntermediateStates *intermediate_state, *next;block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1833
Quoted 10 lines
block.c +10
-0+int ret =-1 ; + + QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; +QSIMPLEQ_INIT (&states_to_delete); + +if (!top->drv || !base->drv) { + goto exit; + } + + new_top_bs =bdrv_find_child (active, top);
new_top_bsis the first BDS that should not be removed from the chain.
Quoted 3 lines
Why do we pass top and then search for
new_topinstead of passingnew_topdirectly?
(Even though we talked about this on IRC, including it here for completeness):
Because the block commit API specifies the image to merge down, so we need to find the image overlay on top of that, to adjust the backing file correctly. And if we change the semantics of the block commit API to mean the image above the image to merge down, then that makes it inconsistent when specifying merging down the active layer later on.
Quoted 5 lines
Quoted 5 lines
block.c +5
-0+ +/* special case of new_top_bs->backing_hd already pointing to base - nothing +* to do, no intermediate images +*/ +if (new_top_bs->backing_hd == base) {
Quoted 9 lines
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1843
Quoted 5 lines
block.c +5
-0+ ret =0 ; + goto exit; + } + +if (new_top_bs ==NULL ) {Never reached, we segfault before. (How about unit tests for this function, in the style of
tests/check-q*.c?)
Ugh, thanks - obviously this should be above the previous check. And, I agree on the unit test - I will add that in tests/check-*.
Quoted 32 lines
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1851
Quoted 8 lines
block.c +8
-0+/* we could not find the image above 'top', this is an error */ + goto exit; + } + +/* if the active and top image passed in are the same, then we +* can't delete the active, so we start one below +*/ + intermediate = (active == top) ? active->backing_hd : top;Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted.
This is equivalent to an unconditional
new_top_bs->backing_hd.Quoted 18 lines
block.c +18
-0+ +/* now we will go down through the list, and add each BDS we find +* into our deletion queue, until we hit the 'base' +*/ +while (intermediate) { + intermediate_state =g_malloc0 (sizeof (BlkIntermediateStates)); + intermediate_state->bs = intermediate; +QSIMPLEQ_INSERT_TAIL (&states_to_delete, intermediate_state, entry); + +if (intermediate->backing_hd == base) { + base_bs = intermediate->backing_hd; +break ; + } + intermediate = intermediate->backing_hd; + } +if (base_bs ==NULL ) { +/* something went wrong, we did not end at the base. safely +* unravel everything, and exit with error */
Quoted 9 lines
block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1875
Quoted 6 lines
block.c +6
-0+ goto exit; + } + +/* success - we can delete the intermediate states, and link top->base */ + ret =bdrv_change_backing_file (new_top_bs, base_bs->filename, + base_bs->drv ? base_bs->drv->format_name :"" );Requires that
new_top_bsis opened r/w. Definitely worth documenting.
Agreed.
Re: [Qemu-devel] [RFC v2 PATCH 1/6] block: add support functions for live commit, to find and delete images.
Am 06.09.2012 16:59, schrieb Jeff Cody:
Quoted 35 lines · Kevin Wolf
Quoted 22 lines
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 21 lines
Add
bdrv_find_child(), andbdrv_delete_intermediate().
bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain,
find the image that is the immediate top of 'bs'
bdrv_delete_intermediate():
Given 3 BDS (active, top, base), delete images above base up to and including top, and set base to be the parent of top's child node.E.g., this converts:
bottom <- base <- intermediate <- top <- active
to
bottom <- base <- active
where top == active is permitted, although active will not be deleted.
Quoted 7 lines
Quoted 1 line
Signed-off-by Jeff Cody <[email protected]>At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.
Hmm, the naming seems consistent with
bdrv_delete(), which does not actually delete the image files either (and, that is essentially what this does... callsbdrv_delete(), on the intermediate images).
Quoted 5 lines
However, here are some other name proposals:
* bdrv_disconnect_intermediate() * bdrv_drop_intermediate() * bdrv_shorten_chain()
bdrv_drop_intermediate() sounds good to me.
Quoted 33 lines
Quoted 23 lines
Quoted 23 lines
block.c +22
-0+ +typedef struct BlkIntermediateStates { +BlockDriverState *bs; +QSIMPLEQ_ENTRY (BlkIntermediateStates) entry; +}BlkIntermediateStates ; + + +/* deletes images above 'base' up to and including 'top', and sets the image +* above 'top' to have base as its backing file. +* +* E.g., this will convert the following chain: +* bottom <- base <- intermediate <- top <- active +* +* to +* +* bottom <- base <- active +* +* It is allowed for bottom==base, in which case it converts: +* +* base <- intermediate <- top <- active +* +* to Quoted 9 lines
Quoted 5 lines
+ *
+ * base <- active
+ *
+ * It is also allowed for top==active, except in that case active is not
+ * deleted:Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing?
Quoted 4 lines
To remain consistent, maybe we should define it as an error if top==active, and return error in that case? The caller can be responsible for checking for that - if the caller wants to merge down the active layer, there are additional steps to be taken anyway.
Yes, why not.
And we can always revisit when implementing the additional functionality.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
block.c +8
-0+/* we could not find the image above 'top', this is an error */ + goto exit; + } + +/* if the active and top image passed in are the same, then we +* can't delete the active, so we start one below +*/ + intermediate = (active == top) ? active->backing_hd : top;
Quoted 4 lines
Quoted 4 lines
Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted.
This is equivalent to an unconditional
new_top_bs->backing_hd.
How about changing this to use the simpler unconditional version?
Kevin
Re: [Qemu-devel] [RFC v2 PATCH 1/6] block: add support functions for live commit, to find and delete images.
Quoted 95 lines · Kevin Wolf
Am 06.09.2012 16:59, schrieb Jeff Cody:
Quoted 35 lines · Kevin Wolf
Quoted 22 lines
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 21 lines
Add
bdrv_find_child(), andbdrv_delete_intermediate().
bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain,
find the image that is the immediate top of 'bs'
bdrv_delete_intermediate():
Given 3 BDS (active, top, base), delete images above base up to and including top, and set base to be the parent of top's child node.E.g., this converts:
bottom <- base <- intermediate <- top <- active
to
bottom <- base <- active
where top == active is permitted, although active will not be deleted.
Quoted 7 lines
Quoted 1 line
Signed-off-by Jeff Cody <[email protected]>At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.
Hmm, the naming seems consistent with
bdrv_delete(), which does not actually delete the image files either (and, that is essentially what this does... callsbdrv_delete(), on the intermediate images).Quoted 5 lines
However, here are some other name proposals:
* bdrv_disconnect_intermediate() * bdrv_drop_intermediate() * bdrv_shorten_chain()
bdrv_drop_intermediate()sounds good to me.Quoted 33 lines
Quoted 23 lines
Quoted 23 lines
block.c +22
-0+ +typedef struct BlkIntermediateStates { +BlockDriverState *bs; +QSIMPLEQ_ENTRY (BlkIntermediateStates) entry; +}BlkIntermediateStates ; + + +/* deletes images above 'base' up to and including 'top', and sets the image +* above 'top' to have base as its backing file. +* +* E.g., this will convert the following chain: +* bottom <- base <- intermediate <- top <- active +* +* to +* +* bottom <- base <- active +* +* It is allowed for bottom==base, in which case it converts: +* +* base <- intermediate <- top <- active +* +* to Quoted 9 lines
Quoted 5 lines
+ *
+ * base <- active
+ *
+ * It is also allowed for top==active, except in that case active is not
+ * deleted:Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing?
Quoted 4 lines
To remain consistent, maybe we should define it as an error if top==active, and return error in that case? The caller can be responsible for checking for that - if the caller wants to merge down the active layer, there are additional steps to be taken anyway.
Yes, why not.
And we can always revisit when implementing the additional functionality.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
block.c +8
-0+/* we could not find the image above 'top', this is an error */ + goto exit; + } + +/* if the active and top image passed in are the same, then we +* can't delete the active, so we start one below +*/ + intermediate = (active == top) ? active->backing_hd : top;
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted.
This is equivalent to an unconditional
new_top_bs->backing_hd.How about changing this to use the simpler unconditional version?
Sure - since active == top is now an error, there is no reason for the more complicated logic. And at this point, the statement (new_top_bs->backing_hd == top) should always be true.
Quoted 3 lines
Kevin
Re: [Qemu-devel] [RFC v2 PATCH 1/6] block: add support functions for live commit, to find and delete images.
In reply to Kevin WolfQuoted 6 lines · Kevin Wolf
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 5 lines
Add
bdrv_find_child(), andbdrv_delete_intermediate().
bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain,
find the image that is the immediate top of 'bs'
Quoted 7 lines
Quoted 1 line
bdrv_delete_intermediate():At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.
Maybe bdrv_unchain_intermediate()?
Signature · 2 lines
Eric Blake [email protected] +1-919-301-3266 Libvirt virtualization library http://libvirt.org
RFC PATCH v2 2/6 block: add live block commit functionality
In reply to Jeff CodyThis adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.
The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.
--- block/Makefile.objs | 1 + block/commit.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block_int.h | 19 +++++ trace-events | 2 + 4 files changed, 224 insertions(+) create mode 100644 block/commit.c
block/Makefile.objs +1 -0
@@ -4,6 +4,7 @@ block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o block-obj-y += qed-check.o block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o block-obj-y += stream.o +block-obj-y += commit.o block-obj-$(CONFIG_WIN32) += raw-win32.o block-obj-$(CONFIG_POSIX) += raw-posix.o block-obj-$(CONFIG_LIBISCSI) += iscsi.o
block/commit.c new file +202 -0
202 lines not on this page · raw message
block_int.h +19 -0
@@ -469,4 +469,23 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, BlockDriverCompletionFunc *cb, void *opaque, Error **errp); +/** +* commit_start: +* @bs: Top Block device +* @base: Block device that will be written into, and become the new top +* @speed: The maximum speed, in bytes per second, or 0 for unlimited. +* @on_error: The action to take upon error. +* @cb: Completion function for the job. +* @opaque: Opaque pointer value passed to @cb. +* @orig_base_flags: The original open flags for the base image +* @orig_top_flags: The original open flags for the top image +* @errp: Error object. +* +*/ +void commit_start (BlockDriverState *bs,BlockDriverState *base, +BlockDriverState *top,int64_t speed, +BlockErrorAction on_error,BlockDriverCompletionFunc *cb, +void *opaque,int orig_base_flags,int orig_top_flags, +Error **errp); +#endif /* BLOCK_INT_H */
trace-events +2 -0
@@ -74,6 +74,8 @@ bdrv_co_do_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t c # block/stream.c stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d" stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p" +commit_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d" +commit_start(void *bs, void *base, void *top, void *s, void *co, void *opaque, int base_flags, int top_flags) "bs %p base %p top %p s %p co %p opaque %p base_flags %d top_flags %d" # blockdev.c qmp_block_job_cancel(void *job) "job %p"
Signature · 1 line
1.7.11.2
Re: [Qemu-devel] [RFC v2 PATCH 2/6] block: add live block commit functionality
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 77 lines
This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.
The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.
Signed-off-by Jeff Cody <[email protected]>--- block/Makefile.objs | 1 + block/commit.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block_int.h | 19 +++++ trace-events | 2 + 4 files changed, 224 insertions(+) create mode 100644 block/commit.cblock/Makefile.objs +1
-0@@ -4,6 +4,7 @@ block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o block-obj-y += qed-check.o block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o block-obj-y += stream.o +block-obj-y += commit.o block-obj-$(CONFIG_WIN32) += raw-win32.o block-obj-$(CONFIG_POSIX) += raw-posix.o block-obj-$(CONFIG_LIBISCSI) += iscsi.oblock/commit.c new file +41
-0@@ -0,0 +1,202 @@ +/* +* Live block commit +* +* Copyright Red Hat, Inc. 2012 +* +* Authors: +* Jeff Cody <[email protected]> +* Based on stream.c by Stefan Hajnoczi +* +* This work is licensed under the terms of the GNU LGPL, version 2 or later. +* See the COPYING.LIB file in the top-level directory. +* +*/ + +#include "trace.h" +#include "block_int.h" +#include "qemu/ratelimit.h" + +enum { +/* +* Size of data buffer for populating the image file. This should be large +* enough to process multiple clusters in a single call, so that populating +* contiguous regions of the image is efficient. +*/ +COMMIT_BUFFER_SIZE =512 *1024 ,/* in bytes */ +}; + +#define SLICE_TIME 100000000ULL/* ns */ + +typedef struct CommitBlockJob { +BlockJob common; +RateLimit limit; +BlockDriverState *active; +BlockDriverState *top; +BlockDriverState *base; +BlockErrorAction on_error; +int base_flags; +int top_flags; +}CommitBlockJob ; + +static int coroutine_fn commit_populate(BlockDriverState *bs,
block/commit.c line 51
Quoted 10 lines
block/commit.c new file +10
-0+BlockDriverState *base, + int64_t sector_num, int nb_sectors, + void *buf) +{ + if (bdrv_read(bs, sector_num, buf, nb_sectors)) { + return -EIO; + } +if (bdrv_write (base, sector_num, buf, nb_sectors)) { +return -EIO ; + }
Don't throw error codes away.
What should we do with backing files that are smaller than the image to commit? In this version, data is copied up to the size of the backing file, and then we get -EIO from bdrv_co_do_writev().
Quoted 37 lines
block/commit.c new file +37
-0+return 0 ; +} + +static void coroutine_fn commit_run(void *opaque) +{ +CommitBlockJob *s = opaque; +BlockDriverState *active = s->active; +BlockDriverState *top = s->top; +BlockDriverState *base = s->base; +BlockDriverState *top_child =NULL ; +int64_t sector_num, end; +int error =0 ; +int ret =0 ; +int n =0 ; +void *buf; +int bytes_written =0 ; + + s->common.len =bdrv_getlength (top); +if (s->common.len <0 ) { +block_job_complete (&s->common, s->common.len); +return ; + } + + top_child =bdrv_find_child (active, top); + + end = s->common.len >>BDRV_SECTOR_BITS ; + buf =qemu_blockalign (top,COMMIT_BUFFER_SIZE ); + +for (sector_num =0 ; sector_num < end; sector_num += n) { +uint64_t delay_ns =0 ; +bool copy; + +wait: +/* Note that even when no rate limit is applied we need to yield +* with no pending I/O here so that qemu_aio_flush() returns. +*/ +block_job_sleep_ns (&s->common, rt_clock, delay_ns);
block/commit.c line 98
Quoted 10 lines
block/commit.c new file +10
-0+if (block_job_is_cancelled (&s->common)) { +break ; + } +/* Copy if allocated above the base */ + ret =bdrv_co_is_allocated_above (top, base, sector_num, +COMMIT_BUFFER_SIZE /BDRV_SECTOR_SIZE , + &n); + copy = (ret ==1 ); +trace_commit_one_iteration (s, sector_num, n, ret); +if (ret >=0 && copy) {
If ret == 1, it's automatically >= 0...
By the way, I'm not sure if the interface is 0/1/-errno or 0/positive/-errno.
block/commit.c line 110
Quoted 12 lines
block/commit.c new file +12
-0+if (s->common.speed) { + delay_ns =ratelimit_calculate_delay (&s->limit, n); +if (delay_ns >0 ) { + goto wait; + } + } + ret =commit_populate (top, base, sector_num, n, buf); + bytes_written += n *BDRV_SECTOR_SIZE ; + } +if (ret <0 ) { +if (s->on_error ==BLOCK_ERR_STOP_ANY || + s->on_error ==BLOCK_ERR_STOP_ENOSPC ) {
Shouldn't there be a check for ret == -ENOSPC then...? And does this error handling do anything useful if you can't pause the job? Wouldn't it retry all the time?
Quoted 11 lines
block/commit.c new file +11
-0+ n =0 ; +continue ; + } +if (error ==0 ) { + error = ret; + } +if (s->on_error ==BLOCK_ERR_REPORT ) { +break ; + } + } + ret =0 ;
block/commit.c line 131
Quoted 10 lines
block/commit.c new file +10
-0+ +/* Publish progress */ + s->common.offset += n *BDRV_SECTOR_SIZE ; + } + +if (!block_job_is_cancelled (&s->common) && sector_num == end && ret ==0 ) { +/* success */ +if (bdrv_delete_intermediate (active, top, base)) { +/* something went wrong! */ +/* TODO:add error reporting here */
Indeed :-)
Quoted 51 lines
block/commit.c new file +51
-0+ } + } + +/* restore base open flags here if appropriate (e.g., change the base back +* to r/o). These reopens do not need to be atomic, since we won't abort +* even on failure here */ + +if (s->base_flags !=bdrv_get_flags (base)) { +bdrv_reopen (base, s->base_flags,NULL ); + } +if (s->top_flags !=bdrv_get_flags (top_child)) { +bdrv_reopen (top_child, s->top_flags,NULL ); + } + +qemu_vfree (buf); +block_job_complete (&s->common, ret); +} + +static void commit_set_speed (BlockJob *job,int64_t speed,Error **errp) +{ +CommitBlockJob *s =container_of (job, CommitBlockJob, common); + +if (speed <0 ) { +error_set (errp,QERR_INVALID_PARAMETER ,"speed" ); +return ; + } +ratelimit_set_speed (&s->limit, speed /BDRV_SECTOR_SIZE ,SLICE_TIME ); +} + +static BlockJobType commit_job_type = { + .instance_size =sizeof (CommitBlockJob), + .job_type ="commit" , + .set_speed = commit_set_speed, +}; + +void commit_start (BlockDriverState *bs,BlockDriverState *base, +BlockDriverState *top,int64_t speed, +BlockErrorAction on_error,BlockDriverCompletionFunc *cb, +void *opaque,int orig_base_flags,int orig_top_flags, +Error **errp) +{ +CommitBlockJob *s; + +if ((on_error ==BLOCK_ERR_STOP_ANY || + on_error ==BLOCK_ERR_STOP_ENOSPC ) && + !bdrv_iostatus_is_enabled (bs)) { +error_set (errp,QERR_INVALID_PARAMETER_COMBINATION ); +return ; + } + + s =block_job_create (&commit_job_type, bs, speed, cb, opaque, errp);
block/commit.c line 192
Quoted 10 lines
block/commit.c new file +10
-0+if (!s) { +return ; + } + + s->base = base; + s->top = top; + s->active = bs; + + s->base_flags = orig_base_flags; + s->top_flags = orig_top_flags;
So it's the caller who is expected to reopen r/w and then pass the original flags in? Can't we do both of it here?
block/commit.c line 201
Quoted 9 lines
block/commit.c new file +9
-0+ + s->on_error = on_error; + s->common.co =qemu_coroutine_create (commit_run); + +trace_commit_start (bs, base, top, s, s->common.co, opaque, + orig_base_flags, orig_top_flags); +qemu_coroutine_enter (s->common.co, s); + +return ;
Unnecessary return.
Kevin
Re: [Qemu-devel] [RFC v2 PATCH 2/6] block: add live block commit functionality
Quoted 82 lines · Kevin Wolf
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 81 lines
This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.
The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.
Signed-off-by Jeff Cody <[email protected]>--- block/Makefile.objs | 1 + block/commit.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block_int.h | 19 +++++ trace-events | 2 + 4 files changed, 224 insertions(+) create mode 100644 block/commit.cblock/Makefile.objs +1
-0@@ -4,6 +4,7 @@ block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o block-obj-y += qed-check.o block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o block-obj-y += stream.o +block-obj-y += commit.o block-obj-$(CONFIG_WIN32) += raw-win32.o block-obj-$(CONFIG_POSIX) += raw-posix.o block-obj-$(CONFIG_LIBISCSI) += iscsi.oblock/commit.c new file +45
-0@@ -0,0 +1,202 @@ +/* +* Live block commit +* +* Copyright Red Hat, Inc. 2012 +* +* Authors: +* Jeff Cody <[email protected]> +* Based on stream.c by Stefan Hajnoczi +* +* This work is licensed under the terms of the GNU LGPL, version 2 or later. +* See the COPYING.LIB file in the top-level directory. +* +*/ + +#include "trace.h" +#include "block_int.h" +#include "qemu/ratelimit.h" + +enum { +/* +* Size of data buffer for populating the image file. This should be large +* enough to process multiple clusters in a single call, so that populating +* contiguous regions of the image is efficient. +*/ +COMMIT_BUFFER_SIZE =512 *1024 ,/* in bytes */ +}; + +#define SLICE_TIME 100000000ULL/* ns */ + +typedef struct CommitBlockJob { +BlockJob common; +RateLimit limit; +BlockDriverState *active; +BlockDriverState *top; +BlockDriverState *base; +BlockErrorAction on_error; +int base_flags; +int top_flags; +}CommitBlockJob ; + +static int coroutine_fn commit_populate(BlockDriverState *bs, +BlockDriverState *base, +int64_t sector_num,int nb_sectors, +void *buf) +{
Quoted 9 lines
block/commit.c line 51
Quoted 6 lines
block/commit.c new file +6
-0+if (bdrv_read (bs, sector_num, buf, nb_sectors)) { +return -EIO ; + } +if (bdrv_write (base, sector_num, buf, nb_sectors)) { +return -EIO ; + }Don't throw error codes away.
OK, I'll pass the return from bdrv_read/write to the caller.
Quoted 4 lines
What should we do with backing files that are smaller than the image to commit? In this version, data is copied up to the size of the backing file, and then we get
-EIOfrombdrv_co_do_writev().
We could leave it like that, and let it receive -EIO in that case. Alternatively, we could try and determine before the commit if the data will fit in the base, and return -ENOSPC if not.
Quoted 50 lines
Quoted 37 lines
block/commit.c new file +37
-0+return 0 ; +} + +static void coroutine_fn commit_run(void *opaque) +{ +CommitBlockJob *s = opaque; +BlockDriverState *active = s->active; +BlockDriverState *top = s->top; +BlockDriverState *base = s->base; +BlockDriverState *top_child =NULL ; +int64_t sector_num, end; +int error =0 ; +int ret =0 ; +int n =0 ; +void *buf; +int bytes_written =0 ; + + s->common.len =bdrv_getlength (top); +if (s->common.len <0 ) { +block_job_complete (&s->common, s->common.len); +return ; + } + + top_child =bdrv_find_child (active, top); + + end = s->common.len >>BDRV_SECTOR_BITS ; + buf =qemu_blockalign (top,COMMIT_BUFFER_SIZE ); + +for (sector_num =0 ; sector_num < end; sector_num += n) { +uint64_t delay_ns =0 ; +bool copy; + +wait: +/* Note that even when no rate limit is applied we need to yield +* with no pending I/O here so that qemu_aio_flush() returns. +*/ +block_job_sleep_ns (&s->common, rt_clock, delay_ns);block/commit.c line 98
Quoted 10 lines
block/commit.c new file +10
-0+if (block_job_is_cancelled (&s->common)) { +break ; + } +/* Copy if allocated above the base */ + ret =bdrv_co_is_allocated_above (top, base, sector_num, +COMMIT_BUFFER_SIZE /BDRV_SECTOR_SIZE , + &n); + copy = (ret ==1 ); +trace_commit_one_iteration (s, sector_num, n, ret); +if (ret >=0 && copy) {If ret == 1, it's automatically >= 0...
Quoted 3 lines
By the way, I'm not sure if the interface is 0/1/-errno or 0/positive/-errno.
The header for bdrv_co_is_allocated() states that it returns true if it is allocated, and false otherwise. However, the function actually returns true if allocated, false if not, and something < 0 on failure.
But either way, you are right, I can just use 'copy' here.
Quoted 13 lines
block/commit.c line 110
Quoted 12 lines
block/commit.c new file +12
-0+if (s->common.speed) { + delay_ns =ratelimit_calculate_delay (&s->limit, n); +if (delay_ns >0 ) { + goto wait; + } + } + ret =commit_populate (top, base, sector_num, n, buf); + bytes_written += n *BDRV_SECTOR_SIZE ; + } +if (ret <0 ) { +if (s->on_error ==BLOCK_ERR_STOP_ANY || + s->on_error ==BLOCK_ERR_STOP_ENOSPC ) {
Quoted 4 lines
Shouldn't there be a check for ret ==
-ENOSPCthen...? And does this error handling do anything useful if you can't pause the job? Wouldn't it retry all the time?
We should check for ret == -ENOSPC, yes. On the error handling, we should probably just stop completely here, and return error.
Quoted 80 lines
Quoted 11 lines
block/commit.c new file +11
-0+ n =0 ; +continue ; + } +if (error ==0 ) { + error = ret; + } +if (s->on_error ==BLOCK_ERR_REPORT ) { +break ; + } + } + ret =0 ;block/commit.c line 131
Quoted 10 lines
block/commit.c new file +10
-0+ +/* Publish progress */ + s->common.offset += n *BDRV_SECTOR_SIZE ; + } + +if (!block_job_is_cancelled (&s->common) && sector_num == end && ret ==0 ) { +/* success */ +if (bdrv_delete_intermediate (active, top, base)) { +/* something went wrong! */ +/* TODO:add error reporting here */ Indeed :-)
Quoted 56 lines
block/commit.c new file +56
-0+ } + } + +/* restore base open flags here if appropriate (e.g., change the base back +* to r/o). These reopens do not need to be atomic, since we won't abort +* even on failure here */ + +if (s->base_flags !=bdrv_get_flags (base)) { +bdrv_reopen (base, s->base_flags,NULL ); + } +if (s->top_flags !=bdrv_get_flags (top_child)) { +bdrv_reopen (top_child, s->top_flags,NULL ); + } + +qemu_vfree (buf); +block_job_complete (&s->common, ret); +} + +static void commit_set_speed (BlockJob *job,int64_t speed,Error **errp) +{ +CommitBlockJob *s =container_of (job, CommitBlockJob, common); + +if (speed <0 ) { +error_set (errp,QERR_INVALID_PARAMETER ,"speed" ); +return ; + } +ratelimit_set_speed (&s->limit, speed /BDRV_SECTOR_SIZE ,SLICE_TIME ); +} + +static BlockJobType commit_job_type = { + .instance_size =sizeof (CommitBlockJob), + .job_type ="commit" , + .set_speed = commit_set_speed, +}; + +void commit_start (BlockDriverState *bs,BlockDriverState *base, +BlockDriverState *top,int64_t speed, +BlockErrorAction on_error,BlockDriverCompletionFunc *cb, +void *opaque,int orig_base_flags,int orig_top_flags, +Error **errp) +{ +CommitBlockJob *s; + +if ((on_error ==BLOCK_ERR_STOP_ANY || + on_error ==BLOCK_ERR_STOP_ENOSPC ) && + !bdrv_iostatus_is_enabled (bs)) { +error_set (errp,QERR_INVALID_PARAMETER_COMBINATION ); +return ; + } + + s =block_job_create (&commit_job_type, bs, speed, cb, opaque, errp); +if (!s) { +return ; + } + + s->base = base;
Quoted 9 lines
block/commit.c line 192
Quoted 5 lines
block/commit.c new file +5
-0+ s->top = top; + s->active = bs; + + s->base_flags = orig_base_flags; + s->top_flags = orig_top_flags;So it's the caller who is expected to reopen r/w and then pass the original flags in? Can't we do both of it here?
Yes, that would be cleaner - I'll move the reopen() to commit_start().
Quoted 14 lines
block/commit.c line 201
Quoted 9 lines
block/commit.c new file +9
-0+ + s->on_error = on_error; + s->common.co =qemu_coroutine_create (commit_run); + +trace_commit_start (bs, base, top, s, s->common.co, opaque, + orig_base_flags, orig_top_flags); +qemu_coroutine_enter (s->common.co, s); + +return ;Unnecessary return.
Kevin
Re: [Qemu-devel] [RFC v2 PATCH 2/6] block: add live block commit functionality
Quoted 13 lines · Jeff Cody
Quoted 10 lines · Kevin Wolf
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 9 lines
This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.
The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.
Quoted 8 lines
Quoted 4 lines
What should we do with backing files that are smaller than the image to commit? In this version, data is copied up to the size of the backing file, and then we get
-EIOfrombdrv_co_do_writev().We could leave it like that, and let it receive
-EIOin that case. Alternatively, we could try and determine before the commit if the data will fit in the base, and return-ENOSPCif not.
Neither sounds appealing. Why can't we first try to resize the base to the new data size being committed, and only fall back to -ENOSPC or -EIO if the resize fails?
Signature · 2 lines
Eric Blake [email protected] +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [RFC v2 PATCH 2/6] block: add live block commit functionality
Quoted 19 lines · Eric Blake
Quoted 18 lines · Jeff Cody
Quoted 16 lines · Kevin Wolf
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 9 lines
This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.
The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.
What should we do with backing files that are smaller than the image to commit? In this version, data is copied up to the size of the backing file, and then we get
-EIOfrombdrv_co_do_writev().
Quoted 8 lines
Quoted 3 lines
We could leave it like that, and let it receive
-EIOin that case. Alternatively, we could try and determine before the commit if the data will fit in the base, and return-ENOSPCif not.Neither sounds appealing. Why can't we first try to resize the base to the new data size being committed, and only fall back to
-ENOSPCor-EIOif the resize fails?
OK - we will attempt to resize the base, and return the appropriate error on failure or if unsupported for the format.
RFC PATCH v2 3/6 blockdev: rename block_stream_cb to a generic block_job_cb
In reply to Jeff Cody--- blockdev.c | 8 +++++--- trace-events | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)
blockdev.c +5 -3
@@ -53,6 +53,8 @@ static const int if_max_devs[IF_COUNT] = { [IF_SCSI] = 7, }; +static void block_job_cb (void *opaque,int ret); +/* * We automatically delete the drive when a device using it gets * unplugged. Questionable feature, but we can't just drop it.
@@ -1063,12 +1065,12 @@ static QObject *qobject_from_block_job(BlockJob *job) job->speed); } -static void block_stream_cb (void *opaque,int ret) +static void block_job_cb (void *opaque,int ret) {BlockDriverState *bs = opaque;QObject *obj; -trace_block_stream_cb (bs, bs->job, ret); +trace_block_job_cb (bs, bs->job, ret);assert (bs->job); obj =qobject_from_block_job (bs->job);
@@ -1110,7 +1112,7 @@ void qmp_block_stream(const char *device, bool has_base, }stream_start (bs, base_bs, base, has_speed ? speed :0 , - block_stream_cb, bs, &local_err); + block_job_cb, bs, &local_err);if (error_is_set (&local_err)) {error_propagate (errp, local_err);return ;
trace-events +1 -1
@@ -79,7 +79,7 @@ commit_start(void *bs, void *base, void *top, void *s, void *co, void *opaque, i # blockdev.c qmp_block_job_cancel(void *job) "job %p" -block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d" +block_job_cb(void *bs, void *job, int ret) "bs %p job %p ret %d" qmp_block_stream(void *bs, void *job) "bs %p job %p" # hw/virtio-blk.c
Signature · 1 line
1.7.11.2
Re: [Qemu-devel] [RFC v2 PATCH 3/6] blockdev: rename block_stream_cb to a generic block_job_cb
Il 30/08/2012 20:47, Jeff Cody ha scritto:
blockdev.c static const int if_max_devs[IF_COUNT] = { · line 60
Quoted 9 lines
blockdev.c +2
-0@@ -53,6 +53,8 @@ static const int if_max_devs[IF_COUNT] = { [IF_SCSI] = 7, }; +static void block_job_cb (void *opaque,int ret); +/* * We automatically delete the drive when a device using it gets * unplugged. Questionable feature, but we can't just drop it.
Can you avoid the forward declaration?
Paolo
Re: [Qemu-devel] [RFC v2 PATCH 3/6] blockdev: rename block_stream_cb to a generic block_job_cb
Quoted 13 lines · Paolo Bonzini
Il 30/08/2012 20:47, Jeff Cody ha scritto:
blockdev.c static const int if_max_devs[IF_COUNT] = { · line 60
Quoted 9 lines
blockdev.c +2
-0@@ -53,6 +53,8 @@ static const int if_max_devs[IF_COUNT] = { [IF_SCSI] = 7, }; +static void block_job_cb (void *opaque,int ret); +/* * We automatically delete the drive when a device using it gets * unplugged. Questionable feature, but we can't just drop it. Can you avoid the forward declaration?
Quoted 2 lines
Paolo
Yes, sure - honestly, I added this patch in, but I assumed that the similar patch of yours to support mirroring would go in first, making this patch moot.
Re: [Qemu-devel] [RFC v2 PATCH 3/6] blockdev: rename block_stream_cb to a generic block_job_cb
Il 07/09/2012 19:04, Jeff Cody ha scritto:
Quoted 14 lines · Paolo Bonzini
Quoted 13 lines
Il 30/08/2012 20:47, Jeff Cody ha scritto:
blockdev.c static const int if_max_devs[IF_COUNT] = { · line 60
Quoted 9 lines
blockdev.c +2
-0@@ -53,6 +53,8 @@ static const int if_max_devs[IF_COUNT] = { [IF_SCSI] = 7, }; +static void block_job_cb (void *opaque,int ret); +/* * We automatically delete the drive when a device using it gets * unplugged. Questionable feature, but we can't just drop it. Can you avoid the forward declaration?
Quoted 6 lines
Quoted 2 lines
Paolo
Yes, sure - honestly, I added this patch in, but I assumed that the similar patch of yours to support mirroring would go in first, making this patch moot.
I now took this patch of yours in my tree (minus the forward declaration), so...
Paolo
RFC PATCH v2 4/6 qerror: new error for live block commit, QERR_TOP_NOT_FOUND
In reply to Jeff Cody--- qerror.h | 3 +++ 1 file changed, 3 insertions(+)
qerror.h +3 -0
@@ -219,6 +219,9 @@ void assert_no_error(Error *err);#define QERR_TOO_MANY_FILES \ ERROR_CLASS_GENERIC_ERROR, "Too many open files" +#define QERR_TOP_NOT_FOUND \ + ERROR_CLASS_GENERIC_ERROR, "Top image file %s not found" +#define QERR_UNDEFINED_ERROR \ ERROR_CLASS_GENERIC_ERROR, "An undefined error has occurred"
Signature · 1 line
1.7.11.2
Re: [Qemu-devel] [RFC v2 PATCH 4/6] qerror: new error for live block commit, QERR_TOP_NOT_FOUND
Quoted 10 lines · Jeff Cody
Signed-off-by Jeff Cody <[email protected]>--- qerror.h | 3 +++ 1 file changed, 3 insertions(+)qerror.h +0
-0
qerror.h void assert_no_error(Error *err); · line 224
Quoted 6 lines
qerror.h +3
-0@@ -219,6 +219,9 @@ void assert_no_error(Error *err);#define QERR_TOO_MANY_FILES \ ERROR_CLASS_GENERIC_ERROR, "Too many open files" +#define QERR_TOP_NOT_FOUND \ + ERROR_CLASS_GENERIC_ERROR, "Top image file %s not found" +
Are you sure this patch is needed, or should you be using error_setg()?
http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04980.html
Signature · 2 lines
Eric Blake [email protected] +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [RFC v2 PATCH 4/6] qerror: new error for live block commit, QERR_TOP_NOT_FOUND
Quoted 14 lines · Eric Blake
Quoted 13 lines · Jeff Cody
Signed-off-by Jeff Cody <[email protected]>--- qerror.h | 3 +++ 1 file changed, 3 insertions(+)qerror.h +0
-0@@ -219,6 +219,9 @@ void assert_no_error(Error *err);#define QERR_TOO_MANY_FILES \ ERROR_CLASS_GENERIC_ERROR, "Too many open files"
Quoted 8 lines
qerror.h void assert_no_error(Error *err); · line 224
Quoted 3 lines
qerror.h +3
-0+#define QERR_TOP_NOT_FOUND \ + ERROR_CLASS_GENERIC_ERROR, "Top image file %s not found" +Are you sure this patch is needed, or should you be using
error_setg()?http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04980.html
I think you are right, error_setg() would be the best course - although I based these patches only on what was in qemu/master currently.
RFC PATCH v2 5/6 block: helper function, to find the base image of a chain
In reply to Jeff CodyThis is a simple helper function, that will return the base image of a given image chain.
--- block.c | 16 ++++++++++++++++ block.h | 1 + 2 files changed, 17 insertions(+)
block.c +16 -0
@@ -3137,6 +3137,22 @@ int bdrv_get_backing_file_depth(BlockDriverState *bs)return 1 +bdrv_get_backing_file_depth (bs->backing_hd); } +BlockDriverState *bdrv_find_base (BlockDriverState *bs) +{ +BlockDriverState *curr_bs =NULL ; + +if (!bs) { +return NULL ; + } + + curr_bs = bs; + +while (curr_bs->backing_hd) { + curr_bs = curr_bs->backing_hd; + } +return curr_bs; +} +#define NB_SUFFIXES 4char *get_human_readable_size (char *buf,int buf_size,int64_t size)
block.h +1 -0
@@ -201,6 +201,7 @@ int bdrv_commit_all(void);int bdrv_change_backing_file (BlockDriverState *bs,const char *backing_file,const char *backing_fmt);void bdrv_register (BlockDriver *bdrv); +BlockDriverState *bdrv_find_base (BlockDriverState *bs);int bdrv_delete_intermediate (BlockDriverState *active,BlockDriverState *top,BlockDriverState *base); BlockDriverState *bdrv_find_child(BlockDriverState *active,
Signature · 1 line
1.7.11.2
RFC PATCH v2 6/6 QAPI: add command for live block commit, 'block-commit'
In reply to Jeff CodyThe 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
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.
--- blockdev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 30 ++++++++++++++++++++ qmp-commands.hx | 6 ++++ 3 files changed, 119 insertions(+)
blockdev.c +83 -0
@@ -827,6 +827,89 @@ exit:return ; } +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, *child_bs; +Error *local_err =NULL ; +int orig_base_flags, orig_top_flags; +BlockReopenQueue *reopen_queue =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 ,"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); +if (top_bs ==NULL ) { +error_set (errp,QERR_TOP_NOT_FOUND , top); +return ; + } + }else { +/* we will eventually default to the top layer,i.e. top_bs = bs */ +error_set (errp,QERR_TOP_NOT_FOUND , top); +return ; + } + + child_bs =bdrv_find_child (bs, top_bs); + + orig_base_flags =bdrv_get_flags (base_bs);/* what we are writing into */ + orig_top_flags =bdrv_get_flags (child_bs);/* to change the backing file */ + +/* convert base_bs to r/w, if necessary */ +if (!(orig_base_flags &BDRV_O_RDWR )) { + reopen_queue =bdrv_reopen_queue (reopen_queue, base_bs, + orig_base_flags |BDRV_O_RDWR ); + } +if (!(orig_top_flags &BDRV_O_RDWR )) { + reopen_queue =bdrv_reopen_queue (reopen_queue, base_bs, + orig_base_flags |BDRV_O_RDWR ); + } +if (reopen_queue) { +bdrv_reopen_multiple (reopen_queue, &local_err); +if (local_err !=NULL ) { +error_propagate (errp, local_err); +return ; + } + } + +commit_start (bs, base_bs, top_bs, speed, on_error, + block_job_cb, bs, orig_base_flags, orig_top_flags, + &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 void eject_device (BlockDriverState *bs,int force,Error **errp) {
qapi-schema.json +30 -0
@@ -1401,6 +1401,36 @@ '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 parent image of the device to write data into. +# If not specified, this is the original parent image. +# +# @top: #optional The child image, above which data will not be committed +# down. If not specified, this is the active layer. +# +# @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.2 +# +## +{ 'command': 'block-commit', + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', + '*speed': 'int' } } + # @migrate_cancel # # Cancel the current executing migration process.
qmp-commands.hx +6 -0
@@ -767,6 +767,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,
Signature · 1 line
1.7.11.2
Re: [Qemu-devel] [RFC v2 PATCH 6/6] QAPI: add command for live block commit, 'block-commit'
Quoted 11 lines · 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 the active image) - see note below
speed: maximum speed, in bytes/sec
Quoted 7 lines
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 errorQERR_TOP_NOT_FOUNDwill be returned.The is done as a block job, so upon completion a
BLOCK_JOB_COMPLETEDwill be emitted.
Likewise, the job can be canceled, and it is possible to track progress of the job or change the speed on the fly, using existing block job commands.
Will the BLOCK_JOB_COMPLETED event have a new category listing that it was a commit job instead of a stream job that completed? That is, I think this patch is incomplete, and that you also need to modify QMP/qmp-events.txt to modify the 'type' field of BLOCK_JOB_{CANCELLED,COMPLETED} to distinguish this new sub-type of event.
Quoted 16 lines
##
qapi-schema.json +14
-0+# @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 parent image of the device to write data into. +# If not specified, this is the original parent image. +# +# @top: #optional The child image, above which data will not be committed +# down. If not specified, this is the active layer. +# +# @speed: #optional the maximum speed, in bytes per second
qapi-schema.json line 1427
Quoted 10 lines
qapi-schema.json +10
-0+# +# 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.2
1.3
Signature · 2 lines
Eric Blake [email protected] +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [RFC v2 PATCH 6/6] QAPI: add command for live block commit, 'block-commit'
Quoted 17 lines · Eric Blake
Quoted 16 lines · 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 the active image) - see note below
speed: maximum speed, in bytes/secnote: 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 errorQERR_TOP_NOT_FOUNDwill be returned.
Quoted 6 lines
Quoted 2 lines
The is done as a block job, so upon completion a
BLOCK_JOB_COMPLETEDwill be emitted.Likewise, the job can be canceled, and it is possible to track progress of the job or change the speed on the fly, using existing block job commands.
Correct.
Quoted 7 lines
Will the
BLOCK_JOB_COMPLETEDevent have a new category listing that it was a commit job instead of a stream job that completed? That is, I think this patch is incomplete, and that you also need to modifyQMP/qmp-events.txtto modify the 'type' field of BLOCK_JOB_{CANCELLED,COMPLETED} to distinguish this new sub-type of event.
You are right, I should add the sub-type info into QMP/qmp-events.txt; the type is 'commit'.
Here is what the BLOCK_JOB_COMPLETED event looks like for commit:
{"timestamp": {"seconds": 1346423249, "microseconds": 551295},
"event": "BLOCK_JOB_COMPLETED",
"data": {"device": "virtio0", "len": 6442450944, "offset": 6442450944,
"speed": 140732751976032, "type": "commit"}}
Quoted 29 lines
Quoted 16 lines
##
qapi-schema.json +14
-0+# @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 parent image of the device to write data into. +# If not specified, this is the original parent image. +# +# @top: #optional The child image, above which data will not be committed +# down. If not specified, this is the active layer. +# +# @speed: #optional the maximum speed, in bytes per secondqapi-schema.json line 1427
Quoted 10 lines
qapi-schema.json +10
-0+# +# 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.21.3
Re: [Qemu-devel] [RFC v2 PATCH 6/6] QAPI: add command for live block commit, 'block-commit'
In reply to Jeff CodyAm 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 56 lines
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/secnote: 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 errorQERR_TOP_NOT_FOUNDwill be returned.The is done as a block job, so upon completion a
BLOCK_JOB_COMPLETEDwill be emitted.Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 30 ++++++++++++++++++++ qmp-commands.hx | 6 ++++ 3 files changed, 119 insertions(+)blockdev.c +22
-0@@ -827,6 +827,89 @@ exit:return ; } +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, *child_bs; +Error *local_err =NULL ; +int orig_base_flags, orig_top_flags; +BlockReopenQueue *reopen_queue =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);
blockdev.c exit: · line 861
Quoted 10 lines
blockdev.c +10
-0+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 ,"NULL" );
Shouldn't it be base rather than "NULL"?
Quoted 20 lines
blockdev.c +20
-0+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); +if (top_bs ==NULL ) { +error_set (errp,QERR_TOP_NOT_FOUND , top); +return ; + } + }else { +/* we will eventually default to the top layer,i.e. top_bs = bs */ +error_set (errp,QERR_TOP_NOT_FOUND , top); +return ; + } + + child_bs =bdrv_find_child (bs, top_bs); + + orig_base_flags =bdrv_get_flags (base_bs);/* what we are writing into */
blockdev.c exit: · line 891
Quoted 10 lines
blockdev.c +10
-0+ orig_top_flags =bdrv_get_flags (child_bs);/* to change the backing file */ + +/* convert base_bs to r/w, if necessary */ +if (!(orig_base_flags &BDRV_O_RDWR )) { + reopen_queue =bdrv_reopen_queue (reopen_queue, base_bs, + orig_base_flags |BDRV_O_RDWR ); + } +if (!(orig_top_flags &BDRV_O_RDWR )) { + reopen_queue =bdrv_reopen_queue (reopen_queue, base_bs, + orig_base_flags |BDRV_O_RDWR );
I think you mean child_bs and orig_top_flags. (Why isn't it called orig_child_flags?)
Quoted 31 lines
blockdev.c +21
-0+ } +if (reopen_queue) { +bdrv_reopen_multiple (reopen_queue, &local_err); +if (local_err !=NULL ) { +error_propagate (errp, local_err); +return ; + } + } + +commit_start (bs, base_bs, top_bs, speed, on_error, + block_job_cb, bs, orig_base_flags, orig_top_flags, + &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 void eject_device (BlockDriverState *bs,int force,Error **errp) {qapi-schema.json +0
-0@@ -1401,6 +1401,36 @@ 'returns': 'str' }
qapi-schema.json line 1412
Quoted 10 lines
qapi-schema.json +9
-0## +# @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 parent image of the device to write data into. +# If not specified, this is the original parent image.
"The file name of the parent image", actually.
Which I'm afraid means that this isn't an API for the long term, but there's little we can do about it now...
qapi-schema.json line 1415
Quoted 3 lines
qapi-schema.json +3
-0+# +# @top: #optional The child image, above which data will not be committed +# down. If not specified, this is the active layer.
Again, the file name.
Kevin
Re: [Qemu-devel] [RFC v2 PATCH 6/6] QAPI: add command for live block commit, 'block-commit'
Quoted 61 lines · Kevin Wolf
Am 30.08.2012 20:47, schrieb Jeff Cody:
Quoted 60 lines
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/secnote: 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 errorQERR_TOP_NOT_FOUNDwill be returned.The is done as a block job, so upon completion a
BLOCK_JOB_COMPLETEDwill be emitted.Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 30 ++++++++++++++++++++ qmp-commands.hx | 6 ++++ 3 files changed, 119 insertions(+)blockdev.c +26
-0@@ -827,6 +827,89 @@ exit:return ; } +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, *child_bs; +Error *local_err =NULL ; +int orig_base_flags, orig_top_flags; +BlockReopenQueue *reopen_queue =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);
Quoted 9 lines
blockdev.c exit: · line 861
Quoted 6 lines
blockdev.c +6
-0+ }else { + base_bs =bdrv_find_base (bs); + } + +if (base_bs ==NULL ) { +error_set (errp,QERR_BASE_NOT_FOUND ,"NULL" );Shouldn't it be base rather than "NULL"?
Yes
Quoted 25 lines
Quoted 25 lines
blockdev.c +25
-0+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); +if (top_bs ==NULL ) { +error_set (errp,QERR_TOP_NOT_FOUND , top); +return ; + } + }else { +/* we will eventually default to the top layer,i.e. top_bs = bs */ +error_set (errp,QERR_TOP_NOT_FOUND , top); +return ; + } + + child_bs =bdrv_find_child (bs, top_bs); + + orig_base_flags =bdrv_get_flags (base_bs);/* what we are writing into */ + orig_top_flags =bdrv_get_flags (child_bs);/* to change the backing file */ + +/* convert base_bs to r/w, if necessary */ +if (!(orig_base_flags &BDRV_O_RDWR )) { + reopen_queue = bdrv_reopen_queue(reopen_queue, base_bs,
Quoted 9 lines
blockdev.c exit: · line 891
Quoted 5 lines
blockdev.c +5
-0+ orig_base_flags | BDRV_O_RDWR); + } +if (!(orig_top_flags &BDRV_O_RDWR )) { + reopen_queue =bdrv_reopen_queue (reopen_queue, base_bs, + orig_base_flags |BDRV_O_RDWR );I think you mean
child_bsandorig_top_flags. (Why isn't it calledorig_child_flags?)
Yes, thanks - and (per your comments on the previous patch), I'll move the reopen bits into commit_start().
Quoted 55 lines
Quoted 31 lines
blockdev.c +21
-0+ } +if (reopen_queue) { +bdrv_reopen_multiple (reopen_queue, &local_err); +if (local_err !=NULL ) { +error_propagate (errp, local_err); +return ; + } + } + +commit_start (bs, base_bs, top_bs, speed, on_error, + block_job_cb, bs, orig_base_flags, orig_top_flags, + &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 void eject_device (BlockDriverState *bs,int force,Error **errp) {qapi-schema.json +0
-0@@ -1401,6 +1401,36 @@ 'returns': 'str' }qapi-schema.json line 1412
Quoted 10 lines
qapi-schema.json +9
-0## +# @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 parent image of the device to write data into. +# If not specified, this is the original parent image."The file name of the parent image", actually.
Which I'm afraid means that this isn't an API for the long term, but there's little we can do about it now...
qapi-schema.json line 1415
Quoted 3 lines
qapi-schema.json +3
-0+# +# @top: #optional The child image, above which data will not be committed +# down. If not specified, this is the active layer.Again, the file name.
Kevin
Quoted 39 lines · Jeff Cody
Live block commit.
I originally had intended for this RFC series to include the more complicated case of a live commit of the active layer, but removed it for this commit in the hopes of making it into the soft feature freeze for 1.2, so this series is the simpler case.
This series adds the basic case, of a live commit between two images below the active layer, e.g.:
[base] <--- [snp-1] <--- [snp-2] <--- [snp-3] <--- [active]
can be collapsed down via commit, into:
[base] <--- [active]
or,
[base] <--- [snp-1] <--- [active],
[base] <--- [snp-3] <--- [active],
etc..
TODO: * qemu-io tests (in progress)
* 'stage-2' of live commit functionality, to be able to push down the active layer. This structured something like mirroring, to allow for convergence.Changes from the RFC v1 series:
* This patch series is not on top of Paolo's blk mirror series yet, to make it easier to apply independently if desired. This means some of what was in the previous RFC series is not in this one (BlockdevOnError, for instance), but that can be easily added in once Paolo's series are in.
* This patches series is dependent on the
reopen()series with transactional reopen.
Quoted 10 lines
* The target release for this series is 1.3
* Found some mistakes in the reopen calls
* Dropped the BlockdevOnError argument (for now), will add in if rebasing on top of Paolo's series.
* Used the new qerror system
I meant to add this to my cover letter, but forgot; if anyone wants to play around with this, you can find it on github:
git://github.com/codyprime/qemu-kvm-jtc.git (branch jtc-live-commit-1.3)
Quoted 22 lines
Jeff Cody (6):
1/6 block: add support functions for live commit, to find and delete
images.2/6 block: add live block commit functionality 3/6 blockdev: rename block_stream_cb to a generic block_job_cb 4/6 qerror: new error for live block commit, QERR_TOP_NOT_FOUND 5/6 block: helper function, to find the base image of a chain 6/6 QAPI: add command for live block commit, 'block-commit'block.c | 158 ++++++++++++++++++++++++++++++++++++++++ block.h | 6 +- block/Makefile.objs | 1 + block/commit.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block_int.h | 19 +++++ blockdev.c | 91 ++++++++++++++++++++++- qapi-schema.json | 30 ++++++++ qerror.h | 3 + qmp-commands.hx | 6 ++ trace-events | 4 +- 10 files changed, 515 insertions(+), 5 deletions(-) create mode 100644 block/commit.c