qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Emanuele Giuseppe Esposito <eesposit@redhat.com>,
	qemu-block@nongnu.org
Cc: John Snow <jsnow@redhat.com>, Kevin Wolf <kwolf@redhat.com>,
	Max Reitz <mreitz@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [RFC PATCH 0/3] block-copy: lock tasks and calls list
Date: Wed, 21 Apr 2021 11:53:07 +0300	[thread overview]
Message-ID: <27d07471-76f0-5c3a-79f8-824663ec14a2@virtuozzo.com> (raw)
In-Reply-To: <98773aac-c971-1451-3d12-aa14c0bd2d40@redhat.com>

21.04.2021 11:38, Paolo Bonzini wrote:
> 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.
> 

That would be great.

I have one more question in mind:

Is it effective to use CoMutex here? We are protecting only some fast manipulations with data, not io path or something like that. Will simple QemuMutex work better? Even if CoMutex doesn't have any overhead, I don't think than if thread A wants to modify task list, but mutex is held by thread B (for similar thing), there is a reason for thread A to yield and do some other things: it can just wait several moments on mutex while B is modifying task list..

-- 
Best regards,
Vladimir


  reply	other threads:[~2021-04-21  8:54 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
2021-04-21  8:53     ` Vladimir Sementsov-Ogievskiy [this message]
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=27d07471-76f0-5c3a-79f8-824663ec14a2@virtuozzo.com \
    --to=vsementsov@virtuozzo.com \
    --cc=eesposit@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /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).