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, Stefan Hajnoczi <stefanha@redhat.com>,
Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v4 6/6] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState
Date: Wed, 23 Jun 2021 12:06:49 +0200 [thread overview]
Message-ID: <248f3bd9-df61-829c-8db9-6669490e5ae0@redhat.com> (raw)
In-Reply-To: <5e019d88-3551-4a08-6a67-e0699dd4f72e@virtuozzo.com>
On 22/06/21 12:39, Vladimir Sementsov-Ogievskiy wrote:
> 22.06.2021 13:20, Paolo Bonzini wrote:
>> On 22/06/21 11:36, Vladimir Sementsov-Ogievskiy wrote:
>>> OK, I agree, let's keep it.
>>
>> You can also have a finished job, but get a stale value for
>> error_is_read or ret. The issue is not in getting the stale value per
>> se, but in block_copy_call_status's caller not expecting it.
>
> Hmm. So, do you mean that we can read ret and error_is_read ONLY after
> explicitly doing load_acquire(finished) and checking that it's true?
>
> That means that we must do it not in assertion (to not be compiled out):
>
> bool finished = load_acquire()
>
> assert(finished);
>
> ... read reat and error_is_read ...
In reality you must have synchronized in some other way; that outside
synchronization outside block-copy.c is what guarantees that the
assertion does not fail. The simplest cases are:
- a mutex: "unlock" counts as release, "lock" counts as acquire;
- a bottom half: "schedule" counts as release, running counts as acquire.
Therefore, removing the assertion would work just fine because the
synchronization would be like this:
write ret/error_is_read
write finished
trigger bottom half or something --> bottom half runs
read ret/error_is_read
So there is no need at all to read ->finished, much less to load it
outside the assertion. At the same time there are two problems with
"assert(qatomic_read(&call_state->finished))". Note that they are not
logic issues, they are maintenance issues.
First, if *there is a bug elsewhere* and the above synchronization
doesn't happen, it may manifest sometimes as an assertion failure and
sometimes as a memory reordering. This is what I was talking about in
the previous message, and it is probably the last thing that you want
when debugging; since we're adding asserts defensively, we might as well
do it well.
Second, if somebody later carelessly changes the function to
if (qatomic_read(&call_state->finished)) {
...
} else {
error_setg(...);
}
that would be broken. Using qatomic_load_acquire makes the code more
future-proof against a change like the one above.
Paolo
prev parent reply other threads:[~2021-06-23 10:07 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-14 7:33 [PATCH v4 0/6] block-copy: protect block-copy internal structures Emanuele Giuseppe Esposito
2021-06-14 7:33 ` [PATCH v4 1/6] block-copy: small refactor in block_copy_task_entry and block_copy_common Emanuele Giuseppe Esposito
2021-06-19 14:33 ` Vladimir Sementsov-Ogievskiy
2021-06-14 7:33 ` [PATCH v4 2/6] block-copy: streamline choice of copy_range vs. read/write Emanuele Giuseppe Esposito
2021-06-19 15:05 ` Vladimir Sementsov-Ogievskiy
2021-06-19 18:23 ` Vladimir Sementsov-Ogievskiy
2021-06-14 7:33 ` [PATCH v4 3/6] block-copy: improve comments of BlockCopyTask and BlockCopyState types and functions Emanuele Giuseppe Esposito
2021-06-19 15:23 ` Vladimir Sementsov-Ogievskiy
2021-06-19 18:31 ` Vladimir Sementsov-Ogievskiy
2021-06-21 8:13 ` Emanuele Giuseppe Esposito
2021-06-22 9:20 ` Vladimir Sementsov-Ogievskiy
2021-06-21 7:59 ` Emanuele Giuseppe Esposito
2021-06-22 9:16 ` Vladimir Sementsov-Ogievskiy
2021-06-19 17:27 ` Vladimir Sementsov-Ogievskiy
2021-06-21 8:21 ` Emanuele Giuseppe Esposito
2021-06-19 18:53 ` Vladimir Sementsov-Ogievskiy
2021-06-21 8:28 ` Emanuele Giuseppe Esposito
2021-06-14 7:33 ` [PATCH v4 4/6] block-copy: move progress_set_remaining in block_copy_task_end Emanuele Giuseppe Esposito
2021-06-14 7:33 ` [PATCH v4 5/6] block-copy: add a CoMutex Emanuele Giuseppe Esposito
2021-06-19 19:34 ` Vladimir Sementsov-Ogievskiy
2021-06-14 7:33 ` [PATCH v4 6/6] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState Emanuele Giuseppe Esposito
2021-06-19 20:06 ` Vladimir Sementsov-Ogievskiy
2021-06-21 9:30 ` Emanuele Giuseppe Esposito
2021-06-22 9:56 ` Vladimir Sementsov-Ogievskiy
2021-06-22 8:15 ` Paolo Bonzini
2021-06-22 9:36 ` Vladimir Sementsov-Ogievskiy
2021-06-22 10:20 ` Paolo Bonzini
2021-06-22 10:39 ` Vladimir Sementsov-Ogievskiy
2021-06-22 20:57 ` Emanuele Giuseppe Esposito
2021-06-23 10:06 ` Paolo Bonzini [this message]
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=248f3bd9-df61-829c-8db9-6669490e5ae0@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=stefanha@redhat.com \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.