Remove the confusing, and most likely wrong, atomics. The only function
that used to be somewhat in a hot path was nbd_client_connected(),
but it is not anymore after the previous patches.
The function nbd_client_connecting_wait() was used mostly to check if
a request had to be reissued (outside requests_lock), but also
under requests_lock in nbd_client_connecting_wait(). The two uses have to
be separated; for the former we rename it to nbd_client_will_reconnect()
and make it take s->requests_lock; for the latter the access can simply
be inlined. The new name is clearer, and ensures that a missing
conversion is caught by the compiler.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
block/nbd.c | 88 +++++++++++++++++++++++++++++------------------------
1 file changed, 48 insertions(+), 40 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index c908ea6ae3..6d80bd59e2 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -35,7 +35,6 @@
#include "qemu/option.h"
#include "qemu/cutils.h"
#include "qemu/main-loop.h"
-#include "qemu/atomic.h"
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qmp/qstring.h"
@@ -73,10 +72,11 @@ typedef struct BDRVNBDState {
NBDExportInfo info;
/*
- * Protects free_sema, in_flight, requests[].coroutine,
+ * Protects state, free_sema, in_flight, requests[].coroutine,
* reconnect_delay_timer.
*/
QemuMutex requests_lock;
+ NBDClientState state;
CoQueue free_sema;
int in_flight;
NBDClientRequest requests[MAX_NBD_REQUESTS];
@@ -84,7 +84,6 @@ typedef struct BDRVNBDState {
CoMutex send_mutex;
CoMutex receive_mutex;
- NBDClientState state;
QEMUTimer *open_timer;
@@ -133,11 +132,6 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs)
s->x_dirty_bitmap = NULL;
}
-static bool nbd_client_connected(BDRVNBDState *s)
-{
- return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED;
-}
-
static bool coroutine_fn nbd_recv_coroutine_wake_one(NBDClientRequest *req)
{
if (req->receiving) {
@@ -160,14 +154,15 @@ static void coroutine_fn
nbd_recv_coroutines_wake(BDRVNBDState *s, bool all)
}
}
-static void coroutine_fn nbd_channel_error(BDRVNBDState *s, int ret)
+/* Called with s->requests_lock held. */
+static void coroutine_fn nbd_channel_error_locked(BDRVNBDState *s, int ret)
{
- if (nbd_client_connected(s)) {
+ if (s->state == NBD_CLIENT_CONNECTED) {
qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}
if (ret == -EIO) {
- if (nbd_client_connected(s)) {
+ if (s->state == NBD_CLIENT_CONNECTED) {
s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
NBD_CLIENT_CONNECTING_NOWAIT;
}
@@ -178,6 +173,12 @@ static void coroutine_fn nbd_channel_error(BDRVNBDState
*s, int ret)
nbd_recv_coroutines_wake(s, true);
}
+static void coroutine_fn nbd_channel_error(BDRVNBDState *s, int ret)
+{
+ QEMU_LOCK_GUARD(&s->requests_lock);
+ nbd_channel_error_locked(s, ret);
+}
+
static void reconnect_delay_timer_del(BDRVNBDState *s)
{
if (s->reconnect_delay_timer) {
@@ -190,20 +191,18 @@ static void reconnect_delay_timer_cb(void *opaque)
{
BDRVNBDState *s = opaque;
- if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT) {
- s->state = NBD_CLIENT_CONNECTING_NOWAIT;
- nbd_co_establish_connection_cancel(s->conn);
- }
-
reconnect_delay_timer_del(s);
+ WITH_QEMU_LOCK_GUARD(&s->requests_lock) {
+ if (s->state != NBD_CLIENT_CONNECTING_WAIT) {
+ return;
+ }
+ s->state = NBD_CLIENT_CONNECTING_NOWAIT;
+ }
+ nbd_co_establish_connection_cancel(s->conn);
}
static void reconnect_delay_timer_init(BDRVNBDState *s, uint64_t
expire_time_ns)
{
- if (qatomic_load_acquire(&s->state) != NBD_CLIENT_CONNECTING_WAIT) {
- return;
- }
-
assert(!s->reconnect_delay_timer);
s->reconnect_delay_timer = aio_timer_new(bdrv_get_aio_context(s->bs),
QEMU_CLOCK_REALTIME,
@@ -226,7 +225,9 @@ static void nbd_teardown_connection(BlockDriverState *bs)
s->ioc = NULL;
}
- s->state = NBD_CLIENT_QUIT;
+ WITH_QEMU_LOCK_GUARD(&s->requests_lock) {
+ s->state = NBD_CLIENT_QUIT;
+ }
}
static void open_timer_del(BDRVNBDState *s)
@@ -255,16 +256,13 @@ static void open_timer_init(BDRVNBDState *s, uint64_t
expire_time_ns)
timer_mod(s->open_timer, expire_time_ns);
}
-static bool nbd_client_connecting(BDRVNBDState *s)
+static bool nbd_client_will_reconnect(BDRVNBDState *s)
{
- NBDClientState state = qatomic_load_acquire(&s->state);
- return state == NBD_CLIENT_CONNECTING_WAIT ||
- state == NBD_CLIENT_CONNECTING_NOWAIT;
-}
-
-static bool nbd_client_connecting_wait(BDRVNBDState *s)
-{
- return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
+ /*
+ * Called only after a socket error, so this is not performance sensitive.
+ */
+ QEMU_LOCK_GUARD(&s->requests_lock);
+ return s->state == NBD_CLIENT_CONNECTING_WAIT;
}
/*
@@ -351,15 +349,24 @@ int coroutine_fn
nbd_co_do_establish_connection(BlockDriverState *bs,
qio_channel_attach_aio_context(s->ioc, bdrv_get_aio_context(bs));
/* successfully connected */
- s->state = NBD_CLIENT_CONNECTED;
+ WITH_QEMU_LOCK_GUARD(&s->requests_lock) {
+ s->state = NBD_CLIENT_CONNECTED;
+ }
return 0;
}
+/* Called with s->requests_lock held. */
+static bool nbd_client_connecting(BDRVNBDState *s)
+{
+ return s->state == NBD_CLIENT_CONNECTING_WAIT ||
+ s->state == NBD_CLIENT_CONNECTING_NOWAIT;
+}
+
/* Called with s->requests_lock taken. */
static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
{
- bool blocking = nbd_client_connecting_wait(s);
+ bool blocking = s->state == NBD_CLIENT_CONNECTING_WAIT;
/*
* Now we are sure that nobody is accessing the channel, and no one will
@@ -475,17 +482,17 @@ static int coroutine_fn
nbd_co_send_request(BlockDriverState *bs,
qemu_mutex_lock(&s->requests_lock);
while (s->in_flight == MAX_NBD_REQUESTS ||
- (!nbd_client_connected(s) && s->in_flight > 0)) {
+ (s->state != NBD_CLIENT_CONNECTED && s->in_flight > 0)) {
qemu_co_queue_wait(&s->free_sema, &s->requests_lock);
}
s->in_flight++;
- if (!nbd_client_connected(s)) {
+ if (s->state != NBD_CLIENT_CONNECTED) {
if (nbd_client_connecting(s)) {
nbd_reconnect_attempt(s);
qemu_co_queue_restart_all(&s->free_sema);
}
- if (!nbd_client_connected(s)) {
+ if (s->state != NBD_CLIENT_CONNECTED) {
rc = -EIO;
goto err;
}
@@ -529,7 +536,7 @@ static int coroutine_fn
nbd_co_send_request(BlockDriverState *bs,
if (rc < 0) {
qemu_mutex_lock(&s->requests_lock);
err:
- nbd_channel_error(s, rc);
+ nbd_channel_error_locked(s, rc);
if (i != -1) {
s->requests[i].coroutine = NULL;
}
@@ -1193,7 +1200,7 @@ static int coroutine_fn nbd_co_request(BlockDriverState
*bs, NBDRequest *request
error_free(local_err);
local_err = NULL;
}
- } while (ret < 0 && nbd_client_connecting_wait(s));
+ } while (ret < 0 && nbd_client_will_reconnect(s));
return ret ? ret : request_ret;
}
@@ -1252,7 +1259,7 @@ static int coroutine_fn
nbd_client_co_preadv(BlockDriverState *bs, int64_t offse
error_free(local_err);
local_err = NULL;
}
- } while (ret < 0 && nbd_client_connecting_wait(s));
+ } while (ret < 0 && nbd_client_will_reconnect(s));
return ret ? ret : request_ret;
}
@@ -1410,7 +1417,7 @@ static int coroutine_fn nbd_client_co_block_status(
error_free(local_err);
local_err = NULL;
}
- } while (ret < 0 && nbd_client_connecting_wait(s));
+ } while (ret < 0 && nbd_client_will_reconnect(s));
if (ret < 0 || request_ret < 0) {
return ret ? ret : request_ret;
@@ -1442,8 +1449,9 @@ static void nbd_yank(void *opaque)
BlockDriverState *bs = opaque;
BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
- qatomic_store_release(&s->state, NBD_CLIENT_QUIT);
+ QEMU_LOCK_GUARD(&s->requests_lock);
qio_channel_shutdown(QIO_CHANNEL(s->ioc), QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+ s->state = NBD_CLIENT_QUIT;
}
static void nbd_client_close(BlockDriverState *bs)
--
2.35.1
