| Message ID | 1374765505-14356-2-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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); > } >
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 > >
> 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 >> >> >
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
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
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); }
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(-)