| Message ID | 1372219161-12209-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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?
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
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?
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
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()
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)
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(-)