Re: [Libguestfs] [PATCH libnbd 4/4] lib: Atomically update h->state when leaving the locked region.

On 6/5/19 6:15 AM, Richard W.M. Jones wrote: The rest of this thread discusses potential other names, such as h->public_state for the state visible to unlocked code and h->state for internal while still locked; I trust that once you pick a naming scheme you like, you can redo this patch accordingly. I'll go ahead and review the changes as spelled here to at least see if I can spot any problems. Lockless code (including nbd_aio_get_direction) can no longer see intermediate states that did not block, which is a lot nicer for deciding whether we will accidentally let the user's poll() loop block forever if it didn't see that we were blocked on POLLIN or POLLOUT. There's still always a TOCTTOU race (anywhere the user reads the public state while another thread holds the lock, their future actions based on the observed state may be unexpected by the time they actually grab the lock because the other thread changed state), as well as the issue I mentioned in on 3/4 about any consecutive user calls to nbd_aio_is_* having a race where the two calls can be done on different public states. I'm still thinking that for any functions that grab the lock but want to do gating on state MUST save the public state check until after grabbing the lock, not beforehand, to avoid the TOCTTOU race. I'm also wondering if we should treat external events more like interrupts, where calling nbd_aio_notify_read() in an incorrect state succeeds by setting a flag that is not cleared until we later reach a state that can actually care about the flag, rather than by rejecting the call out-right for being in the wrong state. (That is, states where we are NOT expecting a notify_read() are the same as setting an IRQ mask that we don't want to be interrupted by a a read notification yet, but we don't want to lose the interrupt when we later clear the mask) In fact, CmdIssue is somewhat already like this - the code determines if the state machine is unmasked (in the READY state) to kick it off immediately; or is masked (in a processing state) to queue things but leave the interrupt set (the next time we enter READY, we unmask, observe the queue is non-empty, and so fire off the next command). Another thought - if we ever want to allow the user the ability to send custom NBD_OPT_ commands during handshake, or even to emulate 'qemu-nbd --list' where the client can make queries to see what the server supports before finally settling on whether to run NBD_OPT_GO or NBD_OPT_ABORT, we'll need to add an external event after nbd_aio_connect but before nbd_aio_is_connected for doing those additional handshake steps. It's easier to think about adding a mandatory nbd_aio_go() called in between nbd_aio_connect*() and the first nbd_aio_pread now, before we have to start worrying about back-compat issues to all existing AIO clients. As said above, I think you're still missing a change at the beginning of the wrapper that grabs the lock to do the state check after the lock. Otherwise the patch looks like you caught the right places. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org