Re: [Libguestfs] [PATCH nbdkit] cow: Use more fine-grained locking.

On 2/19/21 9:43 AM, Richard W.M. Jones wrote: Nice speedups. In my review, I'm double-checking whether my recent proposal to advertise multi-conn will be impacted in any way. So pre-patch, we were locking both the bitmap access and the next_ops->act(). Without reading further, it looks like your goal is to move next_ops->act() out of the lock. Pre-patch, if two threads compete on a read and write, we have exactly one of two outcomes: threadA threadB CMD_READ CMD_WRITE lock blocked on lock... state = not allocated read old data return lock write new data, update state return or threadA threadB CMD_READ CMD_WRITE blocked on lock... lock write new data, update state return lock state = allocated read new data return As we were locking per-block, we could shred large requests (a read could see some old blocks and some new ones), but shredding is already problem for the client to worry about and not unique to our cow filter. The new code now allows reading old data to occur in parallel with writing new data (for more performance), but otherwise doesn't seem to be any worse in behavior; I still only see one of two valid outcomes (the read either sees an entirely old block, or an entirely new block). Large reads may still be shredded by block, but a client that is already avoiding overlapping reads at the same time as a write will not be any worse off. On the other hand, NBD_CMD_CACHE is not frequently used, so I'm less worried about this one being heavy-handed. pre-patch, if two threads contend on a write, we had: threadA threadB CMD_WRITE CMD_WRITE lock blocked on lock... write new data update map return lock write new data update map (no-op) return post-patch, we have: threadA threadB CMD_WRITE CMD_WRITE write new data write new data lock blocked on lock... update map return lock update map (no-op) return Because we always pwrite() exactly one block, we're depending on the OS to guarantee that we end up with exactly one of the two writes as our final state (for client transactions larger than a block, shredding is possible, but that was true without this patch and as above with reads, dealing with shredding is already the responsibility of the client to avoid overlapping I/O). Again, looks safe to me. Looks a bit odd to move the #include, but not wrong. They would be even more infrequent if I ever got around to finishing my patches on exposing block sizing from plugins and filters all the way back to the client... But not this patch's problem. Our extents picture is no longer atomic, but I don't think that makes us any worse off. I agree with your analysis that any read issued by the client on one connection after a write/flush has completed on another connection will see only the cached data. The only race where we can see stale data on a read is when the read is done in parallel with an outstanding write or flush - but those aren't the cases we worry about when deciding whether multi-conn makes sense. So I don't think anything in this patch interferes with my patch to enable multi-conn. ACK. Looks like a nice job! -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org