[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add |
Date: |
Tue, 18 Jun 2013 16:11:40 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Tue, 06/18 09:51, Kevin Wolf wrote:
> Am 18.06.2013 um 09:00 hat Fam Zheng geschrieben:
> > On Tue, 06/18 08:32, Kevin Wolf wrote:
> > > Am 18.06.2013 um 05:58 hat Fam Zheng geschrieben:
> > > > On Mon, 06/17 17:12, Kevin Wolf wrote:
> > > > > Am 17.06.2013 um 16:46 hat Paolo Bonzini geschrieben:
> > > > > > Il 17/06/2013 16:26, Kevin Wolf ha scritto:
> > > > > > > Am 17.06.2013 um 16:01 hat Paolo Bonzini geschrieben:
> > > > > > >> Il 17/06/2013 15:52, Kevin Wolf ha scritto:
> > > > > > >>> It's not a new thought that we need to change the block layer
> > > > > > >>> so that a
> > > > > > >>> BlockDriverState can't be "empty", but that one
> > > > > > >>> BlockDriverState always
> > > > > > >>> refers to one image. If you change media, you attach a different
> > > > > > >>> BlockDriverState to the device. Once you have this, you can
> > > > > > >>> start
> > > > > > >>> refcounting BlockDriverStates, so that the backing file remains
> > > > > > >>> usable
> > > > > > >>> while the guest device already uses a different image.
> > > > > > >>>
> > > > > > >>> Not that it's it easy to get there...
> > > > > > >>
> > > > > > >> I'm not sure that is safe to do.
> > > > > > >>
> > > > > > >> Consider the case where the guest switches from A to B during
> > > > > > >> backup,
> > > > > > >> and then from B to A. You get two BDS for the same file, which
> > > > > > >> pretty
> > > > > > >> much means havoc.
> > > > > > >
> > > > > > > Well, yes, it means that the management tool needs to know what
> > > > > > > it's
> > > > > > > doing. It shouldn't create a second BDS for A, but reattach the
> > > > > > > still
> > > > > > > existing one.
In this case do you mean mgmt tool should give a name of drive instead
of file path? I like this idea, and further more, why don't we make QEMU
smarter to bdrv_find_by_filename() the existing BDS?
> > > > > >
> > > > > > How? That would require the management tool to know the full chain
> > > > > > of
> > > > > > BDSes that were opened in the past.
> > > > >
> > > > > They better know on which files they are operating. It's not like the
> > > > > management could be unaware of running backup jobs or things like
> > > > > that.
> > > > >
> > > >
> > > > Is there any case that QEMU needs to have two BDS pointing to the same
> > > > file?
> > >
> > > No, I think there's no case where this would make sense.
> > >
> > > > If not, can we try to detect such case on opening and try to
> > > > reuse the bs?
> > >
> > > We can't do it reliably, think about symlinks or even hard links, or
> > > things like /dev/fdset/..., let alone remote protocols that refer to the
> > > same image file etc.
> > >
> > > We can check the obvious cases and error out for them, but that's about
> > > what we can do. I don't think we should try to fix things automagically
> > > when we can't do it right.
> >
> > It's impossible to know a remote protocol points to the same image with
> > local file path, that's not in QEMU's scope, but we have a good chance
> > to detect (strcmp with existing bs->filename) and error out Paolo's
> > A-B-A problem, don't we?
>
> Yes, catching 50% of the misuses is better than catching none.
>
> My point was that we shouldn't "try to reuse the bs" when we detect that
> the file is already open, because that makes it a feature that users are
> supposed to use and that doesn't work consistently across backends and
> will therefore cause endless pain.
OK.
>
> If we detect it (in order to protect the user from his own mistakes), we
> must treat it as a misuse and return an error.
>
IIUC, block job is not supposed to affect the guest or the source image,
so from user's PoV, switching to another image, then switching back
seems reasonable, even when a block job runs in the background. As we
know it's already open, could we reattach to it instead, as you
suggested above?
--
Fam
- Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add, Fam Zheng, 2013/06/17
- Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add, Kevin Wolf, 2013/06/18
- Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add, Markus Armbruster, 2013/06/18
- Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add, Fam Zheng, 2013/06/18
- Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add, Markus Armbruster, 2013/06/19
- Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add, Fam Zheng, 2013/06/19
Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add, Markus Armbruster, 2013/06/18