From 80d43cbd46155744ee450d2476ee4fcf2917ae9b Mon Sep 17 00:00:00 2001 From: Md Haris Iqbal Date: Wed, 28 Apr 2021 08:13:56 +0200 Subject: block/rnbd-clt: Change queue_depth type in rnbd_clt_session to size_t The member queue_depth in the structure rnbd_clt_session is read from the rtrs client side using the function rtrs_clt_query, which in turn is read from the rtrs_clt structure. It should really be of type size_t. Fixes: 90426e89f54db ("block/rnbd: client: private header with client structs and functions") Signed-off-by: Md Haris Iqbal Reviewed-by: Guoqing Jiang Signed-off-by: Gioh Kim Link: https://lore.kernel.org/r/20210428061359.206794-2-gi-oh.kim@ionos.com Signed-off-by: Jens Axboe --- drivers/block/rnbd/rnbd-clt.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/block') diff --git a/drivers/block/rnbd/rnbd-clt.h b/drivers/block/rnbd/rnbd-clt.h index 451e7383738f..b5322c5aaac0 100644 --- a/drivers/block/rnbd/rnbd-clt.h +++ b/drivers/block/rnbd/rnbd-clt.h @@ -87,7 +87,7 @@ struct rnbd_clt_session { DECLARE_BITMAP(cpu_queues_bm, NR_CPUS); int __percpu *cpu_rr; /* per-cpu var for CPU round-robin */ atomic_t busy; - int queue_depth; + size_t queue_depth; u32 max_io_size; struct blk_mq_tag_set tag_set; u32 nr_poll_queues; -- cgit v1.2.3 From 292660fa35e8917a78235d39722edf9bbc04cab7 Mon Sep 17 00:00:00 2001 From: Dima Stepanov Date: Wed, 28 Apr 2021 08:13:57 +0200 Subject: block/rnbd: Fix style issues This patch fixes some style issues detected by scripts/checkpatch.pl * Resolve spacing and tab issues * Remove extra braces in rnbd_get_iu * Use num_possible_cpus() instead of NR_CPUS in alloc_sess * Fix the comments styling in rnbd_queue_rq Signed-off-by: Dima Stepanov Signed-off-by: Gioh Kim Signed-off-by: Md Haris Iqbal Signed-off-by: Jack Wang Link: https://lore.kernel.org/r/20210428061359.206794-3-gi-oh.kim@ionos.com Signed-off-by: Jens Axboe --- drivers/block/rnbd/rnbd-clt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index c01786afe1b1..15f831159c31 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -88,7 +88,7 @@ static int rnbd_clt_set_dev_attr(struct rnbd_clt_dev *dev, dev->discard_alignment = le32_to_cpu(rsp->discard_alignment); dev->secure_discard = le16_to_cpu(rsp->secure_discard); dev->rotational = rsp->rotational; - dev->wc = !!(rsp->cache_policy & RNBD_WRITEBACK); + dev->wc = !!(rsp->cache_policy & RNBD_WRITEBACK); dev->fua = !!(rsp->cache_policy & RNBD_FUA); dev->max_hw_sectors = sess->max_io_size / SECTOR_SIZE; @@ -351,9 +351,8 @@ static struct rnbd_iu *rnbd_get_iu(struct rnbd_clt_session *sess, struct rtrs_permit *permit; iu = kzalloc(sizeof(*iu), GFP_KERNEL); - if (!iu) { + if (!iu) return NULL; - } permit = rnbd_get_permit(sess, con_type, wait); if (unlikely(!permit)) { @@ -805,7 +804,7 @@ static struct rnbd_clt_session *alloc_sess(const char *sessname) mutex_init(&sess->lock); INIT_LIST_HEAD(&sess->devs_list); INIT_LIST_HEAD(&sess->list); - bitmap_zero(sess->cpu_queues_bm, NR_CPUS); + bitmap_zero(sess->cpu_queues_bm, num_possible_cpus()); init_waitqueue_head(&sess->rtrs_waitq); refcount_set(&sess->refcount, 1); @@ -1148,7 +1147,8 @@ static blk_status_t rnbd_queue_rq(struct blk_mq_hw_ctx *hctx, iu->sgt.sgl = iu->first_sgl; err = sg_alloc_table_chained(&iu->sgt, /* Even-if the request has no segment, - * sglist must have one entry at least */ + * sglist must have one entry at least. + */ blk_rq_nr_phys_segments(rq) ? : 1, iu->sgt.sgl, RNBD_INLINE_SG_CNT); -- cgit v1.2.3 From 1056ad829ec43f9b705b507c2093b05e2088b0b7 Mon Sep 17 00:00:00 2001 From: Md Haris Iqbal Date: Wed, 28 Apr 2021 08:13:58 +0200 Subject: block/rnbd-clt: Check the return value of the function rtrs_clt_query In case none of the paths are in connected state, the function rtrs_clt_query returns an error. In such a case, error out since the values in the rtrs_attrs structure would be garbage. Fixes: f7a7a5c228d45 ("block/rnbd: client: main functionality") Signed-off-by: Md Haris Iqbal Reviewed-by: Guoqing Jiang Signed-off-by: Jack Wang Signed-off-by: Gioh Kim Link: https://lore.kernel.org/r/20210428061359.206794-4-gi-oh.kim@ionos.com Signed-off-by: Jens Axboe --- drivers/block/rnbd/rnbd-clt.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index 15f831159c31..f855bf1fa8d5 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -691,7 +691,11 @@ static void remap_devs(struct rnbd_clt_session *sess) return; } - rtrs_clt_query(sess->rtrs, &attrs); + err = rtrs_clt_query(sess->rtrs, &attrs); + if (err) { + pr_err("rtrs_clt_query(\"%s\"): %d\n", sess->sessname, err); + return; + } mutex_lock(&sess->lock); sess->max_io_size = attrs.max_io_size; @@ -1294,7 +1298,11 @@ find_and_get_or_create_sess(const char *sessname, err = PTR_ERR(sess->rtrs); goto wake_up_and_put; } - rtrs_clt_query(sess->rtrs, &attrs); + + err = rtrs_clt_query(sess->rtrs, &attrs); + if (err) + goto close_rtrs; + sess->max_io_size = attrs.max_io_size; sess->queue_depth = attrs.queue_depth; sess->nr_poll_queues = nr_poll_queues; -- cgit v1.2.3 From 1e31016b6926c996e9113619c2ce1f42ad74ddd1 Mon Sep 17 00:00:00 2001 From: Gioh Kim Date: Wed, 28 Apr 2021 08:13:59 +0200 Subject: block/rnbd: Remove all likely and unlikely The IO performance test with fio after removing the likely and unlikely macros in all if-statement shows no performance drop. They do not help for the performance of rnbd. The fio test did random read on 32 rnbd devices and 64 processes. Test environment: - AMD Opteron(tm) Processor 6386 SE - 125G memory - kernel version: 5.4.86 - gcc version: gcc (Debian 8.3.0-6) 8.3.0 - Infiniband controller: InfiniBand: Mellanox Technologies MT26428 [ConnectX VPI PCIe 2.0 5GT/s - IB QDR / 10GigE] (rev b0) before read: IOPS=549k, BW=2146MiB/s read: IOPS=544k, BW=2125MiB/s read: IOPS=553k, BW=2158MiB/s read: IOPS=535k, BW=2089MiB/s read: IOPS=543k, BW=2122MiB/s read: IOPS=552k, BW=2154MiB/s average: IOPS=546k, BW=2132MiB/s after read: IOPS=556k, BW=2172MiB/s read: IOPS=561k, BW=2191MiB/s read: IOPS=552k, BW=2156MiB/s read: IOPS=551k, BW=2154MiB/s read: IOPS=562k, BW=2194MiB/s ----------- average: IOPS=556k, BW=2173MiB/s The IOPS and bandwidth got better slightly after removing likely/unlikely. (IOPS= +1.8% BW= +1.9%) But we cannot make sure that removing the likely/unlikely help the performance because it depends on various situations. We only make sure that removing the likely/unlikely does not drop the performance. Signed-off-by: Gioh Kim Reviewed-by: Md Haris Iqbal Link: https://lore.kernel.org/r/20210428061359.206794-5-gi-oh.kim@ionos.com Signed-off-by: Jens Axboe --- drivers/block/rnbd/rnbd-clt.c | 24 ++++++++++++------------ drivers/block/rnbd/rnbd-srv.c | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) (limited to 'drivers/block') diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index f855bf1fa8d5..c604a402cd5c 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -241,7 +241,7 @@ static bool rnbd_rerun_if_needed(struct rnbd_clt_session *sess) cpu_q = rnbd_get_cpu_qlist(sess, nxt_cpu(cpu_q->cpu))) { if (!spin_trylock_irqsave(&cpu_q->requeue_lock, flags)) continue; - if (unlikely(!test_bit(cpu_q->cpu, sess->cpu_queues_bm))) + if (!test_bit(cpu_q->cpu, sess->cpu_queues_bm)) goto unlock; q = list_first_entry_or_null(&cpu_q->requeue_list, typeof(*q), requeue_list); @@ -320,7 +320,7 @@ static struct rtrs_permit *rnbd_get_permit(struct rnbd_clt_session *sess, struct rtrs_permit *permit; permit = rtrs_clt_get_permit(sess->rtrs, con_type, wait); - if (likely(permit)) + if (permit) /* We have a subtle rare case here, when all permits can be * consumed before busy counter increased. This is safe, * because loser will get NULL as a permit, observe 0 busy @@ -355,7 +355,7 @@ static struct rnbd_iu *rnbd_get_iu(struct rnbd_clt_session *sess, return NULL; permit = rnbd_get_permit(sess, con_type, wait); - if (unlikely(!permit)) { + if (!permit) { kfree(iu); return NULL; } @@ -1050,7 +1050,7 @@ static int rnbd_client_xfer_request(struct rnbd_clt_dev *dev, }; err = rtrs_clt_request(rq_data_dir(rq), &req_ops, rtrs, permit, &vec, 1, size, iu->sgt.sgl, sg_cnt); - if (unlikely(err)) { + if (err) { rnbd_clt_err_rl(dev, "RTRS failed to transfer IO, err: %d\n", err); return err; @@ -1081,7 +1081,7 @@ static bool rnbd_clt_dev_add_to_requeue(struct rnbd_clt_dev *dev, cpu_q = get_cpu_ptr(sess->cpu_queues); spin_lock_irqsave(&cpu_q->requeue_lock, flags); - if (likely(!test_and_set_bit_lock(0, &q->in_list))) { + if (!test_and_set_bit_lock(0, &q->in_list)) { if (WARN_ON(!list_empty(&q->requeue_list))) goto unlock; @@ -1093,7 +1093,7 @@ static bool rnbd_clt_dev_add_to_requeue(struct rnbd_clt_dev *dev, */ smp_mb__before_atomic(); } - if (likely(atomic_read(&sess->busy))) { + if (atomic_read(&sess->busy)) { list_add_tail(&q->requeue_list, &cpu_q->requeue_list); } else { /* Very unlikely, but possible: busy counter was @@ -1121,7 +1121,7 @@ static void rnbd_clt_dev_kick_mq_queue(struct rnbd_clt_dev *dev, if (delay != RNBD_DELAY_IFBUSY) blk_mq_delay_run_hw_queue(hctx, delay); - else if (unlikely(!rnbd_clt_dev_add_to_requeue(dev, q))) + else if (!rnbd_clt_dev_add_to_requeue(dev, q)) /* * If session is not busy we have to restart * the queue ourselves. @@ -1138,12 +1138,12 @@ static blk_status_t rnbd_queue_rq(struct blk_mq_hw_ctx *hctx, int err; blk_status_t ret = BLK_STS_IOERR; - if (unlikely(dev->dev_state != DEV_STATE_MAPPED)) + if (dev->dev_state != DEV_STATE_MAPPED) return BLK_STS_IOERR; iu->permit = rnbd_get_permit(dev->sess, RTRS_IO_CON, RTRS_PERMIT_NOWAIT); - if (unlikely(!iu->permit)) { + if (!iu->permit) { rnbd_clt_dev_kick_mq_queue(dev, hctx, RNBD_DELAY_IFBUSY); return BLK_STS_RESOURCE; } @@ -1165,9 +1165,9 @@ static blk_status_t rnbd_queue_rq(struct blk_mq_hw_ctx *hctx, blk_mq_start_request(rq); err = rnbd_client_xfer_request(dev, rq, iu); - if (likely(err == 0)) + if (err == 0) return BLK_STS_OK; - if (unlikely(err == -EAGAIN || err == -ENOMEM)) { + if (err == -EAGAIN || err == -ENOMEM) { rnbd_clt_dev_kick_mq_queue(dev, hctx, 10/*ms*/); ret = BLK_STS_RESOURCE; } @@ -1584,7 +1584,7 @@ struct rnbd_clt_dev *rnbd_clt_map_device(const char *sessname, struct rnbd_clt_dev *dev; int ret; - if (unlikely(exists_devpath(pathname, sessname))) + if (exists_devpath(pathname, sessname)) return ERR_PTR(-EEXIST); sess = find_and_get_or_create_sess(sessname, paths, path_cnt, port_nr, nr_poll_queues); diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c index 899dd9d7c10b..aafecfe97055 100644 --- a/drivers/block/rnbd/rnbd-srv.c +++ b/drivers/block/rnbd/rnbd-srv.c @@ -104,7 +104,7 @@ rnbd_get_sess_dev(int dev_id, struct rnbd_srv_session *srv_sess) rcu_read_lock(); sess_dev = xa_load(&srv_sess->index_idr, dev_id); - if (likely(sess_dev)) + if (sess_dev) ret = kref_get_unless_zero(&sess_dev->kref); rcu_read_unlock(); -- cgit v1.2.3