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

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

[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]

On 10/29/2010 09:29 AM, Kevin Wolf wrote:
> 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?
>    

Yes.  But I think for the purposes of this patch, a bdrv_cancel_all() 
would be just as good.  The intention is to eliminate pending I/O 
requests, the fsync is just a side effect.

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

I agree.  Re: qemu_aio_flush, we have to wait for it to complete which 
gets a little complicated in bdrv_close().  I think it would be better 
to make bdrv_flush() call bdrv_aio_flush() if an explicit bdrv_flush 
method isn't provided.  Something like the attached (still need to test).

Does that seem reasonable?

Regards,

Anthony Liguori

> Kevin
>    


[-- Attachment #2: 0001-block-make-bdrv_flush-fall-back-to-bdrv_aio_flush.patch --]
[-- Type: text/x-patch, Size: 1366 bytes --]

>From 86bf3c9eb5ce43280224f9271a4ad016b0dd3fb1 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Fri, 29 Oct 2010 09:36:53 -0500
Subject: [PATCH 1/2] block: make bdrv_flush() fall back to bdrv_aio_flush

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/block.c b/block.c
index 985d0b7..fc8defd 100644
--- a/block.c
+++ b/block.c
@@ -1453,14 +1453,51 @@ const char *bdrv_get_device_name(BlockDriverState *bs)
     return bs->device_name;
 }
 
+static void bdrv_flush_em_cb(void *opaque, int ret)
+{
+    int *pcomplete = opaque;
+    *pcomplete = 1;
+}
+
+static void bdrv_flush_em(BlockDriverState *bs)
+{
+    int complete = 0;
+    BlockDriverAIOCB *acb;
+
+    if (!bs->drv->bdrv_aio_flush) {
+        return;
+    }
+
+    async_context_push();
+
+    acb = bs->drv->bdrv_aio_flush(bs, bdrv_flush_em_cb, &complete);
+    if (!acb) {
+        goto out;
+    }
+
+    while (!complete) {
+        qemu_aio_wait();
+    }
+
+out:
+    async_context_pop();
+}
+
 void bdrv_flush(BlockDriverState *bs)
 {
     if (bs->open_flags & BDRV_O_NO_FLUSH) {
         return;
     }
 
-    if (bs->drv && bs->drv->bdrv_flush)
+    if (!bs->drv) {
+        return;
+    }
+
+    if (bs->drv->bdrv_flush) {
         bs->drv->bdrv_flush(bs);
+    } else {
+        bdrv_flush_em(bs);
+    }
 }
 
 void bdrv_flush_all(void)
-- 
1.7.0.4


[-- Attachment #3: 0002-block-add-bdrv_flush-to-bdrv_close.patch --]
[-- Type: text/x-patch, Size: 657 bytes --]

>From 094049974796ddf78ee2f1541bffa40fe1176a1a Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Fri, 29 Oct 2010 09:37:25 -0500
Subject: [PATCH 2/2] block: add bdrv_flush to bdrv_close

To ensure that there are no pending completions before destroying a block
device.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/block.c b/block.c
index fc8defd..d2aed1b 100644
--- a/block.c
+++ b/block.c
@@ -644,6 +644,8 @@ unlink_and_fail:
 void bdrv_close(BlockDriverState *bs)
 {
     if (bs->drv) {
+        bdrv_flush(bs);
+
         if (bs == bs_snapshots) {
             bs_snapshots = NULL;
         }
-- 
1.7.0.4