Patchwork [v6,01/18] block: ensure bdrv_drain_all() works during bdrv_delete()

login
register
mail settings
Submitter Stefan Hajnoczi
Date July 25, 2013, 3:18 p.m.
Message ID <1374765505-14356-2-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/261767/
State New
Headers show

Comments

Stefan Hajnoczi - July 25, 2013, 3:18 p.m.
In bdrv_delete() make sure to call bdrv_make_anon() *after* bdrv_close()
so that the device is still seen by bdrv_drain_all() when iterating
bdrv_states.

Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Wayne Xia - July 26, 2013, 6:43 a.m.
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

One question: old code missed itself in bdrv_drain_all(), is that a bug?

> In bdrv_delete() make sure to call bdrv_make_anon() *after* bdrv_close()
> so that the device is still seen by bdrv_drain_all() when iterating
> bdrv_states.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 6cd39fa..9d68270 100644
> --- a/block.c
> +++ b/block.c
> @@ -1600,11 +1600,11 @@ void bdrv_delete(BlockDriverState *bs)
>       assert(!bs->job);
>       assert(!bs->in_use);
> 
> +    bdrv_close(bs);
> +
>       /* remove from list, if necessary */
>       bdrv_make_anon(bs);
> 
> -    bdrv_close(bs);
> -
>       g_free(bs);
>   }
>
Stefan Hajnoczi - Aug. 6, 2013, 3:06 p.m.
On Fri, Jul 26, 2013 at 02:43:44PM +0800, Wenchao Xia wrote:
> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> One question: old code missed itself in bdrv_drain_all(), is that a bug?

Sorry, I don't understand the question.  Can you rephrase it?

> > In bdrv_delete() make sure to call bdrv_make_anon() *after* bdrv_close()
> > so that the device is still seen by bdrv_drain_all() when iterating
> > bdrv_states.
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   block.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 6cd39fa..9d68270 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1600,11 +1600,11 @@ void bdrv_delete(BlockDriverState *bs)
> >       assert(!bs->job);
> >       assert(!bs->in_use);
> > 
> > +    bdrv_close(bs);
> > +
> >       /* remove from list, if necessary */
> >       bdrv_make_anon(bs);
> > 
> > -    bdrv_close(bs);
> > -
> >       g_free(bs);
> >   }
> > 
> 
> 
> -- 
> Best Regards
> 
> Wenchao Xia
> 
>
Wayne Xia - Aug. 7, 2013, 2:42 a.m.
> On Fri, Jul 26, 2013 at 02:43:44PM +0800, Wenchao Xia wrote:
>> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>
>> One question: old code missed itself in bdrv_drain_all(), is that a bug?
>
> Sorry, I don't understand the question.  Can you rephrase it?
>
   Before this patch, in the code path: bdrv_close()->bdrv_drain_all(),
the *bs does not exist in bdrv_states, so the code missed the chance to
drain the request on *bs. That is a bug, and this patch is actually a
bugfix?


>>> In bdrv_delete() make sure to call bdrv_make_anon() *after* bdrv_close()
>>> so that the device is still seen by bdrv_drain_all() when iterating
>>> bdrv_states.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>    block.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 6cd39fa..9d68270 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1600,11 +1600,11 @@ void bdrv_delete(BlockDriverState *bs)
>>>        assert(!bs->job);
>>>        assert(!bs->in_use);
>>>
>>> +    bdrv_close(bs);
>>> +
>>>        /* remove from list, if necessary */
>>>        bdrv_make_anon(bs);
>>>
>>> -    bdrv_close(bs);
>>> -
>>>        g_free(bs);
>>>    }
>>>
>>
>>
>> --
>> Best Regards
>>
>> Wenchao Xia
>>
>>
>
Stefan Hajnoczi - Aug. 7, 2013, 7:31 a.m.
On Wed, Aug 07, 2013 at 10:42:26AM +0800, Wenchao Xia wrote:
> >On Fri, Jul 26, 2013 at 02:43:44PM +0800, Wenchao Xia wrote:
> >>Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>
> >>One question: old code missed itself in bdrv_drain_all(), is that a bug?
> >
> >Sorry, I don't understand the question.  Can you rephrase it?
> >
>   Before this patch, in the code path: bdrv_close()->bdrv_drain_all(),
> the *bs does not exist in bdrv_states, so the code missed the chance to
> drain the request on *bs. That is a bug, and this patch is actually a
> bugfix?

Yes, exactly.  It's a bug fix and therefore I CCed
qemu-stable@nongnu.org.

Stefan
Stefan Hajnoczi - Aug. 7, 2013, 7:42 a.m.
On Thu, Jul 25, 2013 at 05:18:08PM +0200, Stefan Hajnoczi wrote:
> In bdrv_delete() make sure to call bdrv_make_anon() *after* bdrv_close()
> so that the device is still seen by bdrv_drain_all() when iterating
> bdrv_states.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 6cd39fa..9d68270 100644
> --- a/block.c
> +++ b/block.c
> @@ -1600,11 +1600,11 @@ void bdrv_delete(BlockDriverState *bs)
>      assert(!bs->job);
>      assert(!bs->in_use);
>  
> +    bdrv_close(bs);
> +
>      /* remove from list, if necessary */
>      bdrv_make_anon(bs);
>  
> -    bdrv_close(bs);
> -
>      g_free(bs);
>  }

Kevin: Although I haven't seen reports of this bug, it might be a good
idea to merge this single patch for QEMU 1.6.

Stefan

Patch

diff --git a/block.c b/block.c
index 6cd39fa..9d68270 100644
--- a/block.c
+++ b/block.c
@@ -1600,11 +1600,11 @@  void bdrv_delete(BlockDriverState *bs)
     assert(!bs->job);
     assert(!bs->in_use);
 
+    bdrv_close(bs);
+
     /* remove from list, if necessary */
     bdrv_make_anon(bs);
 
-    bdrv_close(bs);
-
     g_free(bs);
 }