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

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

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