From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayjyH-0005HR-JM for qemu-devel@nongnu.org; Fri, 06 May 2016 13:55:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ayjy5-00078K-N1 for qemu-devel@nongnu.org; Fri, 06 May 2016 13:55:20 -0400 References: <20160429150057.GJ4350@noname.redhat.com> From: John Snow Message-ID: <572CDA5C.8010506@redhat.com> Date: Fri, 6 May 2016 13:54:36 -0400 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 05/11] block: allow block jobs in any arbitrary node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , Kevin Wolf Cc: qemu-block@nongnu.org, jcody@redhat.com, qemu-devel@nongnu.org, Max Reitz , Stefan Hajnoczi 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