From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40456) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9vQD-0001YJ-7d for qemu-devel@nongnu.org; Tue, 30 Jun 2015 09:18:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z9vQ6-0001Go-SM for qemu-devel@nongnu.org; Tue, 30 Jun 2015 09:17:57 -0400 References: <558BA2B5.3080107@cn.fujitsu.com> <558D57D4.1090609@redhat.com> <558D6136.2090904@gmail.com> <558D6CD3.6040007@redhat.com> <55909C7B.4000902@cn.fujitsu.com> From: Max Reitz Message-ID: <559296F2.1050009@redhat.com> Date: Tue, 30 Jun 2015 15:17:38 +0200 MIME-Version: 1.0 In-Reply-To: <55909C7B.4000902@cn.fujitsu.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Wen Congyang , Wen Congyang , qemu-devl , Jeff Cody Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org, Stefan Hajnoczi On 29.06.2015 03:16, Wen Congyang wrote: > 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 obsol= ete information. >>> With this patch, the quorum's filename is right. I don't know why quo= rum filename >>> is broken. >> Oh, yes, you are right, I forgot to execute block-job-complete. Howeve= r, 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=3Dnone,id=3Dquorum,drive= r=3Dquorum,children.0.file.filename=3Dtest1.qcow2,children.1.file.filenam= e=3Dtest2.qcow2,children.1.node-name=3Dch1,vote-threshold=3D1 -drive if=3D= none,id=3Ddrv,file=3Dtest2.qcow2 -qmp stdio >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 3, "major": 2}, "p= ackage": ""}, "capabilities": []}} >> {'execute':'qmp_capabilities'} >> {"return": {}} >> {'execute':'drive-mirror','arguments':{'device':'drv','target':'tmp.qc= ow2','format':'qcow2','node-name':'nch1','replaces':'ch1','sync':'full'}}= >> Formatting 'tmp.qcow2', fmt=3Dqcow2 size=3D67108864 encryption=3Doff c= luster_size=3D65536 lazy_refcounts=3Doff refcount_bits=3D16 >> {"return": {}} >> {"timestamp": {"seconds": 1435331363, "microseconds": 217707}, "event"= : "BLOCK_JOB_READY", "data": {"device": "drv", "len": 0, "offset": 0, "sp= eed": 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.qco= w2\"}}], \"driver\": \"quorum\", \"blkverify\": false, \"rewrite-corrupte= d\": 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, "ca= che": {"no-flush": false, "direct": false, "writeback": true}, "file": "j= son:{\"children\": [{\"driver\": \"qcow2\", \"file\": {\"driver\": \"file= \", \"filename\": \"test1.qcow2\"}}, {\"driver\": \"qcow2\", \"file\": {\= "driver\": \"file\", \"filename\": \"test2.qcow2\"}}], \"driver\": \"quor= um\", \"blkverify\": false, >> \"rewrite-corrupted\": false, \"vote-threshold\": 1}", "encryption_key= _missing": false}, "tray_open": false, "type": "unknown"}, {"device": "dr= v", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detec= t_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-refc= ounts": false, "refcount-bits": 16, "corrupt": false}}, "dirty-flag": fal= se}, "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_o= pen": false, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0"= , "locked": false, "removable": true, "tray_open": false, "type": "unknow= n"}, {"device": "floppy0", "locked": fals! > e, >> "removable": true, "tray_open": false, "type": "unknown"}, {"device": = "sd0", "locked": false, "removable": true, "tray_open": false, "type": "u= nknown"}]} >> >> This patch makes mirror_exit() refresh the name of the block job's dev= ice (in this case "drv"), which is not necessarily a parent of the node b= eing replaced. >> Even if it were, imagine a configuration where there are two nested qu= orums, with the inner one being repaired using the "replaces" parameter f= or drive-mirror. >> In this case, this patch would fix the inner Quorum's file name, but n= ot the outer one's. > If both inner and outer quorum are BBs, the outer one's is not fixed. I think this is independent of whether which Quorum has a BB attached to = it; it's just that unless there is a BB at the outer Quorum BDS, you=20 cannot query it with query-block. > >> I see two solutions to this issue: Either, we do it right and that mea= ns, 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 pa= rents. 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. Indeed. Kevin's series adds a generalized parent-child relationship,=20 which makes it possible to both enumerate all the children of a BDS and=20 all its parents. >> 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 be= en lying around for nine months now, and that reason is that we did >> not yet fully examine the impacts of dropping that field, especially c= oncerning libvirt. > If BDS.filename is dropped, this problem will go. Right. But we have to make sure there won't be any negative impact if we = do so, and because that is not really trivial, the series hasn't been=20 applied yet and didn't receive further attention. Also, there was no=20 real reason to do it other than "Because we can" until now. But I think=20 the issue mentioned here is a good reason why we should indeed=20 reconsider it. Max > 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 t= hink >>>> 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 *op= aque) >>>>> bdrv_set_backing_hd(s->base, NULL); >>>>> bdrv_unref(p); >>>>> } >>>>> + if (s->to_replace !=3D s->common.bs) { >>>>> + bdrv_refresh_filename(s->common.bs); >>>>> + } >>>>> } >>>>> if (s->to_replace) { >>>>> bdrv_op_unblock_all(s->to_replace, s->replace_blocker); >>>> >>>> >> . >>