Re: [Qemu-devel] [PATCH v3 09/14] block: Move some bdrv_*_all() functions to BB

[Max Reitz]

On 17.02.2016 16:51, Kevin Wolf wrote:
> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>> Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level.
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  block.c                                       | 20 ------------
>>  block/block-backend.c                         | 44 
>> +++++++++++++++++++++++----
>>  block/io.c                                    | 20 ------------
>>  include/block/block.h                         |  2 --
>>  stubs/Makefile.objs                           |  2 +-
>>  stubs/{bdrv-commit-all.c => blk-commit-all.c} |  4 +--
>>  6 files changed, 41 insertions(+), 51 deletions(-)
>>  rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)
>>

[...]

>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index a10fe44..a918c35 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c

[...]

>> @@ -1361,5 +1356,42 @@ BlockBackendRootState 
>> *blk_get_root_state(BlockBackend *blk)
>>  
>>  int blk_commit_all(void)
>>  {
>> -    return bdrv_commit_all();
>> +    BlockBackend *blk = NULL;
>> +
>> +    while ((blk = blk_all_next(blk)) != NULL) {
>> +        AioContext *aio_context = blk_get_aio_context(blk);
>> +
>> +        aio_context_acquire(aio_context);
>> +        if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) {
> 
> blk->bs->drv is already implied by blk_is_available(), so it's
> unnecessary, even though it doesn't actively hurt.
> 
> Also, using blk_is_available() instead of blk_is_inserted() as in
> blk_flush_all() means that a drive with an open tray, but inserted
> medium isn't committed. I assume you did this on purpose because you're
> using two different functions in this patch, but isn't this a change in
> behaviour? If so, and we really want it, it should at least be mentioned
> in the commit message.

Drives with open trays are strange.

I found it reasonable that every medium in the system should be flushed
on blk_flush_all(), because flushing data should only bring the on-disk
state to a state it is supposed to be in anyway.

On the other hand, the commit operation actively changes data.
Therefore, I decided to treat it like a guest write operation. However,
considering that blk_commit_all() is basically only available through
HMP and is thus more of a legacy operation, I don't think its behavior
should be changed here. I'm therefore going to change this to
blk_is_inserted(), thanks.

Max

signature.asc
Description: OpenPGP digital signature