From f4ad97dd38c134be53f4bbb30d9fb9cd2ef19844 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Fri, 20 Nov 2020 13:47:16 +0100 Subject: [KERNEL] Cleanup thread cleanup, fix closing of device when busy --- src/kernel/blk.c | 73 ++++++++++++------------- src/kernel/dnbd3_main.c | 3 +- src/kernel/net.c | 138 +++++++++++++++++++++++++++++++----------------- 3 files changed, 124 insertions(+), 90 deletions(-) diff --git a/src/kernel/blk.c b/src/kernel/blk.c index 43251d3..7315ccb 100644 --- a/src/kernel/blk.c +++ b/src/kernel/blk.c @@ -36,19 +36,25 @@ static int dnbd3_close_device(dnbd3_device_t *dev) { int result; + if (dev->imgname) { + dev_info(dnbd3_device_to_dev(dev), "closing down device.\n"); + } + /* quickly fail all requests */ dnbd3_blk_fail_all_requests(dev); dev->panic = 0; dev->discover = 0; result = dnbd3_net_disconnect(dev); + if (dev->imgname) { + kfree(dev->imgname); + dev->imgname = NULL; + } + /* new requests might have been queued up, */ + /* but now that imgname is NULL no new ones can show up */ dnbd3_blk_fail_all_requests(dev); blk_mq_freeze_queue(dev->queue); set_capacity(dev->disk, 0); blk_mq_unfreeze_queue(dev->queue); - if (dev->imgname) - { - kfree(dev->imgname); - dev->imgname = NULL; - } + dev->reported_size = 0; return result; } @@ -134,7 +140,7 @@ static int dnbd3_blk_ioctl(struct block_device *bdev, fmode_t mode, unsigned int dev->rid = msg->rid; dev->use_server_provided_alts = msg->use_server_provided_alts; - + dev_info(dnbd3_device_to_dev(dev), "opening device.\n"); if (blk_queue->backing_dev_info != NULL) { blk_queue->backing_dev_info->ra_pages = (msg->read_ahead_kb * 1024) / PAGE_SIZE; } @@ -159,9 +165,7 @@ static int dnbd3_blk_ioctl(struct block_device *bdev, fmode_t mode, unsigned int if (dnbd3_net_connect(dev) != 0) { /* probing server failed, cleanup connection and proceed with next specified server */ - dnbd3_blk_fail_all_requests(dev); dnbd3_net_disconnect(dev); - dnbd3_blk_fail_all_requests(dev); result = -ENOENT; } else { /* probing server succeeds, abort probing of other servers */ @@ -230,7 +234,10 @@ static int dnbd3_blk_ioctl(struct block_device *bdev, fmode_t mode, unsigned int /* specified server is known, so try to switch to it */ if (!is_same_server(&dev->cur_server, alt_server)) { - /* specified server is not working, so switch to it */ + if (alt_server->host.type == HOST_IP4) + dev_info(dnbd3_device_to_dev(dev), "manual server switch to %pI4\n", alt_server->host.addr); + else + dev_info(dnbd3_device_to_dev(dev), "manual server switch to [%pI6]\n", alt_server->host.addr); /* save current working server */ /* lock device to get consistent copy of current working server */ spin_lock_irqsave(&dev->blk_lock, irqflags); @@ -240,8 +247,6 @@ static int dnbd3_blk_ioctl(struct block_device *bdev, fmode_t mode, unsigned int /* disconnect old server */ dnbd3_net_disconnect(dev); - dev_info(dnbd3_device_to_dev(dev), "switching server ...\n"); - /* connect to new specified server (switching) */ memcpy(&dev->cur_server, alt_server, sizeof(dev->cur_server)); result = dnbd3_net_connect(dev); @@ -249,13 +254,15 @@ static int dnbd3_blk_ioctl(struct block_device *bdev, fmode_t mode, unsigned int { /* reconnect with old server if switching has failed */ memcpy(&dev->cur_server, &old_server, sizeof(dev->cur_server)); - if (dnbd3_net_connect(dev) != 0) - { - blk_mq_freeze_queue(dev->queue); - set_capacity(dev->disk, 0); - blk_mq_unfreeze_queue(dev->queue); + if (dnbd3_net_connect(dev) != 0) { + /* we couldn't reconnect to the old server */ + /* device is dangling now and needs another SWITCH call */ + dev_warn(dnbd3_device_to_dev(dev), "switching failed and could not switch back to old server - dangling device\n"); + result = -ECONNABORTED; + } else { + /* switching didn't work but we are back to the old server */ + result = -EAGAIN; } - result = -ECONNABORTED; } } else @@ -324,38 +331,23 @@ static blk_status_t dnbd3_queue_rq(struct blk_mq_hw_ctx *hctx, const struct blk_ dnbd3_device_t *dev = rq->q->queuedata; unsigned long irqflags; - blk_mq_start_request(rq); - if (dev->imgname == NULL) - { - blk_mq_end_request(rq, BLK_STS_IOERR); - goto out; - } + return BLK_STS_IOERR; if (!(dnbd3_req_fs(rq))) - { - blk_mq_end_request(rq, BLK_STS_IOERR); - goto out; - } + return BLK_STS_IOERR; if (PROBE_COUNT_TIMEOUT > 0 && dev->panic_count >= PROBE_COUNT_TIMEOUT) - { - blk_mq_end_request(rq, BLK_STS_TIMEOUT); - goto out; - } + return BLK_STS_TIMEOUT; if (!(dnbd3_req_read(rq))) - { - blk_mq_end_request(rq, BLK_STS_NOTSUPP); - goto out; - } + return BLK_STS_NOTSUPP; + blk_mq_start_request(rq); spin_lock_irqsave(&dev->blk_lock, irqflags); list_add_tail(&rq->queuelist, &dev->request_queue_send); spin_unlock_irqrestore(&dev->blk_lock, irqflags); wake_up(&dev->process_queue_send); - -out: return BLK_STS_OK; } @@ -461,6 +453,7 @@ out: int dnbd3_blk_del_device(dnbd3_device_t *dev) { + dev_dbg(dnbd3_device_to_dev(dev), "dnbd3_blk_del_device called\n"); while (atomic_cmpxchg(&dev->connection_lock, 0, 1) != 0) schedule(); dnbd3_close_device(dev); @@ -491,7 +484,8 @@ void dnbd3_blk_fail_all_requests(dnbd3_device_t *dev) { if (blk_request == blk_request2) { - dev_warn(dnbd3_device_to_dev(dev), "request is in both lists\n"); + dev_warn(dnbd3_device_to_dev(dev), "same request is in request_queue_receive multiple times\n"); + BUG(); dup = 1; break; } @@ -510,6 +504,7 @@ void dnbd3_blk_fail_all_requests(dnbd3_device_t *dev) if (blk_request == blk_request2) { dev_warn(dnbd3_device_to_dev(dev), "request is in both lists\n"); + BUG(); dup = 1; break; } @@ -523,9 +518,7 @@ void dnbd3_blk_fail_all_requests(dnbd3_device_t *dev) list_del_init(&blk_request->queuelist); if (dnbd3_req_fs(blk_request)) { - spin_lock_irqsave(&dev->blk_lock, flags); blk_mq_end_request(blk_request, BLK_STS_IOERR); - spin_unlock_irqrestore(&dev->blk_lock, flags); } else if (dnbd3_req_special(blk_request)) { diff --git a/src/kernel/dnbd3_main.c b/src/kernel/dnbd3_main.c index c44dc44..133c579 100644 --- a/src/kernel/dnbd3_main.c +++ b/src/kernel/dnbd3_main.c @@ -94,6 +94,7 @@ static void __exit dnbd3_exit(void) { int i; + pr_debug("exiting kernel module...\n"); for (i = 0; i < max_devs; i++) { dnbd3_blk_del_device(&dnbd3_devices[i]); @@ -102,7 +103,7 @@ static void __exit dnbd3_exit(void) unregister_blkdev(major, "dnbd3"); kfree(dnbd3_devices); - pr_info("exit kernel module\n"); + pr_info("exit kernel module done\n"); } module_init(dnbd3_init); diff --git a/src/kernel/net.c b/src/kernel/net.c index f460458..d846b64 100644 --- a/src/kernel/net.c +++ b/src/kernel/net.c @@ -36,6 +36,19 @@ #define ktime_to_s(kt) ktime_divns(kt, NSEC_PER_SEC) #endif +#if 1 // CONFIG_DEBUG_DRIVER +#define ASSERT(x) \ +do { \ + if (!(x)) { \ + printk(KERN_EMERG "assertion failed %s: %d: %s\n", \ + __FILE__, __LINE__, #x); \ + BUG(); \ + } \ +} while (0) +#else +#define ASSERT(x) do { } while (0) +#endif + #define dnbd3_sock_create(af,type,proto,sock) sock_create_kern(&init_net, (af) == HOST_IP4 ? AF_INET : AF_INET6, type, proto, sock) // cmd_flags and cmd_type are merged into cmd_flags now @@ -108,8 +121,11 @@ static void dnbd3_net_heartbeat(struct timer_list *arg) // send keepalive if (req) { + unsigned long irqflags; dnbd3_cmd_to_priv(req, CMD_KEEPALIVE); + spin_lock_irqsave(&dev->blk_lock, irqflags); list_add_tail(&req->queuelist, &dev->request_queue_send); + spin_unlock_irqrestore(&dev->blk_lock, irqflags); wake_up(&dev->process_queue_send); } else @@ -178,7 +194,7 @@ static int dnbd3_net_discover(void *data) if (!buf) { dev_err(dnbd3_device_to_dev(dev), "kmalloc failed for payload buf (discover)\n"); - return -ENOMEM; + return ENOMEM; } payload = (serialized_buffer_t *)buf; // Reuse this buffer to save kernel mem @@ -385,6 +401,7 @@ static int dnbd3_net_discover(void *data) dev->better_sock = sock; // Pass over socket to take a shortcut in *_connect(); kfree(buf); + put_task_struct(dev->thread_discover); dev->thread_discover = NULL; dnbd3_net_disconnect(dev); memcpy(&dev->cur_server, &dev->alt_servers[i], sizeof(dev->cur_server)); @@ -535,6 +552,7 @@ error: ; (unsigned long long)best_rtt, (unsigned long long)dev->cur_rtt); kfree(buf); dev->better_sock = best_sock; // Take shortcut by continuing to use open connection + put_task_struct(dev->thread_discover); dev->thread_discover = NULL; dnbd3_net_disconnect(dev); memcpy(&dev->cur_server, &dev->alt_servers[best_server], sizeof(dev->cur_server)); @@ -559,8 +577,11 @@ error: ; } kfree(buf); - dev_dbg(dnbd3_device_to_dev(dev), "kthread dnbd3_net_discover terminated normally\n"); - dev->thread_discover = NULL; + if (kthread_should_stop()) { + dev_dbg(dnbd3_device_to_dev(dev), "kthread dnbd3_net_discover terminated normally\n"); + } else { + dev_dbg(dnbd3_device_to_dev(dev), "kthread dnbd3_net_discover exited unexpectedly\n"); + } return 0; } @@ -653,26 +674,25 @@ static int dnbd3_net_send(void *data) } dev_dbg(dnbd3_device_to_dev(dev), "kthread dnbd3_net_send terminated normally\n"); - dev->thread_send = NULL; - return ret; + return 0; cleanup: - if (dev->sock) - kernel_sock_shutdown(dev->sock, SHUT_RDWR); if (!atomic_read(&dev->connection_lock)) { + if (dev->sock) + kernel_sock_shutdown(dev->sock, SHUT_RDWR); dev->panic = 1; dev->discover = 1; wake_up(&dev->process_queue_discover); } - if (!atomic_read(&dev->connection_lock) && ret != 0) - dev_err(dnbd3_device_to_dev(dev), "kthread dnbd3_net_send terminated abnormally\n"); - else + if (kthread_should_stop() || ret == 0 || atomic_read(&dev->connection_lock)) { dev_dbg(dnbd3_device_to_dev(dev), "kthread dnbd3_net_send terminated normally (cleanup)\n"); + } else { + dev_err(dnbd3_device_to_dev(dev), "kthread dnbd3_net_send terminated abnormally (%d)\n", ret); + } - dev->thread_send = NULL; - return ret; + return 0; } static int dnbd3_net_receive(void *data) @@ -816,8 +836,8 @@ static int dnbd3_net_receive(void *data) } spin_lock_irqsave(&dev->blk_lock, irqflags); list_del_init(&blk_request->queuelist); - blk_mq_end_request(blk_request, BLK_STS_OK); spin_unlock_irqrestore(&dev->blk_lock, irqflags); + blk_mq_end_request(blk_request, BLK_STS_OK); continue; case CMD_GET_SERVERS: @@ -902,26 +922,24 @@ static int dnbd3_net_receive(void *data) } dev_dbg(dnbd3_device_to_dev(dev), "kthread thread_receive terminated normally\n"); - dev->thread_receive = NULL; - return ret; + return 0; cleanup: - if (dev->sock) - kernel_sock_shutdown(dev->sock, SHUT_RDWR); if (!atomic_read(&dev->connection_lock)) { + if (dev->sock) + kernel_sock_shutdown(dev->sock, SHUT_RDWR); dev->panic = 1; dev->discover = 1; wake_up(&dev->process_queue_discover); } - if (!atomic_read(&dev->connection_lock) && ret != 0) - dev_err(dnbd3_device_to_dev(dev), "kthread dnbd3_net_receive terminated abnormally\n"); - else + if (kthread_should_stop() || ret == 0 || atomic_read(&dev->connection_lock)) { dev_dbg(dnbd3_device_to_dev(dev), "kthread dnbd3_net_receive terminated normally (cleanup)\n"); - - dev->thread_receive = NULL; - return ret; + } else { + dev_err(dnbd3_device_to_dev(dev), "kthread dnbd3_net_receive terminated abnormally (%d)\n", ret); + } + return 0; } static struct socket* dnbd3_connect(dnbd3_device_t *dev, dnbd3_host_t *host) @@ -987,7 +1005,7 @@ static struct socket* dnbd3_connect(dnbd3_device_t *dev, dnbd3_host_t *host) } } if (ret != 0) { - // XXX How can we do a connect with short timeout? This is dumb + /* XXX How can we do a connect with short timeout? This is dumb */ ktime_t start = ktime_get_real(); while (ktime_ms_delta(ktime_get_real(), start) < SOCKET_TIMEOUT_CLIENT_DATA * 1000) { struct sockaddr_storage addr; @@ -1010,6 +1028,9 @@ error: int dnbd3_net_connect(dnbd3_device_t *dev) { struct request *req1 = NULL; + unsigned long irqflags; + + ASSERT(atomic_read(&dev->connection_lock)); // do some checks before connecting req1 = kmalloc(sizeof(*req1), GFP_ATOMIC); @@ -1037,11 +1058,16 @@ int dnbd3_net_connect(dnbd3_device_t *dev) goto error; } + ASSERT(dev->thread_send == NULL); + ASSERT(dev->thread_receive == NULL); + ASSERT(dev->thread_discover == NULL); + dnbd3_dev_dbg_host_cur(dev, "connecting ...\n"); if (dev->better_sock == NULL ) { // no established connection yet from discovery thread, start new one + uint64_t reported_size; dnbd3_request_t dnbd3_request; dnbd3_reply_t dnbd3_reply; struct msghdr msg; @@ -1132,16 +1158,22 @@ int dnbd3_net_connect(dnbd3_device_t *dev) goto error; } dev->rid = rid; - dev->reported_size = serializer_get_uint64(&dev->payload_buffer); - if (dev->reported_size < 4096) + reported_size = serializer_get_uint64(&dev->payload_buffer); + if (reported_size < 4096) { dnbd3_dev_err_host_cur(dev, "reported size by server is < 4096\n"); goto error; } - // store image information - set_capacity(dev->disk, dev->reported_size >> 9); /* 512 Byte blocks */ - dnbd3_dev_dbg_host_cur(dev, "filesize: %llu\n", dev->reported_size); - dev->update_available = 0; + if (dev->reported_size != 0 && dev->reported_size != reported_size) { + dnbd3_dev_err_host_cur(dev, "newly connected server reports size %llu, but expected is %llu\n", reported_size, dev->reported_size); + goto error; + } else if (dev->reported_size == 0) { + // store image information + dev->reported_size = reported_size; + set_capacity(dev->disk, dev->reported_size >> 9); /* 512 Byte blocks */ + dnbd3_dev_dbg_host_cur(dev, "image size: %llu\n", dev->reported_size); + dev->update_available = 0; + } } else // Switching server, connection is already established and size request was executed { @@ -1189,7 +1221,9 @@ int dnbd3_net_connect(dnbd3_device_t *dev) // Enqueue request to request_queue_send for a fresh list of alt servers dnbd3_cmd_to_priv(req1, CMD_GET_SERVERS); + spin_lock_irqsave(&dev->blk_lock, irqflags); list_add(&req1->queuelist, &dev->request_queue_send); + spin_unlock_irqrestore(&dev->blk_lock, irqflags); wake_up(&dev->process_queue_send); @@ -1204,6 +1238,24 @@ int dnbd3_net_connect(dnbd3_device_t *dev) return 0; error: + if (dev->thread_send) + { + kthread_stop(dev->thread_send); + put_task_struct(dev->thread_send); + dev->thread_send = NULL; + } + if (dev->thread_receive) + { + kthread_stop(dev->thread_receive); + put_task_struct(dev->thread_receive); + dev->thread_receive = NULL; + } + if (dev->thread_discover) + { + kthread_stop(dev->thread_discover); + put_task_struct(dev->thread_discover); + dev->thread_discover = NULL; + } if (dev->sock) { sock_release(dev->sock); @@ -1220,11 +1272,10 @@ error: int dnbd3_net_disconnect(dnbd3_device_t *dev) { struct task_struct* thread = NULL; - bool thread_not_terminated = false; - int ret = 0; + int ret; dev_dbg(dnbd3_device_to_dev(dev), "disconnecting device ...\n"); - + ASSERT(atomic_read(&dev->connection_lock)); dev->discover = 0; @@ -1246,9 +1297,8 @@ int dnbd3_net_disconnect(dnbd3_device_t *dev) dev_dbg(dnbd3_device_to_dev(dev), "send thread has never run\n"); } else { /* thread has run, check if it has terminated successfully */ - if (dev->thread_send != NULL) { + if (ret < 0) { dev_err(dnbd3_device_to_dev(dev), "send thread was not terminated correctly\n"); - thread_not_terminated = true; } } dev->thread_send = NULL; @@ -1265,9 +1315,8 @@ int dnbd3_net_disconnect(dnbd3_device_t *dev) dev_dbg(dnbd3_device_to_dev(dev), "receive thread has never run\n"); } else { /* thread has run, check if it has terminated successfully */ - if (dev->thread_receive != NULL) { + if (ret < 0) { dev_err(dnbd3_device_to_dev(dev), "receive thread was not terminated correctly\n"); - thread_not_terminated = true; } } dev->thread_receive = NULL; @@ -1284,9 +1333,8 @@ int dnbd3_net_disconnect(dnbd3_device_t *dev) dev_dbg(dnbd3_device_to_dev(dev), "discover thread has never run\n"); } else { /* thread has run, check if it has terminated successfully */ - if (dev->thread_discover != NULL) { - dev_err(dnbd3_device_to_dev(dev), "discover thread was not terminated correctly\n"); - thread_not_terminated = true; + if (ret < 0) { + dev_err(dnbd3_device_to_dev(dev), "discover thread was not terminated correctly (%d)\n", ret); } } dev->thread_discover = NULL; @@ -1300,13 +1348,5 @@ int dnbd3_net_disconnect(dnbd3_device_t *dev) dev->cur_server.host.type = 0; dev->cur_server.host.port = 0; - if (thread_not_terminated) { - dev_err(dnbd3_device_to_dev(dev), "failed to disconnect device\n"); - ret = -ENODEV; - } else { - dev_dbg(dnbd3_device_to_dev(dev), "device is disconnected\n"); - ret = 0; - } - - return ret; + return 0; } -- cgit v1.2.3-55-g7522