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

[Max Reitz]

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.

> > 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

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