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.
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.
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.
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 <we...@cn.fujitsu.com>
---
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);
