On 06/30/2015 09:17 PM, Max Reitz wrote:
> 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 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": fals!
>> e,
>>> "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.
>
>>
>>> 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.
>
> 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?
>
>>> 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
>
> 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 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);
>>>>>
>>>>>
>>> .
>>>
>
>
> .
>
