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

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> On 10/29/2010 09:01 AM, Markus Armbruster wrote:
>> Ryan Harper<address@hidden>  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<address@hidden>
>>> ---
>>>   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.