[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/3] block: make BlockBackend->disable_request_queuing ato
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH v2 2/3] block: make BlockBackend->disable_request_queuing atomic |
Date: |
Thu, 9 Mar 2023 07:12:39 -0500 |
On Thu, Mar 09, 2023 at 12:18:00PM +0100, Paolo Bonzini wrote:
> On 3/7/23 22:04, Stefan Hajnoczi wrote:
> > static int coroutine_fn GRAPH_RDLOCK
> > @@ -1271,7 +1271,8 @@ static void coroutine_fn
> > blk_wait_while_drained(BlockBackend *blk)
> > {
> > assert(blk->in_flight > 0);
> > - if (qatomic_read(&blk->quiesce_counter) &&
> > !blk->disable_request_queuing) {
> > + if (qatomic_read(&blk->quiesce_counter) &&
> > + !qatomic_read(&blk->disable_request_queuing)) {
>
> The qatomic_inc in blk_inc_in_flight() made me a bit nervous that
> smp_mb__after_rmw() was needed there, but it's okay.
Yes. I wrote it under the assumption that sequentially consistent
operations like qatomic_inc() are implicit barriers.
> First, anyway blk_wait_while_drained() has to _eventually_ pause the device,
> not immediately. Even if it misses that blk->quiesce_counter == 1, the I/O
> will proceed and it'll just take a little more polling before
> bdrv_drained_begin() exits.
>
> Second, I checked with CPPMEM the barriers in AIO_WAIT_WHILE() and
> aio_wait_kick() save the day, even if loading blk->quiesce_counter is
> reordered before the incremented value (1) is stored to blk->in_flight.
>
> The CPPMEM model here uses mo_relaxed to force all possible kinds of havoc:
>
> int main() {
> atomic_int quiesce_counter = 0;
> atomic_int waiters = 0;
> atomic_int in_flight = 0;
>
> {{{ { quiesce_counter.store(1, mo_relaxed);
> waiters.store(1, mo_relaxed); // AIO_WAIT_WHILE starts here
> atomic_thread_fence(mo_seq_cst);
> in_flight.load(mo_relaxed).readsvalue(1); } // if 1, sleep
>
> ||| { in_flight.store(1, mo_relaxed); // bdrv_inc_in_flight
> quiesce_counter.load(mo_relaxed).readsvalue(1); // go down "if"
> in_flight.store(0, mo_release); // bdrv_dec_in_flight
> atomic_thread_fence(mo_seq_cst); // aio_wait_kick starts here
> waiters.load(mo_relaxed).readsvalue(0); } // if 0, do not wake
> }}};
>
> return 0;
> }
>
>
> Because CPPMEM shows no execution consistent with the buggy .readsvalue(),
> either AIO_WAIT_WHILE will not go to sleep or it will be woken up with
> in_flight == 0. The polling loop ends and drained_end restarts the
> coroutine from blk->queued_requests.
Okay.
Stefan
signature.asc
Description: PGP signature