From: Paolo Bonzini <pbonzini@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
Emanuele Giuseppe Esposito <eesposit@redhat.com>,
qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, John Snow <jsnow@redhat.com>,
qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>
Subject: Re: [RFC PATCH 0/3] block-copy: lock tasks and calls list
Date: Wed, 21 Apr 2021 10:38:58 +0200 [thread overview]
Message-ID: <98773aac-c971-1451-3d12-aa14c0bd2d40@redhat.com> (raw)
In-Reply-To: <570d2007-fdb9-d90c-4ea9-32a6d1dd14dc@virtuozzo.com>
On 20/04/21 15:12, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2021 13:04, Emanuele Giuseppe Esposito wrote:
>> This serie of patches continues Paolo's series on making the
>> block layer thread safe. Add a CoMutex lock for both tasks and
>> calls list present in block/block-copy.c
>>
>
> I think, we need more information about what kind of thread-safety we
> want. Should the whole interface of block-copy be thread safe? Or only
> part of it? What is going to be shared between different threads? Which
> functions will be called concurrently from different threads? This
> should be documented in include/block/block-copy.h.
I guess all of it. So more state fields should be identified in
BlockCopyState, especially in_flight_bytes. ProgressMeter and
SharedResource should be made thread-safe on their own, just like the
patch I posted for RateLimit.
> What I see here, is that some things are protected by mutex.. Some
> things not. What became thread-safe?
>
> For example, in block_copy_dirty_clusters(), we modify task fields
> without any mutex held:
>
> block_copy_task_shrink doesn't take mutex.
> task->zeroes is set without mutex as well
Agreed, these are bugs in the series.
> Still all these accesses are done when task is already added to the list.
>
> Looping in block_copy_common() is not thread safe as well.
That one should be mostly safe, because only one coroutine ever writes
to all fields except cancelled. cancelled should be accessed with
qatomic_read/qatomic_set, but there's also the problem that coroutine
sleep/wake APIs are hard to use in a thread-safe manner (which affects
block_copy_kick). This is a different topic and it is something I'm
working on,
> You also forget to protect QLIST_REMOVE() call in block_copy_task_end()..
>
> Next, block-copy uses co-shared-resource API, which is not thread-safe
> (as it is directly noted in include/qemu/co-shared-resource.h).
>
> Same thing is block/aio_task API, which is not thread-safe too.
>
> So, we should bring thread-safety first to these smaller helper APIs.
Good point. Emanuele, can you work on ProgressMeter and SharedResource?
AioTaskPool can also be converted to just use CoQueue instead of
manually waking up coroutines.
next prev parent reply other threads:[~2021-04-21 8:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-20 10:04 [RFC PATCH 0/3] block-copy: lock tasks and calls list Emanuele Giuseppe Esposito
2021-04-20 10:04 ` [RFC PATCH 1/3] block-copy: improve documentation for BlockCopyTask type and functions Emanuele Giuseppe Esposito
2021-04-20 12:03 ` Vladimir Sementsov-Ogievskiy
2021-04-20 12:51 ` Emanuele Giuseppe Esposito
2021-04-20 13:11 ` Vladimir Sementsov-Ogievskiy
2021-04-20 10:04 ` [RFC PATCH 2/3] block-copy: add a CoMutex to the BlockCopyTask list Emanuele Giuseppe Esposito
2021-04-20 10:04 ` [RFC PATCH 3/3] block-copy: add CoMutex lock for BlockCopyCallState list Emanuele Giuseppe Esposito
2021-04-20 13:12 ` [RFC PATCH 0/3] block-copy: lock tasks and calls list Vladimir Sementsov-Ogievskiy
2021-04-21 8:38 ` Paolo Bonzini [this message]
2021-04-21 8:53 ` Vladimir Sementsov-Ogievskiy
2021-04-21 12:13 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98773aac-c971-1451-3d12-aa14c0bd2d40@redhat.com \
--to=pbonzini@redhat.com \
--cc=eesposit@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).