Re: [Qemu-devel] [PATCH v9 05/11] block: allow block jobs in any arbitrary node

[John Snow <jsnow@redhat.com>]

On 05/06/2016 06:00 AM, Alberto Garcia wrote:
> On Fri 29 Apr 2016 05:00:57 PM CEST, Kevin Wolf wrote:
> 
>>> - Block jobs can now be identified by the node name of their
>>> BlockDriverState in addition to the device name. Since both device
>>> and node names live in the same namespace there's no ambiguity.
>>
>> Careful, we know this is a part of our API that we have already messed
>> up and we don't want to make things worse while adding new things
>> before we've cleaned it up.
>>
>> Let's keep in mind where we are planning to go with block jobs: They
>> should become background jobs, not tied to any block device. The close
>> connection to a single BDS already doesn't make a lot of sense today
>> because most block jobs involve at least two BDSes.
>>
>> In the final state, we will have a job ID that uniquely identifies the
>> job, and each command that starts a job will take an ID from the
>> client.  For compatibility, we'll use the block device name as the job
>> ID when using old commands that don't get an explicit ID yet.
>>
>> In the existing qemu version, you can't start two block jobs on the
>> same device, and in future versions, you're supposed to specify an ID
>> each time. This is why the default can always be supposed to work
>> without conflicts. If in newer versions, the client mixes both ways
>> (which it shouldn't do), starting a new block job may fail because the
>> device name is already in use as an ID for another job.
>>
>> Now we can probably make the same argument for node names, so we can
>> extend the interface and still keep it compatible.
>>
>> Where we need to be careful is that with device names and node names,
>> we have potentially two names describing the same BDS and therefore
>> the same job. This must not happen, because we won't be able to
>> represent that in the generic background job API. Any job has just a
>> single ID there.
> 
> Ok, what can be done in this case is to keep the name that the client
> used when the job was created.
> 
> block-stream virti0   <-- job id is 'virtio0'
> block-stream node5    <-- job id is 'node5'
> 
> In this case it wouldn't matter if 'node5' is the one attached to
> 'virtio0'.
> 
>>> @@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
>>>  
>>>      *aio_context = NULL;
>>>  
>>> -    blk = blk_by_name(device);
>>> -    if (!blk) {
>>> +    bs = bdrv_lookup_bs(device_or_node, device_or_node, errp);
>>
>> Specifically, this one is bad. It allows two different ways to specify
>> the same job.
> 
> I think we can simply iterate over all block jobs (now that we have a
> function to do that) and return the one with the matching ID.
> 
> Berto
> 

I think it's time to add a proper ID field to Block Jobs. Currently, we
just use the node-name as the ID, but as you are aware this is a poor
mechanism for fetching the job.

I'd really like to avoid continue using any kind of node-name or
block/device-name for job IDs, and instead start using either a
user-provided value or a QEMU auto-generated one.

Then, for legacy support, we'd have an "id" field (that's the new proper
globally unique ID) and an old legacy "node" field or similar.

Then, for events/etc we can report two things back:

(1) Legacy: the name of the node we were created under. This is like it
works currently, and it should keep libvirt happy.
(2) New: the proper, real ID that all management utilities should begin
using in the future.

I've got some patches that work towards this angle, but they're
currently intermingled with a bunch of experimental job re-factoring
stuff. If you give me a few days I can submit a proposal series to re-do
the job ID situation.

--js