Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 09/25/2014 08:20 PM, Kevin Wolf wrote:
> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
>> Right. Cool. So is below what was suggested? I am doublechecking as it does
>> not solve the original issue - the bottomhalf is called first and then
>> nbd_trip() crashes in qcow2_co_flush_to_os().
>>
>> diff --git a/block.c b/block.c
>> index d06dd51..1e6dfd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
>> Error **errp)
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>
>>      ret = refresh_total_sectors(bs, bs->total_sectors);
>>      if (ret < 0) {
>>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>>          return;
>>      }
>> +
>> +    bdrv_drain_all();
>>  }
> 
> Try moving the bdrv_drain_all() call to the top of the function (at
> least it must be called before bs->drv->bdrv_invalidate_cache).


Ok, I did. Did not help.


> 
>> +static QEMUBH *migration_complete_bh;
>> +static void process_incoming_migration_complete(void *opaque);
>> +
>>  static void process_incoming_migration_co(void *opaque)
>>  {
>>      QEMUFile *f = opaque;
>> -    Error *local_err = NULL;
>>      int ret;
>>
>>      ret = qemu_loadvm_state(f);
>>      qemu_fclose(f);
> 
> Paolo suggested to move eveything starting from here, but as far as I
> can tell, leaving the next few lines here shouldn't hurt.


Ouch. I was looking at wrong qcow2_fclose() all this time :)
Aaaany what you suggested did not help -
bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
executed and the situation is still the same.


> 
>>      free_xbzrle_decoded_buf();
>>      if (ret < 0) {
>>          error_report("load of migration failed: %s", strerror(-ret));
>>          exit(EXIT_FAILURE);
>>      }
>>      qemu_announce_self();
>>
>>      bdrv_clear_incoming_migration_all();
>> +
>> +    migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
>> +                                       process_incoming_migration_complete,
>> +                                       NULL);
>> +    qemu_bh_schedule(migration_complete_bh);
>> +}
>> +
>> +static void process_incoming_migration_complete(void *opaque)
>> +{
>> +    Error *local_err = NULL;
>> +
>>      /* Make sure all file formats flush their mutable metadata */
>>      bdrv_invalidate_cache_all(&local_err);
>>      if (local_err) {
>>          qerror_report_err(local_err);
>>          error_free(local_err);
>>          exit(EXIT_FAILURE);
>>      }
>>
>>      if (autostart) {
>>          vm_start();
>>      } else {
>>          runstate_set(RUN_STATE_PAUSED);
>>      }
>> +    qemu_bh_delete(migration_complete_bh);
>> +    migration_complete_bh = NULL;
>>  }
> 
> That part looks good to me. I hope moving bdrv_drain_all() does the
> trick, otherwise there's somthing wrong with our reasoning.
> 
> Kevin
> 


-- 
Alexey