Re: [Qemu-devel] [PATCH 12/21] block: Use BlockBackend more

[Max Reitz <mreitz@redhat.com>]

On 2015-01-29 at 20:12, Eric Blake wrote:
> On 01/26/2015 12:27 PM, Max Reitz wrote:
>> Replace bdrv_drain_all(), bdrv_commmit_all(), bdrv_flush_all(),
>> bdrv_invalidate_cache_all(), bdrv_next() and occurrences of bdrv_states
>> by their BlockBackend equivalents.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 22 ++++++++---------
>>   block/block-backend.c |  8 +++----
>>   block/qapi.c          | 13 ++++++++--
>>   block/snapshot.c      |  3 ++-
>>   blockdev.c            | 14 ++++++-----
>>   cpus.c                |  7 +++---
>>   migration/block.c     | 10 +++++---
>>   migration/migration.c |  4 ++--
>>   monitor.c             | 13 ++++++----
>>   qemu-char.c           |  3 ++-
>>   qemu-io.c             |  2 +-
>>   qmp.c                 | 14 +++++------
>>   savevm.c              | 66 ++++++++++++++++++++++++++++++---------------------
>>   xen-mapcache.c        |  3 ++-
>>   14 files changed, 107 insertions(+), 75 deletions(-)
>>
>> +++ b/block/qapi.c
>> @@ -393,15 +393,24 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
>>   {
>>       BlockStatsList *head = NULL, **p_next = &head;
>>       BlockDriverState *bs = NULL;
>> +    BlockBackend *blk = NULL;
>>   
>>       /* Just to be safe if query_nodes is not always initialized */
>>       query_nodes = has_query_nodes && query_nodes;
>>   
>> -    while ((bs = query_nodes ? bdrv_next_node(bs) : bdrv_next(bs))) {
>> +    while (query_nodes ? (bs = bdrv_next_node(bs)) != NULL
>> +                       : (blk = blk_next_inserted(blk)) != NULL)
> Don't we still want to list empty BBs in the query (that is,
> intentionally list that there are no stats because there is no medium)
> rather than silently omitting them?

Well, maybe, but this patch does not change that behavior. Actually, the 
behavior is changed in the "BlockBackend and media" series, and I guess 
it's probably the patch "blockdev: Do not create BDS for empty drive". 
As of that series in general, there is no BDS for empty drives any more 
and subsequently there are no blockstats any more.

So if anywhere, I'd need to fix it in that series, probably in the "Move 
BlockAcctStats into BlockBackend" patch.

>> @@ -348,6 +349,7 @@ static void unset_dirty_tracking(void)
>>   static void init_blk_migration(QEMUFile *f)
>>   {
>>       BlockDriverState *bs;
>> +    BlockBackend *blk = NULL;
>>       BlkMigDevState *bmds;
>>       int64_t sectors;
>>   
>> @@ -359,7 +361,9 @@ static void init_blk_migration(QEMUFile *f)
>>       block_mig_state.bulk_completed = 0;
>>       block_mig_state.zero_blocks = migrate_zero_blocks();
>>   
>> -    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
>> +    while ((blk = blk_next_inserted(blk)) != NULL) {
>> +        bs = blk_bs(blk);
>> +
> What happens if someone initiates a live migration, then inserts the
> medium?  It looks like this will skip over the drive that was empty at
> the time the migration started.

Well, to me it looks like its your own fault then. Also, there is no 
difference between using blk_next_inserted() like here and using 
blk_next() + blk_is_inserted(), so you probably can still insert a 
medium during this loop. Maybe. I'm not sure, but I don't think anything 
changed from the old bdrv_next() behavior.

(blk_bext_inserted() iterates over all the BlockBackends and if the one 
you're looking for is not inserted by the time the BB is iterated over, 
it is skipped; if it is inserted, it is not skipped. It doesn't matter 
whether it was inserted at the beginning.)

Max