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

[Max Reitz <mreitz@redhat.com>]

[-- 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 --]