From: Long Li The second conditional checking nsock->fallback_index validity is the logical inverse of the first, so drop it and let execution fall through naturally. Consolidate the two identical dev_err_ratelimited() + return paths into a single no_fallback label to reduce duplication. Signed-off-by: Long Li Reviewed-by: Yu Kuai --- drivers/block/nbd.c | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 8f10762e90ef..b1a5acd57426 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -1061,40 +1061,31 @@ static int find_fallback(struct nbd_device *nbd, int index) int new_index = -1; struct nbd_sock *nsock = config->socks[index]; int fallback = nsock->fallback_index; + int i; if (test_bit(NBD_RT_DISCONNECTED, &config->runtime_flags)) return new_index; - if (config->num_connections <= 1) { - dev_err_ratelimited(disk_to_dev(nbd->disk), - "Dead connection, failed to find a fallback\n"); - return new_index; - } + if (config->num_connections <= 1) + goto no_fallback; if (fallback >= 0 && fallback < config->num_connections && !config->socks[fallback]->dead) return fallback; - if (nsock->fallback_index < 0 || - nsock->fallback_index >= config->num_connections || - config->socks[nsock->fallback_index]->dead) { - int i; - for (i = 0; i < config->num_connections; i++) { - if (i == index) - continue; - if (!config->socks[i]->dead) { - new_index = i; - break; - } - } - nsock->fallback_index = new_index; - if (new_index < 0) { - dev_err_ratelimited(disk_to_dev(nbd->disk), - "Dead connection, failed to find a fallback\n"); - return new_index; + for (i = 0; i < config->num_connections; i++) { + if (i != index && !config->socks[i]->dead) { + new_index = i; + break; } } - new_index = nsock->fallback_index; + nsock->fallback_index = new_index; + if (new_index >= 0) + return new_index; + +no_fallback: + dev_err_ratelimited(disk_to_dev(nbd->disk), + "Dead connection, failed to find a fallback\n"); return new_index; } -- 2.52.0 We cannot add a socket to an already running nbd device, the reconfigure for netlink can only active an inactive socket. But for ioctl path, we can call NBD_SET_SOCK after NBD_DO_IT, reject this using nbd->pid which has been setted when NBD_DO_IT. Besides, it is the root cause for commit b98e762e3d71 ("nbd: freeze the queue while we're adding connections"). Reviewed-by: Yu Kuai Signed-off-by: Yang Erkun --- drivers/block/nbd.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index b1a5acd57426..7ec85f94f742 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -1278,6 +1278,13 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg, /* Arg will be cast to int, check it to avoid overflow */ if (arg > INT_MAX) return -EINVAL; + + if (nbd->pid) { + dev_err(disk_to_dev(nbd->disk), + "Cannot add socket to a running device\n"); + return -EBUSY; + } + sock = nbd_get_socket(nbd, arg, &err); if (!sock) return err; -- 2.52.0 An inactive nbd device may refuse any I/O operations. The nbd_config_put function calls invalidate_disk, which sets the device capacity to zero to reject all read and write I/O. For zero-sector flush I/O requests from blkdev_issue_flush, if the write cache is disabled, the zero-sector flush I/O immediately returns 0 in submit_bio_noacct. However, since nbd_config_put does not clear the write cache state, an inactive nbd device might still have the write cache enabled. In this situation, zero-sector flush I/O will return -EIO because there is no active socket. Additionally, BLK_FEAT_FUA and BLK_FEAT_ROTATIONAL flags may also remain stale, resetting all of them ensures consistent behavior. The limits update uses queue_limits_commit_update() (the non-freezing variant) because config_refs == 0 here means every fd is closed and recv threads have drained, so no in-flight I/O can read q->limits concurrently. Signed-off-by: Yang Erkun --- drivers/block/nbd.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 7ec85f94f742..78df6f459da6 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -331,6 +331,26 @@ static void nbd_mark_nsock_dead(struct nbd_device *nbd, struct nbd_sock *nsock, nsock->sent = 0; } +static void nbd_apply_limits(struct queue_limits *lim, u32 flags) +{ + lim->features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA | BLK_FEAT_ROTATIONAL); + lim->max_hw_discard_sectors = 0; + lim->max_write_zeroes_sectors = 0; + + if (flags & NBD_FLAG_SEND_TRIM) + lim->max_hw_discard_sectors = UINT_MAX >> SECTOR_SHIFT; + if (flags & NBD_FLAG_SEND_FLUSH) { + lim->features |= BLK_FEAT_WRITE_CACHE; + if (flags & NBD_FLAG_SEND_FUA) + lim->features |= BLK_FEAT_FUA; + } + + if (flags & NBD_FLAG_ROTATIONAL) + lim->features |= BLK_FEAT_ROTATIONAL; + if (flags & NBD_FLAG_SEND_WRITE_ZEROES) + lim->max_write_zeroes_sectors = UINT_MAX >> SECTOR_SHIFT; +} + static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize) { struct queue_limits lim; @@ -352,23 +372,7 @@ static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize) return 0; lim = queue_limits_start_update(nbd->disk->queue); - if (nbd->config->flags & NBD_FLAG_SEND_TRIM) - lim.max_hw_discard_sectors = UINT_MAX >> SECTOR_SHIFT; - else - lim.max_hw_discard_sectors = 0; - if (!(nbd->config->flags & NBD_FLAG_SEND_FLUSH)) { - lim.features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA); - } else if (nbd->config->flags & NBD_FLAG_SEND_FUA) { - lim.features |= BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA; - } else { - lim.features |= BLK_FEAT_WRITE_CACHE; - lim.features &= ~BLK_FEAT_FUA; - } - if (nbd->config->flags & NBD_FLAG_ROTATIONAL) - lim.features |= BLK_FEAT_ROTATIONAL; - if (nbd->config->flags & NBD_FLAG_SEND_WRITE_ZEROES) - lim.max_write_zeroes_sectors = UINT_MAX >> SECTOR_SHIFT; - + nbd_apply_limits(&lim, nbd->config->flags); lim.logical_block_size = blksize; lim.physical_block_size = blksize; error = queue_limits_commit_update_frozen(nbd->disk->queue, &lim); @@ -1469,8 +1473,13 @@ static void nbd_config_put(struct nbd_device *nbd) if (refcount_dec_and_mutex_lock(&nbd->config_refs, &nbd->config_lock)) { struct nbd_config *config = nbd->config; + struct queue_limits lim; nbd_dev_dbg_close(nbd); invalidate_disk(nbd->disk); + /* reset queue limits to default */ + lim = queue_limits_start_update(nbd->disk->queue); + nbd_apply_limits(&lim, 0); + queue_limits_commit_update(nbd->disk->queue, &lim); if (nbd->config->bytesize) kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE); if (test_and_clear_bit(NBD_RT_HAS_PID_FILE, -- 2.52.0 nbd_add_socket() kreallocs config->socks, which a concurrent reader in nbd_handle_cmd() could UAF; commit b98e762e3d71 ("nbd: freeze the queue while we're adding connections")froze the queue to block that. But the freeze costs an RCU grace period on every socket added, and setup adds them one by one. After the previous patch, nbd_add_socket() is rejected once nbd->pid is set, so it only runs during setup. There the capacity is 0 and the write cache is off (cleared on disconnect by the preceding patch, and re-enabled only later in nbd_set_size), so submit_bio_noacct() rejects every bio before it reaches the driver -- non-zero-sector ones via bio_check_eod(), and flush-only ones via the !bdev_write_cache() branch. No I/O is in flight, so the freeze is unnecessary. Signed-off-by: Yang Erkun --- drivers/block/nbd.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 78df6f459da6..cb7c1f8502f4 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -1276,7 +1276,6 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg, struct socket *sock; struct nbd_sock **socks; struct nbd_sock *nsock; - unsigned int memflags; int err; /* Arg will be cast to int, check it to avoid overflow */ @@ -1294,12 +1293,6 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg, return err; nbd_reclassify_socket(sock); - /* - * We need to make sure we don't get any errant requests while we're - * reallocating the ->socks array. - */ - memflags = blk_mq_freeze_queue(nbd->disk->queue); - if (!netlink && !nbd->task_setup && !test_bit(NBD_RT_BOUND, &config->runtime_flags)) nbd->task_setup = current; @@ -1339,12 +1332,10 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg, INIT_WORK(&nsock->work, nbd_pending_cmd_work); socks[config->num_connections++] = nsock; atomic_inc(&config->live_connections); - blk_mq_unfreeze_queue(nbd->disk->queue, memflags); return 0; put_socket: - blk_mq_unfreeze_queue(nbd->disk->queue, memflags); sockfd_put(sock); return err; } -- 2.52.0 Commit 242a49e5c878 ("nbd: freeze the queue for queue limits updates") added the freeze to keep in-flight commands from seeing torn queue_limits. But at startup the capacity is still 0 (invalidate_disk cleared it) and the write cache is off (the previous patch cleared it on disconnect, and nbd_set_size sets it back only after the commit), so submit_bio_noacct() rejects any bio before it reaches the driver and no I/O is in flight. Drop the freeze by checking capacity and write cache state in nbd_set_size. Signed-off-by: Yang Erkun --- 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 cb7c1f8502f4..cb662ae4b91d 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -375,7 +375,11 @@ static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize) nbd_apply_limits(&lim, nbd->config->flags); lim.logical_block_size = blksize; lim.physical_block_size = blksize; - error = queue_limits_commit_update_frozen(nbd->disk->queue, &lim); + /* No need freeze with 0 capacity and write cache disabled */ + if (!get_capacity(nbd->disk) && !blk_queue_write_cache(nbd->disk->queue)) + error = queue_limits_commit_update(nbd->disk->queue, &lim); + else + error = queue_limits_commit_update_frozen(nbd->disk->queue, &lim); if (error) return error; -- 2.52.0 The NBD_ATTR_SOCKETS walk is duplicated in nbd_genl_connect (add sockets) and nbd_genl_reconfigure (reconnect). Factor out a single helper that walks the list and calls a callback per fd; with a NULL callback it is a pure counter, used by a later patch to learn nr_hw_queues before the device exists. Returns the number of fds walked (>= 0) or a negative errno; a callback >0 will stops early. Signed-off-by: Yang Erkun --- drivers/block/nbd.c | 131 ++++++++++++++++++++++---------------------- 1 file changed, 66 insertions(+), 65 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index cb662ae4b91d..6f4e9fabe52a 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -1399,11 +1399,12 @@ static int nbd_reconnect_socket(struct nbd_device *nbd, unsigned long arg) atomic_inc(&config->live_connections); wake_up(&config->conn_wait); + dev_info(nbd_to_dev(nbd), "reconnected socket\n"); return 0; } sockfd_put(sock); kfree(args); - return -ENOSPC; + return 1; } static void nbd_bdev_reset(struct nbd_device *nbd) @@ -2109,6 +2110,58 @@ static int nbd_genl_size_set(struct genl_info *info, struct nbd_device *nbd) return 0; } +/* + * Walk the NBD_ATTR_SOCKETS nested list can call @cb for each socket fd. + * + * Return the number of fds walked, or a negative errno. + */ +static int nbd_genl_foreach_sock(struct genl_info *info, + int (*cb)(struct nbd_device *nbd, int fd), + struct nbd_device *nbd) +{ + struct nlattr *attr; + int rem, count = 0; + + if (!info->attrs[NBD_ATTR_SOCKETS]) + return 0; + + nla_for_each_nested(attr, info->attrs[NBD_ATTR_SOCKETS], rem) { + struct nlattr *socks[NBD_SOCK_MAX + 1]; + int ret; + + if (nla_type(attr) != NBD_SOCK_ITEM) { + pr_err("socks must be embedded in a SOCK_ITEM attr\n"); + return -EINVAL; + } + + if (nla_parse_nested_deprecated(socks, NBD_SOCK_MAX, + attr, + nbd_sock_policy, + info->extack)) { + pr_err("error processing sock list\n"); + return -EINVAL; + } + + if (!socks[NBD_SOCK_FD]) + continue; + + count++; + if (cb) { + ret = cb(nbd, (int)nla_get_u32(socks[NBD_SOCK_FD])); + if (ret > 0) + return count; + if (ret < 0) + return ret; + } + } + return count; +} + +static int nbd_genl_connect_sock_cb(struct nbd_device *nbd, int fd) +{ + return nbd_add_socket(nbd, fd, true); +} + static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info) { struct nbd_device *nbd; @@ -2228,36 +2281,9 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info) } } - if (info->attrs[NBD_ATTR_SOCKETS]) { - struct nlattr *attr; - int rem, fd; - - nla_for_each_nested(attr, info->attrs[NBD_ATTR_SOCKETS], - rem) { - struct nlattr *socks[NBD_SOCK_MAX+1]; - - if (nla_type(attr) != NBD_SOCK_ITEM) { - pr_err("socks must be embedded in a SOCK_ITEM attr\n"); - ret = -EINVAL; - goto out; - } - ret = nla_parse_nested_deprecated(socks, NBD_SOCK_MAX, - attr, - nbd_sock_policy, - info->extack); - if (ret != 0) { - pr_err("error processing sock list\n"); - ret = -EINVAL; - goto out; - } - if (!socks[NBD_SOCK_FD]) - continue; - fd = (int)nla_get_u32(socks[NBD_SOCK_FD]); - ret = nbd_add_socket(nbd, fd, true); - if (ret) - goto out; - } - } + ret = nbd_genl_foreach_sock(info, nbd_genl_connect_sock_cb, nbd); + if (ret < 0) + goto out; if (info->attrs[NBD_ATTR_BACKEND_IDENTIFIER]) { nbd->backend = nla_strdup(info->attrs[NBD_ATTR_BACKEND_IDENTIFIER], @@ -2346,6 +2372,11 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info) return 0; } +static int nbd_genl_reconnect_sock_cb(struct nbd_device *nbd, int fd) +{ + return nbd_reconnect_socket(nbd, fd); +} + static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info) { struct nbd_device *nbd = NULL; @@ -2442,40 +2473,10 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info) } } - if (info->attrs[NBD_ATTR_SOCKETS]) { - struct nlattr *attr; - int rem, fd; - - nla_for_each_nested(attr, info->attrs[NBD_ATTR_SOCKETS], - rem) { - struct nlattr *socks[NBD_SOCK_MAX+1]; - - if (nla_type(attr) != NBD_SOCK_ITEM) { - pr_err("socks must be embedded in a SOCK_ITEM attr\n"); - ret = -EINVAL; - goto out; - } - ret = nla_parse_nested_deprecated(socks, NBD_SOCK_MAX, - attr, - nbd_sock_policy, - info->extack); - if (ret != 0) { - pr_err("error processing sock list\n"); - ret = -EINVAL; - goto out; - } - if (!socks[NBD_SOCK_FD]) - continue; - fd = (int)nla_get_u32(socks[NBD_SOCK_FD]); - ret = nbd_reconnect_socket(nbd, fd); - if (ret) { - if (ret == -ENOSPC) - ret = 0; - goto out; - } - dev_info(nbd_to_dev(nbd), "reconnected socket\n"); - } - } + ret = nbd_genl_foreach_sock(info, nbd_genl_reconnect_sock_cb, nbd); + /* foreach_sock returns a positive count on success; doit must return 0 */ + if (ret >= 0) + ret = 0; out: mutex_unlock(&nbd->config_lock); nbd_config_put(nbd); -- 2.52.0 Previous commits has removed the queue freeze in nbd_add_socket and nbd_set_size during nbd device setup. However, a queue freeze can still occur when nbd_start_device calls blk_mq_update_nr_hw_queues if the socket connection count does not match nbd->tag_set->nr_hw_queues. The nbd_start_device function can be invoked through either the ioctl or netlink paths. The ioctl path only allows reusing an existing inactivate nbd device, there is nothing more we can do to prevent the queue freeze since the old nbd->tag_set->nr_hw_queues may not match the new socket connection count. Similarly, the netlink path can reuse a preferred inactivate nbd device, and again, we cannot do more in this scenario. However, the netlink path can also add a new nbd device using nbd_dev_add. In this case, we can obtain the new number of socket connections, and by adding a new argument representing the expected nr_hw_queues in nbd_dev_add, we can ensure the queue freeze is avoided for this situation. Reviewed-by: Yu Kuai Signed-off-by: Yang Erkun --- drivers/block/nbd.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 6f4e9fabe52a..b316129b9a17 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -1944,7 +1944,8 @@ static const struct blk_mq_ops nbd_mq_ops = { .timeout = nbd_xmit_timeout, }; -static struct nbd_device *nbd_dev_add(int index, unsigned int refs) +static struct nbd_device *nbd_dev_add(int index, unsigned int refs, + int nr_hw_queues) { struct queue_limits lim = { .max_hw_sectors = 65536, @@ -1961,7 +1962,7 @@ static struct nbd_device *nbd_dev_add(int index, unsigned int refs) goto out; nbd->tag_set.ops = &nbd_mq_ops; - nbd->tag_set.nr_hw_queues = 1; + nbd->tag_set.nr_hw_queues = nr_hw_queues; nbd->tag_set.queue_depth = 128; nbd->tag_set.numa_node = NUMA_NO_NODE; nbd->tag_set.cmd_size = sizeof(struct nbd_cmd); @@ -2214,7 +2215,11 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info) mutex_unlock(&nbd_index_mutex); if (!nbd) { - nbd = nbd_dev_add(index, 2); + ret = nbd_genl_foreach_sock(info, NULL, NULL); + if (ret < 0) + return ret; + + nbd = nbd_dev_add(index, 2, ret > 0 ? ret : 1); if (IS_ERR(nbd)) { pr_err("failed to add new device\n"); return PTR_ERR(nbd); @@ -2729,7 +2734,7 @@ static int __init nbd_init(void) nbd_dbg_init(); for (i = 0; i < nbds_max; i++) - nbd_dev_add(i, 1); + nbd_dev_add(i, 1, 1); return 0; } -- 2.52.0 blk_mq_update_nr_hw_queues() in nbd_start_device() may cause a queue freeze. The previous commit addressed this for newly created nbd devices by setting the expected nr_hw_queues in nbd_dev_add(). However, when reusing an old inactive nbd device, the queue freeze can still occur if the old nbd->tag_set->nr_hw_queues does not match the new socket connection count. Inactive nbd devices can originate from two sources: loading the nbd module with nbds_max, which sets the default nr_hw_queues to 1, and the netlink method, which sets nr_hw_queues according to the expected number of socket connections. For the first case, add a module parameter so the default nr_hw_queues can be changed. Users who know their expected number of connections can then prevent queue freezes on pre-created devices via nbds_max. Before this patchset: real 0m2.195s user 0m0.005s sys 0m0.022s After this patchset: real 0m0.090s user 0m0.004s sys 0m0.018s Signed-off-by: Yang Erkun --- drivers/block/nbd.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index b316129b9a17..f8f51df31ada 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -166,6 +166,7 @@ static struct dentry *nbd_dbg_dir; static unsigned int nbds_max = 16; static int max_part = 16; +static int pre_defined_connections = 1; static int part_shift; static int nbd_dev_dbg_init(struct nbd_device *nbd); @@ -2733,8 +2734,13 @@ static int __init nbd_init(void) } nbd_dbg_init(); + if (pre_defined_connections < 1) + pre_defined_connections = 1; + /* Set to the intended connection count so nbd_start_device() can skip + * the queue-freezing blk_mq_update_nr_hw_queues() call. + */ for (i = 0; i < nbds_max; i++) - nbd_dev_add(i, 1, 1); + nbd_dev_add(i, 1, pre_defined_connections); return 0; } @@ -2795,3 +2801,6 @@ module_param(nbds_max, int, 0444); MODULE_PARM_DESC(nbds_max, "number of network block devices to initialize (default: 16)"); module_param(max_part, int, 0444); MODULE_PARM_DESC(max_part, "number of partitions per device (default: 16)"); +module_param(pre_defined_connections, int, 0444); +MODULE_PARM_DESC(pre_defined_connections, +"number of connections for devices pre-created at module load (default: 1)"); -- 2.52.0