[RFC,1/3] block: add target-id option to drive-backup QMP command
diff mbox

Message ID 1372219161-12209-2-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng June 26, 2013, 3:59 a.m. UTC
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(-)

Comments

Stefan Hajnoczi June 27, 2013, 8:15 a.m. UTC | #1
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
Fam Zheng June 27, 2013, 9:41 a.m. UTC | #2
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?
Paolo Bonzini June 27, 2013, 10:57 a.m. UTC | #3
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
Fam Zheng June 27, 2013, 11:37 a.m. UTC | #4
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?
Paolo Bonzini June 27, 2013, 11:40 a.m. UTC | #5
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
Fam Zheng June 28, 2013, 2:17 a.m. UTC | #6
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()

Patch
diff mbox

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)