Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf <kwolf@redhat.com>]

Am 17.03.2015 um 16:00 hat Alberto Garcia geschrieben:
> On Thu, Mar 12, 2015 at 04:45:17PM +0100, Kevin Wolf wrote:
> 
> > > One issue that I'm finding is that when we move the block-stream
> > > job to an intermediate node, where the device name is empty, we
> > > get messages like "Device '' is busy".
> > > 
> > > I can use node names instead, but they are also not guaranteed to
> > > exist.
> 
> > My first thought was "then make it 'Source/Target device is busy'
> > without mentioning any name". In the context of any given command,
> > it would still be clear which BDS is meant.
> 
> There's a related problem that I discussed on IRC with Kevin and Eric
> but that I think needs further deliberation.
> 
> The BlockJobInfo object returned by query-block-jobs identifies the
> owner of the job using the 'device' field. If jobs can be in any
> intermediate node then we cannot simply rely on the device name. We
> also cannot simply replace it with a node name because 1) it might not
> exist and 2) existing libvirt versions expect a device name.
> 
> So I see several alternatives:
> 
>    a) Add a new 'node-name' field to BlockJobInfo. It's simple,
>       'device' keeps the current semantics so we don't break
>       compatibility.
> 
>    b) Make 'device' return the device name as it currently does, or
>       the node name if it's not present. The main problem is that
>       libvirt cannot easily know what to expect. On the other hand
>       since both device and node-name share the same namespace the
>       returned value is not ambiguous.
> 
>    c) Make 'device' return the same name that was used when the job
>       was created. It's maybe simpler for libvirt than option b),
>       but it would require us to remember how the job was created,
>       possibly in the BlockJob structure. This is personally my least
>       favorite option.
> 
>    d) Create a new query command that returns a different data
>       structure.
> 
> I would opt for a) or b), but I'd like to hear if you have a different
> opinion.

e) Considering that we want to generalise block jobs into background
   jobs, make 'device' optional and fill it only if the owner BDS has a
   device name. If not, the field is omitted.

If e) is possible for libvirt (Eric?), I would vote for that. Otherwise,
I think b) would be nicest.

> Regarding the 'block-stream' command, I think the current option to
> reuse the 'device' parameter to refer to either a device or a node
> name is ok, so I'll go forward with that one.

Great!

> > On the other hand, having an owner BDS for a block job is considered
> > a mistake meanwhile because there is no clear rule which BDS to pick
> > when the job involves more than one.
> 
> Does it really matter as long as all the operations blockers are
> correctly set?

No, it doesn't actively break anything, it's just a little arbitrary.

> > In fact, without tying a job to a BDS, it could be just a background
> > job instead of specifically a block job.
> 
> I don't understand what you mean by this.

We could have other long-running operations in the background that could
make use of the same infrastructure if it weren't tied to block devices
(for no reason, the actual block job infrastructure doesn't need
anything block-like). One example that came up in the past is live
migration.

This is not directly related to your series, but some background to keep
in mind while evolving the external APIs.

Kevin