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

[Wen Congyang]

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