Re: [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate

[Fam Zheng <famz@redhat.com>]

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