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

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

[Fam Zheng]

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?  If not, can we try to detect such case  on opening and try to
reuse the bs?

(Oops, CC'ing the right qemu-devel :p)

-- 
Fam

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

[Kevin Wolf]

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.

Kevin

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

[Fam Zheng]

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?

-- 
Fam

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

[Kevin Wolf]

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

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.

Kevin

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

[Fam Zheng]

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

[Kevin Wolf]

Am 18.06.2013 um 10:11 hat Fam Zheng geschrieben:
> 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?

This is none of the block layer's business. The management tool needs to
know this and reuse an existing BlockDriverState instead of creating a
new one. Everything else will lead to an inconsistent QMP API.

Not that "management tool" could in theory also mean the GTK GUI, so I'm
not totally excluding that qemu could be involved in this, but the block
layer is the wrong level to address this.

Kevin

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> 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?

If it's a local file, then comparing names to check for aliasing is
stupid.  Compare device & inode instead.

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

[Fam Zheng]

On Tue, 06/18 16:18, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> 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.

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

-- 
Fam

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Tue, 06/18 16:18, Markus Armbruster wrote:
>> Fam Zheng <famz@redhat.com> 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?

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.

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

[Fam Zheng]

On Wed, 06/19 08:27, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Tue, 06/18 16:18, Markus Armbruster wrote:
> >> Fam Zheng <famz@redhat.com> 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

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> 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?

Maybe, I don't know.

>        If not, can we try to detect such case  on opening

Gee, what a nice swamp you found there!

For local files, you can compare (dev-major, dev-minor, inode).

Beyond that, you tend not to get comparisons, but best guesses.

>                                                           and try to
> reuse the bs?

I doubt reusing the BDS is correct in the general case.

[...]

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

[Fam Zheng]

On Tue, 06/18 08:37, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > 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?
> 
> Maybe, I don't know.
> 
> >        If not, can we try to detect such case  on opening
> 
> Gee, what a nice swamp you found there!
> 
> For local files, you can compare (dev-major, dev-minor, inode).
> 
> Beyond that, you tend not to get comparisons, but best guesses.
> 
> >                                                           and try to
> > reuse the bs?
> 
> I doubt reusing the BDS is correct in the general case.
> 
Maybe I meant basically the same as Kevin, but just that QEMU finds out
A has existing BDS, and reattach it.
> >> > > 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.
I assume it should be less wrong than having two BDS for the same file.

-- 
Fam

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

[Paolo Bonzini]

Il 18/06/2013 08:37, Markus Armbruster ha scritto:
> >        If not, can we try to detect such case  on opening
> Gee, what a nice swamp you found there!

It is a huge swamp indeed.

Since we stop all block jobs on media change already, what about just
adding a command "block-job-attach" or something like that which exposes
the target of the job as a blockdev (presumably so that you can then add
it to the NBD server)?

Paolo

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

[Kevin Wolf]

Am 18.06.2013 um 10:40 hat Paolo Bonzini geschrieben:
> Il 18/06/2013 08:37, Markus Armbruster ha scritto:
> > >        If not, can we try to detect such case  on opening
> > Gee, what a nice swamp you found there!
> 
> It is a huge swamp indeed.
> 
> Since we stop all block jobs on media change already, what about just
> adding a command "block-job-attach" or something like that which exposes
> the target of the job as a blockdev (presumably so that you can then add
> it to the NBD server)?

This is backwards. You should really give the block job a target BDS to
begin with, not a file name.

Kevin

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

[Paolo Bonzini]

Il 18/06/2013 10:56, Kevin Wolf ha scritto:
>> > 
>> > It is a huge swamp indeed.
>> > 
>> > Since we stop all block jobs on media change already, what about just
>> > adding a command "block-job-attach" or something like that which exposes
>> > the target of the job as a blockdev (presumably so that you can then add
>> > it to the NBD server)?
> This is backwards. You should really give the block job a target BDS to
> begin with, not a file name.

That's also ok...  We can get the backing file right for the target BDS
at the beginning of the backup job with a combination of
close_unused_images (so that it is "as if" block-backup had opened the
file with BDRV_O_NO_BACKING), bdrv_append and bdrv_swap (append the
target to the VM's BDS, swap the two).

So why do we need this new option to drive_add?  The "drive"
ImageCreationMode, for which Fam has a patch too, should do.

Paolo

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

[Fam Zheng]

On Tue, 06/18 10:40, Paolo Bonzini wrote:
> Il 18/06/2013 08:37, Markus Armbruster ha scritto:
> > >        If not, can we try to detect such case  on opening
> > Gee, what a nice swamp you found there!
> 
> It is a huge swamp indeed.
> 
> Since we stop all block jobs on media change already, what about just
> adding a command "block-job-attach" or something like that which exposes
> the target of the job as a blockdev (presumably so that you can then add
> it to the NBD server)?
> 

In fact block job target BDS is already created when job started, so
it's rather about finding a way to "query" the target BDS name, or get
it as command return value, than attach a blockdev. Once we have the
device name, we can add it to the NBD server.

-- 
Fam