Re: [Qemu-block] [Qemu-devel] [PATCH 12/17] block: protect tracked_requests and flush_queue with reqs_lock

[Paolo Bonzini]

On 04/05/2017 09:30, Fam Zheng wrote:
>> @@ -2302,11 +2308,13 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>>      current_gen = atomic_read(&bs->write_gen);
>>  
>>      /* Wait until any previous flushes are completed */
>> +    qemu_co_mutex_lock(&bs->reqs_lock);
>>      while (bs->active_flush_req) {
>> -        qemu_co_queue_wait(&bs->flush_queue, NULL);
>> +        qemu_co_queue_wait(&bs->flush_queue, &bs->reqs_lock);
>>      }
>>  
>>      bs->active_flush_req = true;
>> +    qemu_co_mutex_unlock(&bs->reqs_lock);
>>  
>>      /* Write back all layers by calling one driver function */
>>      if (bs->drv->bdrv_co_flush) {
>> @@ -2328,10 +2336,14 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>>          goto flush_parent;
>>      }
>>  
>> -    /* Check if we really need to flush anything */
>> +    /* Check if we really need to flush anything
>> +     * TODO: use int and atomic access */
>> +    qemu_co_mutex_lock(&bs->reqs_lock);
>>      if (bs->flushed_gen == current_gen) {
> 
> Should the atomic reading of current_gen be moved down here, to avoid TOCTOU?

No, but another change is needed; current_gen needs to be read under the
lock, to ensure that flushes are processed in increasing generation order.

In addition, this access to flushed_gen does not need the lock;
bs->active_flush_req itself acts as a "lock" for bs->flushed_gen, only
the coroutine that set it to true will read it or write it.  I'll adjust
this patch accordingly.

Paolo