[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 21/47] block: add bdrv_ensure_backing_file
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 21/47] block: add bdrv_ensure_backing_file |
Date: |
Tue, 11 Sep 2012 10:10:51 -0400 (EDT) |
----- 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...
> >>> + 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
>