Anthony Liguori <aligu...@linux.vnet.ibm.com> writes:
> On 10/29/2010 09:01 AM, Markus Armbruster wrote:
>> Ryan Harper<ry...@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<ry...@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.
