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 <vsement...@virtuozzo.com>
> ---
> 
> 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

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to