From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBRB8-0007ni-Rt for qemu-devel@nongnu.org; Tue, 11 Sep 2012 10:11:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TBRB2-0006A1-OR for qemu-devel@nongnu.org; Tue, 11 Sep 2012 10:11:02 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:33575) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBRB2-00069v-Fw for qemu-devel@nongnu.org; Tue, 11 Sep 2012 10:10:56 -0400 Date: Tue, 11 Sep 2012 10:10:51 -0400 (EDT) From: Paolo Bonzini Message-ID: <1930915706.67750616.1347372651767.JavaMail.root@redhat.com> In-Reply-To: <504F438E.1010601@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 21/47] block: add bdrv_ensure_backing_file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: jcody@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com ----- Messaggio originale ----- > Da: "Kevin Wolf" > A: "Paolo Bonzini" > Cc: qemu-devel@nongnu.org, eblake@redhat.com, jcody@redhat.com, stefanha@= linux.vnet.ibm.com > Inviato: Marted=C3=AC, 11 settembre 2012 15:58:38 > Oggetto: Re: [PATCH 21/47] block: add bdrv_ensure_backing_file >=20 > Am 11.09.2012 15:46, schrieb Paolo Bonzini: > >=20 > >> Once again, combining code motion and code changes in one patch > >> makes > >> it harder to review. > >=20 > > 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. > >=20 > > 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). >=20 > Yes, that's what I figured eventually. Maybe some documentation for > the function couldn't hurt. >=20 > >> 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? > >=20 > > 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. >=20 > 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_f= ile() 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] =3D=3D '\0') { > >>> + return 0; > >>> + } > >>> + > >>> + bs->backing_hd =3D bdrv_new(""); > >>> + bdrv_get_full_backing_filename(bs, backing_filename, > >>> + sizeof(backing_filename)); > >>> + > >>> + if (bs->backing_format[0] !=3D '\0') { > >>> + back_drv =3D bdrv_find_format(bs->backing_format); > >>> + } > >>> + > >>> + /* backing files always opened read-only */ > >>> + back_flags =3D bs->open_flags & ~(BDRV_O_RDWR | > >>> BDRV_O_SNAPSHOT); > >>> + > >>> + ret =3D 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. > >=20 > > It's a bug, it should have closed bs->backing_hd. >=20 > 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. > >=20 > > Ok. > >=20 > >>> + bs->backing_hd =3D NULL; > >>> + return ret; > >>> + } > >>> + if (bs->is_temporary) { > >>> + bs->backing_hd->keep_read_only =3D !(bs->open_flags & > >>> BDRV_O_RDWR); > >>> + } else { > >>> + /* base images use the same setting as leaf */ > >> > >> Huh, "parent" turned into "leaf"? > >=20 > > Will move this to a separate patch, too. >=20 > 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? >=20 > Kevin >=20