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
