On 4/16/22 14:18, Vladimir Sementsov-Ogievskiy wrote:
14.04.2022 20:57, Paolo Bonzini wrote:
The condition for waiting on the s->free_sema queue depends on
both s->in_flight and s->state. The latter is currently using
atomics, but this is quite dubious and probably wrong.
Because s->state is written in the main thread too, for example by
the yank callback, it cannot be protected by a CoMutex. Introduce a
separate lock that can be used by nbd_co_send_request(); later on this
lock will also be used for s->state. There will not be any contention
on the lock unless there is a yank or reconnect, so this is not
performance sensitive.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
block/nbd.c | 46 +++++++++++++++++++++++++++-------------------
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 62dd338ef3..a2414566d1 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -71,17 +71,22 @@ typedef struct BDRVNBDState {
QIOChannel *ioc; /* The current I/O channel */
NBDExportInfo info;
- CoMutex send_mutex;
+ /*
+ * Protects free_sema, in_flight, requests[].coroutine,
+ * reconnect_delay_timer.
+ */
requests[].coroutine is read without mutex in nbd_receive_replies
reconnect_delay_timer_del() is called without mutex in
nbd_cancel_in_flight() and in reconnect_delay_timer_cb()..
Is it OK? Worth improving the comment?
Yeah, let me check the reconnect_delay_timer idea you had in the
previous patch, too.
Prior to this patch, if request sending fails, we'll not send further
requests. After the patch, we can send more requests after failure on
unlocking send_mutex.
What's wrong if we keep send_mutex critical section as is and just lock
requests_lock additionally inside send_mutex-critical-section?
I wanted to keep send_mutex just for the socket, basically. The cse you
point out exists but it is harmless.
Paolo
