refresh filename after the node is replaced
PATCH 1/1 refresh filename after the node is replaced
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.
--- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+)
block/mirror.c +3 -0
@@ -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);
Signature · 1 line
2.4.3
Quoted 2 lines · Wen Congyang
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 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
Quoted 19 lines
Signed-off-by Wen Congyang <[email protected]>--- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+)block/mirror.c +3
-0@@ -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);
At 2015/6/26 21:47, Max Reitz Wrote:
Quoted 7 lines · Wen Congyang
Quoted 4 lines
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.
Thanks
Wen Congyang
Quoted 30 lines
order to fix this, we need to call
bdrv_refresh_filename()afterbdrv_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
Quoted 19 lines
Signed-off-by Wen Congyang <[email protected]>--- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+)block/mirror.c +3
-0@@ -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);
Quoted 7 lines · Wen Congyang
At 2015/6/26 21:47, Max Reitz Wrote:
Quoted 6 lines · Wen Congyang
Quoted 4 lines
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.
Quoted 6 lines
Quoted 1 line
Which filename? The quorum filename is broken after this patch, too. In
In my test, quorum has two children,
s->common.bs->drvis quorum, ands->to_replaceis 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
Quoted 35 lines
Thanks
Wen CongyangQuoted 31 lines
order to fix this, we need to call
bdrv_refresh_filename()afterbdrv_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
Quoted 20 lines
Signed-off-by Wen Congyang <[email protected]>--- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+)block/mirror.c +3
-0@@ -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);
Quoted 33 lines · Max Reitz
Quoted 7 lines · Wen Congyang
At 2015/6/26 21:47, Max Reitz Wrote:
Quoted 6 lines · Wen Congyang
Quoted 4 lines
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.Quoted 6 lines
Quoted 1 line
Which filename? The quorum filename is broken after this patch, too. In
In my test, quorum has two children,
s->common.bs->drvis quorum, ands->to_replaceis 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"}]}
Quoted 3 lines
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.
Quoted 4 lines
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 callbdrv_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.
Quoted 4 lines
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
Quoted 40 lines
Max
Quoted 34 lines
Thanks
Wen CongyangQuoted 30 lines
order to fix this, we need to call
bdrv_refresh_filename()afterbdrv_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
Quoted 19 lines
Signed-off-by Wen Congyang <[email protected]>--- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+)block/mirror.c +3
-0@@ -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);.
Quoted 29 lines · Wen Congyang
Quoted 14 lines · Max Reitz
Quoted 11 lines · Wen Congyang
At 2015/6/26 21:47, Max Reitz Wrote:
Quoted 6 lines · Wen Congyang
Quoted 4 lines
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->drvis quorum, ands->to_replaceis 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:
Quoted 14 lines
$ 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": fals!
Quoted 7 lines
e,
Quoted 5 lines
"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 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 cannot query it with query-block.
Quoted 5 lines
Quoted 4 lines
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 callbdrv_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.
Indeed. Kevin's series adds a generalized parent-child relationship, which makes it possible to both enumerate all the children of a BDS and all its parents.
Quoted 4 lines
Quoted 3 lines
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.
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 applied yet and didn't receive further attention. Also, there was no real reason to do it other than "Because we can" until now. But I think the issue mentioned here is a good reason why we should indeed reconsider it.
Max
Quoted 39 lines
Thanks
Wen CongyangQuoted 36 lines
Max
Quoted 32 lines
Thanks
Wen CongyangQuoted 29 lines
order to fix this, we need to call
bdrv_refresh_filename()afterbdrv_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
Quoted 19 lines
Signed-off-by Wen Congyang <[email protected]>--- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+)block/mirror.c +3
-0@@ -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);.
Quoted 41 lines · Max Reitz
Quoted 29 lines · Wen Congyang
Quoted 14 lines · Max Reitz
Quoted 11 lines · Wen Congyang
At 2015/6/26 21:47, Max Reitz Wrote:
Quoted 6 lines · Wen Congyang
Quoted 4 lines
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->drvis quorum, ands->to_replaceis 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:
Quoted 14 lines
$ 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": fals!Quoted 7 lines
e,
Quoted 5 lines
"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 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 cannot query it with query-block.
Quoted 1 line
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
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 callbdrv_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.
Indeed. Kevin's series adds a generalized parent-child relationship, which makes it possible to both enumerate all the children of a BDS and all its parents.
How to get all its parents? Is this patch not merged into upstream?
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
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.
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 applied yet and didn't receive further attention. Also, there was no real reason to do it other than "Because we can" until now. But I think the issue mentioned here is a good reason why we should indeed reconsider it.
I don't find this series.
Thanks
Wen Congyang
Quoted 46 lines
Max
Quoted 39 lines
Thanks
Wen CongyangQuoted 36 lines
Max
Quoted 32 lines
Thanks
Wen CongyangQuoted 29 lines
order to fix this, we need to call
bdrv_refresh_filename()afterbdrv_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
Quoted 19 lines
Signed-off-by Wen Congyang <[email protected]>--- block/mirror.c | 3 +++ 1 file changed, 3 insertions(+)block/mirror.c +3
-0@@ -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);.
.
Quoted 9 lines · Wen Congyang
Quoted 7 lines · Max Reitz
Quoted 5 lines · Wen Congyang
Quoted 3 lines · Max Reitz
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 callbdrv_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.
Indeed. Kevin's series adds a generalized parent-child relationship, which makes it possible to both enumerate all the children of a BDS and all its parents.
How to get all its parents? Is this patch not merged into upstream?
The patch is actually already merged upstream (6e93e7c41fdfdee3068770cae79380e1d986b76a), but it does not add a way to enumerate a BDS's parents, only a BDS's children. It should not be too difficult to implement the reverse however, I guess, based on that patch. Konsole output
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
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.
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 applied yet and didn't receive further attention. Also, there was no real reason to do it other than "Because we can" until now. But I think the issue mentioned here is a good reason why we should indeed reconsider it.
I don't find this series.
Maybe because it's pretty old. :-) I sent it last September: http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04878.html
Max