[Qemu-devel] [RFC PATCH 0/3] Point-in-time snapshot exporting with drive-backup

[Qemu-devel] [RFC PATCH 0/3] Point-in-time snapshot exporting with drive-backup

[Fam Zheng]

This series adds partial support for point-in-time snapshot NBD exporting based
on drive-backup. The ideas is described below and patches followed (the missing
part is item 3, which work is in progress by Ian Main and will have another
patch on it). As the work does not overlap, these series should be quite
reviewable by itself.

Background
==========

The goal of image fleecing is to provide a interface to inspect a point-in-time
snapshot of guest image data, not being interfered with guest overwrites after
it's created.  With drive-backup we already have the point-in-time snapshot
image (the target image), we only need three modifications to realize this:

 1. Give backup target an id, so we can add it to NBD server.

 2. Assign source device as backing of target, so reading the unallocated will
    be passed to source.
    As there's copy-on-write mechanism with drive-backup job, all the modified
    data after snapshot created is copied to target, the unallocated data is
    guaranteed to be unchanged, so reading from the source is correct. Note
    that this requires target format supports backing file.

 3. Adding sync mode 'none' to drive-backup, so the block job only copy changed
    data from source, which has minimal IO overhead.

Usage
=====

With above three, we can simply export a point-in-time snapshot with two QMP commands:

    drive-backup device=virtio0 format=qcow2 target=point-in-time.qcow2 target-id=pit0 sync=none

    nbd-server-add device=pit0 writable=no

Lifecycle
=========

The block job originally had total control over target bs: when the job
completes, the target is deleted. Now it's shared with NBD, so we should make
sure the job completion or canceling wouldn't crash NBD server. This patch
doesn't handle this case: if the block job is ended before NBD exporting,
there'll be problem.

To avoid this, we either add more delicate condition checks as we do with
bs->in_use, or introduce reference count to BlockDriverState, which seems much
better in long term but needs more work. I have done some work on it, but it's
not ready yet. As we don't depend on it that heavily, let's do the things one
by one.

Any comments?

Fam Zheng (3):
  block: add target-id option to drive-backup QMP command
  block: assign backing relationship in drive-backup
  nbd: don't get ref if bs has no drive

 blockdev-nbd.c   | 6 +++++-
 blockdev.c       | 8 +++++++-
 qapi-schema.json | 7 +++++--
 qmp-commands.hx  | 3 ++-
 4 files changed, 19 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Fam Zheng]

Add target-id (optional) to drive-backup command, to make the target bs
a named drive so that we can operate on it (e.g. export with NBD).

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 4 +++-
 qapi-schema.json | 7 +++++--
 qmp-commands.hx  | 3 ++-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b3a57e0..5e694f3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -935,6 +935,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
     backup = common->action->drive_backup;
 
     qmp_drive_backup(backup->device, backup->target,
+                     backup->has_target_id, backup->target_id,
                      backup->has_format, backup->format,
                      backup->has_mode, backup->mode,
                      backup->has_speed, backup->speed,
@@ -1420,6 +1421,7 @@ void qmp_block_commit(const char *device,
 }
 
 void qmp_drive_backup(const char *device, const char *target,
+                      bool has_target_id, const char *target_id,
                       bool has_format, const char *format,
                       bool has_mode, enum NewImageMode mode,
                       bool has_speed, int64_t speed,
@@ -1494,7 +1496,7 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    target_bs = bdrv_new("");
+    target_bs = bdrv_new(has_target_id ? target_id : "");
     ret = bdrv_open(target_bs, target, NULL, flags, drv);
     if (ret < 0) {
         bdrv_delete(target_bs);
diff --git a/qapi-schema.json b/qapi-schema.json
index 30b1edb..abd29c3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1646,7 +1646,8 @@
 # Since: 1.6
 ##
 { 'type': 'DriveBackup',
-  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
+  'data': { 'device': 'str', 'target': 'str',
+            '*target-id': 'str', '*format': 'str',
             '*mode': 'NewImageMode', '*speed': 'int',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
@@ -1799,6 +1800,7 @@
 #          is a device, the existing file/device will be used as the new
 #          destination.  If it does not exist, a new file will be created.
 #
+# @target-id: #optional the drive id of the target.
 # @format: #optional the format of the new destination, default is to
 #          probe if @mode is 'existing', else the format of the source
 #
@@ -1825,7 +1827,8 @@
 # Since 1.6
 ##
 { 'command': 'drive-backup',
-  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
+  'data': { 'device': 'str', 'target': 'str',
+            '*target-id': 'str', '*format': 'str',
             '*mode': 'NewImageMode', '*speed': 'int',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 362f0e1..c90e132 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -913,7 +913,7 @@ EQMP
 
     {
         .name       = "drive-backup",
-        .args_type  = "device:B,target:s,speed:i?,mode:s?,format:s?,"
+        .args_type  = "device:B,target:s,target-id:s?,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_drive_backup,
     },
@@ -936,6 +936,7 @@ Arguments:
             device, the existing file/device will be used as the new
             destination.  If it does not exist, a new file will be created.
             (json-string)
+- "target_id": the drive id of the target image.
 - "format": the format of the new destination, default is to probe if 'mode' is
             'existing', else the format of the source
             (json-string, optional)
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Stefan Hajnoczi]

On Wed, Jun 26, 2013 at 11:59:19AM +0800, Fam Zheng wrote:
> Add target-id (optional) to drive-backup command, to make the target bs
> a named drive so that we can operate on it (e.g. export with NBD).
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c       | 4 +++-
>  qapi-schema.json | 7 +++++--
>  qmp-commands.hx  | 3 ++-
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b3a57e0..5e694f3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -935,6 +935,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>      backup = common->action->drive_backup;
>  
>      qmp_drive_backup(backup->device, backup->target,
> +                     backup->has_target_id, backup->target_id,
>                       backup->has_format, backup->format,
>                       backup->has_mode, backup->mode,
>                       backup->has_speed, backup->speed,
> @@ -1420,6 +1421,7 @@ void qmp_block_commit(const char *device,
>  }
>  
>  void qmp_drive_backup(const char *device, const char *target,
> +                      bool has_target_id, const char *target_id,
>                        bool has_format, const char *format,
>                        bool has_mode, enum NewImageMode mode,
>                        bool has_speed, int64_t speed,
> @@ -1494,7 +1496,7 @@ void qmp_drive_backup(const char *device, const char *target,
>          return;
>      }
>  
> -    target_bs = bdrv_new("");
> +    target_bs = bdrv_new(has_target_id ? target_id : "");

This raises a new issue:

Now that the target can be named, what happens when the user issues a
monitor command, e.g. drive-del, block-resize, or drive-backup :)?

We have a clumsy form of protection with bdrv_set_in_use().  It makes
several monitor commands refuse with -EBUSY.

Perhaps we should have a command permission set so it's possible to
allow/deny specific commands.

Stefan

Re: [Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Fam Zheng]

On Thu, 06/27 10:15, Stefan Hajnoczi wrote:
> On Wed, Jun 26, 2013 at 11:59:19AM +0800, Fam Zheng wrote:
> > Add target-id (optional) to drive-backup command, to make the target bs
> > a named drive so that we can operate on it (e.g. export with NBD).
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c       | 4 +++-
> >  qapi-schema.json | 7 +++++--
> >  qmp-commands.hx  | 3 ++-
> >  3 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index b3a57e0..5e694f3 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -935,6 +935,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
> >      backup = common->action->drive_backup;
> >  
> >      qmp_drive_backup(backup->device, backup->target,
> > +                     backup->has_target_id, backup->target_id,
> >                       backup->has_format, backup->format,
> >                       backup->has_mode, backup->mode,
> >                       backup->has_speed, backup->speed,
> > @@ -1420,6 +1421,7 @@ void qmp_block_commit(const char *device,
> >  }
> >  
> >  void qmp_drive_backup(const char *device, const char *target,
> > +                      bool has_target_id, const char *target_id,
> >                        bool has_format, const char *format,
> >                        bool has_mode, enum NewImageMode mode,
> >                        bool has_speed, int64_t speed,
> > @@ -1494,7 +1496,7 @@ void qmp_drive_backup(const char *device, const char *target,
> >          return;
> >      }
> >  
> > -    target_bs = bdrv_new("");
> > +    target_bs = bdrv_new(has_target_id ? target_id : "");
> 
> This raises a new issue:
> 
> Now that the target can be named, what happens when the user issues a
> monitor command, e.g. drive-del, block-resize, or drive-backup :)?
> 
> We have a clumsy form of protection with bdrv_set_in_use().  It makes
> several monitor commands refuse with -EBUSY.
> 
> Perhaps we should have a command permission set so it's possible to
> allow/deny specific commands.
> 

Yes, this makes me realize that ref count it not a solution to retire
bs->in_use, because we can't tell if drive-del or block-resize is safe
with only reference number. But I can't think of two situations to deny
different subsets of commands, shouldn't a general blocker, like in_use
does, be good enough?

-- 
Fam

Re: [Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Paolo Bonzini]

Il 27/06/2013 11:41, Fam Zheng ha scritto:
> On Thu, 06/27 10:15, Stefan Hajnoczi wrote:
>> On Wed, Jun 26, 2013 at 11:59:19AM +0800, Fam Zheng wrote:
>>> Add target-id (optional) to drive-backup command, to make the target bs
>>> a named drive so that we can operate on it (e.g. export with NBD).
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  blockdev.c       | 4 +++-
>>>  qapi-schema.json | 7 +++++--
>>>  qmp-commands.hx  | 3 ++-
>>>  3 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index b3a57e0..5e694f3 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -935,6 +935,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>>>      backup = common->action->drive_backup;
>>>  
>>>      qmp_drive_backup(backup->device, backup->target,
>>> +                     backup->has_target_id, backup->target_id,
>>>                       backup->has_format, backup->format,
>>>                       backup->has_mode, backup->mode,
>>>                       backup->has_speed, backup->speed,
>>> @@ -1420,6 +1421,7 @@ void qmp_block_commit(const char *device,
>>>  }
>>>  
>>>  void qmp_drive_backup(const char *device, const char *target,
>>> +                      bool has_target_id, const char *target_id,
>>>                        bool has_format, const char *format,
>>>                        bool has_mode, enum NewImageMode mode,
>>>                        bool has_speed, int64_t speed,
>>> @@ -1494,7 +1496,7 @@ void qmp_drive_backup(const char *device, const char *target,
>>>          return;
>>>      }
>>>  
>>> -    target_bs = bdrv_new("");
>>> +    target_bs = bdrv_new(has_target_id ? target_id : "");
>>
>> This raises a new issue:
>>
>> Now that the target can be named, what happens when the user issues a
>> monitor command, e.g. drive-del, block-resize, or drive-backup :)?
>>
>> We have a clumsy form of protection with bdrv_set_in_use().  It makes
>> several monitor commands refuse with -EBUSY.
>>
>> Perhaps we should have a command permission set so it's possible to
>> allow/deny specific commands.
>>
> 
> Yes, this makes me realize that ref count it not a solution to retire
> bs->in_use, because we can't tell if drive-del or block-resize is safe
> with only reference number. But I can't think of two situations to deny
> different subsets of commands, shouldn't a general blocker, like in_use
> does, be good enough?

For example, right now nbd-server-add does not check bdrv_in_use.  But
shrinking a device that is exposed via NBD could be surprising to the
NBD clients.

Paolo

Re: [Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Fam Zheng]

On Thu, 06/27 12:57, Paolo Bonzini wrote:
> Il 27/06/2013 11:41, Fam Zheng ha scritto:
> > On Thu, 06/27 10:15, Stefan Hajnoczi wrote:
> >> On Wed, Jun 26, 2013 at 11:59:19AM +0800, Fam Zheng wrote:
> >>> Add target-id (optional) to drive-backup command, to make the target bs
> >>> a named drive so that we can operate on it (e.g. export with NBD).
> >>>
> >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>> ---
> >>>  blockdev.c       | 4 +++-
> >>>  qapi-schema.json | 7 +++++--
> >>>  qmp-commands.hx  | 3 ++-
> >>>  3 files changed, 10 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/blockdev.c b/blockdev.c
> >>> index b3a57e0..5e694f3 100644
> >>> --- a/blockdev.c
> >>> +++ b/blockdev.c
> >>> @@ -935,6 +935,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
> >>>      backup = common->action->drive_backup;
> >>>  
> >>>      qmp_drive_backup(backup->device, backup->target,
> >>> +                     backup->has_target_id, backup->target_id,
> >>>                       backup->has_format, backup->format,
> >>>                       backup->has_mode, backup->mode,
> >>>                       backup->has_speed, backup->speed,
> >>> @@ -1420,6 +1421,7 @@ void qmp_block_commit(const char *device,
> >>>  }
> >>>  
> >>>  void qmp_drive_backup(const char *device, const char *target,
> >>> +                      bool has_target_id, const char *target_id,
> >>>                        bool has_format, const char *format,
> >>>                        bool has_mode, enum NewImageMode mode,
> >>>                        bool has_speed, int64_t speed,
> >>> @@ -1494,7 +1496,7 @@ void qmp_drive_backup(const char *device, const char *target,
> >>>          return;
> >>>      }
> >>>  
> >>> -    target_bs = bdrv_new("");
> >>> +    target_bs = bdrv_new(has_target_id ? target_id : "");
> >>
> >> This raises a new issue:
> >>
> >> Now that the target can be named, what happens when the user issues a
> >> monitor command, e.g. drive-del, block-resize, or drive-backup :)?
> >>
> >> We have a clumsy form of protection with bdrv_set_in_use().  It makes
> >> several monitor commands refuse with -EBUSY.
> >>
> >> Perhaps we should have a command permission set so it's possible to
> >> allow/deny specific commands.
> >>
> > 
> > Yes, this makes me realize that ref count it not a solution to retire
> > bs->in_use, because we can't tell if drive-del or block-resize is safe
> > with only reference number. But I can't think of two situations to deny
> > different subsets of commands, shouldn't a general blocker, like in_use
> > does, be good enough?
> 
> For example, right now nbd-server-add does not check bdrv_in_use.  But
> shrinking a device that is exposed via NBD could be surprising to the
> NBD clients.
> 

So it seems to me that both block job and nbd server have the same
restriction on device: don't resize, and notify on close. So my question
is if we implement bdrv_add_command_blocker(), do the callers still need to distinguish what actions to block, or it's generally to block all the actions those change the device parameter?

-- 
Fam

Re: [Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Paolo Bonzini]

Il 27/06/2013 13:37, Fam Zheng ha scritto:
>>> > > 
>>> > > Yes, this makes me realize that ref count it not a solution to retire
>>> > > bs->in_use, because we can't tell if drive-del or block-resize is safe
>>> > > with only reference number. But I can't think of two situations to deny
>>> > > different subsets of commands, shouldn't a general blocker, like in_use
>>> > > does, be good enough?
>> > 
>> > For example, right now nbd-server-add does not check bdrv_in_use.  But
>> > shrinking a device that is exposed via NBD could be surprising to the
>> > NBD clients.
>> > 
> So it seems to me that both block job and nbd server have the same
> restriction on device: don't resize, and notify on close. So my question
> is if we implement bdrv_add_command_blocker(), do the callers still need to distinguish what actions to block, or it's generally to block all the actions those change the device parameter?

It would be a good start to have a list of things that are setting and
checking bdrv_in_use.  Then we can make a matrix.

Paolo

Re: [Qemu-devel] [RFC PATCH 1/3] block: add target-id option to drive-backup QMP command

[Fam Zheng]

On Thu, 06/27 13:40, Paolo Bonzini wrote:
> Il 27/06/2013 13:37, Fam Zheng ha scritto:
> >>> > > 
> >>> > > Yes, this makes me realize that ref count it not a solution to retire
> >>> > > bs->in_use, because we can't tell if drive-del or block-resize is safe
> >>> > > with only reference number. But I can't think of two situations to deny
> >>> > > different subsets of commands, shouldn't a general blocker, like in_use
> >>> > > does, be good enough?
> >> > 
> >> > For example, right now nbd-server-add does not check bdrv_in_use.  But
> >> > shrinking a device that is exposed via NBD could be surprising to the
> >> > NBD clients.
> >> > 
> > So it seems to me that both block job and nbd server have the same
> > restriction on device: don't resize, and notify on close. So my question
> > is if we implement bdrv_add_command_blocker(), do the callers still need to distinguish what actions to block, or it's generally to block all the actions those change the device parameter?
> 
> It would be a good start to have a list of things that are setting and
> checking bdrv_in_use.  Then we can make a matrix.
> 

Grapping the code and get:

Commands fail with -EBUSY if bdrv_in_use():
    bdrv_commit()
    bdrv_truncate()
    external_snapshot_prepare()
    eject_device()
    drive_del()
    drive_mirror()
    block_job_create()

Commands to set bdrv in_use to 1:
    init_blk_migration()
    block_job_create()
    virtio_blk_data_plane_create()

-- 
Fam

[Qemu-devel] [RFC PATCH 2/3] block: assign backing relationship in drive-backup

[Fam Zheng]

Assign source image as the backing hd of target bs, so reading target bs
gets the point-in-time copy of data from source image.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 5e694f3..a1d816e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1504,6 +1504,10 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
+    target_bs->backing_hd = bs;
+    pstrcpy(target_bs->backing_file, sizeof(target_bs->backing_file),
+            bs->filename);
+
     backup_start(bs, target_bs, speed, on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 2/3] block: assign backing relationship in drive-backup

[Paolo Bonzini]

Il 26/06/2013 05:59, Fam Zheng ha scritto:
> Assign source image as the backing hd of target bs, so reading target bs
> gets the point-in-time copy of data from source image.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 5e694f3..a1d816e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1504,6 +1504,10 @@ void qmp_drive_backup(const char *device, const char *target,
>          return;
>      }
>  
> +    target_bs->backing_hd = bs;
> +    pstrcpy(target_bs->backing_file, sizeof(target_bs->backing_file),
> +            bs->filename);

backing_format missing.

Paolo

>      backup_start(bs, target_bs, speed, on_source_error, on_target_error,
>                   block_job_cb, bs, &local_err);
>      if (local_err != NULL) {
> 

[Qemu-devel] [RFC PATCH 3/3] nbd: don't get ref if bs has no drive

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev-nbd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 95f10c8..2657a90 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -81,6 +81,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     BlockDriverState *bs;
     NBDExport *exp;
     NBDCloseNotifier *n;
+    DriveInfo *dinfo;
 
     if (server_fd == -1) {
         error_setg(errp, "NBD server not running");
@@ -109,7 +110,10 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
                          nbd_server_put_ref);
 
     nbd_export_set_name(exp, device);
-    drive_get_ref(drive_get_by_blockdev(bs));
+    dinfo = drive_get_by_blockdev(bs);
+    if (dinfo) {
+        drive_get_ref(dinfo);
+    }
 
     n = g_malloc0(sizeof(NBDCloseNotifier));
     n->n.notify = nbd_close_notifier;
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 0/3] Point-in-time snapshot exporting with drive-backup

[Paolo Bonzini]

Il 26/06/2013 05:59, Fam Zheng ha scritto:
> 
> The block job originally had total control over target bs: when the job
> completes, the target is deleted. Now it's shared with NBD, so we should make
> sure the job completion or canceling wouldn't crash NBD server. This patch
> doesn't handle this case: if the block job is ended before NBD exporting,
> there'll be problem.

This should not be a problem.  When the job completes or cancels it will
close s->target.  The NBD server has installed a close notifier for
s->target, and will close connections with the clients.

Similarly, if the source disk is hot-unplugged, the bdrv_close will
cancel the job, which in turn closes connections with the clients.

So this just works as long as no hot-unplug happens and as long as the
job doesn't complete: the client can use the point-in-time snapshot via
NBD as long as it cares to.

This leads to another observation: a sync:'none' block-backup job
probably should never complete, and should instead go on until explicit
cancellation.  This is because the job does no background writes, and
thus completion would only happen after the guest has written the whole
disk.  Writing the whole disk is rare enough that it will likely cause
bugs in the clients.  It is easier just to never complete the job.

Paolo

Re: [Qemu-devel] [RFC PATCH 0/3] Point-in-time snapshot exporting with drive-backup

[Fam Zheng]

On Wed, 06/26 09:23, Paolo Bonzini wrote:
> Il 26/06/2013 05:59, Fam Zheng ha scritto:
> > 
> > The block job originally had total control over target bs: when the job
> > completes, the target is deleted. Now it's shared with NBD, so we should make
> > sure the job completion or canceling wouldn't crash NBD server. This patch
> > doesn't handle this case: if the block job is ended before NBD exporting,
> > there'll be problem.
> 
> This should not be a problem.  When the job completes or cancels it will
> close s->target.  The NBD server has installed a close notifier for
> s->target, and will close connections with the clients.
> 
> Similarly, if the source disk is hot-unplugged, the bdrv_close will
> cancel the job, which in turn closes connections with the clients.
> 
> So this just works as long as no hot-unplug happens and as long as the
> job doesn't complete: the client can use the point-in-time snapshot via
> NBD as long as it cares to.

OK, thanks for pointing this out.

> 
> This leads to another observation: a sync:'none' block-backup job
> probably should never complete, and should instead go on until explicit
> cancellation.  This is because the job does no background writes, and
> thus completion would only happen after the guest has written the whole
> disk.  Writing the whole disk is rare enough that it will likely cause
> bugs in the clients.  It is easier just to never complete the job.
> 

Yes, the sync mode none will simply run forever until cancelled.

-- 
Fam

Re: [Qemu-devel] [RFC PATCH 0/3] Point-in-time snapshot exporting with drive-backup

[Stefan Hajnoczi]

On Wed, Jun 26, 2013 at 03:31:19PM +0800, Fam Zheng wrote:
> On Wed, 06/26 09:23, Paolo Bonzini wrote:
> > Il 26/06/2013 05:59, Fam Zheng ha scritto:
> > This leads to another observation: a sync:'none' block-backup job
> > probably should never complete, and should instead go on until explicit
> > cancellation.  This is because the job does no background writes, and
> > thus completion would only happen after the guest has written the whole
> > disk.  Writing the whole disk is rare enough that it will likely cause
> > bugs in the clients.  It is easier just to never complete the job.
> > 
> 
> Yes, the sync mode none will simply run forever until cancelled.

There is a dedicated command to successfully complete a job:

block-job-complete

Stefan

Re: [Qemu-devel] [RFC PATCH 0/3] Point-in-time snapshot exporting with drive-backup

[Paolo Bonzini]

Il 27/06/2013 10:17, Stefan Hajnoczi ha scritto:
>>> > > Il 26/06/2013 05:59, Fam Zheng ha scritto:
>>> > > This leads to another observation: a sync:'none' block-backup job
>>> > > probably should never complete, and should instead go on until explicit
>>> > > cancellation.  This is because the job does no background writes, and
>>> > > thus completion would only happen after the guest has written the whole
>>> > > disk.  Writing the whole disk is rare enough that it will likely cause
>>> > > bugs in the clients.  It is easier just to never complete the job.
>>> > > 
>> > 
>> > Yes, the sync mode none will simply run forever until cancelled.
> There is a dedicated command to successfully complete a job:
> 
> block-job-complete

block-job-complete should only be called after a BLOCK_JOB_READY event,
but when would the backup job raise it?  Immediately after starting?

There is no behavioral difference in this case between cancel and
complete in fact as far as I understand, so I think cancelling the job
would match existing practice better.

Paolo

Re: [Qemu-devel] [RFC PATCH 0/3] Point-in-time snapshot exporting with drive-backup

[Stefan Hajnoczi]

On Thu, Jun 27, 2013 at 12:06:44PM +0200, Paolo Bonzini wrote:
> Il 27/06/2013 10:17, Stefan Hajnoczi ha scritto:
> >>> > > Il 26/06/2013 05:59, Fam Zheng ha scritto:
> >>> > > This leads to another observation: a sync:'none' block-backup job
> >>> > > probably should never complete, and should instead go on until explicit
> >>> > > cancellation.  This is because the job does no background writes, and
> >>> > > thus completion would only happen after the guest has written the whole
> >>> > > disk.  Writing the whole disk is rare enough that it will likely cause
> >>> > > bugs in the clients.  It is easier just to never complete the job.
> >>> > > 
> >> > 
> >> > Yes, the sync mode none will simply run forever until cancelled.
> > There is a dedicated command to successfully complete a job:
> > 
> > block-job-complete
> 
> block-job-complete should only be called after a BLOCK_JOB_READY event,
> but when would the backup job raise it?  Immediately after starting?
> 
> There is no behavioral difference in this case between cancel and
> complete in fact as far as I understand, so I think cancelling the job
> would match existing practice better.

Okay.  Ian Main also preferred using block-job-cancel without the
BLOCK_JOB_READY/block-job-complete lifecycle.

Thanks for the input!

Stefan