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

[Paolo Bonzini <pbonzini@redhat.com>]

----- Messaggio originale -----
> Da: "Kevin Wolf" <kwolf@redhat.com>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, eblake@redhat.com, jcody@redhat.com, stefanha@linux.vnet.ibm.com
> 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...

> >>> +    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.

> >>> +        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.

Paolo

> The
> only tree that I can think of (so something like leaves exists) is
> built by bs->file and bs->backing_hd. But in this case, the top-level
> image, from which the flags are taken, is the root and not a leaf?
> 
> Kevin
>