Re: [Qemu-devel] [PATCH] blockdev-backup: enable non-root nodes for backup
On 2017-11-09 15:16, Vladimir Sementsov-Ogievskiy wrote: > This is needed to implement image-fleecing scheme, when we create > a temporary node, mark our active node to be backing for the temp, > and start backup(sync=none) from active node to the temp node. > Temp node then represents a kind of snapshot and may be used > for external backup through NBD. > > Signed-off-by: Vladimir Sementsov-Ogievskiy> --- > > What was the reason to abandon non-root nodes? > > blockdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/blockdev.c b/blockdev.c > index 56a6b24a0b..d0a5a107f0 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3345,7 +3345,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, > BlockJobTxn *txn, > backup->compress = false; > } > > -bs = qmp_get_root_bs(backup->device, errp); > +bs = bdrv_lookup_bs(backup->device, backup->device, errp); > if (!bs) { > return NULL; > } I can't find anything wrong with it, but it will need some more changes. One is the comment in bdrv_backing_attach() that unsets the BLOCK_OP_TYPE_BACKUP_SOURCE blocker which claims that it's safe to do so because a backing file can never be a backup source anyway. Question here: Why did that seem to be important? Other places to look into are all callers of bdrv_op_block_all() that do not unblock BLOCK_OP_TYPE_BACKUP_SOURCE afterwards (i.e. all but bdrv_backing_attach()). I suspect this is because they were not sure whether they could guarantee consistent data... But does that extend to their children? (Because the exact node that is blocked will still be seen as blocked by the backup job, so that is safe.) This wouldn't give us any data corruption, though, because the user just gets garbage data in their backup -- their fault for using a garbage node, I'd argue. Maybe it doesn't matter at all. If you really want to you can probably already attach some raw node to any node you want to backup and hey, that's a root node, so congratulations, you can run blockdev-backup on it. Well, I mean, you can run it but it won't do you any good because the before_write notifiers don't work then. (Same for mirror, by the way -- the dirty bitmap of that raw node wouldn't get set.) ((By the way, I don't suppose that's how it should work... But I don't suppose that we want propagation of dirtying towards the BDS roots, do we? :-/)) Finally, there is the BlockdevBackup and DriveBackup QAPI documentation that would each need one "root" removed. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] blockdev-backup: enable non-root nodes for backup
Am 09.11.2017 um 17:33 hat Eric Blake geschrieben: > On 11/09/2017 08:16 AM, Vladimir Sementsov-Ogievskiy wrote: > > This is needed to implement image-fleecing scheme, when we create > > a temporary node, mark our active node to be backing for the temp, > > and start backup(sync=none) from active node to the temp node. > > Temp node then represents a kind of snapshot and may be used > > for external backup through NBD. > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy> > --- > > > > What was the reason to abandon non-root nodes? > > I think the original restriction was that we didn't know all the > implications to having multiple readers to an intermediate node, so it > was easier to prevent it with plans to add it later than to add it up > front and deal with the fallout. But I think that now we are > sufficiently versed in handling BDS trees with multiple readers, with > proper op-blocking in place; so you are right that we can probably > support it now. Op blockers are actually the reason why I'm not so sure. The old blockers often still only work on the top level, and we haven't fully replaced them yet. In particular, graph modifications might not be protected well enough by the new system yet. But then, backup works only on a single node in the source tree, not on a whole subchain, so I don't see what other operation could conflict with a backup job. The only thing is resizes, but that is covered by the new system. So in the end, I think this specific change should be okay. > However, I'm a bit worried that there is no documentation change about > this in a .json QAPI file, nor any easy way to introspect via QMP > whether a particular qemu implementation supports this (short of trying > it and seeing whether it works). I'm also thinking that this is 2.12 > material, unless we can prove it is fixing a bug for 2.11 that was not > previously present. Sounds like another use case for the capability annotations that Markus wants to introduce for commands in the QAPI schema. Kevin signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] blockdev-backup: enable non-root nodes for backup
On 11/09/2017 08:16 AM, Vladimir Sementsov-Ogievskiy wrote: > This is needed to implement image-fleecing scheme, when we create > a temporary node, mark our active node to be backing for the temp, > and start backup(sync=none) from active node to the temp node. > Temp node then represents a kind of snapshot and may be used > for external backup through NBD. > > Signed-off-by: Vladimir Sementsov-Ogievskiy> --- > > What was the reason to abandon non-root nodes? I think the original restriction was that we didn't know all the implications to having multiple readers to an intermediate node, so it was easier to prevent it with plans to add it later than to add it up front and deal with the fallout. But I think that now we are sufficiently versed in handling BDS trees with multiple readers, with proper op-blocking in place; so you are right that we can probably support it now. However, I'm a bit worried that there is no documentation change about this in a .json QAPI file, nor any easy way to introspect via QMP whether a particular qemu implementation supports this (short of trying it and seeing whether it works). I'm also thinking that this is 2.12 material, unless we can prove it is fixing a bug for 2.11 that was not previously present. > > blockdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/blockdev.c b/blockdev.c > index 56a6b24a0b..d0a5a107f0 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3345,7 +3345,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, > BlockJobTxn *txn, > backup->compress = false; > } > > -bs = qmp_get_root_bs(backup->device, errp); > +bs = bdrv_lookup_bs(backup->device, backup->device, errp); > if (!bs) { > return NULL; > } > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH] blockdev-backup: enable non-root nodes for backup
This is needed to implement image-fleecing scheme, when we create a temporary node, mark our active node to be backing for the temp, and start backup(sync=none) from active node to the temp node. Temp node then represents a kind of snapshot and may be used for external backup through NBD. Signed-off-by: Vladimir Sementsov-Ogievskiy--- What was the reason to abandon non-root nodes? blockdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index 56a6b24a0b..d0a5a107f0 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3345,7 +3345,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, backup->compress = false; } -bs = qmp_get_root_bs(backup->device, errp); +bs = bdrv_lookup_bs(backup->device, backup->device, errp); if (!bs) { return NULL; } -- 2.11.1