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

[Kevin Wolf]

Am 06.05.2016 um 19:54 hat John Snow geschrieben:
> 
> 
> 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.

We already have a job->id, we just need to start using it not only for
events, but also for finding block jobs. The existing auto-generation
mechanism is a very simple one (just copy the device name), but it's
effective and produces unique IDs in our current environment where only
a single block job per device is allowed.

> 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 don't think that's actually necessary, legacy and new can be the same
thing. What would remain from the legacy model is the field name
"device" both in commands and returned objects, which would then be a
misnomer. But I'm not sure if a bad name is a good reason for
duplicating a field.

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

Sounds good.

Kevin