Re: [Qemu-block] [Qemu-devel] [PATCH] refresh filename after the node is replaced

[Wen Congyang]

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 <address@hidden>
>>>>>> ---
>>>>>>    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);
>>>>>
>>>>>
>>> .
>>>
> 
> 
> .
>