Re: (ITS#8209) Broken MDB_CP_COMPACT threading

[h.b.furuseth@usit.uio.no]

Okay, that was one confused ITS in response to equally
confused code.  Trying again.   Draft fixes in my updated
branch "mdb/copyfd1" (non-fast-forward).


It's broken if mdb_env_copythr() has a write error:

* copyfd1 can (A) overrun buffer mc_wlen, and (B) hang, if
   if cthr_toggle() is in cond_wait() when the error occurs:

   It sets mc_new and returns success.  cwalk() proceeds to write
   at mc_wbuf+mc_wlen, but copythr() did not reset mc_wlen. Then
   copyfd1() waits for the now-dead copythr() to clear mc_new.

   Fixes:
   (A) join the if(mc_status) and while(mc_new==1) in cthr_toggle.
   (B) Drop the wait before THREAD_FINISH(), it seems pointless.
       Both Posix and Windows doc say that THREAD_FINISH waits.
       Or if Windows needs it for some reason, then copythr must
       live until it receives mc_new=-1, so it can set mc_new=0.

* copyfd1() loses any error from the final cthr_toggle().

   Fix: Return rc ? rc : my.mc_status.

Tested on Linux with fake write errors, with/without fixes.


While not a bug, I also see no point in both threads' initial
waits.  copyfd1() can own the buffer initially (set mc_new=0).
I'm guessing the waits are there because the code tries to guide
the threads from step to step, but it broke because you didn't
think of every possibility.  It's easier to define and maintain
one cond_wait state per thread and keep things consistent there.


More on error checking:

copyfd1() omits most error checking and the cleanup after errors.
E.g. it does not tell copythr to finish if txn_begin fails.

Each thread is slow to react to errors in the other thread. I/O
will cease quicker if both threads set and check mc_status.  If
so, mc_status can no longer be part of the cond_wait condition,
since it won't be mutex-protected.  So the threads might chat a
bit more before finishing.

I'd rename mc_status -> mc_error. "while (!my->mc_error && ...)"
is quite self-explanatory, unlike "while (!my->mc_status && ...)".


About lock/unlock outside vs. inside the loop:
When outside, copythr() must block for the threads to exchange
buffers.  When inside, the slowest thread does not block, but
there are more lock/unlock/wait calls.  Testing: Locks inside is
clearly faster on tmpfs.  On a regular FS I don't know - it may
have been a bit slower, but times varied more than the difference.


Finally, cwalk() could re-use the page buffers it mallocs.

-- 
Hallvard