From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52626) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WxgSo-0006Ai-GQ for qemu-devel@nongnu.org; Thu, 19 Jun 2014 13:49:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WxgSh-0001SH-03 for qemu-devel@nongnu.org; Thu, 19 Jun 2014 13:49:30 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:51643 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WxgSg-0001RK-Mn for qemu-devel@nongnu.org; Thu, 19 Jun 2014 13:49:22 -0400 Date: Thu, 19 Jun 2014 19:49:20 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140619174920.GA17222@irqsave.net> References: <20140619091716.GS21236@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140619091716.GS21236@stefanha-thinkpad.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 for 2.1 00/10] Modify block jobs to use node-names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, benoit.canet@irqsave.net, pkrempa@redhat.com, famz@redhat.com, Jeff Cody , qemu-devel@nongnu.org, stefanha@redhat.com The Thursday 19 Jun 2014 =E0 17:17:16 (+0800), Stefan Hajnoczi wrote : > On Tue, Jun 17, 2014 at 05:53:48PM -0400, Jeff Cody wrote: > > Changes from v5->v6: > >=20 > > * Check for attempt to commit an image to itself (Eric) > > * Add a comment to the bdrv_find for block-commit, indicating > > that libvirt uses the error case for probing (Eric) > > * Added Benoit's R-b's > >=20 > > Changes from v4->v5: > >=20 > > * Rebased on master > > * Fixed commit log typos / stale paragraphs (Eric) > > * Fixed comment typo (Eric) > > * Added Eric's remaining R-b's > >=20 > >=20 > > Changes from v3->v4: > >=20 > > * Rebased on master > > * Dropped overlay pointers, Eric's concerns are correct > > * Require "device" for all arguments, in light of the above, > > so we can find the active layer in all cases. > > * Simplify more operations! > > * Dropped Eric's Reviewed-by: on patches 3,5,6 > > Added Eric's Reviewed-by: on patches 8,9 > >=20 > >=20 > > Changes from v2->v3: > >=20 > > * Add Eric's reviewed-by > > * Addressed Eric's review comments > > * Dropped HMP changes > > * Added helper function for setting the overlay, and > > set the overlay in bdrv_append() > > * Use bs->backing_file instead of bs->backing_hd->filename in block_s= tream=20 > >=20 > > Using node-names instead of filenames for block job operations > > over QMP is a superior method of identifying the block driver > > images to operate on, as it removes all pathname ambiguity. > >=20 > > This series modifies block-commit and block-stream to use node-names, > > and creates a new QAPI command to allow stand-alone backing file > > changes on an image file. > >=20 > > So that node-names can be used as desired for all block job > > operations, this series also auto-generates node-names for every > > BDS. User-specified node-names will override any autogenerated > >=20 > > Jeff Cody (10): > > block: Auto-generate node_names for each BDS entry > > block: add helper function to determine if a BDS is in a chain > > block: simplify bdrv_find_base() and bdrv_find_overlay() > > block: make 'top' argument to block-commit optional > > block: Accept node-name arguments for block-commit > > block: extend block-commit to accept a string for the backing file > > block: add ability for block-stream to use node-name > > block: add backing-file option to block-stream > > block: Add QMP documentation for block-stream > > block: add QAPI command to allow live backing file change > >=20 > > block.c | 80 ++++++++-------- > > block/commit.c | 9 +- > > block/stream.c | 11 +-- > > blockdev.c | 238 ++++++++++++++++++++++++++++++++++++= ++++++---- > > hmp.c | 1 + > > include/block/block.h | 4 +- > > include/block/block_int.h | 3 +- > > qapi/block-core.json | 145 +++++++++++++++++++++++++--- > > qmp-commands.hx | 181 +++++++++++++++++++++++++++++++++-- > > tests/qemu-iotests/040 | 28 ++++-- > > 10 files changed, 602 insertions(+), 98 deletions(-) >=20 > This series side-steps lack of child op blockers by checking only the > root node/drive. >=20 > Existing node-name commands like resize and snapshot-sync check for op > blockers on the actual node. They do not take the same approach as thi= s > patch series. >=20 > We have a mess and I don't want to commit this series before we've > figured out what to do about child op blockers. >=20 > Let's discuss this topic in a sub-thread and figure out what to do for > QEMU 2.1. This is an important issue to solve before the release > because we can't change QMP command semantics easily later. >=20 > My questions are: > a. How do we fix resize, snapshot-sync, etc? It seems like we need to > propagate child op blockers. >=20 > b. Is it a good idea to perform op blocker checks on the root node? > It's inconsistent with resize, snapshot-sync, etc. Permissions in > BDS graphs with multiple root nodes (e.g. guest device and NBD > run-time server) will be different depending on which root you > specify. >=20 > c. We're painting ourselves into a corner by using the root node for op > blocker checks. We'll have to apply the same op blockers to all > nodes in a graph. There's no opportunity to apply different op > blockers to a subset of the child nodes. I *think* this can be > changed later without affecting the QMP API, so it's not a critical > issue. >=20 > The answer seems to be that op blockers should be propagated to all > child nodes and commands should test the node, not the drive/root node. > That gives us the flexibility for per-node op blockers in the future an= d > maintains compatibility with existing node-name users. I tried to write a recursive function to block and unblock all child node= s today. It turn that even if the recursion functions are easy to write there are = some hurdle with the approach. In drive mirror when a subtree is swapped nobody will be here to unlock i= t and bdrv_unref will fail. In block-commmit the drive mirror run part form a BDS loop that is totall= y uneasy to unblock properly in bdrv_set_backing_file. So writing these functions is not trivial and we are late in the release = cycle. Are you sure that there is no lighter path for the quorum and node name p= atchset ? Best regards Beno=EEt >=20 > Stefan