Re: [Qemu-block] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Max Reitz]

On 01.07.2015 21:41, Alberto Garcia wrote:

On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz  wrote:


@@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd)
   bs->backing_blocker = NULL;
   goto out;
   }
+
+bdrv_attach_child(bs, backing_hd, &child_backing);
+backing_hd->inherits_from = bs;
+backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);


Do we really want this, unconditionally? ... After looking through the
code, I can't find a place where we wouldn't. It just seems strange to
have it here.


Yeah, I understand. In general I think that the API for handling
bs->children is rather unclear and I wanted to avoid that callers need
to call bdrv_set_backing_hd() and bdrv_attach/detach_child() separately.


Oh, sorry, I was unclear. The bdrv_attach_child() is fine, but I find 
unconditionally inheriting the flags from the backed BDS strange.



@@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState 
*parent_bs,
 BlockDriverState *child_bs,
 const BdrvChildRole *child_role)
   {
-BdrvChild *child = g_new(BdrvChild, 1);
+BdrvChild *child;
+
+/* Don't attach the child if it's already attached */
+QLIST_FOREACH(child, &parent_bs->children, next) {
+if (child->bs == child_bs) {
+return;
+}
+}


Hm, it may have been attached with a different role, though... I guess
that would be a bug, however. But if it's the same role, trying to
re-attach it seems wrong, too. So where could this happen?


The reason I'm doing this is because of bdrv_open_backing_file(). That
function attaches the backing file to the parent file twice: once in
bdrv_open_inherit() and the second time in bdrv_set_backing_hd().


Okay, that's fine then.


One alternative would be not to attach it in bdrv_set_backing_hd(), but
since that function is used in many other places we would have to add
new calls to bdrv_attach_child() everywhere.

That's one example of the situation I mentioned earlier: it seems
logical that bdrv_set_backing_hd() and bdrv_attach_child() go together,
but none of the solutions that came to my mind feels 100% right.


I think putting it into bdrv_set_backing_hd() is fine.

Still feeling a bit bad about overwriting the backing BDS's flags and 
making it inherit its flags from the backed BDS in 
bdrv_set_backing_hd(), but anyway:


Reviewed-by: Max Reitz 

(I do think it is fine and can't think of any better solution)

Re: [Qemu-block] [Qemu-devel] [PATCH COLO-BLOCK v7 00/17] Block replication for continuous checkpoints

[Wen Congyang]

At 2015/7/3 23:30, Dr. David Alan Gilbert Wrote:

* Wen Congyang (we...@cn.fujitsu.com) wrote:

Block replication is a very important feature which is used for
continuous checkpoints(for example: COLO).

Usage:
Please refer to docs/block-replication.txt

You can get the patch here:
https://github.com/wencongyang/qemu-colo/commits/block-replication-v7

You can get ths patch with framework here:
https://github.com/wencongyang/qemu-colo/commits/colo_framework_v7.2


Hi,
   I seem to be having problems with the new listed syntax on the wiki;
on the secondary I'm getting the error

  Block format 'replication' used by device 'virtio0' doesn't support the 
option 'export'

./try/bin/qemu-system-x86_64 -enable-kvm -nographic \
  -boot c -m 4096 -smp 4 -S \
  -name debug-threads=on -trace events=trace-file \
  -netdev tap,id=hn0,script=$PWD/ifup-slave,\
downscript=no,colo_script=$PWD/qemu/scripts/colo-proxy-script.sh,colo_nicname=em4
 \
  -device e1000,mac=9c:da:4d:1c:b5:89,id=net-pci0,netdev=hn0 \
  -device virtio-rng-pci \
  -drive 
if=none,driver=raw,file=/home/localvms/bugzilla.raw,id=colo1,cache=none,aio=native
 \
  -drive 
if=virtio,driver=replication,mode=secondary,export=colo1,throttling.bps-total-max=7000,\
file.file.filename=$TMPDISKS/colo-active-disk.qcow2,\
file.driver=qcow2,\
file.backing.file.filename=$TMPDISKS/colo-hidden-disk.qcow2,\
file.backing.driver=qcow2,\
file.backing.backing.backing_reference=colo1,\
file.backing.allow-write-backing-file=on \
  -incoming tcp:0:


Sorry, the option export is removed, because we use the qmp command 
nbd-server-add to let a BB be NBD server.




This is using 6fd6ce32 from the colo_framework_v7.2  tag.

What have I missed?

Dave
P.S. the name 'file.backing.backing.backing_reference' is not nice!


Which name is better? My native language is not English, so any such 
suggestion is welcome.


Thanks
Wen Congyang





TODO:
1. Continuous block replication. It will be started after basic functions
are accepted.

Changs Log:
V7:
1. Implement adding/removing quorum child. Remove the option non-connect.
2. Simplify the backing refrence option according to Stefan Hajnoczi's 
suggestion
V6:
1. Rebase to the newest qemu.
V5:
1. Address the comments from Gong Lei
2. Speed the failover up. The secondary vm can take over very quickly even
if there are too many I/O requests.
V4:
1. Introduce a new driver replication to avoid touch nbd and qcow2.
V3:
1: use error_setg() instead of error_set()
2. Add a new block job API
3. Active disk, hidden disk and nbd target uses the same AioContext
4. Add a testcase to test new hbitmap API
V2:
1. Redesign the secondary qemu(use image-fleecing)
2. Use Error objects to return error message
3. Address the comments from Max Reitz and Eric Blake


Wen Congyang (17):
   Add new block driver interface to add/delete a BDS's child
   quorum: implement block driver interfaces add/delete a BDS's child
   hmp: add monitor command to add/remove a child
   introduce a new API qemu_opts_absorb_qdict_by_index()
   quorum: allow ignoring child errors
   introduce a new API to enable/disable attach device model
   introduce a new API to check if blk is attached
   block: make bdrv_put_ref_bh_schedule() as a public API
   Backup: clear all bitmap when doing block checkpoint
   allow writing to the backing file
   Allow creating backup jobs when opening BDS
   block: Allow references for backing files
   docs: block replication's description
   Add new block driver interfaces to control block replication
   skip nbd_target when starting block replication
   quorum: implement block driver interfaces for block replication
   Implement new driver for block replication

  block.c| 198 +-
  block/Makefile.objs|   3 +-
  block/backup.c |  13 ++
  block/block-backend.c  |  33 +++
  block/quorum.c | 244 ++-
  block/replication.c| 443 +
  blockdev.c |  90 ++---
  blockjob.c |  10 +
  docs/block-replication.txt | 179 +
  hmp-commands.hx|  28 +++
  include/block/block.h  |  11 +
  include/block/block_int.h  |  19 ++
  include/block/blockjob.h   |  12 ++
  include/qemu/option.h  |   2 +
  include/sysemu/block-backend.h |   3 +
  include/sysemu/blockdev.h  |   2 +
  qapi/block.json|  16 ++
  util/qemu-option.c |  44 
  18 files changed, 1303 insertions(+), 47 deletions(-)
  create mode 100644 block/replication.c
  create mode 100644 docs/block-replication.txt

--
2.4.3


--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK