Re: [Qemu-devel] [PATCH] block: add 'backing' option to drive_add

[Fam Zheng]

On Wed, 06/19 08:27, Markus Armbruster wrote:
> Fam Zheng <address@hidden> writes:
> 
> > On Tue, 06/18 16:18, Markus Armbruster wrote:
> >> Fam Zheng <address@hidden> writes:
> >> 
> >> > 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.
> >> >> > > > 
> >> >> > > > 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?
> >> 
> >> Is comparing bs->filename always a good idea, or only if it's a local
> >> image file?
> >
> > It's never sufficient by comparing filename to tell if they are the
> > same, things can be tricky here, but in many cases it can be helpful,
> > both local and remote.
> 
> Let me rephrase my question.
> 
> We all understand that different bs->filename can alias the same
> resource (which is not necessarily a file).  This makes a "same
> resource" test based on bs->filename incomplete.
> 
> Does identical bs->filename *always* imply same resource?

No, I'm afraid we can't make too much assumption on this.

> 
> If yes, the test is correct but incomplete.  That can be useful.
> 
> If no, the test is incorrect and incomplete, thus useless.
> 
> >> If it's a local file, then comparing names to check for aliasing is
> >> stupid.  Compare device & inode instead.
> >
> > Device and inode is not something to block layer's knowledge, I think.
> 
> They are one stat(2) or fstat(2) away.
> 

-- 
Fam