[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] block: Add blklogwrites
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] block: Add blklogwrites |
Date: |
Mon, 4 Jun 2018 10:59:48 +0100 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
On Fri, Jun 01, 2018 at 07:26:03AM -0500, Eric Blake wrote:
> On 05/31/2018 04:17 PM, Ari Sundholm wrote:
> > +static void blk_log_writes_co_do_file(void *opaque)
> > +{
> > + BlkLogWritesFileReq *fr = opaque;
> > +
> > + fr->file_ret = fr->func(fr);
> > +
> > + fr->r->done++;
>
> Two non-atomic increments...
>
> > + qemu_coroutine_enter_if_inactive(fr->r->co);
> > +}
> > +
> > +static int coroutine_fn
> > +blk_log_writes_co_log(BlockDriverState *bs, uint64_t offset, uint64_t
> > bytes,
> > + QEMUIOVector *qiov, int flags,
> > + int (*file_func)(BlkLogWritesFileReq *r),
> > + uint64_t entry_flags)
> > +{
>
> > + qemu_coroutine_enter(co_file);
> > + qemu_coroutine_enter(co_log);
> > +
> > + while (r.done < 2) {
> > + qemu_coroutine_yield();
> > + }
>
> ...used as the condition for waiting. Since the point of coroutines is to
> allow (restricted) parallel operation, there's a chance that the coroutine
> implementation can be utilizing parallel threads; if that's the case, then
> on the rare race when both threads try to increment at near the same time,
> they can both read 0 and write 1, at which point this wait loop would be an
> infinite loop. You're probably better off using atomics (even if I'm wrong
> about coroutines being able to race each other on the increment, as the
> other point of coroutines is that they provide restricted parallelism where
> you can also implement them in only a single thread because of well-defined
> yield points).
In this case the coroutines run from a single event loop (the
BlockDriverState's AioContext) so they cannot race.
As QEMU transitions to a multi-queue block layer we will need to think
about parallelism more. But the multi-queue block layer isn't
implemented yet, so I prefer writing straightforward code now without
trying to anticipate what parallelism issues might arise in the future.
Stefan
signature.asc
Description: PGP signature