Re: [Qemu-devel] [PATCH 21/47] block: add bdrv_ensure_backing_file

[Kevin Wolf]

Am 11.09.2012 16:10, schrieb Paolo Bonzini:
> 
> 
> ----- Messaggio originale -----
>> Da: "Kevin Wolf" <address@hidden>
>> A: "Paolo Bonzini" <address@hidden>
>> Cc: address@hidden, address@hidden, address@hidden, address@hidden
>> Inviato: Martedì, 11 settembre 2012 15:58:38
>> Oggetto: Re: [PATCH 21/47] block: add bdrv_ensure_backing_file
>>
>> Am 11.09.2012 15:46, schrieb Paolo Bonzini:
>>>
>>>> Once again, combining code motion and code changes in one patch
>>>> makes
>>>> it harder to review.
>>>
>>> bdrv_ensure_backing_file() is a new standalone function that
>>> happens to be
>>> usable in bdrv_open as well.  But I can separate the changes/fixes
>>> to a
>>> separate patch.
>>>
>>> In particular it is can be used after a file has been opened with
>>> BDRV_O_NO_BACKING (at which point the flag does not reflect reality
>>> anymore, hence the removal of the flag).
>>
>> Yes, that's what I figured eventually. Maybe some documentation for
>> the function couldn't hurt.
>>
>>>> bdrv_ensure_backing_file() isn't a good name, after reading only
>>>> the
>>>> subject line I had no idea what this function might do. It's still
>>>> not entirely clear to me what the different to a
>>>> bdrv_open_backing_file()
>>>> is, except that it doesn't do anything if a backing file is
>>>> already
>>>> open. In which cases do we rely on this behaviour?
>>>
>>> We open the mirroring target with BDRV_O_NO_BACKING usually, but
>>> require
>>> the backing file if the cluster size is larger than the dirty block
>>> granularity.  Later, COW is done in the mirror job, so this is not
>>> needed anymore at the end of the series.
>>
>> Can we then put a /* FIXME */ comment there and revert that behaviour
>> at the end of the series? Then we can call it bdrv_open_backing_file()
>> and it's meaning becomes more obvious.
> 
> Actually, now that my machine finished upgrading and I can look at the
> source code, we do use the functionality even at the end of this series.
> If you call block-job-complete to complete mirroring, 
> bdrv_ensure_backing_file()
> is called.  But block-job-complete can be called multiple times, because
> completion is entirely asynchronous.  I can check bs->backing_hd in the
> completion callback, but I think it's less clean (there is no reason in
> principle why block/mirror.c should include block_int.h, and adding a
> function just to use it once seems not worth).
> 
> I would like to add documentation to all functions in block.h in 1.3,
> I can start from this function if that would mean keeping it as is...

I'm not against keeping the logic, just adding documentation to new
functions would be good; even more so if their semantics isn't obvious.

I think I'd consider renaming the function anyway.

>>>>> +    if (bs->backing_file[0] == '\0') {
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    bs->backing_hd = bdrv_new("");
>>>>> +    bdrv_get_full_backing_filename(bs, backing_filename,
>>>>> +                                   sizeof(backing_filename));
>>>>> +
>>>>> +    if (bs->backing_format[0] != '\0') {
>>>>> +        back_drv = bdrv_find_format(bs->backing_format);
>>>>> +    }
>>>>> +
>>>>> +    /* backing files always opened read-only */
>>>>> +    back_flags = bs->open_flags & ~(BDRV_O_RDWR |
>>>>> BDRV_O_SNAPSHOT);
>>>>> +
>>>>> +    ret = bdrv_open(bs->backing_hd, backing_filename,
>>>>> back_flags,
>>>>> back_drv);
>>>>> +    if (ret < 0) {
>>>>> +        bdrv_close(bs);
>>>>
>>>> I don't like this because it makes the invalid assumption that the
>>>> caller has just opened bs and wants to close it if opening the
>>>> backing file fails. I think this is part of the error handling
>>>> that belongs
>>>> in the caller: It opened bs, so it is responsible for closing it
>>>> in
>>>> error cases.
>>>
>>> It's a bug, it should have closed bs->backing_hd.
>>
>> Are you sure? You removed the bdrv_close(bs) in the caller, so that
>> it's missing there would be a second bug.
>> An explicit bdrv_close(bs->backing_hd) isn't required here, it is
>> implicitly called in bdrv_delete(bs->backing_hd).
> 
> True.  But likely my mental process was to add the bdrv_close(bs) here
> thinking that it would match the bdrv_delete below.  Note that
> bdrv_close(bs) already does delete bs->backing_hd.

Whatever, as long as the bug gets fixed... ;-)

>>>>> +        bdrv_delete(bs->backing_hd);
>>>>
>>>> This is a bug fix of its own, should be a separate patch.
>>>
>>> Ok.
>>>
>>>>> +        bs->backing_hd = NULL;
>>>>> +        return ret;
>>>>> +    }
>>>>> +    if (bs->is_temporary) {
>>>>> +        bs->backing_hd->keep_read_only = !(bs->open_flags &
>>>>> BDRV_O_RDWR);
>>>>> +    } else {
>>>>> +        /* base images use the same setting as leaf */
>>>>
>>>> Huh, "parent" turned into "leaf"?
>>>
>>> Will move this to a separate patch, too.
>>
>> I don't even understand what you're trying to say in this comment.
> 
> Well, I couldn't understand the original comment either. :)  To me,
> base image and parent is a synonym...
> 
> The images form a tree (snapshots being nodes and with each node
> having a parent pointer); what we open is a path from root to leaf,
> so the top-level image is a leaf.

We definitely need to get the terminology clarified...

Yes, you're right, the images files on the disk form a tree and your
terminology matches this. But in a running qemu, we have a tree of
BlockDriverStates (as I mentioned bs->file and bs->backing_hd create it,
along with driver specific links), and unfortunately it's direction is
exactly the opposite. This is what the terminology of the original
comment was based on and what I thought of. Both approaches are right in
a way.

I had a similar confusion when talking to Jeff recently, and there we
decided to avoid talking about children and parents at all. Maybe this
is the best in order to avoid misunderstandings. For the image files on
the disk "backing file/overlay" are good alternatives; I'm not sure if
we have any for the BDS tree.

Kevin