Re: [Qemu-devel] [Qemu-block] [PATCH 1/5] block: Add blklogwrites

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]