| Message ID | 1288030956-28383-3-git-send-email-ryanh@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Ryan Harper <ryanh@us.ibm.com> writes: > Block hot unplug is racy since the guest is required to acknowlege the ACPI > unplug event; this may not happen synchronously with the device removal command > > This series aims to close a gap where by mgmt applications that assume the > block resource has been removed without confirming that the guest has > acknowledged the removal may re-assign the underlying device to a second guest > leading to data leakage. > > This series introduces a new montor command to decouple asynchornous device > removal from restricting guest access to a block device. We do this by creating > a new monitor command drive_unplug which maps to a bdrv_unplug() command which > does a qemu_aio_flush; bdrv_flush() and bdrv_close(). Once complete, subsequent > IO is rejected from the device and the guest will get IO errors but continue to > function. > > A subsequent device removal command can be issued to remove the device, to which > the guest may or maynot respond, but as long as the unplugged bit is set, no IO > will be sumbitted. > > Changes since v1: > - Added qemu_aio_flush() before bdrv_flush() to wait on pending io > > Signed-off-by: Ryan Harper <ryanh@us.ibm.com> > --- > block.c | 7 +++++++ > block.h | 1 + > blockdev.c | 26 ++++++++++++++++++++++++++ > blockdev.h | 1 + > hmp-commands.hx | 15 +++++++++++++++ > 5 files changed, 50 insertions(+), 0 deletions(-) > > diff --git a/block.c b/block.c > index a19374d..be47655 100644 > --- a/block.c > +++ b/block.c > @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable) > } > } > > +void bdrv_unplug(BlockDriverState *bs) > +{ > + qemu_aio_flush(); > + bdrv_flush(bs); > + bdrv_close(bs); > +} Stupid question: why doesn't bdrv_close() flush automatically? And why do we have to flush here, but not before other uses of bdrv_close(), such as eject_device()? > + > int bdrv_is_removable(BlockDriverState *bs) > { > return bs->removable; > diff --git a/block.h b/block.h > index 5f64380..732f63e 100644 > --- a/block.h > +++ b/block.h > @@ -171,6 +171,7 @@ void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, > BlockErrorAction on_write_error); > BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); > void bdrv_set_removable(BlockDriverState *bs, int removable); > +void bdrv_unplug(BlockDriverState *bs); > int bdrv_is_removable(BlockDriverState *bs); > int bdrv_is_read_only(BlockDriverState *bs); > int bdrv_is_sg(BlockDriverState *bs); > diff --git a/blockdev.c b/blockdev.c > index 5fc3b9b..68eb329 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -610,3 +610,29 @@ int do_change_block(Monitor *mon, const char *device, > } > return monitor_read_bdrv_key_start(mon, bs, NULL, NULL); > } > + > +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data) > +{ > + DriveInfo *dinfo; > + BlockDriverState *bs; > + const char *id; > + > + if (!qdict_haskey(qdict, "id")) { > + qerror_report(QERR_MISSING_PARAMETER, "id"); > + return -1; > + } As Luiz pointed out, this check is redundant. > + > + id = qdict_get_str(qdict, "id"); > + dinfo = drive_get_by_id(id); > + if (!dinfo) { > + qerror_report(QERR_DEVICE_NOT_FOUND, id); > + return -1; > + } > + > + /* mark block device unplugged */ > + bs = dinfo->bdrv; > + bdrv_unplug(bs); > + > + return 0; > +} > + What about: const char *id = qdict_get_str(qdict, "id"); BlockDriverState *bs; bs = bdrv_find(id); if (!bs) { qerror_report(QERR_DEVICE_NOT_FOUND, id); return -1; } bdrv_unplug(bs); return 0; Precedence: commit f8b6cc00 replaced uses of drive_get_by_id() by bdrv_find(). > diff --git a/blockdev.h b/blockdev.h > index 19c6915..ecb9ac8 100644 > --- a/blockdev.h > +++ b/blockdev.h > @@ -52,5 +52,6 @@ int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data); > int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data); > int do_change_block(Monitor *mon, const char *device, > const char *filename, const char *fmt); > +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data); > > #endif > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 81999aa..7a32a2e 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -68,6 +68,21 @@ Eject a removable medium (use -f to force it). > ETEXI > > { > + .name = "drive_unplug", > + .args_type = "id:s", > + .params = "device", > + .help = "unplug block device", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_drive_unplug, > + }, > + > +STEXI > +@item unplug @var{device} > +@findex unplug > +Unplug block device. A bit terse, isn't it? What does it mean to unplug a block device? What's its observable effect on the guest? Does it look like disk gone completely south, perhaps? > +ETEXI > + > + { > .name = "change", > .args_type = "device:B,target:F,arg:s?", > .params = "device filename [format]",
On 10/29/2010 09:01 AM, Markus Armbruster wrote: > Ryan Harper<ryanh@us.ibm.com> writes: > > >> Block hot unplug is racy since the guest is required to acknowlege the ACPI >> unplug event; this may not happen synchronously with the device removal command >> >> This series aims to close a gap where by mgmt applications that assume the >> block resource has been removed without confirming that the guest has >> acknowledged the removal may re-assign the underlying device to a second guest >> leading to data leakage. >> >> This series introduces a new montor command to decouple asynchornous device >> removal from restricting guest access to a block device. We do this by creating >> a new monitor command drive_unplug which maps to a bdrv_unplug() command which >> does a qemu_aio_flush; bdrv_flush() and bdrv_close(). Once complete, subsequent >> IO is rejected from the device and the guest will get IO errors but continue to >> function. >> >> A subsequent device removal command can be issued to remove the device, to which >> the guest may or maynot respond, but as long as the unplugged bit is set, no IO >> will be sumbitted. >> >> Changes since v1: >> - Added qemu_aio_flush() before bdrv_flush() to wait on pending io >> >> Signed-off-by: Ryan Harper<ryanh@us.ibm.com> >> --- >> block.c | 7 +++++++ >> block.h | 1 + >> blockdev.c | 26 ++++++++++++++++++++++++++ >> blockdev.h | 1 + >> hmp-commands.hx | 15 +++++++++++++++ >> 5 files changed, 50 insertions(+), 0 deletions(-) >> >> diff --git a/block.c b/block.c >> index a19374d..be47655 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable) >> } >> } >> >> +void bdrv_unplug(BlockDriverState *bs) >> +{ >> + qemu_aio_flush(); >> + bdrv_flush(bs); >> + bdrv_close(bs); >> +} >> > Stupid question: why doesn't bdrv_close() flush automatically? > I don't think it's a bad idea to do that but to the extent that the block API is designed after posix file I/O, close does not usually imply flush. > And why do we have to flush here, but not before other uses of > bdrv_close(), such as eject_device()? > Good question. Kevin should also confirm, but looking at the code, I think flush() is needed before close. If there's a pending I/O event and you close before the I/O event is completed, you'll get a callback for completion against a bogus BlockDriverState. I can't find anything in either raw-posix or the generic block layer that would mitigate this. Regards, Anthony Liguori
Am 29.10.2010 16:15, schrieb Anthony Liguori: > On 10/29/2010 09:01 AM, Markus Armbruster wrote: >> Ryan Harper<ryanh@us.ibm.com> writes: >>> diff --git a/block.c b/block.c >>> index a19374d..be47655 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable) >>> } >>> } >>> >>> +void bdrv_unplug(BlockDriverState *bs) >>> +{ >>> + qemu_aio_flush(); >>> + bdrv_flush(bs); >>> + bdrv_close(bs); >>> +} >>> >> Stupid question: why doesn't bdrv_close() flush automatically? >> > > I don't think it's a bad idea to do that but to the extent that the > block API is designed after posix file I/O, close does not usually imply > flush. I don't think it really resembles POSIX. More or less the only thing they have in common is that both provide open, read, write and close, which is something that probably any API for file accesses provides. The operation you're talking about here is bdrv_flush/fsync that is not implied by a POSIX close? >> And why do we have to flush here, but not before other uses of >> bdrv_close(), such as eject_device()? >> > > Good question. Kevin should also confirm, but looking at the code, I > think flush() is needed before close. If there's a pending I/O event > and you close before the I/O event is completed, you'll get a callback > for completion against a bogus BlockDriverState. > > I can't find anything in either raw-posix or the generic block layer > that would mitigate this. I'm not aware of anything either. This is what qemu_aio_flush would do. It seems reasonable to me to call both qemu_aio_flush and bdrv_flush in bdrv_close. We probably don't really need to call bdrv_flush to operate correctly, but it can't hurt and bdrv_close shouldn't happen that often anyway. Kevin
Anthony Liguori <aliguori@linux.vnet.ibm.com> writes: > On 10/29/2010 09:01 AM, Markus Armbruster wrote: >> Ryan Harper<ryanh@us.ibm.com> writes: >> >> >>> Block hot unplug is racy since the guest is required to acknowlege the ACPI >>> unplug event; this may not happen synchronously with the device removal command >>> >>> This series aims to close a gap where by mgmt applications that assume the >>> block resource has been removed without confirming that the guest has >>> acknowledged the removal may re-assign the underlying device to a second guest >>> leading to data leakage. >>> >>> This series introduces a new montor command to decouple asynchornous device >>> removal from restricting guest access to a block device. We do this by creating >>> a new monitor command drive_unplug which maps to a bdrv_unplug() command which >>> does a qemu_aio_flush; bdrv_flush() and bdrv_close(). Once complete, subsequent >>> IO is rejected from the device and the guest will get IO errors but continue to >>> function. >>> >>> A subsequent device removal command can be issued to remove the device, to which >>> the guest may or maynot respond, but as long as the unplugged bit is set, no IO >>> will be sumbitted. >>> >>> Changes since v1: >>> - Added qemu_aio_flush() before bdrv_flush() to wait on pending io >>> >>> Signed-off-by: Ryan Harper<ryanh@us.ibm.com> >>> --- >>> block.c | 7 +++++++ >>> block.h | 1 + >>> blockdev.c | 26 ++++++++++++++++++++++++++ >>> blockdev.h | 1 + >>> hmp-commands.hx | 15 +++++++++++++++ >>> 5 files changed, 50 insertions(+), 0 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index a19374d..be47655 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable) >>> } >>> } >>> >>> +void bdrv_unplug(BlockDriverState *bs) >>> +{ >>> + qemu_aio_flush(); >>> + bdrv_flush(bs); >>> + bdrv_close(bs); >>> +} >>> >> Stupid question: why doesn't bdrv_close() flush automatically? >> > > I don't think it's a bad idea to do that but to the extent that the > block API is designed after posix file I/O, close does not usually > imply flush. There is no flush() in POSIX file I/O. There is fsync(). There is fflush() in stdio. fclose() flushes automatically. Flushing only affects stdio buffers, it doesn't imply fsync(). Based on that, a reasonable programmer could be led to believe that bdrv_close() flushes automatically, and flushing doesn't fsync(). >> And why do we have to flush here, but not before other uses of >> bdrv_close(), such as eject_device()? >> > > Good question. Kevin should also confirm, but looking at the code, I > think flush() is needed before close. If there's a pending I/O event > and you close before the I/O event is completed, you'll get a callback > for completion against a bogus BlockDriverState. > > I can't find anything in either raw-posix or the generic block layer > that would mitigate this. Then bdrv_close() is too hard to use.
* Markus Armbruster <armbru@redhat.com> [2010-10-29 09:08]: > Ryan Harper <ryanh@us.ibm.com> writes: > > > Block hot unplug is racy since the guest is required to acknowlege the ACPI > > unplug event; this may not happen synchronously with the device removal command > > > > This series aims to close a gap where by mgmt applications that assume the > > block resource has been removed without confirming that the guest has > > acknowledged the removal may re-assign the underlying device to a second guest > > leading to data leakage. > > > > This series introduces a new montor command to decouple asynchornous device > > removal from restricting guest access to a block device. We do this by creating > > a new monitor command drive_unplug which maps to a bdrv_unplug() command which > > does a qemu_aio_flush; bdrv_flush() and bdrv_close(). Once complete, subsequent > > IO is rejected from the device and the guest will get IO errors but continue to > > function. > > > > A subsequent device removal command can be issued to remove the device, to which > > the guest may or maynot respond, but as long as the unplugged bit is set, no IO > > will be sumbitted. > > > > Changes since v1: > > - Added qemu_aio_flush() before bdrv_flush() to wait on pending io > > > > Signed-off-by: Ryan Harper <ryanh@us.ibm.com> > > --- > > block.c | 7 +++++++ > > block.h | 1 + > > blockdev.c | 26 ++++++++++++++++++++++++++ > > blockdev.h | 1 + > > hmp-commands.hx | 15 +++++++++++++++ > > 5 files changed, 50 insertions(+), 0 deletions(-) > > > > diff --git a/block.c b/block.c > > index a19374d..be47655 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable) > > } > > } > > > > +void bdrv_unplug(BlockDriverState *bs) > > +{ > > + qemu_aio_flush(); > > + bdrv_flush(bs); > > + bdrv_close(bs); > > +} > > Stupid question: why doesn't bdrv_close() flush automatically? > > And why do we have to flush here, but not before other uses of > bdrv_close(), such as eject_device()? > > > + > > int bdrv_is_removable(BlockDriverState *bs) > > { > > return bs->removable; > > diff --git a/block.h b/block.h > > index 5f64380..732f63e 100644 > > --- a/block.h > > +++ b/block.h > > @@ -171,6 +171,7 @@ void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, > > BlockErrorAction on_write_error); > > BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); > > void bdrv_set_removable(BlockDriverState *bs, int removable); > > +void bdrv_unplug(BlockDriverState *bs); > > int bdrv_is_removable(BlockDriverState *bs); > > int bdrv_is_read_only(BlockDriverState *bs); > > int bdrv_is_sg(BlockDriverState *bs); > > diff --git a/blockdev.c b/blockdev.c > > index 5fc3b9b..68eb329 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -610,3 +610,29 @@ int do_change_block(Monitor *mon, const char *device, > > } > > return monitor_read_bdrv_key_start(mon, bs, NULL, NULL); > > } > > + > > +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data) > > +{ > > + DriveInfo *dinfo; > > + BlockDriverState *bs; > > + const char *id; > > + > > + if (!qdict_haskey(qdict, "id")) { > > + qerror_report(QERR_MISSING_PARAMETER, "id"); > > + return -1; > > + } > > As Luiz pointed out, this check is redundant. > > > + > > + id = qdict_get_str(qdict, "id"); > > + dinfo = drive_get_by_id(id); > > + if (!dinfo) { > > + qerror_report(QERR_DEVICE_NOT_FOUND, id); > > + return -1; > > + } > > + > > + /* mark block device unplugged */ > > + bs = dinfo->bdrv; > > + bdrv_unplug(bs); > > + > > + return 0; > > +} > > + > > What about: > > const char *id = qdict_get_str(qdict, "id"); > BlockDriverState *bs; > > bs = bdrv_find(id); > if (!bs) { > qerror_report(QERR_DEVICE_NOT_FOUND, id); > return -1; > } > > bdrv_unplug(bs); > > return 0; > > Precedence: commit f8b6cc00 replaced uses of drive_get_by_id() by > bdrv_find(). That works out nicely; and I can drop the drive_get_by_id() patch as well. Thanks. > > > diff --git a/blockdev.h b/blockdev.h > > index 19c6915..ecb9ac8 100644 > > --- a/blockdev.h > > +++ b/blockdev.h > > @@ -52,5 +52,6 @@ int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data); > > int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data); > > int do_change_block(Monitor *mon, const char *device, > > const char *filename, const char *fmt); > > +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data); > > > > #endif > > diff --git a/hmp-commands.hx b/hmp-commands.hx > > index 81999aa..7a32a2e 100644 > > --- a/hmp-commands.hx > > +++ b/hmp-commands.hx > > @@ -68,6 +68,21 @@ Eject a removable medium (use -f to force it). > > ETEXI > > > > { > > + .name = "drive_unplug", > > + .args_type = "id:s", > > + .params = "device", > > + .help = "unplug block device", > > + .user_print = monitor_user_noop, > > + .mhandler.cmd_new = do_drive_unplug, > > + }, > > + > > +STEXI > > +@item unplug @var{device} > > +@findex unplug > > +Unplug block device. > > A bit terse, isn't it? What does it mean to unplug a block device? > What's its observable effect on the guest? Does it look like disk gone > completely south, perhaps? Well, most of the info in here is rather sparse as well, so there is clear precedence for it's terseness; I'll be a bit more verbose in the next version.
diff --git a/block.c b/block.c index a19374d..be47655 100644 --- a/block.c +++ b/block.c @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable) } } +void bdrv_unplug(BlockDriverState *bs) +{ + qemu_aio_flush(); + bdrv_flush(bs); + bdrv_close(bs); +} + int bdrv_is_removable(BlockDriverState *bs) { return bs->removable; diff --git a/block.h b/block.h index 5f64380..732f63e 100644 --- a/block.h +++ b/block.h @@ -171,6 +171,7 @@ void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, BlockErrorAction on_write_error); BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); void bdrv_set_removable(BlockDriverState *bs, int removable); +void bdrv_unplug(BlockDriverState *bs); int bdrv_is_removable(BlockDriverState *bs); int bdrv_is_read_only(BlockDriverState *bs); int bdrv_is_sg(BlockDriverState *bs); diff --git a/blockdev.c b/blockdev.c index 5fc3b9b..68eb329 100644 --- a/blockdev.c +++ b/blockdev.c @@ -610,3 +610,29 @@ int do_change_block(Monitor *mon, const char *device, } return monitor_read_bdrv_key_start(mon, bs, NULL, NULL); } + +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data) +{ + DriveInfo *dinfo; + BlockDriverState *bs; + const char *id; + + if (!qdict_haskey(qdict, "id")) { + qerror_report(QERR_MISSING_PARAMETER, "id"); + return -1; + } + + id = qdict_get_str(qdict, "id"); + dinfo = drive_get_by_id(id); + if (!dinfo) { + qerror_report(QERR_DEVICE_NOT_FOUND, id); + return -1; + } + + /* mark block device unplugged */ + bs = dinfo->bdrv; + bdrv_unplug(bs); + + return 0; +} + diff --git a/blockdev.h b/blockdev.h index 19c6915..ecb9ac8 100644 --- a/blockdev.h +++ b/blockdev.h @@ -52,5 +52,6 @@ int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data); int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data); int do_change_block(Monitor *mon, const char *device, const char *filename, const char *fmt); +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data); #endif diff --git a/hmp-commands.hx b/hmp-commands.hx index 81999aa..7a32a2e 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -68,6 +68,21 @@ Eject a removable medium (use -f to force it). ETEXI { + .name = "drive_unplug", + .args_type = "id:s", + .params = "device", + .help = "unplug block device", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_drive_unplug, + }, + +STEXI +@item unplug @var{device} +@findex unplug +Unplug block device. +ETEXI + + { .name = "change", .args_type = "device:B,target:F,arg:s?", .params = "device filename [format]",
Block hot unplug is racy since the guest is required to acknowlege the ACPI unplug event; this may not happen synchronously with the device removal command This series aims to close a gap where by mgmt applications that assume the block resource has been removed without confirming that the guest has acknowledged the removal may re-assign the underlying device to a second guest leading to data leakage. This series introduces a new montor command to decouple asynchornous device removal from restricting guest access to a block device. We do this by creating a new monitor command drive_unplug which maps to a bdrv_unplug() command which does a qemu_aio_flush; bdrv_flush() and bdrv_close(). Once complete, subsequent IO is rejected from the device and the guest will get IO errors but continue to function. A subsequent device removal command can be issued to remove the device, to which the guest may or maynot respond, but as long as the unplugged bit is set, no IO will be sumbitted. Changes since v1: - Added qemu_aio_flush() before bdrv_flush() to wait on pending io Signed-off-by: Ryan Harper <ryanh@us.ibm.com> --- block.c | 7 +++++++ block.h | 1 + blockdev.c | 26 ++++++++++++++++++++++++++ blockdev.h | 1 + hmp-commands.hx | 15 +++++++++++++++ 5 files changed, 50 insertions(+), 0 deletions(-)