[Qemu-devel] [PATCH] block: use drained section in bdrv_close

[Qemu-devel] [PATCH] block: use drained section in bdrv_close

[Paolo Bonzini]

bdrv_close is used when ejecting a medium.  Use a drained section to ensure
that all I/O goes to either the old medium or the bitbucket.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 411edbf..01655de 100644
--- a/block.c
+++ b/block.c
@@ -2154,9 +2154,10 @@ void bdrv_close(BlockDriverState *bs)
         bdrv_io_limits_disable(bs);
     }
 
-    bdrv_drain(bs); /* complete I/O */
+    bdrv_drained_begin(bs); /* complete I/O */
     bdrv_flush(bs);
     bdrv_drain(bs); /* in case flush left pending I/O */
+
     notifier_list_notify(&bs->close_notifiers, bs);
 
     if (bs->blk) {
@@ -2206,6 +2207,7 @@ void bdrv_close(BlockDriverState *bs)
         g_free(ban);
     }
     QLIST_INIT(&bs->aio_notifiers);
+    bdrv_drained_end(bs);
 }
 
 void bdrv_close_all(void)
-- 
2.5.0

Re: [Qemu-devel] [Qemu-block] [PATCH] block: use drained section in bdrv_close

[Max Reitz]

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

On 23.12.2015 11:48, Paolo Bonzini wrote:
> bdrv_close is used when ejecting a medium.

Is it still? Other than it maybe being indirectly called through
bdrv_delete(), it shouldn't be.

>                                             Use a drained section to ensure
> that all I/O goes to either the old medium or the bitbucket.

Since the BDS will be deleted after bdrv_close() has been called, where
else would the I/O go now?

Max

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 411edbf..01655de 100644
> --- a/block.c
> +++ b/block.c
> @@ -2154,9 +2154,10 @@ void bdrv_close(BlockDriverState *bs)
>          bdrv_io_limits_disable(bs);
>      }
>  
> -    bdrv_drain(bs); /* complete I/O */
> +    bdrv_drained_begin(bs); /* complete I/O */
>      bdrv_flush(bs);
>      bdrv_drain(bs); /* in case flush left pending I/O */
> +
>      notifier_list_notify(&bs->close_notifiers, bs);
>  
>      if (bs->blk) {
> @@ -2206,6 +2207,7 @@ void bdrv_close(BlockDriverState *bs)
>          g_free(ban);
>      }
>      QLIST_INIT(&bs->aio_notifiers);
> +    bdrv_drained_end(bs);
>  }
>  
>  void bdrv_close_all(void)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: use drained section in bdrv_close

[Paolo Bonzini]

> On 23.12.2015 11:48, Paolo Bonzini wrote:
> > bdrv_close is used when ejecting a medium.
> 
> Is it still? Other than it maybe being indirectly called through
> bdrv_delete(), it shouldn't be.

Yes, through blk_remove_bs -> bdrv_unref -> bdrv_delete.

> >                                             Use a drained section to ensure
> > that all I/O goes to either the old medium or the bitbucket.
> 
> Since the BDS will be deleted after bdrv_close() has been called, where
> else would the I/O go now?

The ioeventfd could be processed during bs->drv->bdrv_close.  For
example I/O could be submitted while qcow2_close's qcow2_cache_flush
yields, and the result would probably be corrupted metadata.

Paolo

> Max
> 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  block.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index 411edbf..01655de 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2154,9 +2154,10 @@ void bdrv_close(BlockDriverState *bs)
> >          bdrv_io_limits_disable(bs);
> >      }
> >  
> > -    bdrv_drain(bs); /* complete I/O */
> > +    bdrv_drained_begin(bs); /* complete I/O */
> >      bdrv_flush(bs);
> >      bdrv_drain(bs); /* in case flush left pending I/O */
> > +
> >      notifier_list_notify(&bs->close_notifiers, bs);
> >  
> >      if (bs->blk) {
> > @@ -2206,6 +2207,7 @@ void bdrv_close(BlockDriverState *bs)
> >          g_free(ban);
> >      }
> >      QLIST_INIT(&bs->aio_notifiers);
> > +    bdrv_drained_end(bs);
> >  }
> >  
> >  void bdrv_close_all(void)
> > 
> 
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH] block: use drained section in bdrv_close

[Max Reitz]

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

On 23.12.2015 22:55, Paolo Bonzini wrote:
> 
>> On 23.12.2015 11:48, Paolo Bonzini wrote:
>>> bdrv_close is used when ejecting a medium.
>>
>> Is it still? Other than it maybe being indirectly called through
>> bdrv_delete(), it shouldn't be.
> 
> Yes, through blk_remove_bs -> bdrv_unref -> bdrv_delete.

OK, I was asking because bdrv_close() was invoked directly by the
ejection code until recently and thought that maybe you were referring
to that, and that there might have been a way for I/O to spill to the
new medium if one issued a change command.

>>>                                             Use a drained section to ensure
>>> that all I/O goes to either the old medium or the bitbucket.
>>
>> Since the BDS will be deleted after bdrv_close() has been called, where
>> else would the I/O go now?
> 
> The ioeventfd could be processed during bs->drv->bdrv_close.  For
> example I/O could be submitted while qcow2_close's qcow2_cache_flush
> yields, and the result would probably be corrupted metadata.

Ah, OK, so you meant “Either all or no I/O should go to the medium”, I
thought it was about the fact that I/O might go somewhere else than the
old medium or /dev/null (e.g. the new medium in case of a change).

That makes more sense now, thanks! :-)

Applied to my block tree:

https://github.com/XanClic/qemu/commits/block

Max

> 
> Paolo
> 
>> Max
>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  block.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 411edbf..01655de 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2154,9 +2154,10 @@ void bdrv_close(BlockDriverState *bs)
>>>          bdrv_io_limits_disable(bs);
>>>      }
>>>  
>>> -    bdrv_drain(bs); /* complete I/O */
>>> +    bdrv_drained_begin(bs); /* complete I/O */
>>>      bdrv_flush(bs);
>>>      bdrv_drain(bs); /* in case flush left pending I/O */
>>> +
>>>      notifier_list_notify(&bs->close_notifiers, bs);
>>>  
>>>      if (bs->blk) {
>>> @@ -2206,6 +2207,7 @@ void bdrv_close(BlockDriverState *bs)
>>>          g_free(ban);
>>>      }
>>>      QLIST_INIT(&bs->aio_notifiers);
>>> +    bdrv_drained_end(bs);
>>>  }
>>>  
>>>  void bdrv_close_all(void)
>>>
>>
>>
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]