| Message ID | 1386836626-6436-5-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben: > Dropping intermediate could be useful both for commit and stream, and > BDS refcnt plus bdrv_swap could do most of the job nicely. It also need > some improvements in preparation for op blockers. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 152 +++++++++++++++++++++++++++------------------------------ > block/commit.c | 1 + > 2 files changed, 74 insertions(+), 79 deletions(-) > > diff --git a/block.c b/block.c > index 41562fd..681d3be 100644 > --- a/block.c > +++ b/block.c > @@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active, > return overlay; > } > > -typedef struct BlkIntermediateStates { > - BlockDriverState *bs; > - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; > -} BlkIntermediateStates; > - > +static void bdrv_set_backing_hd(BlockDriverState *bs, > + BlockDriverState *new_backing) > +{ > + if (bs->backing_hd) { > + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); > + } > + bs->backing_hd = new_backing; > + if (new_backing) { > + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); What about unblocking commit, like you did in patch 3? Should bdrv_open_backing_file() be using this function? > + } > +} > > /* > - * Drops images above 'base' up to and including 'top', and sets the image > - * above 'top' to have base as its backing file. > + * Drops images above 'base' up to and including 'top', and sets new 'base' > + * as backing_hd of top_overlay (the image orignally has 'top' as backing > + * file). top_overlay may be NULL if 'top' is active, no such update needed. > + * Requires that the top_overlay to 'top' is opened r/w. > * > - * Requires that the overlay to 'top' is opened r/w, so that the backing file > - * information in 'bs' can be properly updated. > + * 1) This will convert the following chain: > + * ... <- base <- ... <- top <- overlay <-... <- active > * > - * E.g., this will convert the following chain: > - * bottom <- base <- intermediate <- top <- active > + * to > + * > + * ... <- base <- overlay <- active > + * > + * 2) It is allowed for bottom==base, in which case it converts: > + * > + * ... <- base <- ... <- top <- overlay <- ... <- active > * > * to > * > - * bottom <- base <- active > + * base <- overlay <- active > * > - * It is allowed for bottom==base, in which case it converts: > + * 2) It also allows active==top, in which case it converts: > * > - * base <- intermediate <- top <- active > + * ... <- base <- ... <- top (active) > * > * to > * > - * base <- active > + * base == active == top, i.e. only base and lower remains: *top == *base when > + * return. > + * > + * 3) If base==NULL, it will drop all the BDS below overlay and set its > + * backing_hd to NULL. I.e.: > * > - * Error conditions: > - * if active == top, that is considered an error > + * base(NULL) <- ... <- overlay <- ... <- active > * > + * to > + * > + * overlay <- ... <- active > */ > int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, > BlockDriverState *base) > { > - BlockDriverState *intermediate; > - BlockDriverState *base_bs = NULL; > - BlockDriverState *new_top_bs = NULL; > - BlkIntermediateStates *intermediate_state, *next; > - int ret = -EIO; > - > - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; > - QSIMPLEQ_INIT(&states_to_delete); > - > - if (!top->drv || !base->drv) { > - goto exit; > - } > + BlockDriverState *drop_start, *overlay; > + int ret = -EINVAL; > > - new_top_bs = bdrv_find_overlay(active, top); > - > - if (new_top_bs == NULL) { > - /* we could not find the image above 'top', this is an error */ > + if (!top->drv || (base && !base->drv)) { > goto exit; > } > - > - /* 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) { > + if (top == base) { > ret = 0; > - goto exit; > - } > - > - intermediate = 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; > + } else if (top == active) { > + assert(base); > + drop_start = active->backing_hd; > + bdrv_swap(active, base); > + base->backing_hd = NULL; > + bdrv_unref(drop_start); > + ret = 0; > + } else { > + /* If there's an overlay, its backing_hd points to top's BDS now, > + * the top image is dropped but this BDS structure is kept and swapped > + * with base, this way we keep the pointers valid after dropping top */ > + overlay = bdrv_find_overlay(active, top); > + if (!overlay) { > + goto exit; > + } > + if (base) { > + ret = bdrv_change_backing_file(overlay, base->filename, > + base->drv->format_name); > + } else { > + ret = bdrv_change_backing_file(overlay, "", ""); This should be NULL, NULL instead of empty strings. Kevin
On 2013年12月12日 21:24, Kevin Wolf wrote: > Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben: >> Dropping intermediate could be useful both for commit and stream, and >> BDS refcnt plus bdrv_swap could do most of the job nicely. It also need >> some improvements in preparation for op blockers. >> >> Signed-off-by: Fam Zheng <famz@redhat.com> >> --- >> block.c | 152 +++++++++++++++++++++++++++------------------------------ >> block/commit.c | 1 + >> 2 files changed, 74 insertions(+), 79 deletions(-) >> >> diff --git a/block.c b/block.c >> index 41562fd..681d3be 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active, >> return overlay; >> } >> >> -typedef struct BlkIntermediateStates { >> - BlockDriverState *bs; >> - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; >> -} BlkIntermediateStates; >> - >> +static void bdrv_set_backing_hd(BlockDriverState *bs, >> + BlockDriverState *new_backing) >> +{ >> + if (bs->backing_hd) { >> + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); >> + } >> + bs->backing_hd = new_backing; >> + if (new_backing) { >> + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); > > What about unblocking commit, like you did in patch 3? > > Should bdrv_open_backing_file() be using this function? > Yes, will do that. >> + } >> +} >> >> /* >> - * Drops images above 'base' up to and including 'top', and sets the image >> - * above 'top' to have base as its backing file. >> + * Drops images above 'base' up to and including 'top', and sets new 'base' >> + * as backing_hd of top_overlay (the image orignally has 'top' as backing >> + * file). top_overlay may be NULL if 'top' is active, no such update needed. >> + * Requires that the top_overlay to 'top' is opened r/w. >> * >> - * Requires that the overlay to 'top' is opened r/w, so that the backing file >> - * information in 'bs' can be properly updated. >> + * 1) This will convert the following chain: >> + * ... <- base <- ... <- top <- overlay <-... <- active >> * >> - * E.g., this will convert the following chain: >> - * bottom <- base <- intermediate <- top <- active >> + * to >> + * >> + * ... <- base <- overlay <- active >> + * >> + * 2) It is allowed for bottom==base, in which case it converts: >> + * >> + * ... <- base <- ... <- top <- overlay <- ... <- active >> * >> * to >> * >> - * bottom <- base <- active >> + * base <- overlay <- active >> * >> - * It is allowed for bottom==base, in which case it converts: >> + * 2) It also allows active==top, in which case it converts: >> * >> - * base <- intermediate <- top <- active >> + * ... <- base <- ... <- top (active) >> * >> * to >> * >> - * base <- active >> + * base == active == top, i.e. only base and lower remains: *top == *base when >> + * return. >> + * >> + * 3) If base==NULL, it will drop all the BDS below overlay and set its >> + * backing_hd to NULL. I.e.: >> * >> - * Error conditions: >> - * if active == top, that is considered an error >> + * base(NULL) <- ... <- overlay <- ... <- active >> * >> + * to >> + * >> + * overlay <- ... <- active >> */ >> int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, >> BlockDriverState *base) >> { >> - BlockDriverState *intermediate; >> - BlockDriverState *base_bs = NULL; >> - BlockDriverState *new_top_bs = NULL; >> - BlkIntermediateStates *intermediate_state, *next; >> - int ret = -EIO; >> - >> - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; >> - QSIMPLEQ_INIT(&states_to_delete); >> - >> - if (!top->drv || !base->drv) { >> - goto exit; >> - } >> + BlockDriverState *drop_start, *overlay; >> + int ret = -EINVAL; >> >> - new_top_bs = bdrv_find_overlay(active, top); >> - >> - if (new_top_bs == NULL) { >> - /* we could not find the image above 'top', this is an error */ >> + if (!top->drv || (base && !base->drv)) { >> goto exit; >> } >> - >> - /* 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) { >> + if (top == base) { >> ret = 0; >> - goto exit; >> - } >> - >> - intermediate = 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; >> + } else if (top == active) { >> + assert(base); >> + drop_start = active->backing_hd; >> + bdrv_swap(active, base); >> + base->backing_hd = NULL; >> + bdrv_unref(drop_start); >> + ret = 0; >> + } else { >> + /* If there's an overlay, its backing_hd points to top's BDS now, >> + * the top image is dropped but this BDS structure is kept and swapped >> + * with base, this way we keep the pointers valid after dropping top */ >> + overlay = bdrv_find_overlay(active, top); >> + if (!overlay) { >> + goto exit; >> + } >> + if (base) { >> + ret = bdrv_change_backing_file(overlay, base->filename, >> + base->drv->format_name); >> + } else { >> + ret = bdrv_change_backing_file(overlay, "", ""); > > This should be NULL, NULL instead of empty strings. > OK, thanks. Fam
diff --git a/block.c b/block.c index 41562fd..681d3be 100644 --- a/block.c +++ b/block.c @@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active, return overlay; } -typedef struct BlkIntermediateStates { - BlockDriverState *bs; - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; -} BlkIntermediateStates; - +static void bdrv_set_backing_hd(BlockDriverState *bs, + BlockDriverState *new_backing) +{ + if (bs->backing_hd) { + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); + } + bs->backing_hd = new_backing; + if (new_backing) { + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); + } +} /* - * Drops images above 'base' up to and including 'top', and sets the image - * above 'top' to have base as its backing file. + * Drops images above 'base' up to and including 'top', and sets new 'base' + * as backing_hd of top_overlay (the image orignally has 'top' as backing + * file). top_overlay may be NULL if 'top' is active, no such update needed. + * Requires that the top_overlay to 'top' is opened r/w. * - * Requires that the overlay to 'top' is opened r/w, so that the backing file - * information in 'bs' can be properly updated. + * 1) This will convert the following chain: + * ... <- base <- ... <- top <- overlay <-... <- active * - * E.g., this will convert the following chain: - * bottom <- base <- intermediate <- top <- active + * to + * + * ... <- base <- overlay <- active + * + * 2) It is allowed for bottom==base, in which case it converts: + * + * ... <- base <- ... <- top <- overlay <- ... <- active * * to * - * bottom <- base <- active + * base <- overlay <- active * - * It is allowed for bottom==base, in which case it converts: + * 2) It also allows active==top, in which case it converts: * - * base <- intermediate <- top <- active + * ... <- base <- ... <- top (active) * * to * - * base <- active + * base == active == top, i.e. only base and lower remains: *top == *base when + * return. + * + * 3) If base==NULL, it will drop all the BDS below overlay and set its + * backing_hd to NULL. I.e.: * - * Error conditions: - * if active == top, that is considered an error + * base(NULL) <- ... <- overlay <- ... <- active * + * to + * + * overlay <- ... <- active */ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, BlockDriverState *base) { - BlockDriverState *intermediate; - BlockDriverState *base_bs = NULL; - BlockDriverState *new_top_bs = NULL; - BlkIntermediateStates *intermediate_state, *next; - int ret = -EIO; - - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; - QSIMPLEQ_INIT(&states_to_delete); - - if (!top->drv || !base->drv) { - goto exit; - } + BlockDriverState *drop_start, *overlay; + int ret = -EINVAL; - new_top_bs = bdrv_find_overlay(active, top); - - if (new_top_bs == NULL) { - /* we could not find the image above 'top', this is an error */ + if (!top->drv || (base && !base->drv)) { goto exit; } - - /* 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) { + if (top == base) { ret = 0; - goto exit; - } - - intermediate = 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; + } else if (top == active) { + assert(base); + drop_start = active->backing_hd; + bdrv_swap(active, base); + base->backing_hd = NULL; + bdrv_unref(drop_start); + ret = 0; + } else { + /* If there's an overlay, its backing_hd points to top's BDS now, + * the top image is dropped but this BDS structure is kept and swapped + * with base, this way we keep the pointers valid after dropping top */ + overlay = bdrv_find_overlay(active, top); + if (!overlay) { + goto exit; + } + if (base) { + ret = bdrv_change_backing_file(overlay, base->filename, + base->drv->format_name); + } else { + ret = bdrv_change_backing_file(overlay, "", ""); + } + if (ret) { + goto exit; + } + if (base) { + drop_start = top->backing_hd; + bdrv_swap(top, base); + /* Break the loop formed by bdrv_swap */ + bdrv_set_backing_hd(base, NULL); + } else { + bdrv_set_backing_hd(overlay, NULL); + drop_start = top; } - 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_unref(intermediate_state->bs); + bdrv_unref(drop_start); } - 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) { diff --git a/block/commit.c b/block/commit.c index d4090cb..4d8cd05 100644 --- a/block/commit.c +++ b/block/commit.c @@ -142,6 +142,7 @@ wait: if (!block_job_is_cancelled(&s->common) && sector_num == end) { /* success */ ret = bdrv_drop_intermediate(active, top, base); + base = top; } exit_free_buf:
Dropping intermediate could be useful both for commit and stream, and BDS refcnt plus bdrv_swap could do most of the job nicely. It also need some improvements in preparation for op blockers. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 152 +++++++++++++++++++++++++++------------------------------ block/commit.c | 1 + 2 files changed, 74 insertions(+), 79 deletions(-)