From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54938) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9Nd7-0000hA-U7 for qemu-devel@nongnu.org; Sun, 28 Jun 2015 21:13:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z9Nd6-0002G6-8i for qemu-devel@nongnu.org; Sun, 28 Jun 2015 21:13:01 -0400 Message-ID: <55909C7B.4000902@cn.fujitsu.com> Date: Mon, 29 Jun 2015 09:16:43 +0800 From: Wen Congyang MIME-Version: 1.0 References: <558BA2B5.3080107@cn.fujitsu.com> <558D57D4.1090609@redhat.com> <558D6136.2090904@gmail.com> <558D6CD3.6040007@redhat.com> In-Reply-To: <558D6CD3.6040007@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] refresh filename after the node is replaced List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , Wen Congyang , qemu-devl , Jeff Cody Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org, Stefan Hajnoczi On 06/26/2015 11:16 PM, Max Reitz wrote: > On 26.06.2015 16:27, Wen Congyang wrote: >> At 2015/6/26 21:47, Max Reitz Wrote: >>> On 25.06.2015 08:41, Wen Congyang wrote: >>>> We can use block job mirror to repair broken quorum files. But the >>>> command >>>> 'info block' doesn't output correct filename after block job mirror >>>> finishes. >>> >>> Which filename? The quorum filename is broken after this patch, too. In >> >> In my test, quorum has two children, s->common.bs->drv is quorum, and s->to_replace >> is one of his child. Without this patch, info block will output obsolete information. >> With this patch, the quorum's filename is right. I don't know why quorum filename >> is broken. > > Oh, yes, you are right, I forgot to execute block-job-complete. However, this patch relies on the Quorum BDS being the mirror source, which is the intentional use case but certainly not the only one: > > $ x86_64-softmmu/qemu-system-x86_64 -drive if=none,id=quorum,driver=quorum,children.0.file.filename=test1.qcow2,children.1.file.filename=test2.qcow2,children.1.node-name=ch1,vote-threshold=1 -drive if=none,id=drv,file=test2.qcow2 -qmp stdio > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 3, "major": 2}, "package": ""}, "capabilities": []}} > {'execute':'qmp_capabilities'} > {"return": {}} > {'execute':'drive-mirror','arguments':{'device':'drv','target':'tmp.qcow2','format':'qcow2','node-name':'nch1','replaces':'ch1','sync':'full'}} > Formatting 'tmp.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 > {"return": {}} > {"timestamp": {"seconds": 1435331363, "microseconds": 217707}, "event": "BLOCK_JOB_READY", "data": {"device": "drv", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} > {'execute':'block-job-complete','arguments':{'device':'drv'}} > {"return": {}} > {"timestamp": {"seconds": 1435331373, "microseconds": 152945}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} > {'execute':'query-block'} > {"return": [{"device": "quorum", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 67108864, "filename": "json:{\"children\": [{\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"test1.qcow2\"}}, {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"test2.qcow2\"}}], \"driver\": \"quorum\", \"blkverify\": false, \"rewrite-corrupted\": false, \"vote-threshold\": 1}", "format": "quorum"}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "quorum", "iops": 0, "bps_wr": 0, "write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "cache": {"no-flush": false, "direct": false, "writeback": true}, "file": "json:{\"children\": [{\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"test1.qcow2\"}}, {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"test2.qcow2\"}}], \"driver\": \"quorum\", \"blkverify\": false, > \"rewrite-corrupted\": false, \"vote-threshold\": 1}", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}, {"device": "drv", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 67108864, "filename": "test2.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": 200704, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false, "refcount-bits": 16, "corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "cache": {"no-flush": false, "direct": false, "writeback": true}, "file": "test2.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, > "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]} > > This patch makes mirror_exit() refresh the name of the block job's device (in this case "drv"), which is not necessarily a parent of the node being replaced. > Even if it were, imagine a configuration where there are two nested quorums, with the inner one being repaired using the "replaces" parameter for drive-mirror. > In this case, this patch would fix the inner Quorum's file name, but not the outer one's. If both inner and outer quorum are BBs, the outer one's is not fixed. > > I see two solutions to this issue: Either, we do it right and that means, if there is a change in the BDS graph (e.g. because of bdrv_swap()), > we have to call bdrv_refresh_filename() on all of the changed BDS's parents. In order to be able to enumerate a BDS's parents, we need Kevin's > series, as said before. IIUC, the BDS can have more than one parent. > > Or we revive my series "block: Drop BDS.filename" which drops the BDS.filename field completely and always rebuilds it if it is queried. > This would fix the issue as well, however, there is a reason it has been lying around for nine months now, and that reason is that we did > not yet fully examine the impacts of dropping that field, especially concerning libvirt. If BDS.filename is dropped, this problem will go. Thanks Wen Congyang > > Max > >> >> Thanks >> Wen Congyang >> >>> order to fix this, we need to call bdrv_refresh_filename() after >>> bdrv_swap() on all BDSs which reference one of the swapped BDSs. I think >>> this will not be reasonably possible until Kevin's "bdrv_reopen() >>> overhaul" series is merged which introduces a generic parent-child >>> relationship for BDSs. >>> >>> Max >>> >>>> Signed-off-by: Wen Congyang >>>> --- >>>> block/mirror.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/block/mirror.c b/block/mirror.c >>>> index 8aa2b21..2ca2c21 100644 >>>> --- a/block/mirror.c >>>> +++ b/block/mirror.c >>>> @@ -351,6 +351,9 @@ static void mirror_exit(BlockJob *job, void *opaque) >>>> bdrv_set_backing_hd(s->base, NULL); >>>> bdrv_unref(p); >>>> } >>>> + if (s->to_replace != s->common.bs) { >>>> + bdrv_refresh_filename(s->common.bs); >>>> + } >>>> } >>>> if (s->to_replace) { >>>> bdrv_op_unblock_all(s->to_replace, s->replace_blocker); >>> >>> >>> > > . >