Re: [Libguestfs] [PATCH nbdkit 0/2] connections: Protect open and close callbacks with the request lock.

On 04/12/2018 12:43 PM, Richard W.M. Jones wrote: I'm debugging the testsuite breakage: the problem is a hang in parallel thread cleanup. For example, in test-parallel-nbd.sh, we have two nbdkit processes (one running plugin 'nbd', the other running 'file'). The hang is in the nbdkit running 'file'; it is spawning multiple threads to process work requests, but then does pthread_join() on each thread in order. But each worker thread is competing to obtain read_lock prior to read()ing from the client. So, unless we get lucky that the scheduler wakes up the threads in the right order, the pthread_join is waiting for the wrong thread (one that is blocked on obtaining read_lock), while the thread that DOES have read_lock is blocked waiting for recv() from the client to pass in another command or to indicate EOF. Why did your patch expose it? It's because pre-patch, the nbdkit running 'nbd' closed the socket early without the locks (the nbdkit running 'file' then gets a return of -1 from recv(), and from there, each worker thread quickly exits, so that the while loop joining threads gets to complete); while with your patch, the nbdkit running 'nbd' does not close the socket until the unload_prevention_lock gives the all clear (the nbdkit running 'file' is thus hung waiting on recv(), even though it already knows that the client will be shutting down soon). But there's also a problem with the nbdkit running 'nbd': it is trying to pthread_join() the reader thread (to see if the server has any remaining replies); pre-patch, there was no problem joining the thread, but post-patch, both nbdkit processes are stuck in a read() waiting on the other process. I'm still trying to come up with an appropriate patch (or maybe even two patches). I think it is desirable to continue read()ing from the client even after the client has requested shutdown (NBD_CMD_DISC), if only to gracefully reply with an error message to all subsequent commands erroneously sent by an ill-behaved client. But it also seems like it is fairly obvious that once we are in shutdown mode, parallel processing is no longer important, and that it is pointless to have threads compete on the read lock if a well-behaved client will not be sending any more data, rather than blocking the worker threads on a read() that won't complete until the client actually hangs up. So the nbdkit running 'file' needs to be more robust (perhaps once we have detected that the client has requested shutdown, then limit all further read()s to a worker thread 0, so that all other threads eventually get reaped quickly); but also the nbdkit running 'nbd' needs to be fixed to not keep the socket around forever. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org