From 213cbada7b07bf66409604e0d0dcd66a6a14891a Mon Sep 17 00:00:00 2001 From: Hannes Reinecke Date: Tue, 30 Apr 2024 15:19:24 +0200 Subject: [PATCH 01/24] nvmet: lock config semaphore when accessing DH-HMAC-CHAP key When the DH-HMAC-CHAP key is accessed via configfs we need to take the config semaphore as a reconnect might be running at the same time. Reviewed-by: Sagi Grimberg Reviewed-by: Christoph Hellwig Signed-off-by: Hannes Reinecke Signed-off-by: Daniel Wagner Signed-off-by: Keith Busch --- drivers/nvme/target/auth.c | 2 ++ drivers/nvme/target/configfs.c | 22 +++++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c index 3ddbc3880cac..9afc28f1ffac 100644 --- a/drivers/nvme/target/auth.c +++ b/drivers/nvme/target/auth.c @@ -44,6 +44,7 @@ int nvmet_auth_set_key(struct nvmet_host *host, const char *secret, dhchap_secret = kstrdup(secret, GFP_KERNEL); if (!dhchap_secret) return -ENOMEM; + down_write(&nvmet_config_sem); if (set_ctrl) { kfree(host->dhchap_ctrl_secret); host->dhchap_ctrl_secret = strim(dhchap_secret); @@ -53,6 +54,7 @@ int nvmet_auth_set_key(struct nvmet_host *host, const char *secret, host->dhchap_secret = strim(dhchap_secret); host->dhchap_key_hash = key_hash; } + up_write(&nvmet_config_sem); return 0; } diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c index 77a6e817b315..7c28b9c0ee57 100644 --- a/drivers/nvme/target/configfs.c +++ b/drivers/nvme/target/configfs.c @@ -1990,11 +1990,17 @@ static struct config_group nvmet_ports_group; static ssize_t nvmet_host_dhchap_key_show(struct config_item *item, char *page) { - u8 *dhchap_secret = to_host(item)->dhchap_secret; + u8 *dhchap_secret; + ssize_t ret; + down_read(&nvmet_config_sem); + dhchap_secret = to_host(item)->dhchap_secret; if (!dhchap_secret) - return sprintf(page, "\n"); - return sprintf(page, "%s\n", dhchap_secret); + ret = sprintf(page, "\n"); + else + ret = sprintf(page, "%s\n", dhchap_secret); + up_read(&nvmet_config_sem); + return ret; } static ssize_t nvmet_host_dhchap_key_store(struct config_item *item, @@ -2018,10 +2024,16 @@ static ssize_t nvmet_host_dhchap_ctrl_key_show(struct config_item *item, char *page) { u8 *dhchap_secret = to_host(item)->dhchap_ctrl_secret; + ssize_t ret; + down_read(&nvmet_config_sem); + dhchap_secret = to_host(item)->dhchap_ctrl_secret; if (!dhchap_secret) - return sprintf(page, "\n"); - return sprintf(page, "%s\n", dhchap_secret); + ret = sprintf(page, "\n"); + else + ret = sprintf(page, "%s\n", dhchap_secret); + up_read(&nvmet_config_sem); + return ret; } static ssize_t nvmet_host_dhchap_ctrl_key_store(struct config_item *item, From 44e3c25efae8575e06f1c5d1dc40058a991e3cb2 Mon Sep 17 00:00:00 2001 From: Hannes Reinecke Date: Tue, 30 Apr 2024 15:19:25 +0200 Subject: [PATCH 02/24] nvmet: return DHCHAP status codes from nvmet_setup_auth() A failure in nvmet_setup_auth() does not mean that the NVMe authentication command failed, so we should rather return a protocol error with a 'failure1' response than an NVMe status. Also update the type used for dhchap_step and dhchap_status to u8 to avoid confusions with nvme status. Furthermore, split dhchap_status and nvme status so we don't accidentally mix these return values. Reviewed-by: Christoph Hellwig Signed-off-by: Hannes Reinecke [dwagner: - use u8 as type for dhchap_{step|status} - separate nvme status from dhcap_status] Signed-off-by: Daniel Wagner Signed-off-by: Keith Busch --- drivers/nvme/target/auth.c | 20 +++++------ drivers/nvme/target/fabrics-cmd-auth.c | 49 +++++++++++++------------- drivers/nvme/target/fabrics-cmd.c | 11 +++--- drivers/nvme/target/nvmet.h | 8 ++--- 4 files changed, 43 insertions(+), 45 deletions(-) diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c index 9afc28f1ffac..53bf1a084469 100644 --- a/drivers/nvme/target/auth.c +++ b/drivers/nvme/target/auth.c @@ -126,12 +126,11 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id) return ret; } -int nvmet_setup_auth(struct nvmet_ctrl *ctrl) +u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl) { int ret = 0; struct nvmet_host_link *p; struct nvmet_host *host = NULL; - const char *hash_name; down_read(&nvmet_config_sem); if (nvmet_is_disc_subsys(ctrl->subsys)) @@ -149,13 +148,16 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl) } if (!host) { pr_debug("host %s not found\n", ctrl->hostnqn); - ret = -EPERM; + ret = NVME_AUTH_DHCHAP_FAILURE_FAILED; goto out_unlock; } ret = nvmet_setup_dhgroup(ctrl, host->dhchap_dhgroup_id); - if (ret < 0) + if (ret < 0) { pr_warn("Failed to setup DH group"); + ret = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE; + goto out_unlock; + } if (!host->dhchap_secret) { pr_debug("No authentication provided\n"); @@ -166,12 +168,6 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl) pr_debug("Re-use existing hash ID %d\n", ctrl->shash_id); } else { - hash_name = nvme_auth_hmac_name(host->dhchap_hash_id); - if (!hash_name) { - pr_warn("Hash ID %d invalid\n", host->dhchap_hash_id); - ret = -EINVAL; - goto out_unlock; - } ctrl->shash_id = host->dhchap_hash_id; } @@ -180,7 +176,7 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl) ctrl->host_key = nvme_auth_extract_key(host->dhchap_secret + 10, host->dhchap_key_hash); if (IS_ERR(ctrl->host_key)) { - ret = PTR_ERR(ctrl->host_key); + ret = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE; ctrl->host_key = NULL; goto out_free_hash; } @@ -198,7 +194,7 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl) ctrl->ctrl_key = nvme_auth_extract_key(host->dhchap_ctrl_secret + 10, host->dhchap_ctrl_key_hash); if (IS_ERR(ctrl->ctrl_key)) { - ret = PTR_ERR(ctrl->ctrl_key); + ret = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE; ctrl->ctrl_key = NULL; goto out_free_hash; } diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c index eb7785be0ca7..d61b8c6ff3b2 100644 --- a/drivers/nvme/target/fabrics-cmd-auth.c +++ b/drivers/nvme/target/fabrics-cmd-auth.c @@ -31,7 +31,7 @@ void nvmet_auth_sq_init(struct nvmet_sq *sq) sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE; } -static u16 nvmet_auth_negotiate(struct nvmet_req *req, void *d) +static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d) { struct nvmet_ctrl *ctrl = req->sq->ctrl; struct nvmf_auth_dhchap_negotiate_data *data = d; @@ -109,7 +109,7 @@ static u16 nvmet_auth_negotiate(struct nvmet_req *req, void *d) return 0; } -static u16 nvmet_auth_reply(struct nvmet_req *req, void *d) +static u8 nvmet_auth_reply(struct nvmet_req *req, void *d) { struct nvmet_ctrl *ctrl = req->sq->ctrl; struct nvmf_auth_dhchap_reply_data *data = d; @@ -172,7 +172,7 @@ static u16 nvmet_auth_reply(struct nvmet_req *req, void *d) return 0; } -static u16 nvmet_auth_failure2(void *d) +static u8 nvmet_auth_failure2(void *d) { struct nvmf_auth_dhchap_failure_data *data = d; @@ -186,6 +186,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req) void *d; u32 tl; u16 status = 0; + u8 dhchap_status; if (req->cmd->auth_send.secp != NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) { status = NVME_SC_INVALID_FIELD | NVME_SC_DNR; @@ -237,30 +238,32 @@ void nvmet_execute_auth_send(struct nvmet_req *req) if (data->auth_type == NVME_AUTH_COMMON_MESSAGES) { if (data->auth_id == NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE) { /* Restart negotiation */ - pr_debug("%s: ctrl %d qid %d reset negotiation\n", __func__, - ctrl->cntlid, req->sq->qid); + pr_debug("%s: ctrl %d qid %d reset negotiation\n", + __func__, ctrl->cntlid, req->sq->qid); if (!req->sq->qid) { - if (nvmet_setup_auth(ctrl) < 0) { - status = NVME_SC_INTERNAL; - pr_err("ctrl %d qid 0 failed to setup" - "re-authentication", + dhchap_status = nvmet_setup_auth(ctrl); + if (dhchap_status) { + pr_err("ctrl %d qid 0 failed to setup re-authentication\n", ctrl->cntlid); - goto done_failure1; + req->sq->dhchap_status = dhchap_status; + req->sq->dhchap_step = + NVME_AUTH_DHCHAP_MESSAGE_FAILURE1; + goto done_kfree; } } - req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE; + req->sq->dhchap_step = + NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE; } else if (data->auth_id != req->sq->dhchap_step) goto done_failure1; /* Validate negotiation parameters */ - status = nvmet_auth_negotiate(req, d); - if (status == 0) + dhchap_status = nvmet_auth_negotiate(req, d); + if (dhchap_status == 0) req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_CHALLENGE; else { req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_FAILURE1; - req->sq->dhchap_status = status; - status = 0; + req->sq->dhchap_status = dhchap_status; } goto done_kfree; } @@ -284,15 +287,14 @@ void nvmet_execute_auth_send(struct nvmet_req *req) switch (data->auth_id) { case NVME_AUTH_DHCHAP_MESSAGE_REPLY: - status = nvmet_auth_reply(req, d); - if (status == 0) + dhchap_status = nvmet_auth_reply(req, d); + if (dhchap_status == 0) req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_SUCCESS1; else { req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_FAILURE1; - req->sq->dhchap_status = status; - status = 0; + req->sq->dhchap_status = dhchap_status; } goto done_kfree; case NVME_AUTH_DHCHAP_MESSAGE_SUCCESS2: @@ -301,13 +303,12 @@ void nvmet_execute_auth_send(struct nvmet_req *req) __func__, ctrl->cntlid, req->sq->qid); goto done_kfree; case NVME_AUTH_DHCHAP_MESSAGE_FAILURE2: - status = nvmet_auth_failure2(d); - if (status) { + dhchap_status = nvmet_auth_failure2(d); + if (dhchap_status) { pr_warn("ctrl %d qid %d: authentication failed (%d)\n", - ctrl->cntlid, req->sq->qid, status); - req->sq->dhchap_status = status; + ctrl->cntlid, req->sq->qid, dhchap_status); + req->sq->dhchap_status = dhchap_status; req->sq->authenticated = false; - status = 0; } goto done_kfree; default: diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c index b23f4cf840bd..042b379cbb36 100644 --- a/drivers/nvme/target/fabrics-cmd.c +++ b/drivers/nvme/target/fabrics-cmd.c @@ -211,7 +211,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req) struct nvmf_connect_data *d; struct nvmet_ctrl *ctrl = NULL; u16 status; - int ret; + u8 dhchap_status; if (!nvmet_check_transfer_len(req, sizeof(struct nvmf_connect_data))) return; @@ -254,11 +254,12 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req) uuid_copy(&ctrl->hostid, &d->hostid); - ret = nvmet_setup_auth(ctrl); - if (ret < 0) { - pr_err("Failed to setup authentication, error %d\n", ret); + dhchap_status = nvmet_setup_auth(ctrl); + if (dhchap_status) { + pr_err("Failed to setup authentication, dhchap status %u\n", + dhchap_status); nvmet_ctrl_put(ctrl); - if (ret == -EPERM) + if (dhchap_status == NVME_AUTH_DHCHAP_FAILURE_FAILED) status = (NVME_SC_CONNECT_INVALID_HOST | NVME_SC_DNR); else status = NVME_SC_INTERNAL; diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h index f460728e1df1..e4ba54b85511 100644 --- a/drivers/nvme/target/nvmet.h +++ b/drivers/nvme/target/nvmet.h @@ -113,8 +113,8 @@ struct nvmet_sq { bool authenticated; struct delayed_work auth_expired_work; u16 dhchap_tid; - u16 dhchap_status; - int dhchap_step; + u8 dhchap_status; + u8 dhchap_step; u8 *dhchap_c1; u8 *dhchap_c2; u32 dhchap_s1; @@ -713,7 +713,7 @@ void nvmet_execute_auth_receive(struct nvmet_req *req); int nvmet_auth_set_key(struct nvmet_host *host, const char *secret, bool set_ctrl); int nvmet_auth_set_host_hash(struct nvmet_host *host, const char *hash); -int nvmet_setup_auth(struct nvmet_ctrl *ctrl); +u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl); void nvmet_auth_sq_init(struct nvmet_sq *sq); void nvmet_destroy_auth(struct nvmet_ctrl *ctrl); void nvmet_auth_sq_free(struct nvmet_sq *sq); @@ -732,7 +732,7 @@ int nvmet_auth_ctrl_exponential(struct nvmet_req *req, int nvmet_auth_ctrl_sesskey(struct nvmet_req *req, u8 *buf, int buf_size); #else -static inline int nvmet_setup_auth(struct nvmet_ctrl *ctrl) +static inline u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl) { return 0; } From 44350336fd9dab7a337dec686978a160cf1abfc5 Mon Sep 17 00:00:00 2001 From: Hannes Reinecke Date: Tue, 30 Apr 2024 15:19:26 +0200 Subject: [PATCH 03/24] nvme: return kernel error codes for admin queue connect nvmf_connect_admin_queue returns NVMe error status codes and kernel error codes. This mixes the different domains which makes maintainability difficult. Reviewed-by: Christoph Hellwig Reviewed-by: Sagi Grimberg Signed-off-by: Hannes Reinecke Signed-off-by: Daniel Wagner Signed-off-by: Keith Busch --- drivers/nvme/host/core.c | 6 +++--- drivers/nvme/host/fabrics.c | 31 +++++++++++++------------------ drivers/nvme/host/nvme.h | 2 +- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index c9955ecd1790..a066429b790d 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -383,14 +383,14 @@ static inline enum nvme_disposition nvme_decide_disposition(struct request *req) if (likely(nvme_req(req)->status == 0)) return COMPLETE; - if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED) - return AUTHENTICATE; - if (blk_noretry_request(req) || (nvme_req(req)->status & NVME_SC_DNR) || nvme_req(req)->retries >= nvme_max_retries) return COMPLETE; + if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED) + return AUTHENTICATE; + if (req->cmd_flags & REQ_NVME_MPATH) { if (nvme_is_path_error(nvme_req(req)->status) || blk_queue_dying(req->q)) diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index 1f0ea1f32d22..f7eaf9580b4f 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -428,12 +428,6 @@ static void nvmf_connect_cmd_prep(struct nvme_ctrl *ctrl, u16 qid, * fabrics-protocol connection of the NVMe Admin queue between the * host system device and the allocated NVMe controller on the * target system via a NVMe Fabrics "Connect" command. - * - * Return: - * 0: success - * > 0: NVMe error status code - * < 0: Linux errno error code - * */ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl) { @@ -467,7 +461,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl) if (result & NVME_CONNECT_AUTHREQ_ASCR) { dev_warn(ctrl->device, "qid 0: secure concatenation is not supported\n"); - ret = NVME_SC_AUTH_REQUIRED; + ret = -EOPNOTSUPP; goto out_free_data; } /* Authentication required */ @@ -475,14 +469,14 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl) if (ret) { dev_warn(ctrl->device, "qid 0: authentication setup failed\n"); - ret = NVME_SC_AUTH_REQUIRED; goto out_free_data; } ret = nvme_auth_wait(ctrl, 0); - if (ret) + if (ret) { dev_warn(ctrl->device, - "qid 0: authentication failed\n"); - else + "qid 0: authentication failed, error %d\n", + ret); + } else dev_info(ctrl->device, "qid 0: authenticated\n"); } @@ -542,7 +536,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid) if (result & NVME_CONNECT_AUTHREQ_ASCR) { dev_warn(ctrl->device, "qid 0: secure concatenation is not supported\n"); - ret = NVME_SC_AUTH_REQUIRED; + ret = -EOPNOTSUPP; goto out_free_data; } /* Authentication required */ @@ -550,12 +544,13 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid) if (ret) { dev_warn(ctrl->device, "qid %d: authentication setup failed\n", qid); - ret = NVME_SC_AUTH_REQUIRED; - } else { - ret = nvme_auth_wait(ctrl, qid); - if (ret) - dev_warn(ctrl->device, - "qid %u: authentication failed\n", qid); + goto out_free_data; + } + ret = nvme_auth_wait(ctrl, qid); + if (ret) { + dev_warn(ctrl->device, + "qid %u: authentication failed, error %d\n", + qid, ret); } } out_free_data: diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 24193fcb8bd5..f243a5822c2b 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -1114,7 +1114,7 @@ static inline int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid) } static inline int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid) { - return NVME_SC_AUTH_REQUIRED; + return -EPROTONOSUPPORT; } static inline void nvme_auth_free(struct nvme_ctrl *ctrl) {}; #endif From adfde7ed0b301ef14c37efe3143a8b26849843f6 Mon Sep 17 00:00:00 2001 From: Hannes Reinecke Date: Tue, 30 Apr 2024 15:19:27 +0200 Subject: [PATCH 04/24] nvme-fabrics: short-circuit reconnect retries Returning a nvme status from nvme_tcp_setup_ctrl() indicates that the association was established and we have received a status from the controller; consequently we should honour the DNR bit. If not any future reconnect attempts will just return the same error, so we can short-circuit the reconnect attempts and fail the connection directly. Signed-off-by: Hannes Reinecke [dwagner: - extended nvme_should_reconnect] Signed-off-by: Daniel Wagner Signed-off-by: Keith Busch --- drivers/nvme/host/fabrics.c | 14 +++++++++++++- drivers/nvme/host/fabrics.h | 2 +- drivers/nvme/host/fc.c | 4 +--- drivers/nvme/host/rdma.c | 19 ++++++++++++------- drivers/nvme/host/tcp.c | 22 ++++++++++++++-------- 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index f7eaf9580b4f..36d3e2ff27f3 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -559,8 +559,20 @@ out_free_data: } EXPORT_SYMBOL_GPL(nvmf_connect_io_queue); -bool nvmf_should_reconnect(struct nvme_ctrl *ctrl) +/* + * Evaluate the status information returned by the transport in order to decided + * if a reconnect attempt should be scheduled. + * + * Do not retry when: + * + * - the DNR bit is set and the specification states no further connect + * attempts with the same set of paramenters should be attempted. + */ +bool nvmf_should_reconnect(struct nvme_ctrl *ctrl, int status) { + if (status > 0 && (status & NVME_SC_DNR)) + return false; + if (ctrl->opts->max_reconnects == -1 || ctrl->nr_reconnects < ctrl->opts->max_reconnects) return true; diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h index 37c974c38dcb..602135910ae9 100644 --- a/drivers/nvme/host/fabrics.h +++ b/drivers/nvme/host/fabrics.h @@ -223,7 +223,7 @@ int nvmf_register_transport(struct nvmf_transport_ops *ops); void nvmf_unregister_transport(struct nvmf_transport_ops *ops); void nvmf_free_options(struct nvmf_ctrl_options *opts); int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size); -bool nvmf_should_reconnect(struct nvme_ctrl *ctrl); +bool nvmf_should_reconnect(struct nvme_ctrl *ctrl, int status); bool nvmf_ip_options_match(struct nvme_ctrl *ctrl, struct nvmf_ctrl_options *opts); void nvmf_set_io_queues(struct nvmf_ctrl_options *opts, u32 nr_io_queues, diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c index 68a5d971657b..b330a6a7b63a 100644 --- a/drivers/nvme/host/fc.c +++ b/drivers/nvme/host/fc.c @@ -3310,12 +3310,10 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status) dev_info(ctrl->ctrl.device, "NVME-FC{%d}: reset: Reconnect attempt failed (%d)\n", ctrl->cnum, status); - if (status > 0 && (status & NVME_SC_DNR)) - recon = false; } else if (time_after_eq(jiffies, rport->dev_loss_end)) recon = false; - if (recon && nvmf_should_reconnect(&ctrl->ctrl)) { + if (recon && nvmf_should_reconnect(&ctrl->ctrl, status)) { if (portptr->port_state == FC_OBJSTATE_ONLINE) dev_info(ctrl->ctrl.device, "NVME-FC{%d}: Reconnect attempt in %ld " diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index 366f0bb4ebfc..821ab3e0fd3b 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -982,7 +982,8 @@ free_ctrl: kfree(ctrl); } -static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl) +static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl, + int status) { enum nvme_ctrl_state state = nvme_ctrl_state(&ctrl->ctrl); @@ -992,7 +993,7 @@ static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl) return; } - if (nvmf_should_reconnect(&ctrl->ctrl)) { + if (nvmf_should_reconnect(&ctrl->ctrl, status)) { dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n", ctrl->ctrl.opts->reconnect_delay); queue_delayed_work(nvme_wq, &ctrl->reconnect_work, @@ -1104,10 +1105,12 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work) { struct nvme_rdma_ctrl *ctrl = container_of(to_delayed_work(work), struct nvme_rdma_ctrl, reconnect_work); + int ret; ++ctrl->ctrl.nr_reconnects; - if (nvme_rdma_setup_ctrl(ctrl, false)) + ret = nvme_rdma_setup_ctrl(ctrl, false); + if (ret) goto requeue; dev_info(ctrl->ctrl.device, "Successfully reconnected (%d attempts)\n", @@ -1120,7 +1123,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work) requeue: dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n", ctrl->ctrl.nr_reconnects); - nvme_rdma_reconnect_or_remove(ctrl); + nvme_rdma_reconnect_or_remove(ctrl, ret); } static void nvme_rdma_error_recovery_work(struct work_struct *work) @@ -1145,7 +1148,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work) return; } - nvme_rdma_reconnect_or_remove(ctrl); + nvme_rdma_reconnect_or_remove(ctrl, 0); } static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl) @@ -2169,6 +2172,7 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work) { struct nvme_rdma_ctrl *ctrl = container_of(work, struct nvme_rdma_ctrl, ctrl.reset_work); + int ret; nvme_stop_ctrl(&ctrl->ctrl); nvme_rdma_shutdown_ctrl(ctrl, false); @@ -2179,14 +2183,15 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work) return; } - if (nvme_rdma_setup_ctrl(ctrl, false)) + ret = nvme_rdma_setup_ctrl(ctrl, false); + if (ret) goto out_fail; return; out_fail: ++ctrl->ctrl.nr_reconnects; - nvme_rdma_reconnect_or_remove(ctrl); + nvme_rdma_reconnect_or_remove(ctrl, ret); } static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = { diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index fdbcdcedcee9..3e0c33323320 100644 --- a/drivers/nvme/host/tcp.c +++ b/drivers/nvme/host/tcp.c @@ -2155,7 +2155,8 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl, nvme_tcp_destroy_io_queues(ctrl, remove); } -static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl) +static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl, + int status) { enum nvme_ctrl_state state = nvme_ctrl_state(ctrl); @@ -2165,13 +2166,14 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl) return; } - if (nvmf_should_reconnect(ctrl)) { + if (nvmf_should_reconnect(ctrl, status)) { dev_info(ctrl->device, "Reconnecting in %d seconds...\n", ctrl->opts->reconnect_delay); queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work, ctrl->opts->reconnect_delay * HZ); } else { - dev_info(ctrl->device, "Removing controller...\n"); + dev_info(ctrl->device, "Removing controller (%d)...\n", + status); nvme_delete_ctrl(ctrl); } } @@ -2252,10 +2254,12 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work) struct nvme_tcp_ctrl *tcp_ctrl = container_of(to_delayed_work(work), struct nvme_tcp_ctrl, connect_work); struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl; + int ret; ++ctrl->nr_reconnects; - if (nvme_tcp_setup_ctrl(ctrl, false)) + ret = nvme_tcp_setup_ctrl(ctrl, false); + if (ret) goto requeue; dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n", @@ -2268,7 +2272,7 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work) requeue: dev_info(ctrl->device, "Failed reconnect attempt %d\n", ctrl->nr_reconnects); - nvme_tcp_reconnect_or_remove(ctrl); + nvme_tcp_reconnect_or_remove(ctrl, ret); } static void nvme_tcp_error_recovery_work(struct work_struct *work) @@ -2295,7 +2299,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work) return; } - nvme_tcp_reconnect_or_remove(ctrl); + nvme_tcp_reconnect_or_remove(ctrl, 0); } static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown) @@ -2315,6 +2319,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work) { struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, reset_work); + int ret; nvme_stop_ctrl(ctrl); nvme_tcp_teardown_ctrl(ctrl, false); @@ -2328,14 +2333,15 @@ static void nvme_reset_ctrl_work(struct work_struct *work) return; } - if (nvme_tcp_setup_ctrl(ctrl, false)) + ret = nvme_tcp_setup_ctrl(ctrl, false); + if (ret) goto out_fail; return; out_fail: ++ctrl->nr_reconnects; - nvme_tcp_reconnect_or_remove(ctrl); + nvme_tcp_reconnect_or_remove(ctrl, ret); } static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl) From 0e34bd9605f6c95609c32aa82f0a394e2a945908 Mon Sep 17 00:00:00 2001 From: Daniel Wagner Date: Tue, 30 Apr 2024 15:19:28 +0200 Subject: [PATCH 05/24] nvme: do not retry authentication failures When the key is invalid there is no point in retrying. Because the auth code returns kernel error codes only, we can't test on the DNR bit. Signed-off-by: Daniel Wagner Signed-off-by: Keith Busch --- drivers/nvme/host/auth.c | 6 +++--- drivers/nvme/host/fabrics.c | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c index a264b3ae078b..371e14f0a203 100644 --- a/drivers/nvme/host/auth.c +++ b/drivers/nvme/host/auth.c @@ -730,7 +730,7 @@ static void nvme_queue_auth_work(struct work_struct *work) NVME_AUTH_DHCHAP_MESSAGE_CHALLENGE); if (ret) { chap->status = ret; - chap->error = -ECONNREFUSED; + chap->error = -EKEYREJECTED; return; } @@ -797,7 +797,7 @@ static void nvme_queue_auth_work(struct work_struct *work) NVME_AUTH_DHCHAP_MESSAGE_SUCCESS1); if (ret) { chap->status = ret; - chap->error = -ECONNREFUSED; + chap->error = -EKEYREJECTED; return; } @@ -818,7 +818,7 @@ static void nvme_queue_auth_work(struct work_struct *work) ret = nvme_auth_process_dhchap_success1(ctrl, chap); if (ret) { /* Controller authentication failed */ - chap->error = -ECONNREFUSED; + chap->error = -EKEYREJECTED; goto fail2; } diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index 36d3e2ff27f3..c6ad2148c2e0 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -567,12 +567,18 @@ EXPORT_SYMBOL_GPL(nvmf_connect_io_queue); * * - the DNR bit is set and the specification states no further connect * attempts with the same set of paramenters should be attempted. + * + * - when the authentication attempt fails, because the key was invalid. + * This error code is set on the host side. */ bool nvmf_should_reconnect(struct nvme_ctrl *ctrl, int status) { if (status > 0 && (status & NVME_SC_DNR)) return false; + if (status == -EKEYREJECTED) + return false; + if (ctrl->opts->max_reconnects == -1 || ctrl->nr_reconnects < ctrl->opts->max_reconnects) return true; From c51a22e63ffde3033f74865a6e7b7d6e27cd6ab4 Mon Sep 17 00:00:00 2001 From: Sagi Grimberg Date: Tue, 7 May 2024 09:54:44 +0300 Subject: [PATCH 06/24] nvmet-rdma: Avoid o(n^2) loop in delete_ctrl When deleting a nvmet-rdma ctrl, we essentially loop over all queues that belong to the controller and schedule a removal of each. Instead of restarting the loop every time a queue is found, do a simple safe list traversal. This addresses an unneeded time spent scheduling queue removal in cases there a lot of queues. Signed-off-by: Sagi Grimberg Reviewed-by: Christoph Hellwig Signed-off-by: Keith Busch --- drivers/nvme/target/rdma.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c index 5b8c63e74639..955296ac377f 100644 --- a/drivers/nvme/target/rdma.c +++ b/drivers/nvme/target/rdma.c @@ -1814,18 +1814,14 @@ static int nvmet_rdma_cm_handler(struct rdma_cm_id *cm_id, static void nvmet_rdma_delete_ctrl(struct nvmet_ctrl *ctrl) { - struct nvmet_rdma_queue *queue; + struct nvmet_rdma_queue *queue, *n; -restart: mutex_lock(&nvmet_rdma_queue_mutex); - list_for_each_entry(queue, &nvmet_rdma_queue_list, queue_list) { - if (queue->nvme_sq.ctrl == ctrl) { - list_del_init(&queue->queue_list); - mutex_unlock(&nvmet_rdma_queue_mutex); - - __nvmet_rdma_queue_disconnect(queue); - goto restart; - } + list_for_each_entry_safe(queue, n, &nvmet_rdma_queue_list, queue_list) { + if (queue->nvme_sq.ctrl != ctrl) + continue; + list_del_init(&queue->queue_list); + __nvmet_rdma_queue_disconnect(queue); } mutex_unlock(&nvmet_rdma_queue_mutex); } From 54a76c8732b265aa86030134d4af6a5a3c59fe52 Mon Sep 17 00:00:00 2001 From: Tokunori Ikegami Date: Mon, 6 May 2024 00:24:59 +0900 Subject: [PATCH 07/24] nvme-rdma, nvme-tcp: include max reconnects for reconnect logging Makes clear max reconnects translated by ctrl loss tmo and reconnect delay. Signed-off-by: Tokunori Ikegami Signed-off-by: Keith Busch --- drivers/nvme/host/rdma.c | 4 ++-- drivers/nvme/host/tcp.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index 821ab3e0fd3b..51a62b0c645a 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1121,8 +1121,8 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work) return; requeue: - dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n", - ctrl->ctrl.nr_reconnects); + dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d/%d\n", + ctrl->ctrl.nr_reconnects, ctrl->ctrl.opts->max_reconnects); nvme_rdma_reconnect_or_remove(ctrl, ret); } diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index 3e0c33323320..9efeecc560b7 100644 --- a/drivers/nvme/host/tcp.c +++ b/drivers/nvme/host/tcp.c @@ -2262,16 +2262,16 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work) if (ret) goto requeue; - dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n", - ctrl->nr_reconnects); + dev_info(ctrl->device, "Successfully reconnected (attempt %d/%d)\n", + ctrl->nr_reconnects, ctrl->opts->max_reconnects); ctrl->nr_reconnects = 0; return; requeue: - dev_info(ctrl->device, "Failed reconnect attempt %d\n", - ctrl->nr_reconnects); + dev_info(ctrl->device, "Failed reconnect attempt %d/%d\n", + ctrl->nr_reconnects, ctrl->opts->max_reconnects); nvme_tcp_reconnect_or_remove(ctrl, ret); } From 928b607d1a0941f02899e5490201ec071d5d4ff5 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Sat, 11 May 2024 08:37:52 -0600 Subject: [PATCH 08/24] ublk_drv: set DMA alignment mask to 3 By default, this will be 511, as that's the block layer default. But drivers these days can support memory alignments that aren't tied to the sector sizes, instead just being limited by what the DMA engine supports. An example is NVMe, where it's generally set to a 32-bit or 64-bit boundary. As ublk itself doesn't really care, just set it low enough that we don't run into issues with NVMe where the required O_DIRECT memory alignment is now more restrictive on ublk than it is on the underlying device. This was triggered by spurious -EINVAL returns on O_DIRECT IO on a setup with ublk managing NVMe devices, which previously worked just fine on the NVMe device itself. With the alignment relaxed, the test works fine. Reviewed-by: Ming Lei Signed-off-by: Jens Axboe --- drivers/block/ublk_drv.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 176657dce3e3..4e159948c912 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -2178,6 +2178,7 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd) .virt_boundary_mask = p->virt_boundary_mask, .max_segments = USHRT_MAX, .max_segment_size = UINT_MAX, + .dma_alignment = 3, }; struct gendisk *disk; int ret = -EINVAL; From 25260555b118e71c64dfdb6786144f654fe1a27d Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 10 May 2024 13:18:16 -0700 Subject: [PATCH 09/24] null_blk: Fix two sparse warnings Fix the following sparse warnings: drivers/block/null_blk/main.c:1243:35: warning: incorrect type in return expression (different base types) drivers/block/null_blk/main.c:1243:35: expected int drivers/block/null_blk/main.c:1243:35: got restricted blk_status_t drivers/block/null_blk/main.c:1291:30: warning: incorrect type in return expression (different base types) drivers/block/null_blk/main.c:1291:30: expected restricted blk_status_t drivers/block/null_blk/main.c:1291:30: got int Cc: Christoph Hellwig Cc: Damien Le Moal Signed-off-by: Bart Van Assche Link: https://lore.kernel.org/r/20240510201816.24921-1-bvanassche@acm.org Signed-off-by: Jens Axboe --- drivers/block/null_blk/main.c | 2 +- drivers/block/null_blk/trace.h | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 4005a8b685e8..5d56ad4ce01a 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1218,7 +1218,7 @@ static int null_transfer(struct nullb *nullb, struct page *page, return err; } -static int null_handle_rq(struct nullb_cmd *cmd) +static blk_status_t null_handle_rq(struct nullb_cmd *cmd) { struct request *rq = blk_mq_rq_from_pdu(cmd); struct nullb *nullb = cmd->nq->dev->nullb; diff --git a/drivers/block/null_blk/trace.h b/drivers/block/null_blk/trace.h index ef2d05d5f0df..82b8f6a5e5f0 100644 --- a/drivers/block/null_blk/trace.h +++ b/drivers/block/null_blk/trace.h @@ -36,7 +36,12 @@ TRACE_EVENT(nullb_zone_op, TP_ARGS(cmd, zone_no, zone_cond), TP_STRUCT__entry( __array(char, disk, DISK_NAME_LEN) - __field(enum req_op, op) + /* + * __field() uses is_signed_type(). is_signed_type() does not + * support bitwise types. Use __field_struct() instead because + * it does not use is_signed_type(). + */ + __field_struct(enum req_op, op) __field(unsigned int, zone_no) __field(unsigned int, zone_cond) ), From 9ead7efc6f3f2b46c4ec68209bca4888cfbd4c19 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Mon, 29 Apr 2024 03:23:08 -0700 Subject: [PATCH 10/24] brd: implement discard support The ramdisk memory utilization can only go up when data is written to new pages. Implement discard to provide the possibility to reduce memory usage for pages no longer in use. Aligned discards will free the associated pages, if any, and determinisitically return zeroed data until written again. Signed-off-by: Keith Busch Link: https://lore.kernel.org/r/20240429102308.147627-1-kbusch@meta.com Signed-off-by: Jens Axboe --- drivers/block/brd.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/drivers/block/brd.c b/drivers/block/brd.c index b900fe9e0030..558d8e670566 100644 --- a/drivers/block/brd.c +++ b/drivers/block/brd.c @@ -222,6 +222,23 @@ out: return err; } +static void brd_do_discard(struct brd_device *brd, sector_t sector, u32 size) +{ + sector_t aligned_sector = (sector + PAGE_SECTORS) & ~PAGE_SECTORS; + struct page *page; + + size -= (aligned_sector - sector) * SECTOR_SIZE; + xa_lock(&brd->brd_pages); + while (size >= PAGE_SIZE && aligned_sector < rd_size * 2) { + page = __xa_erase(&brd->brd_pages, aligned_sector >> PAGE_SECTORS_SHIFT); + if (page) + __free_page(page); + aligned_sector += PAGE_SECTORS; + size -= PAGE_SIZE; + } + xa_unlock(&brd->brd_pages); +} + static void brd_submit_bio(struct bio *bio) { struct brd_device *brd = bio->bi_bdev->bd_disk->private_data; @@ -229,6 +246,12 @@ static void brd_submit_bio(struct bio *bio) struct bio_vec bvec; struct bvec_iter iter; + if (unlikely(op_is_discard(bio->bi_opf))) { + brd_do_discard(brd, sector, bio->bi_iter.bi_size); + bio_endio(bio); + return; + } + bio_for_each_segment(bvec, bio, iter) { unsigned int len = bvec.bv_len; int err; @@ -309,6 +332,9 @@ static int brd_alloc(int i) * is harmless) */ .physical_block_size = PAGE_SIZE, + .max_hw_discard_sectors = UINT_MAX, + .max_discard_segments = 1, + .discard_granularity = PAGE_SIZE, }; list_for_each_entry(brd, &brd_devices, brd_list) From 08190cc4d8a62f2a07b4158751afd3a01638c4c5 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 10 May 2024 13:23:09 -0700 Subject: [PATCH 11/24] nbd: Use NULL to represent a pointer This patch fixes the following sparse warnings: drivers/block/nbd.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, include/trace/events/nbd.h): ./include/trace/events/nbd.h:61:1: warning: Using plain integer as NULL pointer drivers/block/nbd.c: note: in included file (through include/trace/perf.h, include/trace/define_trace.h, include/trace/events/nbd.h): ./include/trace/events/nbd.h:61:1: warning: Using plain integer as NULL pointer Cc: Christoph Hellwig Cc: Josef Bacik Cc: Yu Kuai Cc: Markus Pargmann Signed-off-by: Bart Van Assche Link: https://lore.kernel.org/r/20240510202313.25209-2-bvanassche@acm.org Signed-off-by: Jens Axboe --- include/trace/events/nbd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/trace/events/nbd.h b/include/trace/events/nbd.h index 9849956f34d8..390d98a05c9d 100644 --- a/include/trace/events/nbd.h +++ b/include/trace/events/nbd.h @@ -72,7 +72,7 @@ DECLARE_EVENT_CLASS(nbd_send_request, ), TP_fast_assign( - __entry->nbd_request = 0; + __entry->nbd_request = NULL; __entry->dev_index = index; __entry->request = rq; ), From 40639e9a0f6e49edd4ef520e6c0e070e1a04a330 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 10 May 2024 13:23:10 -0700 Subject: [PATCH 12/24] nbd: Remove superfluous casts In Linux kernel code it is preferred not to use a cast when converting a void pointer to another pointer type. Cc: Christoph Hellwig Cc: Josef Bacik Cc: Yu Kuai Cc: Markus Pargmann Signed-off-by: Bart Van Assche Link: https://lore.kernel.org/r/20240510202313.25209-3-bvanassche@acm.org Signed-off-by: Jens Axboe --- drivers/block/nbd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 9d4ec9273bf9..90760f27824d 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -222,7 +222,7 @@ static ssize_t pid_show(struct device *dev, struct device_attribute *attr, char *buf) { struct gendisk *disk = dev_to_disk(dev); - struct nbd_device *nbd = (struct nbd_device *)disk->private_data; + struct nbd_device *nbd = disk->private_data; return sprintf(buf, "%d\n", nbd->pid); } @@ -236,7 +236,7 @@ static ssize_t backend_show(struct device *dev, struct device_attribute *attr, char *buf) { struct gendisk *disk = dev_to_disk(dev); - struct nbd_device *nbd = (struct nbd_device *)disk->private_data; + struct nbd_device *nbd = disk->private_data; return sprintf(buf, "%s\n", nbd->backend ?: ""); } From 2a6751e052ab4789630bc889c814037068723bc1 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 10 May 2024 13:23:11 -0700 Subject: [PATCH 13/24] nbd: Improve the documentation of the locking assumptions Document locking assumptions with lockdep_assert_held() instead of source code comments. The advantage of lockdep_assert_held() is that it is verified at runtime if lockdep is enabled in the kernel config. Cc: Christoph Hellwig Cc: Josef Bacik Cc: Yu Kuai Cc: Markus Pargmann Signed-off-by: Bart Van Assche Link: https://lore.kernel.org/r/20240510202313.25209-4-bvanassche@acm.org Signed-off-by: Jens Axboe --- drivers/block/nbd.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 90760f27824d..05f69710afe8 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -588,7 +588,6 @@ static inline int was_interrupted(int result) return result == -ERESTARTSYS || result == -EINTR; } -/* always call with the tx_lock held */ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index) { struct request *req = blk_mq_rq_from_pdu(cmd); @@ -605,6 +604,9 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index) u32 nbd_cmd_flags = 0; int sent = nsock->sent, skip = 0; + lockdep_assert_held(&cmd->lock); + lockdep_assert_held(&nsock->tx_lock); + iov_iter_kvec(&from, ITER_SOURCE, &iov, 1, sizeof(request)); type = req_to_nbd_cmd_type(req); @@ -1015,6 +1017,8 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index) struct nbd_sock *nsock; int ret; + lockdep_assert_held(&cmd->lock); + config = nbd_get_config_unlocked(nbd); if (!config) { dev_err_ratelimited(disk_to_dev(nbd->disk), From f6cb9a2c3d2e893a8d493d34ed3e0400fe8afe28 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 10 May 2024 13:23:12 -0700 Subject: [PATCH 14/24] nbd: Remove a local variable from nbd_send_cmd() blk_rq_bytes() returns an unsigned int while 'size' has type unsigned long. This is confusing. Improve code readability by removing the local variable 'size'. Cc: Christoph Hellwig Cc: Josef Bacik Cc: Yu Kuai Cc: Markus Pargmann Signed-off-by: Bart Van Assche Link: https://lore.kernel.org/r/20240510202313.25209-5-bvanassche@acm.org Signed-off-by: Jens Axboe --- drivers/block/nbd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 05f69710afe8..29e43ab1650c 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -597,7 +597,6 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index) struct nbd_request request = {.magic = htonl(NBD_REQUEST_MAGIC)}; struct kvec iov = {.iov_base = &request, .iov_len = sizeof(request)}; struct iov_iter from; - unsigned long size = blk_rq_bytes(req); struct bio *bio; u64 handle; u32 type; @@ -646,7 +645,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index) request.type = htonl(type | nbd_cmd_flags); if (type != NBD_CMD_FLUSH) { request.from = cpu_to_be64((u64)blk_rq_pos(req) << 9); - request.len = htonl(size); + request.len = htonl(blk_rq_bytes(req)); } handle = nbd_cmd_handle(cmd); request.cookie = cpu_to_be64(handle); From e56d4b633fffea9510db468085bed0799cba4ecd Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Fri, 10 May 2024 13:23:13 -0700 Subject: [PATCH 15/24] nbd: Fix signal handling Both nbd_send_cmd() and nbd_handle_cmd() return either a negative error number or a positive blk_status_t value. nbd_queue_rq() converts these return values into a blk_status_t value. There is a bug in the conversion code: if nbd_send_cmd() returns BLK_STS_RESOURCE, nbd_queue_rq() should return BLK_STS_RESOURCE instead of BLK_STS_OK. Fix this, move the conversion code into nbd_handle_cmd() and fix the remaining sparse warnings. This patch fixes the following sparse warnings: drivers/block/nbd.c:673:32: warning: incorrect type in return expression (different base types) drivers/block/nbd.c:673:32: expected int drivers/block/nbd.c:673:32: got restricted blk_status_t [usertype] drivers/block/nbd.c:714:48: warning: incorrect type in return expression (different base types) drivers/block/nbd.c:714:48: expected int drivers/block/nbd.c:714:48: got restricted blk_status_t [usertype] drivers/block/nbd.c:1120:21: warning: incorrect type in assignment (different base types) drivers/block/nbd.c:1120:21: expected int [assigned] ret drivers/block/nbd.c:1120:21: got restricted blk_status_t [usertype] drivers/block/nbd.c:1125:16: warning: incorrect type in return expression (different base types) drivers/block/nbd.c:1125:16: expected restricted blk_status_t drivers/block/nbd.c:1125:16: got int [assigned] ret Cc: Christoph Hellwig Cc: Josef Bacik Cc: Yu Kuai Cc: Markus Pargmann Fixes: fc17b6534eb8 ("blk-mq: switch ->queue_rq return value to blk_status_t") Cc: stable@vger.kernel.org Signed-off-by: Bart Van Assche Link: https://lore.kernel.org/r/20240510202313.25209-6-bvanassche@acm.org Signed-off-by: Jens Axboe --- drivers/block/nbd.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 29e43ab1650c..22a79a62cc4e 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -588,6 +588,10 @@ static inline int was_interrupted(int result) return result == -ERESTARTSYS || result == -EINTR; } +/* + * Returns BLK_STS_RESOURCE if the caller should retry after a delay. Returns + * -EAGAIN if the caller should requeue @cmd. Returns -EIO if sending failed. + */ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index) { struct request *req = blk_mq_rq_from_pdu(cmd); @@ -670,7 +674,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index) nsock->sent = sent; } set_bit(NBD_CMD_REQUEUED, &cmd->flags); - return BLK_STS_RESOURCE; + return (__force int)BLK_STS_RESOURCE; } dev_err_ratelimited(disk_to_dev(nbd->disk), "Send control failed (result %d)\n", result); @@ -711,7 +715,7 @@ send_pages: nsock->pending = req; nsock->sent = sent; set_bit(NBD_CMD_REQUEUED, &cmd->flags); - return BLK_STS_RESOURCE; + return (__force int)BLK_STS_RESOURCE; } dev_err(disk_to_dev(nbd->disk), "Send data failed (result %d)\n", @@ -1008,7 +1012,7 @@ static int wait_for_reconnect(struct nbd_device *nbd) return !test_bit(NBD_RT_DISCONNECTED, &config->runtime_flags); } -static int nbd_handle_cmd(struct nbd_cmd *cmd, int index) +static blk_status_t nbd_handle_cmd(struct nbd_cmd *cmd, int index) { struct request *req = blk_mq_rq_from_pdu(cmd); struct nbd_device *nbd = cmd->nbd; @@ -1022,14 +1026,14 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index) if (!config) { dev_err_ratelimited(disk_to_dev(nbd->disk), "Socks array is empty\n"); - return -EINVAL; + return BLK_STS_IOERR; } if (index >= config->num_connections) { dev_err_ratelimited(disk_to_dev(nbd->disk), "Attempted send on invalid socket\n"); nbd_config_put(nbd); - return -EINVAL; + return BLK_STS_IOERR; } cmd->status = BLK_STS_OK; again: @@ -1052,7 +1056,7 @@ again: */ sock_shutdown(nbd); nbd_config_put(nbd); - return -EIO; + return BLK_STS_IOERR; } goto again; } @@ -1065,7 +1069,7 @@ again: blk_mq_start_request(req); if (unlikely(nsock->pending && nsock->pending != req)) { nbd_requeue_cmd(cmd); - ret = 0; + ret = BLK_STS_OK; goto out; } /* @@ -1084,19 +1088,19 @@ again: "Request send failed, requeueing\n"); nbd_mark_nsock_dead(nbd, nsock, 1); nbd_requeue_cmd(cmd); - ret = 0; + ret = BLK_STS_OK; } out: mutex_unlock(&nsock->tx_lock); nbd_config_put(nbd); - return ret; + return ret < 0 ? BLK_STS_IOERR : (__force blk_status_t)ret; } static blk_status_t nbd_queue_rq(struct blk_mq_hw_ctx *hctx, const struct blk_mq_queue_data *bd) { struct nbd_cmd *cmd = blk_mq_rq_to_pdu(bd->rq); - int ret; + blk_status_t ret; /* * Since we look at the bio's to send the request over the network we @@ -1116,10 +1120,6 @@ static blk_status_t nbd_queue_rq(struct blk_mq_hw_ctx *hctx, * appropriate. */ ret = nbd_handle_cmd(cmd, hctx->queue_num); - if (ret < 0) - ret = BLK_STS_IOERR; - else if (!ret) - ret = BLK_STS_OK; mutex_unlock(&cmd->lock); return ret; From efb905aeb44b0e99c0e6b07865b1885ae0471ebf Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Tue, 7 May 2024 23:25:20 +0100 Subject: [PATCH 16/24] cdrom: rearrange last_media_change check to avoid unintentional overflow When running syzkaller with the newly reintroduced signed integer wrap sanitizer we encounter this splat: [ 366.015950] UBSAN: signed-integer-overflow in ../drivers/cdrom/cdrom.c:2361:33 [ 366.021089] -9223372036854775808 - 346321 cannot be represented in type '__s64' (aka 'long long') [ 366.025894] program syz-executor.4 is using a deprecated SCSI ioctl, please convert it to SG_IO [ 366.027502] CPU: 5 PID: 28472 Comm: syz-executor.7 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1 [ 366.027512] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 [ 366.027518] Call Trace: [ 366.027523] [ 366.027533] dump_stack_lvl+0x93/0xd0 [ 366.027899] handle_overflow+0x171/0x1b0 [ 366.038787] ata1.00: invalid multi_count 32 ignored [ 366.043924] cdrom_ioctl+0x2c3f/0x2d10 [ 366.063932] ? __pm_runtime_resume+0xe6/0x130 [ 366.071923] sr_block_ioctl+0x15d/0x1d0 [ 366.074624] ? __pfx_sr_block_ioctl+0x10/0x10 [ 366.077642] blkdev_ioctl+0x419/0x500 [ 366.080231] ? __pfx_blkdev_ioctl+0x10/0x10 ... Historically, the signed integer overflow sanitizer did not work in the kernel due to its interaction with `-fwrapv` but this has since been changed [1] in the newest version of Clang. It was re-enabled in the kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow sanitizer"). Let's rearrange the check to not perform any arithmetic, thus not tripping the sanitizer. Link: https://github.com/llvm/llvm-project/pull/82432 [1] Closes: https://github.com/KSPP/linux/issues/354 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt Link: https://lore.kernel.org/lkml/20240507-b4-sio-ata1-v1-1-810ffac6080a@google.com Reviewed-by: Phillip Potter Link: https://lore.kernel.org/lkml/ZjqU0fbzHrlnad8D@equinox Signed-off-by: Phillip Potter Link: https://lore.kernel.org/r/20240507222520.1445-2-phil@philpotter.co.uk Signed-off-by: Jens Axboe --- drivers/cdrom/cdrom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index a5e07270e0d4..20c90ebb3a3f 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -2358,7 +2358,7 @@ static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi, return -EFAULT; tmp_info.media_flags = 0; - if (tmp_info.last_media_change - cdi->last_media_change_ms < 0) + if (cdi->last_media_change_ms > tmp_info.last_media_change) tmp_info.media_flags |= MEDIA_CHANGED_FLAG; tmp_info.last_media_change = cdi->last_media_change_ms; From 6da6680632792709cecf2b006f2fe3ca7857e791 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Wed, 15 May 2024 09:31:56 +0800 Subject: [PATCH 17/24] blk-cgroup: fix list corruption from resetting io stat Since commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"), each iostat instance is added to blkcg percpu list, so blkcg_reset_stats() can't reset the stat instance by memset(), otherwise the llist may be corrupted. Fix the issue by only resetting the counter part. Cc: Tejun Heo Cc: Waiman Long Cc: Jay Shin Fixes: 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()") Signed-off-by: Ming Lei Acked-by: Tejun Heo Reviewed-by: Waiman Long Link: https://lore.kernel.org/r/20240515013157.443672-2-ming.lei@redhat.com Signed-off-by: Jens Axboe --- block/blk-cgroup.c | 58 ++++++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 4b1a35ab0ea4..8699f193cf31 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -618,12 +618,45 @@ restart: spin_unlock_irq(&q->queue_lock); } +static void blkg_iostat_set(struct blkg_iostat *dst, struct blkg_iostat *src) +{ + int i; + + for (i = 0; i < BLKG_IOSTAT_NR; i++) { + dst->bytes[i] = src->bytes[i]; + dst->ios[i] = src->ios[i]; + } +} + +static void __blkg_clear_stat(struct blkg_iostat_set *bis) +{ + struct blkg_iostat cur = {0}; + unsigned long flags; + + flags = u64_stats_update_begin_irqsave(&bis->sync); + blkg_iostat_set(&bis->cur, &cur); + blkg_iostat_set(&bis->last, &cur); + u64_stats_update_end_irqrestore(&bis->sync, flags); +} + +static void blkg_clear_stat(struct blkcg_gq *blkg) +{ + int cpu; + + for_each_possible_cpu(cpu) { + struct blkg_iostat_set *s = per_cpu_ptr(blkg->iostat_cpu, cpu); + + __blkg_clear_stat(s); + } + __blkg_clear_stat(&blkg->iostat); +} + static int blkcg_reset_stats(struct cgroup_subsys_state *css, struct cftype *cftype, u64 val) { struct blkcg *blkcg = css_to_blkcg(css); struct blkcg_gq *blkg; - int i, cpu; + int i; mutex_lock(&blkcg_pol_mutex); spin_lock_irq(&blkcg->lock); @@ -634,18 +667,7 @@ static int blkcg_reset_stats(struct cgroup_subsys_state *css, * anyway. If you get hit by a race, retry. */ hlist_for_each_entry(blkg, &blkcg->blkg_list, blkcg_node) { - for_each_possible_cpu(cpu) { - struct blkg_iostat_set *bis = - per_cpu_ptr(blkg->iostat_cpu, cpu); - memset(bis, 0, sizeof(*bis)); - - /* Re-initialize the cleared blkg_iostat_set */ - u64_stats_init(&bis->sync); - bis->blkg = blkg; - } - memset(&blkg->iostat, 0, sizeof(blkg->iostat)); - u64_stats_init(&blkg->iostat.sync); - + blkg_clear_stat(blkg); for (i = 0; i < BLKCG_MAX_POLS; i++) { struct blkcg_policy *pol = blkcg_policy[i]; @@ -948,16 +970,6 @@ void blkg_conf_exit(struct blkg_conf_ctx *ctx) } EXPORT_SYMBOL_GPL(blkg_conf_exit); -static void blkg_iostat_set(struct blkg_iostat *dst, struct blkg_iostat *src) -{ - int i; - - for (i = 0; i < BLKG_IOSTAT_NR; i++) { - dst->bytes[i] = src->bytes[i]; - dst->ios[i] = src->ios[i]; - } -} - static void blkg_iostat_add(struct blkg_iostat *dst, struct blkg_iostat *src) { int i; From d0aac2363549e12cc79b8e285f13d5a9f42fd08e Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Wed, 15 May 2024 09:31:57 +0800 Subject: [PATCH 18/24] blk-cgroup: fix list corruption from reorder of WRITE ->lqueued __blkcg_rstat_flush() can be run anytime, especially when blk_cgroup_bio_start is being executed. If WRITE of `->lqueued` is re-ordered with READ of 'bisc->lnode.next' in the loop of __blkcg_rstat_flush(), `next_bisc` can be assigned with one stat instance being added in blk_cgroup_bio_start(), then the local list in __blkcg_rstat_flush() could be corrupted. Fix the issue by adding one barrier. Cc: Tejun Heo Cc: Waiman Long Fixes: 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()") Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20240515013157.443672-3-ming.lei@redhat.com Signed-off-by: Jens Axboe --- block/blk-cgroup.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 8699f193cf31..52367a4501d0 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -1035,6 +1035,16 @@ static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu) struct blkg_iostat cur; unsigned int seq; + /* + * Order assignment of `next_bisc` from `bisc->lnode.next` in + * llist_for_each_entry_safe and clearing `bisc->lqueued` for + * avoiding to assign `next_bisc` with new next pointer added + * in blk_cgroup_bio_start() in case of re-ordering. + * + * The pair barrier is implied in llist_add() in blk_cgroup_bio_start(). + */ + smp_mb(); + WRITE_ONCE(bisc->lqueued, false); /* fetch the current per-cpu values */ From 9d230c09964e6e18c8f6e4f0d41ee90eef45ec1c Mon Sep 17 00:00:00 2001 From: Waiman Long Date: Wed, 15 May 2024 10:30:59 -0400 Subject: [PATCH 19/24] blk-cgroup: Properly propagate the iostat update up the hierarchy During a cgroup_rstat_flush() call, the lowest level of nodes are flushed first before their parents. Since commit 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()"), iostat propagation was still done to the parent. Grandparent, however, may not get the iostat update if the parent has no blkg_iostat_set queued in its lhead lockless list. Fix this iostat propagation problem by queuing the parent's global blkg->iostat into one of its percpu lockless lists to make sure that the delta will always be propagated up to the grandparent and so on toward the root blkcg. Note that successive calls to __blkcg_rstat_flush() are serialized by the cgroup_rstat_lock. So no special barrier is used in the reading and writing of blkg->iostat.lqueued. Fixes: 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()") Reported-by: Dan Schatzberg Closes: https://lore.kernel.org/lkml/ZkO6l%2FODzadSgdhC@dschatzberg-fedora-PF3DHTBV/ Signed-off-by: Waiman Long Reviewed-by: Ming Lei Acked-by: Tejun Heo Link: https://lore.kernel.org/r/20240515143059.276677-1-longman@redhat.com Signed-off-by: Jens Axboe --- block/blk-cgroup.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 52367a4501d0..37e6cc91d576 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -322,6 +322,7 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct gendisk *disk, blkg->q = disk->queue; INIT_LIST_HEAD(&blkg->q_node); blkg->blkcg = blkcg; + blkg->iostat.blkg = blkg; #ifdef CONFIG_BLK_CGROUP_PUNT_BIO spin_lock_init(&blkg->async_bio_lock); bio_list_init(&blkg->async_bios); @@ -1046,6 +1047,8 @@ static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu) smp_mb(); WRITE_ONCE(bisc->lqueued, false); + if (bisc == &blkg->iostat) + goto propagate_up; /* propagate up to parent only */ /* fetch the current per-cpu values */ do { @@ -1055,10 +1058,24 @@ static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu) blkcg_iostat_update(blkg, &cur, &bisc->last); +propagate_up: /* propagate global delta to parent (unless that's root) */ - if (parent && parent->parent) + if (parent && parent->parent) { blkcg_iostat_update(parent, &blkg->iostat.cur, &blkg->iostat.last); + /* + * Queue parent->iostat to its blkcg's lockless + * list to propagate up to the grandparent if the + * iostat hasn't been queued yet. + */ + if (!parent->iostat.lqueued) { + struct llist_head *plhead; + + plhead = per_cpu_ptr(parent->blkcg->lhead, cpu); + llist_add(&parent->iostat.lnode, plhead); + parent->iostat.lqueued = true; + } + } } raw_spin_unlock_irqrestore(&blkg_stat_lock, flags); out: From 7b815817aa58d2e2101feb2fcf64c60cae0b2695 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Fri, 17 May 2024 10:05:14 +0800 Subject: [PATCH 20/24] blk-mq: add helper for checking if one CPU is mapped to specified hctx Commit a46c27026da1 ("blk-mq: don't schedule block kworker on isolated CPUs") rules out isolated CPUs from hctx->cpumask, and hctx->cpumask should only be used for scheduling kworker. Add helper blk_mq_cpu_mapped_to_hctx() and apply it into cpuhp handlers. This patch avoids to forget clearing INACTIVE of hctx state in case that one isolated CPU becomes online, and fixes hang issue when allocating request from this hctx's tags. Cc: Raju Cheerla Fixes: a46c27026da1 ("blk-mq: don't schedule block kworker on isolated CPUs") Signed-off-by: Ming Lei Link: https://lore.kernel.org/r/20240517020514.149771-1-ming.lei@redhat.com Tested-by: Raju Cheerla Signed-off-by: Jens Axboe --- block/blk-mq.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/block/blk-mq.c b/block/blk-mq.c index 8e01e4b32e10..579921a2f1c6 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -3545,12 +3545,28 @@ static int blk_mq_hctx_notify_offline(unsigned int cpu, struct hlist_node *node) return 0; } +/* + * Check if one CPU is mapped to the specified hctx + * + * Isolated CPUs have been ruled out from hctx->cpumask, which is supposed + * to be used for scheduling kworker only. For other usage, please call this + * helper for checking if one CPU belongs to the specified hctx + */ +static bool blk_mq_cpu_mapped_to_hctx(unsigned int cpu, + const struct blk_mq_hw_ctx *hctx) +{ + struct blk_mq_hw_ctx *mapped_hctx = blk_mq_map_queue_type(hctx->queue, + hctx->type, cpu); + + return mapped_hctx == hctx; +} + static int blk_mq_hctx_notify_online(unsigned int cpu, struct hlist_node *node) { struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node, struct blk_mq_hw_ctx, cpuhp_online); - if (cpumask_test_cpu(cpu, hctx->cpumask)) + if (blk_mq_cpu_mapped_to_hctx(cpu, hctx)) clear_bit(BLK_MQ_S_INACTIVE, &hctx->state); return 0; } @@ -3568,7 +3584,7 @@ static int blk_mq_hctx_notify_dead(unsigned int cpu, struct hlist_node *node) enum hctx_type type; hctx = hlist_entry_safe(node, struct blk_mq_hw_ctx, cpuhp_dead); - if (!cpumask_test_cpu(cpu, hctx->cpumask)) + if (!blk_mq_cpu_mapped_to_hctx(cpu, hctx)) return 0; ctx = __blk_mq_get_ctx(hctx->queue, cpu); From f0eab3e8d1530b87f3523cee060004dd513a6d2b Mon Sep 17 00:00:00 2001 From: Jeff Johnson Date: Thu, 16 May 2024 17:15:06 -0700 Subject: [PATCH 21/24] block: t10-pi: add MODULE_DESCRIPTION() Fix the allmodconfig 'make W=1' issue: WARNING: modpost: missing MODULE_DESCRIPTION() in block/t10-pi.o Signed-off-by: Jeff Johnson Reviewed-by: Martin K. Petersen Link: https://lore.kernel.org/r/20240516-md-t10-pi-v1-1-44a3469374aa@quicinc.com Signed-off-by: Jens Axboe --- block/t10-pi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/t10-pi.c b/block/t10-pi.c index d90892fd6f2a..f4cc91156da1 100644 --- a/block/t10-pi.c +++ b/block/t10-pi.c @@ -495,5 +495,5 @@ const struct blk_integrity_profile ext_pi_type3_crc64 = { }; EXPORT_SYMBOL_GPL(ext_pi_type3_crc64); -MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("T10 Protection Information module"); MODULE_LICENSE("GPL"); From 9a42891c35d50a8472b42c61256867b4dfcc1941 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Wed, 22 May 2024 04:03:08 +0800 Subject: [PATCH 22/24] block: fix lost bio for plug enabled bio based device With the following two conditions, bio will be lost: 1) blk plug is not enabled, for example, __blkdev_direct_IO_simple() and __blkdev_direct_IO_async(); 2) bio plug is enabled, for example write IO for raid1/raid10 while bitmap is enabled; Root cause is that blk_finish_plug() will add the bio to curent->bio_list, while such bio will not be handled: __submit_bio_noacct current->bio_list = bio_list_on_stack; blk_start_plug do { dm_submit_bio md_handle_request raid10_write_request -> generate new bio for underlying disks raid1_add_bio_to_plug -> bio is added to plug } while ((bio = bio_list_pop(&bio_list_on_stack[0]))) -> previous bio are all handled blk_finish_plug raid10_unplug raid1_submit_write submit_bio_noacct if (current->bio_list) bio_list_add(¤t->bio_list[0], bio) -> add new bio current->bio_list = NULL -> new bio is lost Fix the problem by moving the plug into the while loop, so that current->bio_list will still be handled after blk_finish_plug(). By the way, enable plug for raid1/raid10 in this case will also prevent delay IO handling into daemon thread, which should also improve IO performance. Fixes: 060406c61c7c ("block: add plug while submitting IO") Reported-by: Changhui Zhong Closes: https://lore.kernel.org/all/CAGVVp+Xsmzy2G9YuEatfMT6qv1M--YdOCQ0g7z7OVmcTbBxQAg@mail.gmail.com/ Signed-off-by: Yu Kuai Tested-by: Changhui Zhong Link: https://lore.kernel.org/r/20240521200308.983986-1-yukuai1@huaweicloud.com Signed-off-by: Jens Axboe --- block/blk-core.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/block/blk-core.c b/block/blk-core.c index 01186333c88e..dd29d5465af6 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -613,9 +613,14 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q, static void __submit_bio(struct bio *bio) { + /* If plug is not used, add new plug here to cache nsecs time. */ + struct blk_plug plug; + if (unlikely(!blk_crypto_bio_prep(&bio))) return; + blk_start_plug(&plug); + if (!bio->bi_bdev->bd_has_submit_bio) { blk_mq_submit_bio(bio); } else if (likely(bio_queue_enter(bio) == 0)) { @@ -624,6 +629,8 @@ static void __submit_bio(struct bio *bio) disk->fops->submit_bio(bio); blk_queue_exit(disk->queue); } + + blk_finish_plug(&plug); } /* @@ -648,13 +655,11 @@ static void __submit_bio(struct bio *bio) static void __submit_bio_noacct(struct bio *bio) { struct bio_list bio_list_on_stack[2]; - struct blk_plug plug; BUG_ON(bio->bi_next); bio_list_init(&bio_list_on_stack[0]); current->bio_list = bio_list_on_stack; - blk_start_plug(&plug); do { struct request_queue *q = bdev_get_queue(bio->bi_bdev); @@ -688,23 +693,19 @@ static void __submit_bio_noacct(struct bio *bio) bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]); } while ((bio = bio_list_pop(&bio_list_on_stack[0]))); - blk_finish_plug(&plug); current->bio_list = NULL; } static void __submit_bio_noacct_mq(struct bio *bio) { struct bio_list bio_list[2] = { }; - struct blk_plug plug; current->bio_list = bio_list; - blk_start_plug(&plug); do { __submit_bio(bio); } while ((bio = bio_list_pop(&bio_list[0]))); - blk_finish_plug(&plug); current->bio_list = NULL; } From 4a482e691c8b8a188b1ea3d6a80180e9fa925fd0 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Wed, 22 May 2024 18:24:58 +0100 Subject: [PATCH 23/24] blk-throttle: remove unused struct 'avg_latency_bucket' 'avg_latency_bucket' is unused since commit bf20ab538c81 ("blk-throttle: remove CONFIG_BLK_DEV_THROTTLING_LOW") Remove it. Signed-off-by: Dr. David Alan Gilbert Link: https://lore.kernel.org/r/20240522172458.334173-1-linux@treblig.org Signed-off-by: Jens Axboe --- block/blk-throttle.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/block/blk-throttle.c b/block/blk-throttle.c index 80aaca18bfb0..0be180f9a789 100644 --- a/block/blk-throttle.c +++ b/block/blk-throttle.c @@ -39,11 +39,6 @@ struct latency_bucket { int samples; }; -struct avg_latency_bucket { - unsigned long latency; /* ns / 1024 */ - bool valid; -}; - struct throtl_data { /* service tree for active throtl groups */ From a2db328b0839312c169eb42746ec46fc1ab53ed2 Mon Sep 17 00:00:00 2001 From: Yu Kuai Date: Thu, 23 May 2024 23:39:34 +0800 Subject: [PATCH 24/24] null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues' Writing 'power' and 'submit_queues' concurrently will trigger kernel panic: Test script: modprobe null_blk nr_devices=0 mkdir -p /sys/kernel/config/nullb/nullb0 while true; do echo 1 > submit_queues; echo 4 > submit_queues; done & while true; do echo 1 > power; echo 0 > power; done Test result: BUG: kernel NULL pointer dereference, address: 0000000000000148 Oops: 0000 [#1] PREEMPT SMP RIP: 0010:__lock_acquire+0x41d/0x28f0 Call Trace: lock_acquire+0x121/0x450 down_write+0x5f/0x1d0 simple_recursive_removal+0x12f/0x5c0 blk_mq_debugfs_unregister_hctxs+0x7c/0x100 blk_mq_update_nr_hw_queues+0x4a3/0x720 nullb_update_nr_hw_queues+0x71/0xf0 [null_blk] nullb_device_submit_queues_store+0x79/0xf0 [null_blk] configfs_write_iter+0x119/0x1e0 vfs_write+0x326/0x730 ksys_write+0x74/0x150 This is because del_gendisk() can concurrent with blk_mq_update_nr_hw_queues(): nullb_device_power_store nullb_apply_submit_queues null_del_dev del_gendisk nullb_update_nr_hw_queues if (!dev->nullb) // still set while gendisk is deleted return 0 blk_mq_update_nr_hw_queues dev->nullb = NULL Fix this problem by resuing the global mutex to protect nullb_device_power_store() and nullb_update_nr_hw_queues() from configfs. Fixes: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured") Reported-and-tested-by: Yi Zhang Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/ Signed-off-by: Yu Kuai Reviewed-by: Zhu Yanjun Link: https://lore.kernel.org/r/20240523153934.1937851-1-yukuai1@huaweicloud.com Signed-off-by: Jens Axboe --- drivers/block/null_blk/main.c | 40 +++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 5d56ad4ce01a..eb023d267369 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -413,13 +413,25 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev, static int nullb_apply_submit_queues(struct nullb_device *dev, unsigned int submit_queues) { - return nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues); + int ret; + + mutex_lock(&lock); + ret = nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues); + mutex_unlock(&lock); + + return ret; } static int nullb_apply_poll_queues(struct nullb_device *dev, unsigned int poll_queues) { - return nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues); + int ret; + + mutex_lock(&lock); + ret = nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues); + mutex_unlock(&lock); + + return ret; } NULLB_DEVICE_ATTR(size, ulong, NULL); @@ -468,28 +480,31 @@ static ssize_t nullb_device_power_store(struct config_item *item, if (ret < 0) return ret; + ret = count; + mutex_lock(&lock); if (!dev->power && newp) { if (test_and_set_bit(NULLB_DEV_FL_UP, &dev->flags)) - return count; + goto out; + ret = null_add_dev(dev); if (ret) { clear_bit(NULLB_DEV_FL_UP, &dev->flags); - return ret; + goto out; } set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); dev->power = newp; } else if (dev->power && !newp) { if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) { - mutex_lock(&lock); dev->power = newp; null_del_dev(dev->nullb); - mutex_unlock(&lock); } clear_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); } - return count; +out: + mutex_unlock(&lock); + return ret; } CONFIGFS_ATTR(nullb_device_, power); @@ -1932,15 +1947,12 @@ static int null_add_dev(struct nullb_device *dev) nullb->q->queuedata = nullb; blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q); - mutex_lock(&lock); rv = ida_alloc(&nullb_indexes, GFP_KERNEL); - if (rv < 0) { - mutex_unlock(&lock); + if (rv < 0) goto out_cleanup_disk; - } + nullb->index = rv; dev->index = rv; - mutex_unlock(&lock); if (config_item_name(&dev->group.cg_item)) { /* Use configfs dir name as the device name */ @@ -1969,9 +1981,7 @@ static int null_add_dev(struct nullb_device *dev) if (rv) goto out_ida_free; - mutex_lock(&lock); list_add_tail(&nullb->list, &nullb_list); - mutex_unlock(&lock); pr_info("disk %s created\n", nullb->disk_name); @@ -2020,7 +2030,9 @@ static int null_create_dev(void) if (!dev) return -ENOMEM; + mutex_lock(&lock); ret = null_add_dev(dev); + mutex_unlock(&lock); if (ret) { null_free_dev(dev); return ret;