Re: [Qemu-devel] [PATCH 2/3] v2 Fix Block Hotplug race with drive_unplug()

[Ryan Harper <ryanh@us.ibm.com>]

* 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.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com