qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 4/8] qcow2-threads: split out generic path


From: Alberto Garcia
Subject: Re: [Qemu-devel] [PATCH v2 4/8] qcow2-threads: split out generic path
Date: Fri, 04 Jan 2019 15:59:03 +0100
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

On Fri 14 Dec 2018 09:51:12 AM CET, Vladimir Sementsov-Ogievskiy wrote:
> 14.12.2018 2:28, Paolo Bonzini wrote:
>> On 11/12/18 17:43, Vladimir Sementsov-Ogievskiy wrote:
>>> +    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
>>> +
>>> +    while (s->nb_threads >= QCOW2_MAX_THREADS) {
>>> +        qemu_co_queue_wait(&s->thread_task_queue, NULL);
>>> +    }
>>> +
>>> +    s->nb_threads++;
>>> +    ret = thread_pool_submit_co(pool, func, arg);
>>> +    s->nb_threads--;
>>> +
>>> +    qemu_co_queue_next(&s->thread_task_queue);
>> 
>> What lock is protecting this manipulation?  I'd rather avoid adding more
>> users of the AioContext lock, especially manipulation of CoQueues.
>> 
>> One possibility is to do the s->lock unlock/lock here, and then also
>> rely on that in patch 8.
>
> Hm, can you please give more details? It's refactoring commit, so, do
> you mean that there is an existing bug?

I think there's an existing problem but not a currently reproducible bug
as such: accesses to the qcow2 BlockDriverState are protected by the
same AioContext lock, but as soon as you want to implement multiqueue
you'll have multiple iothreads (and contexts) accessing the same BDS.

For that to work you need that the in-memory qcow2 metadata is properly
locked, and that's what s->lock is for. However this CoQueue here is not
protected by any lock (s->lock is taken _after_ the data is compressed
in qcow2_co_pwritev_compressed()).

And I think that's it in a nutshell, Paolo correct me if I'm wrong.

Berto



reply via email to

[Prev in Thread] Current Thread [Next in Thread]