From a231e6283e1c79a33d6f3bdf786832f10e355d0c Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Fri, 17 Jul 2020 18:19:58 +0200 Subject: [KERNEL] Trying to get locking under control Current code doesn't pay much attention to properly locking when accessing structures that are being used in multiple workers/threads. Try to fix this, slowly. --- src/kernel/core.c | 12 ++++--- src/kernel/dnbd3.h | 1 - src/kernel/mq.c | 55 +++++++++++++++++--------------- src/kernel/net-txrx.c | 2 +- src/kernel/net.c | 86 ++++++++++++++++++++------------------------------- 5 files changed, 71 insertions(+), 85 deletions(-) diff --git a/src/kernel/core.c b/src/kernel/core.c index 35ea1ef..8da2b39 100644 --- a/src/kernel/core.c +++ b/src/kernel/core.c @@ -124,6 +124,7 @@ static int dnbd3_ioctl(struct block_device *bdev, fmode_t mode, switch (cmd) { case IOCTL_OPEN: debug_dev(dev, "ioctl open"); + mutex_lock(&dev->device_lock); if (dev->imgname != NULL) { result = -EBUSY; } else if (imgname == NULL) { @@ -131,7 +132,6 @@ static int dnbd3_ioctl(struct block_device *bdev, fmode_t mode, } else if (msg == NULL) { result = -EINVAL; } else { - mutex_lock(&dev->device_lock); if (sizeof(msg->host) != sizeof(dnbd3_host_t)) { warn_dev(dev, "odd size bug#1 triggered in ioctl"); } @@ -154,7 +154,6 @@ static int dnbd3_ioctl(struct block_device *bdev, fmode_t mode, } else { dev->number_connections = msg->number_connections; } - mutex_unlock(&dev->device_lock); result = dnbd3_net_connect(dev); if (result) { @@ -165,10 +164,12 @@ static int dnbd3_ioctl(struct block_device *bdev, fmode_t mode, } imgname = NULL; } + mutex_unlock(&dev->device_lock); break; case IOCTL_CLOSE: debug_dev(dev, "ioctl close"); + mutex_lock(&dev->device_lock); result = dnbd3_net_disconnect(dev); set_capacity(dev->disk, 0); if (dev->imgname) { @@ -177,6 +178,7 @@ static int dnbd3_ioctl(struct block_device *bdev, fmode_t mode, } dev->rid = 0; dev->reported_size = 0; + mutex_unlock(&dev->device_lock); break; case IOCTL_SWITCH: @@ -187,6 +189,7 @@ static int dnbd3_ioctl(struct block_device *bdev, fmode_t mode, case IOCTL_ADD_SRV: case IOCTL_REM_SRV: debug_dev(dev, "ioctl add/rem srv"); + mutex_lock(&dev->device_lock); if (dev->imgname == NULL) { result = -ENOENT; } else if (dev->new_servers_num >= NUMBER_SERVERS) { @@ -194,16 +197,15 @@ static int dnbd3_ioctl(struct block_device *bdev, fmode_t mode, } else if (msg == NULL) { result = -EINVAL; } else { - mutex_lock(&dev->device_lock); memcpy(&dev->new_servers[dev->new_servers_num].host, &msg->host, sizeof(msg->host)); /* 0 = ADD, 1 = REM */ dev->new_servers[dev->new_servers_num].failures = (cmd == IOCTL_ADD_SRV ? 0 : 1); dev->new_servers_num++; - mutex_unlock(&dev->device_lock); result = 0; } + mutex_unlock(&dev->device_lock); break; case BLKFLSBUF: @@ -417,7 +419,7 @@ static void dnbd3_dev_remove(struct dnbd3_device *dev) struct gendisk *disk = dev->disk; struct request_queue *q; - if (dev->connected) { + if (dev->socks) { dnbd3_net_disconnect(dev); } diff --git a/src/kernel/dnbd3.h b/src/kernel/dnbd3.h index f8d4bae..87145f8 100644 --- a/src/kernel/dnbd3.h +++ b/src/kernel/dnbd3.h @@ -138,7 +138,6 @@ struct dnbd3_device { struct mutex device_lock; - bool connected; uint8_t number_connections; struct dnbd3_sock *socks; char *imgname; diff --git a/src/kernel/mq.c b/src/kernel/mq.c index f9e1bd2..cc710b2 100644 --- a/src/kernel/mq.c +++ b/src/kernel/mq.c @@ -89,14 +89,14 @@ static void dnbd3_busy_iter_requeue(struct request *req, void *priv, bool arg) /* request is in sending, so we will not requeue */ return; } - if (!cmd->dnbd3->connected) { + if (!cmd->dnbd3->socks) { dnbd3_end_cmd(cmd, BLK_STS_IOERR); mutex_unlock(&cmd->lock); return; } debug_sock(sock, "requeue busy request %p", req); - dnbd3_requeue_cmd(cmd); mutex_unlock(&cmd->lock); + dnbd3_requeue_cmd(cmd); } /** @@ -116,16 +116,27 @@ void dndb3_reque_busy_requests(struct dnbd3_sock *sock) static void dnbd3_wait_for_sockets(struct dnbd3_device *dev) { int i; bool wait = true; - while (wait && dev->connected) { + bool msg = false; + mutex_lock(&dev->device_lock); + while (dev->socks) { for (i = 0; i < dev->number_connections; i++) { if (dnbd3_is_sock_alive(dev->socks[i])) { wait = false; } } - if (wait && dev->connected) { - debug("no socket alive sleeping"); - msleep(100); + if (!wait) + break; + mutex_unlock(&dev->device_lock); + if (!msg) { + debug_dev(dev, "no socket alive sleeping"); + msg = true; } + msleep(100); + mutex_lock(&dev->device_lock); + } + mutex_unlock(&dev->device_lock); + if (msg) { + debug_dev(dev, "done sleeping"); } } @@ -136,7 +147,7 @@ static void dnbd3_wait_for_sockets(struct dnbd3_device *dev) { void dnbd3_requeue_cmd(struct dnbd3_cmd *cmd) { struct request *req = blk_mq_rq_from_pdu(cmd); - dnbd3_wait_for_sockets(cmd->dnbd3); + //dnbd3_wait_for_sockets(cmd->dnbd3); if (!cmd->requed/* && blk_queued_rq(req)*/) { cmd->requed = true; @@ -158,6 +169,7 @@ void dnbd3_end_cmd(struct dnbd3_cmd *cmd, blk_status_t error) /** * dnbd3_handle_cmd - handles a mq command + * !! cmd->lock is held when being called !! * @cmd: the cmd to send * @index: the index of the queue */ @@ -165,25 +177,19 @@ static int dnbd3_handle_cmd(struct dnbd3_cmd *cmd, int index) { struct request *req = blk_mq_rq_from_pdu(cmd); struct dnbd3_device *dev = cmd->dnbd3; - struct dnbd3_sock *sock = &dev->socks[index]; + struct dnbd3_sock *sock; int ret = -1; // debug_dev(dev, "handle request at position %lu, size %d, index %d", // blk_rq_pos(req), blk_rq_bytes(req), index); - if (!dev->socks || !dnbd3_is_sock_alive(*sock)) { - - if (dev->connected) { - info_dev(dev, "reset request to new socket"); - dnbd3_requeue_cmd(cmd); - return 0; - } else { - error_dev(dev, "ending request device not connected"); - dnbd3_end_cmd(cmd, BLK_STS_IOERR); - return -EIO; - } + if (!dev->socks || !dnbd3_is_sock_alive(dev->socks[index])) { + error_dev(dev, "ending request device not connected"); + dnbd3_end_cmd(cmd, BLK_STS_IOERR); + return -EIO; } + sock = &dev->socks[index]; mutex_lock(&sock->tx_lock); if (unlikely(!sock->sock)) { @@ -270,16 +276,15 @@ static enum blk_eh_timer_return dnbd3_xmit_timeout(struct request *req, return BLK_EH_RESET_TIMER; } - if (dev->connected) { + if (dev->socks) { info_dev(dev, "reset request to new socket"); + mutex_unlock(&cmd->lock); dnbd3_requeue_cmd(cmd); + } else { + error_dev(dev, "connection timed out"); + dnbd3_end_cmd(cmd, BLK_STS_TIMEOUT); mutex_unlock(&cmd->lock); - return BLK_EH_DONE; } - - error_dev(dev, "connection timed out"); - dnbd3_end_cmd(cmd, BLK_STS_TIMEOUT); - mutex_unlock(&cmd->lock); return BLK_EH_DONE; } diff --git a/src/kernel/net-txrx.c b/src/kernel/net-txrx.c index c839561..51c684d 100644 --- a/src/kernel/net-txrx.c +++ b/src/kernel/net-txrx.c @@ -345,8 +345,8 @@ int dnbd3_receive_cmd_get_block_mq(struct dnbd3_sock *sock, error_sock(sock, "unexpected reply (%d) %p", tag, req); if (req) { cmd = blk_mq_rq_to_pdu(req); - mutex_lock(&cmd->lock); debug_sock(sock, "requeue request"); + mutex_lock(&cmd->lock); dnbd3_requeue_cmd(cmd); mutex_unlock(&cmd->lock); } diff --git a/src/kernel/net.c b/src/kernel/net.c index 6801659..1dd6725 100644 --- a/src/kernel/net.c +++ b/src/kernel/net.c @@ -35,12 +35,10 @@ sock_create_kern(&init_net, (af) == HOST_IP4 ? AF_INET : AF_INET6, \ type, proto, sock) -#define dnbd3_sock_release(sock) \ - do { \ - struct socket *s = (sock)->sock; \ - sock->sock = NULL; \ - sock_release(s); \ - }while (0) +static void dnbd3_sock_release_nolock(struct dnbd3_sock *sock) { + sock_release(sock->sock); + sock->sock = NULL; +} #define dnbd3_is_sock_alive(s) ((s).sock && (s).server) @@ -49,7 +47,7 @@ static int __dnbd3_socket_connect(struct dnbd3_sock *sock, struct dnbd3_server *server); static int dnbd3_socket_connect(struct dnbd3_sock *sock, struct dnbd3_server *server); -static int dnbd3_socket_disconnect(struct dnbd3_sock *sock); +static void dnbd3_socket_disconnect(struct dnbd3_sock *sock); /* * Timer and workers @@ -160,11 +158,6 @@ static void dnbd3_receive_worker(struct work_struct *work) error: if (result == 0) { info_sock(sock, "result is 0, socket seems to be down"); - /* be sure nobody is sending on the socket */ - while (unlikely(sock->pending)) { - msleep(50); - } - dnbd3_sock_release(sock); break; } else if (result < 0) { /* discovery takes care of to many failures */ @@ -187,12 +180,12 @@ static void dnbd3_keepalive_worker(struct work_struct *work) { struct dnbd3_sock *sock; sock = container_of(work, struct dnbd3_sock, keepalive_worker); - if (dnbd3_is_sock_alive(*sock)) { - debug_sock(sock, "starting keepalive worker"); - if (mutex_trylock(&sock->tx_lock)) { + if (mutex_trylock(&sock->tx_lock)) { + if (dnbd3_is_sock_alive(*sock)) { + debug_sock(sock, "starting keepalive worker"); dnbd3_send_request_cmd(sock, CMD_KEEPALIVE); - mutex_unlock(&sock->tx_lock); } + mutex_unlock(&sock->tx_lock); } } @@ -321,12 +314,9 @@ static int dnbd3_adjust_connections(struct dnbd3_device *dev) { alive = 0; for (i = 0; i < dev->number_connections; i++) { - if (plan[i] != dev->socks[i].server || - !dnbd3_is_sock_alive(dev->socks[i])) { - - if (dnbd3_is_sock_alive(dev->socks[i])) { - dnbd3_socket_disconnect(&dev->socks[i]); - } + if (plan[i] != dev->socks[i].server + || !dnbd3_is_sock_alive(dev->socks[i])) { + dnbd3_socket_disconnect(&dev->socks[i]); if (!dnbd3_socket_connect(&dev->socks[i], plan[i])) { alive++; @@ -414,7 +404,7 @@ static int dnbd3_meassure_rtt(struct dnbd3_device *dev, }; result = __dnbd3_socket_connect(&sock, server); - if (result) { + if (result != 0) { error_sock(&sock, "socket connect failed in rtt measurement"); goto error; } @@ -658,13 +648,14 @@ static int __dnbd3_socket_connect(struct dnbd3_sock *sock, return 0; error: if (sock->sock) { - dnbd3_sock_release(sock); + dnbd3_sock_release_nolock(sock); } return result; } /** * dnbd3_socket_connect - connect a socket to a server + * !! HOLD TX_LOCK WHEN CALLING !! * @sock: the socket * @server: the server to connect * @@ -745,7 +736,7 @@ error: if (sock->sock) { kernel_sock_shutdown(sock->sock, SHUT_RDWR); cancel_work_sync(&sock->receive_worker); - dnbd3_sock_release(sock); + dnbd3_sock_release_nolock(sock); } return result; } @@ -753,14 +744,15 @@ error: /** * dnbd3_socket_disconnect - disconnect a socket + * !! HOLD TX_LOCK WHEN CALLING !! * @sock: the socket to disconnect */ -static int dnbd3_socket_disconnect(struct dnbd3_sock *sock) +static void dnbd3_socket_disconnect(struct dnbd3_sock *sock) { + if (!sock->sock) + return; cancel_work_sync(&sock->keepalive_worker); - debug_sock(sock, "socket disconnect"); - /* * Important sequence to shut down socket * 1. kernel_sock_shutdown @@ -772,25 +764,18 @@ static int dnbd3_socket_disconnect(struct dnbd3_sock *sock) * 3. sock_release * release the socket and set to NULL */ - if (sock->sock) { - kernel_sock_shutdown(sock->sock, SHUT_RDWR); - } - + kernel_sock_shutdown(sock->sock, SHUT_RDWR); cancel_work_sync(&sock->receive_worker); - dndb3_reque_busy_requests(sock); - - if (sock->sock) { - dnbd3_sock_release(sock); - } - + dnbd3_sock_release_nolock(sock); sock->server = NULL; - return 0; + return; } /** * dnbd3_net_connect - connect device + * Hold the device_lock when calling this! * @dev: the device to connect * * dnbd3_device.alt_servers[0] must be set @@ -799,6 +784,10 @@ int dnbd3_net_connect(struct dnbd3_device *dev) { int i, result; debug_dev(dev, "connecting to server"); + + if (dev->alt_servers[0].host.type == 0) { + return -ENONET; + } dev->socks = kmalloc(sizeof(struct dnbd3_sock) * dev->number_connections, GFP_KERNEL); if (!dev->socks) { @@ -815,17 +804,12 @@ int dnbd3_net_connect(struct dnbd3_device *dev) debug_dev(dev, "set nr hw queues to %d", dev->number_connections); blk_mq_update_nr_hw_queues(&dev->tag_set, dev->number_connections); - if (dev->alt_servers[0].host.type == 0) { - return -ENONET; - } - result = dnbd3_adjust_connections(dev); if (result) { error_dev(dev, "failed to connect to initial server"); dnbd3_net_disconnect(dev); return -ENOENT; } - dev->connected = true; debug_dev(dev, "connected, starting workers"); INIT_WORK(&dev->discovery_worker, dnbd3_discovery_worker); @@ -839,6 +823,7 @@ int dnbd3_net_connect(struct dnbd3_device *dev) /** * dnbd3_net_disconnect - disconnect device + * Hold the device_lock when calling this! * @dev: the device to disconnect */ int dnbd3_net_disconnect(struct dnbd3_device *dev) @@ -851,20 +836,15 @@ int dnbd3_net_disconnect(struct dnbd3_device *dev) return 0; } - dev->connected = false; del_timer_sync(&dev->timer); /* be sure it does not recover while disconnecting */ cancel_work_sync(&dev->discovery_worker); cancel_work_sync(&dev->panic_worker); for (i = 0; i < dev->number_connections; i++) { - if (dev->socks[i].sock) { - mutex_lock(&dev->socks[i].tx_lock); - if (dnbd3_socket_disconnect(&dev->socks[i])) { - result = -EIO; - } - mutex_unlock(&dev->socks[i].tx_lock); - mutex_destroy(&dev->socks[i].tx_lock); - } + mutex_lock(&dev->socks[i].tx_lock); + dnbd3_socket_disconnect(&dev->socks[i]); + mutex_unlock(&dev->socks[i].tx_lock); + mutex_destroy(&dev->socks[i].tx_lock); } kfree(dev->socks); dev->socks = NULL; -- cgit v1.2.3-55-g7522