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 <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);
>>>
>>>
>>>
>
> .
>
