[PATCH RFC 00/12] Misc changes for rtrs

[PATCH RFC 00/12] Misc changes for rtrs

[Guoqing Jiang]

Hi,

Here are some changes for rtrs, please review them.

Thanks,
Guoqing

Guoqing Jiang (12):
  RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx
  RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler
  RDMA/rtrs-srv: Only close srv_path if it is just allocated
  RDMA/rtrs-srv: refactor the handling of failure case in map_cont_bufs
  RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg
  RDMA/rtrs-clt: Correct the checking of ib_map_mr_sg
  RDMA/rtrs-srv: Remove outdated comments from create_con
  RDMA/rtrs: Kill recon_cnt from several structs
  RDMA/rtrs: Clean up rtrs_rdma_dev_pd_ops
  RDMA/rtrs-srv: Remove paths_num
  RDMA/rtrs-srv: fix several issues in rtrs_srv_destroy_path_files
  RDMA/rtrs-srv: Remove kobject_del from
    rtrs_srv_destroy_once_sysfs_root_folders

 drivers/infiniband/ulp/rtrs/rtrs-clt.c       |  12 +-
 drivers/infiniband/ulp/rtrs/rtrs-pri.h       |   6 -
 drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c |  13 ++-
 drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 110 ++++++-------------
 drivers/infiniband/ulp/rtrs/rtrs-srv.h       |   2 -
 drivers/infiniband/ulp/rtrs/rtrs.c           |  22 +---
 6 files changed, 47 insertions(+), 118 deletions(-)

-- 
2.31.1

[PATCH RFC 01/12] RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx

[Guoqing Jiang]

The ib_dev_count is supposed to track the number of added ib devices
which is only used in rtrs_srv_{add,remove}_one.

However we only trigger rtrs_srv_add_one from rnbd_srv_init_module
-> rtrs_srv_open -> ib_register_client -> client->add which should
happen only once. And so is rtrs_srv_close since it is only called
by unload rnbd-server or failure case when load rnbd-server.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 16 ----------------
 drivers/infiniband/ulp/rtrs/rtrs-srv.h |  1 -
 2 files changed, 17 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 22d7ba05e9fe..79504aaef0cc 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -2097,8 +2097,6 @@ static int rtrs_srv_add_one(struct ib_device *device)
 	int ret = 0;
 
 	mutex_lock(&ib_ctx.ib_dev_mutex);
-	if (ib_ctx.ib_dev_count)
-		goto out;
 
 	/*
 	 * Since our CM IDs are NOT bound to any ib device we will create them
@@ -2108,21 +2106,12 @@ static int rtrs_srv_add_one(struct ib_device *device)
 	ret = rtrs_srv_rdma_init(ctx, ib_ctx.port);
 	if (ret) {
 		/*
-		 * We errored out here.
 		 * According to the ib code, if we encounter an error here then the
 		 * error code is ignored, and no more calls to our ops are made.
 		 */
 		pr_err("Failed to initialize RDMA connection");
-		goto err_out;
 	}
 
-out:
-	/*
-	 * Keep a track on the number of ib devices added
-	 */
-	ib_ctx.ib_dev_count++;
-
-err_out:
 	mutex_unlock(&ib_ctx.ib_dev_mutex);
 	return ret;
 }
@@ -2132,10 +2121,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data)
 	struct rtrs_srv_ctx *ctx;
 
 	mutex_lock(&ib_ctx.ib_dev_mutex);
-	ib_ctx.ib_dev_count--;
-
-	if (ib_ctx.ib_dev_count)
-		goto out;
 
 	/*
 	 * Since our CM IDs are NOT bound to any ib device we will remove them
@@ -2145,7 +2130,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data)
 	rdma_destroy_id(ctx->cm_id_ip);
 	rdma_destroy_id(ctx->cm_id_ib);
 
-out:
 	mutex_unlock(&ib_ctx.ib_dev_mutex);
 }
 
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
index 2f8a638e36fa..eccc432b0715 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
@@ -126,7 +126,6 @@ struct rtrs_srv_ib_ctx {
 	struct rtrs_srv_ctx	*srv_ctx;
 	u16			port;
 	struct mutex            ib_dev_mutex;
-	int			ib_dev_count;
 };
 
 extern struct class *rtrs_dev_class;
-- 
2.31.1

[PATCH RFC 02/12] RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler

[Guoqing Jiang]

The RDMA_CM_EVENT_CONNECT_REQUEST is quite different to other types,
let's checking it separately at the beginning of routine, then we can
avoid the identation accordingly.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 79504aaef0cc..2cc8b423bcaa 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1948,24 +1948,19 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 static int rtrs_srv_rdma_cm_handler(struct rdma_cm_id *cm_id,
 				     struct rdma_cm_event *ev)
 {
-	struct rtrs_srv_path *srv_path = NULL;
-	struct rtrs_path *s = NULL;
-
-	if (ev->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
-		struct rtrs_con *c = cm_id->context;
-
-		s = c->path;
-		srv_path = to_srv_path(s);
-	}
+	struct rtrs_con *c = cm_id->context;
+	struct rtrs_path *s = c->path;
+	struct rtrs_srv_path *srv_path = to_srv_path(s);
 
-	switch (ev->event) {
-	case RDMA_CM_EVENT_CONNECT_REQUEST:
+	if (ev->event == RDMA_CM_EVENT_CONNECT_REQUEST)
 		/*
 		 * In case of error cma.c will destroy cm_id,
 		 * see cma_process_remove()
 		 */
 		return rtrs_rdma_connect(cm_id, ev->param.conn.private_data,
 					  ev->param.conn.private_data_len);
+
+	switch (ev->event) {
 	case RDMA_CM_EVENT_ESTABLISHED:
 		/* Nothing here */
 		break;
-- 
2.31.1

[PATCH RFC 03/12] RDMA/rtrs-srv: Only close srv_path if it is just allocated

[Guoqing Jiang]

RTRS creates several connections per nr_cpu_ids, it makes more sense
to only close the path when it just allocated.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 2cc8b423bcaa..063082d29fc6 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1833,6 +1833,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 	u16 version, con_num, cid;
 	u16 recon_cnt;
 	int err = -ECONNRESET;
+	bool alloc_path = false;
 
 	if (len < sizeof(*msg)) {
 		pr_err("Invalid RTRS connection request\n");
@@ -1906,6 +1907,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 			pr_err("RTRS server session allocation failed: %d\n", err);
 			goto reject_w_err;
 		}
+		alloc_path = true;
 	}
 	err = create_con(srv_path, cm_id, cid);
 	if (err) {
@@ -1940,7 +1942,8 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 
 close_and_return_err:
 	mutex_unlock(&srv->paths_mutex);
-	close_path(srv_path);
+	if (alloc_path)
+		close_path(srv_path);
 
 	return err;
 }
-- 
2.31.1

[PATCH RFC 04/12] RDMA/rtrs-srv: refactor the handling of failure case in map_cont_bufs

[Guoqing Jiang]

Let's call unmap_cont_bufs when failure happens, and also only update
mrs_num after everything is settled which means we can remove 'mri'.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 47 +++++++++++---------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 063082d29fc6..88eae0dcf87f 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -561,9 +561,11 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
 {
 	struct rtrs_srv_sess *srv = srv_path->srv;
 	struct rtrs_path *ss = &srv_path->s;
-	int i, mri, err, mrs_num;
+	int i, err, mrs_num;
 	unsigned int chunk_bits;
 	int chunks_per_mr = 1;
+	struct ib_mr *mr;
+	struct sg_table *sgt;
 
 	/*
 	 * Here we map queue_depth chunks to MR.  Firstly we have to
@@ -586,16 +588,14 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
 	if (!srv_path->mrs)
 		return -ENOMEM;
 
-	srv_path->mrs_num = mrs_num;
-
-	for (mri = 0; mri < mrs_num; mri++) {
-		struct rtrs_srv_mr *srv_mr = &srv_path->mrs[mri];
-		struct sg_table *sgt = &srv_mr->sgt;
+	for (srv_path->mrs_num = 0; srv_path->mrs_num < mrs_num;
+	     srv_path->mrs_num++) {
+		struct rtrs_srv_mr *srv_mr = &srv_path->mrs[srv_path->mrs_num];
 		struct scatterlist *s;
-		struct ib_mr *mr;
 		int nr, nr_sgt, chunks;
 
-		chunks = chunks_per_mr * mri;
+		sgt = &srv_mr->sgt;
+		chunks = chunks_per_mr * srv_path->mrs_num;
 		if (!always_invalidate)
 			chunks_per_mr = min_t(int, chunks_per_mr,
 					      srv->queue_depth - chunks);
@@ -644,31 +644,24 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
 
 		ib_update_fast_reg_key(mr, ib_inc_rkey(mr->rkey));
 		srv_mr->mr = mr;
-
-		continue;
-err:
-		while (mri--) {
-			srv_mr = &srv_path->mrs[mri];
-			sgt = &srv_mr->sgt;
-			mr = srv_mr->mr;
-			rtrs_iu_free(srv_mr->iu, srv_path->s.dev->ib_dev, 1);
-dereg_mr:
-			ib_dereg_mr(mr);
-unmap_sg:
-			ib_dma_unmap_sg(srv_path->s.dev->ib_dev, sgt->sgl,
-					sgt->nents, DMA_BIDIRECTIONAL);
-free_sg:
-			sg_free_table(sgt);
-		}
-		kfree(srv_path->mrs);
-
-		return err;
 	}
 
 	chunk_bits = ilog2(srv->queue_depth - 1) + 1;
 	srv_path->mem_bits = (MAX_IMM_PAYL_BITS - chunk_bits);
 
 	return 0;
+
+dereg_mr:
+	ib_dereg_mr(mr);
+unmap_sg:
+	ib_dma_unmap_sg(srv_path->s.dev->ib_dev, sgt->sgl,
+			sgt->nents, DMA_BIDIRECTIONAL);
+free_sg:
+	sg_free_table(sgt);
+err:
+	unmap_cont_bufs(srv_path);
+
+	return err;
 }
 
 static void rtrs_srv_hb_err_handler(struct rtrs_con *c)
-- 
2.31.1

[PATCH RFC 05/12] RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg

[Guoqing Jiang]

We should check with nr_sgt, also the only successful case is that
all sg elements are mapped, so make it explict.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 88eae0dcf87f..f3bf5bbb4377 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
 		}
 		nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt,
 				  NULL, max_chunk_size);
-		if (nr < 0 || nr < sgt->nents) {
-			err = nr < 0 ? nr : -EINVAL;
+		if (nr != nr_sgt) {
+			err = -EINVAL;
 			goto dereg_mr;
 		}
 
-- 
2.31.1

[PATCH RFC 06/12] RDMA/rtrs-clt: Correct the checking of ib_map_mr_sg

[Guoqing Jiang]

We should check with count, also the only successful case is that
all sg elements are mapped, so make it explict.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 8546b8816524..5ffc170dae8c 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1064,9 +1064,7 @@ static int rtrs_map_sg_fr(struct rtrs_clt_io_req *req, size_t count)
 
 	/* Align the MR to a 4K page size to match the block virt boundary */
 	nr = ib_map_mr_sg(req->mr, req->sglist, count, NULL, SZ_4K);
-	if (nr < 0)
-		return nr;
-	if (nr < req->sg_cnt)
+	if (nr != count)
 		return -EINVAL;
 	ib_update_fast_reg_key(req->mr, ib_inc_rkey(req->mr->rkey));
 
-- 
2.31.1

[PATCH RFC 07/12] RDMA/rtrs-srv: Remove outdated comments from create_con

[Guoqing Jiang]

Remove the orphan comments.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index f3bf5bbb4377..4c883c57c2ef 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1671,12 +1671,6 @@ static int create_con(struct rtrs_srv_path *srv_path,
 				      srv->queue_depth * (1 + 2) + 1);
 
 		max_recv_wr = srv->queue_depth + 1;
-		/*
-		 * If we have all receive requests posted and
-		 * all write requests posted and each read request
-		 * requires an invalidate request + drain
-		 * and qp gets into error state.
-		 */
 	}
 	cq_num = max_send_wr + max_recv_wr;
 	atomic_set(&con->c.sq_wr_avail, max_send_wr);
-- 
2.31.1

[PATCH RFC 08/12] RDMA/rtrs: Kill recon_cnt from several structs

[Guoqing Jiang]

Seems the only relevant comment about recon_cnt is,

/*
 * On every new session connections increase reconnect counter
 * to avoid clashes with previous sessions not yet closed
 * sessions on a server side.
 */

However, it is not clear how the recon_cnt avoid clashed at these places
in the commit since no where checks it.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 8 --------
 drivers/infiniband/ulp/rtrs/rtrs-pri.h | 3 ---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 7 +------
 3 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 5ffc170dae8c..dcc8c041a141 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1802,7 +1802,6 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con)
 		.version = cpu_to_le16(RTRS_PROTO_VER),
 		.cid = cpu_to_le16(con->c.cid),
 		.cid_num = cpu_to_le16(clt_path->s.con_num),
-		.recon_cnt = cpu_to_le16(clt_path->s.recon_cnt),
 	};
 	msg.first_conn = clt_path->for_new_clt ? FIRST_CONN : 0;
 	uuid_copy(&msg.sess_uuid, &clt_path->s.uuid);
@@ -2336,13 +2335,6 @@ static int init_conns(struct rtrs_clt_path *clt_path)
 	unsigned int cid;
 	int err;
 
-	/*
-	 * On every new session connections increase reconnect counter
-	 * to avoid clashes with previous sessions not yet closed
-	 * sessions on a server side.
-	 */
-	clt_path->s.recon_cnt++;
-
 	/* Establish all RDMA connections  */
 	for (cid = 0; cid < clt_path->s.con_num; cid++) {
 		err = create_con(clt_path, cid);
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
index a2420eecaf5a..c4ddaeba1c59 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
@@ -109,7 +109,6 @@ struct rtrs_path {
 	struct rtrs_con	**con;
 	unsigned int		con_num;
 	unsigned int		irq_con_num;
-	unsigned int		recon_cnt;
 	unsigned int		signal_interval;
 	struct rtrs_ib_dev	*dev;
 	int			dev_ref;
@@ -177,7 +176,6 @@ struct rtrs_sg_desc {
  * @version:	   RTRS protocol version
  * @cid:	   Current connection id
  * @cid_num:	   Number of connections per session
- * @recon_cnt:	   Reconnections counter
  * @sess_uuid:	   UUID of a session (path)
  * @paths_uuid:	   UUID of a group of sessions (paths)
  *
@@ -196,7 +194,6 @@ struct rtrs_msg_conn_req {
 	__le16		version;
 	__le16		cid;
 	__le16		cid_num;
-	__le16		recon_cnt;
 	uuid_t		sess_uuid;
 	uuid_t		paths_uuid;
 	u8		first_conn : 1;
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 4c883c57c2ef..e2ea09a8def7 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1712,7 +1712,6 @@ static int create_con(struct rtrs_srv_path *srv_path,
 static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
 					   struct rdma_cm_id *cm_id,
 					   unsigned int con_num,
-					   unsigned int recon_cnt,
 					   const uuid_t *uuid)
 {
 	struct rtrs_srv_path *srv_path;
@@ -1768,7 +1767,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
 
 	srv_path->s.con_num = con_num;
 	srv_path->s.irq_con_num = con_num;
-	srv_path->s.recon_cnt = recon_cnt;
 	uuid_copy(&srv_path->s.uuid, uuid);
 	spin_lock_init(&srv_path->state_lock);
 	INIT_WORK(&srv_path->close_work, rtrs_srv_close_work);
@@ -1818,7 +1816,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 	struct rtrs_srv_sess *srv;
 
 	u16 version, con_num, cid;
-	u16 recon_cnt;
 	int err = -ECONNRESET;
 	bool alloc_path = false;
 
@@ -1848,7 +1845,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 		pr_err("Incorrect cid: %d >= %d\n", cid, con_num);
 		goto reject_w_err;
 	}
-	recon_cnt = le16_to_cpu(msg->recon_cnt);
 	srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
 	if (IS_ERR(srv)) {
 		err = PTR_ERR(srv);
@@ -1885,8 +1881,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 			goto reject_w_err;
 		}
 	} else {
-		srv_path = __alloc_path(srv, cm_id, con_num, recon_cnt,
-				    &msg->sess_uuid);
+		srv_path = __alloc_path(srv, cm_id, con_num, &msg->sess_uuid);
 		if (IS_ERR(srv_path)) {
 			mutex_unlock(&srv->paths_mutex);
 			put_srv(srv);
-- 
2.31.1

[PATCH RFC 09/12] RDMA/rtrs: Clean up rtrs_rdma_dev_pd_ops

[Guoqing Jiang]

Let's remove them since the three members are not used.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-pri.h |  3 ---
 drivers/infiniband/ulp/rtrs/rtrs.c     | 22 ++++------------------
 2 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
index c4ddaeba1c59..4d15a6fd96b6 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
@@ -68,10 +68,7 @@ enum {
 struct rtrs_ib_dev;
 
 struct rtrs_rdma_dev_pd_ops {
-	struct rtrs_ib_dev *(*alloc)(void);
-	void (*free)(struct rtrs_ib_dev *dev);
 	int (*init)(struct rtrs_ib_dev *dev);
-	void (*deinit)(struct rtrs_ib_dev *dev);
 };
 
 struct rtrs_rdma_dev_pd {
diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
index ed324b47d93a..4bf9d868cc52 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs.c
@@ -557,7 +557,6 @@ EXPORT_SYMBOL(rtrs_addr_to_sockaddr);
 void rtrs_rdma_dev_pd_init(enum ib_pd_flags pd_flags,
 			    struct rtrs_rdma_dev_pd *pool)
 {
-	WARN_ON(pool->ops && (!pool->ops->alloc ^ !pool->ops->free));
 	INIT_LIST_HEAD(&pool->list);
 	mutex_init(&pool->mutex);
 	pool->pd_flags = pd_flags;
@@ -583,15 +582,8 @@ static void dev_free(struct kref *ref)
 	list_del(&dev->entry);
 	mutex_unlock(&pool->mutex);
 
-	if (pool->ops && pool->ops->deinit)
-		pool->ops->deinit(dev);
-
 	ib_dealloc_pd(dev->ib_pd);
-
-	if (pool->ops && pool->ops->free)
-		pool->ops->free(dev);
-	else
-		kfree(dev);
+	kfree(dev);
 }
 
 int rtrs_ib_dev_put(struct rtrs_ib_dev *dev)
@@ -618,11 +610,8 @@ rtrs_ib_dev_find_or_add(struct ib_device *ib_dev,
 			goto out_unlock;
 	}
 	mutex_unlock(&pool->mutex);
-	if (pool->ops && pool->ops->alloc)
-		dev = pool->ops->alloc();
-	else
-		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (IS_ERR_OR_NULL(dev))
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
 		goto out_err;
 
 	kref_init(&dev->ref);
@@ -644,10 +633,7 @@ rtrs_ib_dev_find_or_add(struct ib_device *ib_dev,
 out_free_pd:
 	ib_dealloc_pd(dev->ib_pd);
 out_free_dev:
-	if (pool->ops && pool->ops->free)
-		pool->ops->free(dev);
-	else
-		kfree(dev);
+	kfree(dev);
 out_err:
 	return NULL;
 }
-- 
2.31.1

[PATCH RFC 10/12] RDMA/rtrs-srv: Remove paths_num

[Guoqing Jiang]

The paths_num is only increased by rtrs_rdma_connect -> __alloc_path
which is only one time thing, so is the decreasing of it given only
rtrs_srv_close_work -> del_path_from_srv, which means paths_num should
always be 1.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 --------
 drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 -
 2 files changed, 9 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index e2ea09a8def7..400cf8ae34a3 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1437,8 +1437,6 @@ static void __add_path_to_srv(struct rtrs_srv_sess *srv,
 			      struct rtrs_srv_path *srv_path)
 {
 	list_add_tail(&srv_path->s.entry, &srv->paths_list);
-	srv->paths_num++;
-	WARN_ON(srv->paths_num >= MAX_PATHS_NUM);
 }
 
 static void del_path_from_srv(struct rtrs_srv_path *srv_path)
@@ -1450,8 +1448,6 @@ static void del_path_from_srv(struct rtrs_srv_path *srv_path)
 
 	mutex_lock(&srv->paths_mutex);
 	list_del(&srv_path->s.entry);
-	WARN_ON(!srv->paths_num);
-	srv->paths_num--;
 	mutex_unlock(&srv->paths_mutex);
 }
 
@@ -1719,10 +1715,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
 	char str[NAME_MAX];
 	struct rtrs_addr path;
 
-	if (srv->paths_num >= MAX_PATHS_NUM) {
-		err = -ECONNRESET;
-		goto err;
-	}
 	if (__is_path_w_addr_exists(srv, &cm_id->route.addr)) {
 		err = -EEXIST;
 		pr_err("Path with same addr exists\n");
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
index eccc432b0715..8e4fcb578f49 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
@@ -100,7 +100,6 @@ struct rtrs_srv_sess {
 	struct list_head	paths_list;
 	int			paths_up;
 	struct mutex		paths_ev_mutex;
-	size_t			paths_num;
 	struct mutex		paths_mutex;
 	uuid_t			paths_uuid;
 	refcount_t		refcount;
-- 
2.31.1

[PATCH RFC 11/12] RDMA/rtrs-srv: fix several issues in rtrs_srv_destroy_path_files

[Guoqing Jiang]

There are several issues in the function which is supposed to be paired
with rtrs_srv_create_path_files.

1. rtrs_srv_stats_attr_group is not removed though it is created in
   rtrs_srv_create_stats_files.

2. it makes more sense to check kobj_stats.state_in_sysfs before destroy
   kobj_stats instead of rely on kobj.state_in_sysfs.

3. kobject_init_and_add is used for both kobjs (srv_path->kobj and
   srv_path->stats->kobj_stats), however we missed to call kobject_del
   for srv_path->kobj which was called in free_path.

4. rtrs_srv_destroy_once_sysfs_root_folders is independant of either
   kobj or kobj_stats.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
index 2a3c9ac64a42..da8e205ce331 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
@@ -304,12 +304,18 @@ int rtrs_srv_create_path_files(struct rtrs_srv_path *srv_path)
 
 void rtrs_srv_destroy_path_files(struct rtrs_srv_path *srv_path)
 {
-	if (srv_path->kobj.state_in_sysfs) {
+	if (srv_path->stats->kobj_stats.state_in_sysfs) {
+		sysfs_remove_group(&srv_path->stats->kobj_stats,
+				   &rtrs_srv_stats_attr_group);
 		kobject_del(&srv_path->stats->kobj_stats);
 		kobject_put(&srv_path->stats->kobj_stats);
+	}
+
+	if (srv_path->kobj.state_in_sysfs) {
 		sysfs_remove_group(&srv_path->kobj, &rtrs_srv_path_attr_group);
+		kobject_del(&srv_path->kobj);
 		kobject_put(&srv_path->kobj);
-
-		rtrs_srv_destroy_once_sysfs_root_folders(srv_path);
 	}
+
+	rtrs_srv_destroy_once_sysfs_root_folders(srv_path);
 }
-- 
2.31.1

[PATCH RFC 12/12] RDMA/rtrs-srv: Remove kobject_del from rtrs_srv_destroy_once_sysfs_root_folders

[Guoqing Jiang]

The kobj_paths which is created dynamically by kobject_create_and_add,
and per the comment above kobject_create_and_add, we only need to call
kobject_put which is not same as other kobjs such as stats->kobj_stats
and srv_path->kobj.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
index da8e205ce331..c76ba29da1e2 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
@@ -203,7 +203,6 @@ rtrs_srv_destroy_once_sysfs_root_folders(struct rtrs_srv_path *srv_path)
 
 	mutex_lock(&srv->paths_mutex);
 	if (!--srv->dev_ref) {
-		kobject_del(srv->kobj_paths);
 		kobject_put(srv->kobj_paths);
 		mutex_unlock(&srv->paths_mutex);
 		device_del(&srv->dev);
-- 
2.31.1

Re: [PATCH RFC 01/12] RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx

[Jinpu Wang]

Hi Guoqing,

Thx for the patch, see comments below.
On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> The ib_dev_count is supposed to track the number of added ib devices
> which is only used in rtrs_srv_{add,remove}_one.
>
> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module
> -> rtrs_srv_open -> ib_register_client -> client->add which should
> happen only once.
client->add is call per ib_device, eg:
jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_*
lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_0 ->
../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0
lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_1 ->
../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1
rtrs will be call twice for  mlx5_0 and mlx5_1 devices

So this one is NACK

And so is rtrs_srv_close since it is only called
> by unload rnbd-server or failure case when load rnbd-server.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 16 ----------------
>  drivers/infiniband/ulp/rtrs/rtrs-srv.h |  1 -
>  2 files changed, 17 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 22d7ba05e9fe..79504aaef0cc 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -2097,8 +2097,6 @@ static int rtrs_srv_add_one(struct ib_device *device)
>         int ret = 0;
>
>         mutex_lock(&ib_ctx.ib_dev_mutex);
> -       if (ib_ctx.ib_dev_count)
> -               goto out;
>
>         /*
>          * Since our CM IDs are NOT bound to any ib device we will create them
> @@ -2108,21 +2106,12 @@ static int rtrs_srv_add_one(struct ib_device *device)
>         ret = rtrs_srv_rdma_init(ctx, ib_ctx.port);
>         if (ret) {
>                 /*
> -                * We errored out here.
>                  * According to the ib code, if we encounter an error here then the
>                  * error code is ignored, and no more calls to our ops are made.
>                  */
>                 pr_err("Failed to initialize RDMA connection");
> -               goto err_out;
>         }
>
> -out:
> -       /*
> -        * Keep a track on the number of ib devices added
> -        */
> -       ib_ctx.ib_dev_count++;
> -
> -err_out:
>         mutex_unlock(&ib_ctx.ib_dev_mutex);
>         return ret;
>  }
> @@ -2132,10 +2121,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data)
>         struct rtrs_srv_ctx *ctx;
>
>         mutex_lock(&ib_ctx.ib_dev_mutex);
> -       ib_ctx.ib_dev_count--;
> -
> -       if (ib_ctx.ib_dev_count)
> -               goto out;
>
>         /*
>          * Since our CM IDs are NOT bound to any ib device we will remove them
> @@ -2145,7 +2130,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data)
>         rdma_destroy_id(ctx->cm_id_ip);
>         rdma_destroy_id(ctx->cm_id_ib);
>
> -out:
>         mutex_unlock(&ib_ctx.ib_dev_mutex);
>  }
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> index 2f8a638e36fa..eccc432b0715 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> @@ -126,7 +126,6 @@ struct rtrs_srv_ib_ctx {
>         struct rtrs_srv_ctx     *srv_ctx;
>         u16                     port;
>         struct mutex            ib_dev_mutex;
> -       int                     ib_dev_count;
>  };
>
>  extern struct class *rtrs_dev_class;
> --
> 2.31.1
>

Re: [PATCH RFC 01/12] RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx

[Guoqing Jiang]

Hi Jinpu,

On 11/14/22 3:39 PM, Jinpu Wang wrote:
> Hi Guoqing,
>
> Thx for the patch, see comments below.
> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> The ib_dev_count is supposed to track the number of added ib devices
>> which is only used in rtrs_srv_{add,remove}_one.
>>
>> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module
>> -> rtrs_srv_open -> ib_register_client -> client->add which should
>> happen only once.
> client->add is call per ib_device, eg:
> jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_*
> lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_0 ->
> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0
> lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_1 ->
> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1
> rtrs will be call twice for  mlx5_0 and mlx5_1 devices

Ah, yes.

But still we can only load/unload module once, I guess it was used to avoid
racy condition (concurrent loading/unloading module?), could you elaborate
why it is needed?

Thanks,
Guoqing

Re: [PATCH RFC 01/12] RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx

[Jinpu Wang]

On Mon, Nov 14, 2022 at 9:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Hi Jinpu,
>
> On 11/14/22 3:39 PM, Jinpu Wang wrote:
> > Hi Guoqing,
> >
> > Thx for the patch, see comments below.
> > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >> The ib_dev_count is supposed to track the number of added ib devices
> >> which is only used in rtrs_srv_{add,remove}_one.
> >>
> >> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module
> >> -> rtrs_srv_open -> ib_register_client -> client->add which should
> >> happen only once.
> > client->add is call per ib_device, eg:
> > jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_*
> > lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_0 ->
> > ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0
> > lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_1 ->
> > ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1
> > rtrs will be call twice for  mlx5_0 and mlx5_1 devices
>
> Ah, yes.
>
> But still we can only load/unload module once, I guess it was used to avoid
> racy condition (concurrent loading/unloading module?), could you elaborate
> why it is needed?
The change was introduced due to  a bug report, you can follow the
discussion here for the history and reason:
https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/

>
> Thanks,
> Guoqing
Thx!

Re: [PATCH RFC 00/12] Misc changes for rtrs

[Leon Romanovsky]

On Sun, Nov 13, 2022 at 09:08:11AM +0800, Guoqing Jiang wrote:
> Hi,
> 
> Here are some changes for rtrs, please review them.
> 
> Thanks,
> Guoqing
> 
> Guoqing Jiang (12):
>   RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx
>   RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler
>   RDMA/rtrs-srv: Only close srv_path if it is just allocated
>   RDMA/rtrs-srv: refactor the handling of failure case in map_cont_bufs
>   RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg
>   RDMA/rtrs-clt: Correct the checking of ib_map_mr_sg
>   RDMA/rtrs-srv: Remove outdated comments from create_con
>   RDMA/rtrs: Kill recon_cnt from several structs
>   RDMA/rtrs: Clean up rtrs_rdma_dev_pd_ops
>   RDMA/rtrs-srv: Remove paths_num
>   RDMA/rtrs-srv: fix several issues in rtrs_srv_destroy_path_files
>   RDMA/rtrs-srv: Remove kobject_del from
>     rtrs_srv_destroy_once_sysfs_root_folders
> 
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c       |  12 +-
>  drivers/infiniband/ulp/rtrs/rtrs-pri.h       |   6 -
>  drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c |  13 ++-
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 110 ++++++-------------
>  drivers/infiniband/ulp/rtrs/rtrs-srv.h       |   2 -
>  drivers/infiniband/ulp/rtrs/rtrs.c           |  22 +---
>  6 files changed, 47 insertions(+), 118 deletions(-)

Why is this series marked as RFC?

Thanks

> 
> -- 
> 2.31.1
> 

Re: [PATCH RFC 01/12] RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx

[Guoqing Jiang]

On 11/14/22 4:24 PM, Jinpu Wang wrote:
> On Mon, Nov 14, 2022 at 9:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> Hi Jinpu,
>>
>> On 11/14/22 3:39 PM, Jinpu Wang wrote:
>>> Hi Guoqing,
>>>
>>> Thx for the patch, see comments below.
>>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>>>> The ib_dev_count is supposed to track the number of added ib devices
>>>> which is only used in rtrs_srv_{add,remove}_one.
>>>>
>>>> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module
>>>> -> rtrs_srv_open -> ib_register_client -> client->add which should
>>>> happen only once.
>>> client->add is call per ib_device, eg:
>>> jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_*
>>> lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_0 ->
>>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0
>>> lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_1 ->
>>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1
>>> rtrs will be call twice for  mlx5_0 and mlx5_1 devices
>> Ah, yes.
>>
>> But still we can only load/unload module once, I guess it was used to avoid
>> racy condition (concurrent loading/unloading module?), could you elaborate
>> why it is needed?
> The change was introduced due to  a bug report, you can follow the
> discussion here for the history and reason:
> https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/

Thanks for the link.

I probably missed something but I don't know how rnbd_server module can be
initialized before cma module since we have the dependency chain as 
follows.

INFINIBAND_RTRS_SERVER depends on INFINIBAND_ADDR_TRANS
BLK_DEV_RNBD_SERVER depends on INFINIBAND_RTRS_SERVER

But commit 558d52b2976b ("RDMA/rtrs-srv: Incorporate ib_register_client into
rtrs server init") did mention this.

"and if the rnbd_server module is initialized before RDMA cma module, a 
null ptr
dereference occurs during the RDMA bind operation."

Thanks,
Guoqing

Re: [PATCH RFC 00/12] Misc changes for rtrs

[Guoqing Jiang]

On 11/14/22 4:32 PM, Leon Romanovsky wrote:
> On Sun, Nov 13, 2022 at 09:08:11AM +0800, Guoqing Jiang wrote:
>> Hi,
>>
>> Here are some changes for rtrs, please review them.
>>
>> Thanks,
>> Guoqing
>>
>> Guoqing Jiang (12):
>>    RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx
>>    RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler
>>    RDMA/rtrs-srv: Only close srv_path if it is just allocated
>>    RDMA/rtrs-srv: refactor the handling of failure case in map_cont_bufs
>>    RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg
>>    RDMA/rtrs-clt: Correct the checking of ib_map_mr_sg
>>    RDMA/rtrs-srv: Remove outdated comments from create_con
>>    RDMA/rtrs: Kill recon_cnt from several structs
>>    RDMA/rtrs: Clean up rtrs_rdma_dev_pd_ops
>>    RDMA/rtrs-srv: Remove paths_num
>>    RDMA/rtrs-srv: fix several issues in rtrs_srv_destroy_path_files
>>    RDMA/rtrs-srv: Remove kobject_del from
>>      rtrs_srv_destroy_once_sysfs_root_folders
>>
>>   drivers/infiniband/ulp/rtrs/rtrs-clt.c       |  12 +-
>>   drivers/infiniband/ulp/rtrs/rtrs-pri.h       |   6 -
>>   drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c |  13 ++-
>>   drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 110 ++++++-------------
>>   drivers/infiniband/ulp/rtrs/rtrs-srv.h       |   2 -
>>   drivers/infiniband/ulp/rtrs/rtrs.c           |  22 +---
>>   6 files changed, 47 insertions(+), 118 deletions(-)
> Why is this series marked as RFC?

Because I am not quite sure about some of them, and there is no rush for
the series.

Thanks,
Guoqing

Re: [PATCH RFC 01/12] RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx

[Haris Iqbal]

On Mon, Nov 14, 2022 at 9:45 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 11/14/22 4:24 PM, Jinpu Wang wrote:
> > On Mon, Nov 14, 2022 at 9:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >> Hi Jinpu,
> >>
> >> On 11/14/22 3:39 PM, Jinpu Wang wrote:
> >>> Hi Guoqing,
> >>>
> >>> Thx for the patch, see comments below.
> >>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >>>> The ib_dev_count is supposed to track the number of added ib devices
> >>>> which is only used in rtrs_srv_{add,remove}_one.
> >>>>
> >>>> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module
> >>>> -> rtrs_srv_open -> ib_register_client -> client->add which should
> >>>> happen only once.
> >>> client->add is call per ib_device, eg:
> >>> jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_*
> >>> lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_0 ->
> >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0
> >>> lrwxrwxrwx 1 root root 0 Nov  8 13:49 /sys/class/infiniband/mlx5_1 ->
> >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1
> >>> rtrs will be call twice for  mlx5_0 and mlx5_1 devices
> >> Ah, yes.
> >>
> >> But still we can only load/unload module once, I guess it was used to avoid
> >> racy condition (concurrent loading/unloading module?), could you elaborate
> >> why it is needed?
> > The change was introduced due to  a bug report, you can follow the
> > discussion here for the history and reason:
> > https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/
>
> Thanks for the link.
>
> I probably missed something but I don't know how rnbd_server module can be
> initialized before cma module since we have the dependency chain as
> follows.

Hi Guoqing,

One of the ways this was happening was, when one builds the RNBD/RTRS
module into the kernel bzImage and then boots up the kernel. Depending
on the module init sequence, if the RNBD/RTRS modules are picked
before the RDMA one, then this issue would hit.

With the changes, RNBD/RTRS just register itself to ib. If any devices
are present at that moment, for example when the module is modprob'ed
later into the kernel, then .add gets called through
ib_register_client. If no devices are present, for example in case if
the module is built into the bzImage, and is inserted before RDMA
module, then ib_register_client simply registers RTRS, and returns
without calling .add
Now, when RDMA gets initialized, and it detects devices, then .add is
called for each device, which will land into rtrs_srv_add_one

>
> INFINIBAND_RTRS_SERVER depends on INFINIBAND_ADDR_TRANS
> BLK_DEV_RNBD_SERVER depends on INFINIBAND_RTRS_SERVER
>
> But commit 558d52b2976b ("RDMA/rtrs-srv: Incorporate ib_register_client into
> rtrs server init") did mention this.
>
> "and if the rnbd_server module is initialized before RDMA cma module, a
> null ptr
> dereference occurs during the RDMA bind operation."
>
> Thanks,
> Guoqing

Re: [PATCH RFC 02/12] RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> The RDMA_CM_EVENT_CONNECT_REQUEST is quite different to other types,
> let's checking it separately at the beginning of routine, then we can
> avoid the identation accordingly.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 79504aaef0cc..2cc8b423bcaa 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -1948,24 +1948,19 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>  static int rtrs_srv_rdma_cm_handler(struct rdma_cm_id *cm_id,
>                                      struct rdma_cm_event *ev)
>  {
> -       struct rtrs_srv_path *srv_path = NULL;
> -       struct rtrs_path *s = NULL;
> -
> -       if (ev->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
> -               struct rtrs_con *c = cm_id->context;
> -
> -               s = c->path;
> -               srv_path = to_srv_path(s);
> -       }
> +       struct rtrs_con *c = cm_id->context;
> +       struct rtrs_path *s = c->path;
> +       struct rtrs_srv_path *srv_path = to_srv_path(s);

This isn't correct for the RDMA_CM_EVENT_CONNECT_REQUEST event. At
that moment, cm_id->context is still holding a pointer to struct
rtrs_srv_ctx. Even though it never gets used for this event here, its
not right IMO to dereference in this incorrect manner.

How about we move the check for the RDMA_CM_EVENT_CONNECT_REQUEST
event outside (just like you did), but let the pointers be NULL, and
only dereference after the if condition for
RDMA_CM_EVENT_CONNECT_REQUEST event?

>
> -       switch (ev->event) {
> -       case RDMA_CM_EVENT_CONNECT_REQUEST:
> +       if (ev->event == RDMA_CM_EVENT_CONNECT_REQUEST)
>                 /*
>                  * In case of error cma.c will destroy cm_id,
>                  * see cma_process_remove()
>                  */
>                 return rtrs_rdma_connect(cm_id, ev->param.conn.private_data,
>                                           ev->param.conn.private_data_len);
> +
> +       switch (ev->event) {
>         case RDMA_CM_EVENT_ESTABLISHED:
>                 /* Nothing here */
>                 break;
> --
> 2.31.1
>

Re: [PATCH RFC 03/12] RDMA/rtrs-srv: Only close srv_path if it is just allocated

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> RTRS creates several connections per nr_cpu_ids, it makes more sense
> to only close the path when it just allocated.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 2cc8b423bcaa..063082d29fc6 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -1833,6 +1833,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>         u16 version, con_num, cid;
>         u16 recon_cnt;
>         int err = -ECONNRESET;
> +       bool alloc_path = false;
>
>         if (len < sizeof(*msg)) {
>                 pr_err("Invalid RTRS connection request\n");
> @@ -1906,6 +1907,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>                         pr_err("RTRS server session allocation failed: %d\n", err);
>                         goto reject_w_err;
>                 }
> +               alloc_path = true;
>         }
>         err = create_con(srv_path, cm_id, cid);
>         if (err) {
> @@ -1940,7 +1942,8 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>
>  close_and_return_err:
>         mutex_unlock(&srv->paths_mutex);
> -       close_path(srv_path);
> +       if (alloc_path)
> +               close_path(srv_path);

I think the way this is coded is, if there is a problem opening a
connection in that srv_path, then it closes that srv_path gracefully,
which results in all other connections in that path getting closed,
and the IOs being failover'ed to the other path (in case of
multipath), and then the client would trigger an auto reconnect.

@Jinpu can shed some more light, what would be the preferred course of
action if there is a failure in one connection. To keep the current
design or to avoid closing the path in case of a connection issue.

>
>         return err;
>  }
> --
> 2.31.1
>

Re: [PATCH RFC 04/12] RDMA/rtrs-srv: refactor the handling of failure case in map_cont_bufs

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Let's call unmap_cont_bufs when failure happens, and also only update
> mrs_num after everything is settled which means we can remove 'mri'.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
Acked-by: Md Haris Iqbal <haris.iqbal@ionos.com>

Thanks for this. It looks much better now without the weird while loop
and the gotos

> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 47 +++++++++++---------------
>  1 file changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 063082d29fc6..88eae0dcf87f 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -561,9 +561,11 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
>  {
>         struct rtrs_srv_sess *srv = srv_path->srv;
>         struct rtrs_path *ss = &srv_path->s;
> -       int i, mri, err, mrs_num;
> +       int i, err, mrs_num;
>         unsigned int chunk_bits;
>         int chunks_per_mr = 1;
> +       struct ib_mr *mr;
> +       struct sg_table *sgt;
>
>         /*
>          * Here we map queue_depth chunks to MR.  Firstly we have to
> @@ -586,16 +588,14 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
>         if (!srv_path->mrs)
>                 return -ENOMEM;
>
> -       srv_path->mrs_num = mrs_num;
> -
> -       for (mri = 0; mri < mrs_num; mri++) {
> -               struct rtrs_srv_mr *srv_mr = &srv_path->mrs[mri];
> -               struct sg_table *sgt = &srv_mr->sgt;
> +       for (srv_path->mrs_num = 0; srv_path->mrs_num < mrs_num;
> +            srv_path->mrs_num++) {
> +               struct rtrs_srv_mr *srv_mr = &srv_path->mrs[srv_path->mrs_num];
>                 struct scatterlist *s;
> -               struct ib_mr *mr;
>                 int nr, nr_sgt, chunks;
>
> -               chunks = chunks_per_mr * mri;
> +               sgt = &srv_mr->sgt;
> +               chunks = chunks_per_mr * srv_path->mrs_num;
>                 if (!always_invalidate)
>                         chunks_per_mr = min_t(int, chunks_per_mr,
>                                               srv->queue_depth - chunks);
> @@ -644,31 +644,24 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
>
>                 ib_update_fast_reg_key(mr, ib_inc_rkey(mr->rkey));
>                 srv_mr->mr = mr;
> -
> -               continue;
> -err:
> -               while (mri--) {
> -                       srv_mr = &srv_path->mrs[mri];
> -                       sgt = &srv_mr->sgt;
> -                       mr = srv_mr->mr;
> -                       rtrs_iu_free(srv_mr->iu, srv_path->s.dev->ib_dev, 1);
> -dereg_mr:
> -                       ib_dereg_mr(mr);
> -unmap_sg:
> -                       ib_dma_unmap_sg(srv_path->s.dev->ib_dev, sgt->sgl,
> -                                       sgt->nents, DMA_BIDIRECTIONAL);
> -free_sg:
> -                       sg_free_table(sgt);
> -               }
> -               kfree(srv_path->mrs);
> -
> -               return err;
>         }
>
>         chunk_bits = ilog2(srv->queue_depth - 1) + 1;
>         srv_path->mem_bits = (MAX_IMM_PAYL_BITS - chunk_bits);
>
>         return 0;
> +
> +dereg_mr:
> +       ib_dereg_mr(mr);
> +unmap_sg:
> +       ib_dma_unmap_sg(srv_path->s.dev->ib_dev, sgt->sgl,
> +                       sgt->nents, DMA_BIDIRECTIONAL);
> +free_sg:
> +       sg_free_table(sgt);
> +err:
> +       unmap_cont_bufs(srv_path);
> +
> +       return err;
>  }
>
>  static void rtrs_srv_hb_err_handler(struct rtrs_con *c)
> --
> 2.31.1
>

Re: [PATCH RFC 07/12] RDMA/rtrs-srv: Remove outdated comments from create_con

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Remove the orphan comments.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
Acked-by: Md Haris Iqbal <haris.iqbal@ionos.com>

Thanks

> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index f3bf5bbb4377..4c883c57c2ef 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -1671,12 +1671,6 @@ static int create_con(struct rtrs_srv_path *srv_path,
>                                       srv->queue_depth * (1 + 2) + 1);
>
>                 max_recv_wr = srv->queue_depth + 1;
> -               /*
> -                * If we have all receive requests posted and
> -                * all write requests posted and each read request
> -                * requires an invalidate request + drain
> -                * and qp gets into error state.
> -                */
>         }
>         cq_num = max_send_wr + max_recv_wr;
>         atomic_set(&con->c.sq_wr_avail, max_send_wr);
> --
> 2.31.1
>

Re: [PATCH RFC 11/12] RDMA/rtrs-srv: fix several issues in rtrs_srv_destroy_path_files

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> There are several issues in the function which is supposed to be paired
> with rtrs_srv_create_path_files.
>
> 1. rtrs_srv_stats_attr_group is not removed though it is created in
>    rtrs_srv_create_stats_files.
>
> 2. it makes more sense to check kobj_stats.state_in_sysfs before destroy
>    kobj_stats instead of rely on kobj.state_in_sysfs.
>
> 3. kobject_init_and_add is used for both kobjs (srv_path->kobj and
>    srv_path->stats->kobj_stats), however we missed to call kobject_del
>    for srv_path->kobj which was called in free_path.
>
> 4. rtrs_srv_destroy_once_sysfs_root_folders is independant of either
>    kobj or kobj_stats.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
Acked-by: Md Haris Iqbal <haris.iqbal@ionos.com>

Thanks

> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
> index 2a3c9ac64a42..da8e205ce331 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
> @@ -304,12 +304,18 @@ int rtrs_srv_create_path_files(struct rtrs_srv_path *srv_path)
>
>  void rtrs_srv_destroy_path_files(struct rtrs_srv_path *srv_path)
>  {
> -       if (srv_path->kobj.state_in_sysfs) {
> +       if (srv_path->stats->kobj_stats.state_in_sysfs) {
> +               sysfs_remove_group(&srv_path->stats->kobj_stats,
> +                                  &rtrs_srv_stats_attr_group);
>                 kobject_del(&srv_path->stats->kobj_stats);
>                 kobject_put(&srv_path->stats->kobj_stats);
> +       }
> +
> +       if (srv_path->kobj.state_in_sysfs) {
>                 sysfs_remove_group(&srv_path->kobj, &rtrs_srv_path_attr_group);
> +               kobject_del(&srv_path->kobj);
>                 kobject_put(&srv_path->kobj);
> -
> -               rtrs_srv_destroy_once_sysfs_root_folders(srv_path);
>         }
> +
> +       rtrs_srv_destroy_once_sysfs_root_folders(srv_path);
>  }
> --
> 2.31.1
>

Re: [PATCH RFC 12/12] RDMA/rtrs-srv: Remove kobject_del from rtrs_srv_destroy_once_sysfs_root_folders

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> The kobj_paths which is created dynamically by kobject_create_and_add,
> and per the comment above kobject_create_and_add, we only need to call
> kobject_put which is not same as other kobjs such as stats->kobj_stats
> and srv_path->kobj.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
Acked-by: Md Haris Iqbal <haris.iqbal@ionos.com>

Thanks

> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
> index da8e205ce331..c76ba29da1e2 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
> @@ -203,7 +203,6 @@ rtrs_srv_destroy_once_sysfs_root_folders(struct rtrs_srv_path *srv_path)
>
>         mutex_lock(&srv->paths_mutex);
>         if (!--srv->dev_ref) {
> -               kobject_del(srv->kobj_paths);
>                 kobject_put(srv->kobj_paths);
>                 mutex_unlock(&srv->paths_mutex);
>                 device_del(&srv->dev);
> --
> 2.31.1
>

Re: [PATCH RFC 09/12] RDMA/rtrs: Clean up rtrs_rdma_dev_pd_ops

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Let's remove them since the three members are not used.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
Acked-by: Md Haris Iqbal <haris.iqbal@ionos.com>

Thanks

> ---
>  drivers/infiniband/ulp/rtrs/rtrs-pri.h |  3 ---
>  drivers/infiniband/ulp/rtrs/rtrs.c     | 22 ++++------------------
>  2 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> index c4ddaeba1c59..4d15a6fd96b6 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> @@ -68,10 +68,7 @@ enum {
>  struct rtrs_ib_dev;
>
>  struct rtrs_rdma_dev_pd_ops {
> -       struct rtrs_ib_dev *(*alloc)(void);
> -       void (*free)(struct rtrs_ib_dev *dev);
>         int (*init)(struct rtrs_ib_dev *dev);
> -       void (*deinit)(struct rtrs_ib_dev *dev);
>  };
>
>  struct rtrs_rdma_dev_pd {
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
> index ed324b47d93a..4bf9d868cc52 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs.c
> @@ -557,7 +557,6 @@ EXPORT_SYMBOL(rtrs_addr_to_sockaddr);
>  void rtrs_rdma_dev_pd_init(enum ib_pd_flags pd_flags,
>                             struct rtrs_rdma_dev_pd *pool)
>  {
> -       WARN_ON(pool->ops && (!pool->ops->alloc ^ !pool->ops->free));
>         INIT_LIST_HEAD(&pool->list);
>         mutex_init(&pool->mutex);
>         pool->pd_flags = pd_flags;
> @@ -583,15 +582,8 @@ static void dev_free(struct kref *ref)
>         list_del(&dev->entry);
>         mutex_unlock(&pool->mutex);
>
> -       if (pool->ops && pool->ops->deinit)
> -               pool->ops->deinit(dev);
> -
>         ib_dealloc_pd(dev->ib_pd);
> -
> -       if (pool->ops && pool->ops->free)
> -               pool->ops->free(dev);
> -       else
> -               kfree(dev);
> +       kfree(dev);
>  }
>
>  int rtrs_ib_dev_put(struct rtrs_ib_dev *dev)
> @@ -618,11 +610,8 @@ rtrs_ib_dev_find_or_add(struct ib_device *ib_dev,
>                         goto out_unlock;
>         }
>         mutex_unlock(&pool->mutex);
> -       if (pool->ops && pool->ops->alloc)
> -               dev = pool->ops->alloc();
> -       else
> -               dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> -       if (IS_ERR_OR_NULL(dev))
> +       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +       if (!dev)
>                 goto out_err;
>
>         kref_init(&dev->ref);
> @@ -644,10 +633,7 @@ rtrs_ib_dev_find_or_add(struct ib_device *ib_dev,
>  out_free_pd:
>         ib_dealloc_pd(dev->ib_pd);
>  out_free_dev:
> -       if (pool->ops && pool->ops->free)
> -               pool->ops->free(dev);
> -       else
> -               kfree(dev);
> +       kfree(dev);
>  out_err:
>         return NULL;
>  }
> --
> 2.31.1
>

Re: [PATCH RFC 08/12] RDMA/rtrs: Kill recon_cnt from several structs

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Seems the only relevant comment about recon_cnt is,
>
> /*
>  * On every new session connections increase reconnect counter
>  * to avoid clashes with previous sessions not yet closed
>  * sessions on a server side.
>  */
>
> However, it is not clear how the recon_cnt avoid clashed at these places
> in the commit since no where checks it.

It does seem redundant. This predates my time, so I don't know if
there was a change which removed the usage of this. I tried to search
in commit history, but couldn't.

@Jinpu Your thoughts?

>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 8 --------
>  drivers/infiniband/ulp/rtrs/rtrs-pri.h | 3 ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 7 +------
>  3 files changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 5ffc170dae8c..dcc8c041a141 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -1802,7 +1802,6 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con)
>                 .version = cpu_to_le16(RTRS_PROTO_VER),
>                 .cid = cpu_to_le16(con->c.cid),
>                 .cid_num = cpu_to_le16(clt_path->s.con_num),
> -               .recon_cnt = cpu_to_le16(clt_path->s.recon_cnt),
>         };
>         msg.first_conn = clt_path->for_new_clt ? FIRST_CONN : 0;
>         uuid_copy(&msg.sess_uuid, &clt_path->s.uuid);
> @@ -2336,13 +2335,6 @@ static int init_conns(struct rtrs_clt_path *clt_path)
>         unsigned int cid;
>         int err;
>
> -       /*
> -        * On every new session connections increase reconnect counter
> -        * to avoid clashes with previous sessions not yet closed
> -        * sessions on a server side.
> -        */
> -       clt_path->s.recon_cnt++;
> -
>         /* Establish all RDMA connections  */
>         for (cid = 0; cid < clt_path->s.con_num; cid++) {
>                 err = create_con(clt_path, cid);
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> index a2420eecaf5a..c4ddaeba1c59 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> @@ -109,7 +109,6 @@ struct rtrs_path {
>         struct rtrs_con **con;
>         unsigned int            con_num;
>         unsigned int            irq_con_num;
> -       unsigned int            recon_cnt;
>         unsigned int            signal_interval;
>         struct rtrs_ib_dev      *dev;
>         int                     dev_ref;
> @@ -177,7 +176,6 @@ struct rtrs_sg_desc {
>   * @version:      RTRS protocol version
>   * @cid:          Current connection id
>   * @cid_num:      Number of connections per session
> - * @recon_cnt:    Reconnections counter
>   * @sess_uuid:    UUID of a session (path)
>   * @paths_uuid:           UUID of a group of sessions (paths)
>   *
> @@ -196,7 +194,6 @@ struct rtrs_msg_conn_req {
>         __le16          version;
>         __le16          cid;
>         __le16          cid_num;
> -       __le16          recon_cnt;
>         uuid_t          sess_uuid;
>         uuid_t          paths_uuid;
>         u8              first_conn : 1;
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 4c883c57c2ef..e2ea09a8def7 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -1712,7 +1712,6 @@ static int create_con(struct rtrs_srv_path *srv_path,
>  static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
>                                            struct rdma_cm_id *cm_id,
>                                            unsigned int con_num,
> -                                          unsigned int recon_cnt,
>                                            const uuid_t *uuid)
>  {
>         struct rtrs_srv_path *srv_path;
> @@ -1768,7 +1767,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
>
>         srv_path->s.con_num = con_num;
>         srv_path->s.irq_con_num = con_num;
> -       srv_path->s.recon_cnt = recon_cnt;
>         uuid_copy(&srv_path->s.uuid, uuid);
>         spin_lock_init(&srv_path->state_lock);
>         INIT_WORK(&srv_path->close_work, rtrs_srv_close_work);
> @@ -1818,7 +1816,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>         struct rtrs_srv_sess *srv;
>
>         u16 version, con_num, cid;
> -       u16 recon_cnt;
>         int err = -ECONNRESET;
>         bool alloc_path = false;
>
> @@ -1848,7 +1845,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>                 pr_err("Incorrect cid: %d >= %d\n", cid, con_num);
>                 goto reject_w_err;
>         }
> -       recon_cnt = le16_to_cpu(msg->recon_cnt);
>         srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
>         if (IS_ERR(srv)) {
>                 err = PTR_ERR(srv);
> @@ -1885,8 +1881,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>                         goto reject_w_err;
>                 }
>         } else {
> -               srv_path = __alloc_path(srv, cm_id, con_num, recon_cnt,
> -                                   &msg->sess_uuid);
> +               srv_path = __alloc_path(srv, cm_id, con_num, &msg->sess_uuid);
>                 if (IS_ERR(srv_path)) {
>                         mutex_unlock(&srv->paths_mutex);
>                         put_srv(srv);
> --
> 2.31.1
>

Re: [PATCH RFC 10/12] RDMA/rtrs-srv: Remove paths_num

[Haris Iqbal]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> The paths_num is only increased by rtrs_rdma_connect -> __alloc_path
> which is only one time thing, so is the decreasing of it given only
> rtrs_srv_close_work -> del_path_from_srv, which means paths_num should
> always be 1.

It would actually go up to the number of paths a session will have in
a multipath setup. It is the exact counter part of paths_num in the
structure rtrs_clt_sess. But whereas on the client side, the number is
used to access the path list for making decisions in multipathing IO
like round-robin, etc. On the server side, I don't see the use of it.
Maybe just for sanity checks.

@Jinpu Any thoughts?

>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 --------
>  drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 -
>  2 files changed, 9 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index e2ea09a8def7..400cf8ae34a3 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -1437,8 +1437,6 @@ static void __add_path_to_srv(struct rtrs_srv_sess *srv,
>                               struct rtrs_srv_path *srv_path)
>  {
>         list_add_tail(&srv_path->s.entry, &srv->paths_list);
> -       srv->paths_num++;
> -       WARN_ON(srv->paths_num >= MAX_PATHS_NUM);
>  }
>
>  static void del_path_from_srv(struct rtrs_srv_path *srv_path)
> @@ -1450,8 +1448,6 @@ static void del_path_from_srv(struct rtrs_srv_path *srv_path)
>
>         mutex_lock(&srv->paths_mutex);
>         list_del(&srv_path->s.entry);
> -       WARN_ON(!srv->paths_num);
> -       srv->paths_num--;
>         mutex_unlock(&srv->paths_mutex);
>  }
>
> @@ -1719,10 +1715,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
>         char str[NAME_MAX];
>         struct rtrs_addr path;
>
> -       if (srv->paths_num >= MAX_PATHS_NUM) {
> -               err = -ECONNRESET;
> -               goto err;
> -       }
>         if (__is_path_w_addr_exists(srv, &cm_id->route.addr)) {
>                 err = -EEXIST;
>                 pr_err("Path with same addr exists\n");
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> index eccc432b0715..8e4fcb578f49 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> @@ -100,7 +100,6 @@ struct rtrs_srv_sess {
>         struct list_head        paths_list;
>         int                     paths_up;
>         struct mutex            paths_ev_mutex;
> -       size_t                  paths_num;
>         struct mutex            paths_mutex;
>         uuid_t                  paths_uuid;
>         refcount_t              refcount;
> --
> 2.31.1
>

Re: [PATCH RFC 08/12] RDMA/rtrs: Kill recon_cnt from several structs

[Jinpu Wang]

On Tue, Nov 15, 2022 at 10:39 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
>
> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >
> > Seems the only relevant comment about recon_cnt is,
> >
> > /*
> >  * On every new session connections increase reconnect counter
> >  * to avoid clashes with previous sessions not yet closed
> >  * sessions on a server side.
> >  */
> >
> > However, it is not clear how the recon_cnt avoid clashed at these places
> > in the commit since no where checks it.
>
> It does seem redundant. This predates my time, so I don't know if
> there was a change which removed the usage of this. I tried to search
> in commit history, but couldn't.
>
> @Jinpu Your thoughts?
>
> >
> > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 8 --------
> >  drivers/infiniband/ulp/rtrs/rtrs-pri.h | 3 ---
> >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 7 +------
> >  3 files changed, 1 insertion(+), 17 deletions(-)
> >
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > index 5ffc170dae8c..dcc8c041a141 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > @@ -1802,7 +1802,6 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con)
> >                 .version = cpu_to_le16(RTRS_PROTO_VER),
> >                 .cid = cpu_to_le16(con->c.cid),
> >                 .cid_num = cpu_to_le16(clt_path->s.con_num),
> > -               .recon_cnt = cpu_to_le16(clt_path->s.recon_cnt),
> >         };
> >         msg.first_conn = clt_path->for_new_clt ? FIRST_CONN : 0;
> >         uuid_copy(&msg.sess_uuid, &clt_path->s.uuid);
> > @@ -2336,13 +2335,6 @@ static int init_conns(struct rtrs_clt_path *clt_path)
> >         unsigned int cid;
> >         int err;
> >
> > -       /*
> > -        * On every new session connections increase reconnect counter
> > -        * to avoid clashes with previous sessions not yet closed
> > -        * sessions on a server side.
> > -        */
> > -       clt_path->s.recon_cnt++;
> > -
> >         /* Establish all RDMA connections  */
> >         for (cid = 0; cid < clt_path->s.con_num; cid++) {
> >                 err = create_con(clt_path, cid);
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> > index a2420eecaf5a..c4ddaeba1c59 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> > @@ -109,7 +109,6 @@ struct rtrs_path {
> >         struct rtrs_con **con;
> >         unsigned int            con_num;
> >         unsigned int            irq_con_num;
> > -       unsigned int            recon_cnt;
> >         unsigned int            signal_interval;
> >         struct rtrs_ib_dev      *dev;
> >         int                     dev_ref;
> > @@ -177,7 +176,6 @@ struct rtrs_sg_desc {
> >   * @version:      RTRS protocol version
> >   * @cid:          Current connection id
> >   * @cid_num:      Number of connections per session
> > - * @recon_cnt:    Reconnections counter
> >   * @sess_uuid:    UUID of a session (path)
> >   * @paths_uuid:           UUID of a group of sessions (paths)
> >   *
> > @@ -196,7 +194,6 @@ struct rtrs_msg_conn_req {
> >         __le16          version;
> >         __le16          cid;
> >         __le16          cid_num;
> > -       __le16          recon_cnt;
> >         uuid_t          sess_uuid;
> >         uuid_t          paths_uuid;
> >         u8              first_conn : 1;
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > index 4c883c57c2ef..e2ea09a8def7 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > @@ -1712,7 +1712,6 @@ static int create_con(struct rtrs_srv_path *srv_path,
> >  static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
> >                                            struct rdma_cm_id *cm_id,
> >                                            unsigned int con_num,
> > -                                          unsigned int recon_cnt,
> >                                            const uuid_t *uuid)
> >  {
> >         struct rtrs_srv_path *srv_path;
> > @@ -1768,7 +1767,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
> >
> >         srv_path->s.con_num = con_num;
> >         srv_path->s.irq_con_num = con_num;
> > -       srv_path->s.recon_cnt = recon_cnt;
> >         uuid_copy(&srv_path->s.uuid, uuid);
> >         spin_lock_init(&srv_path->state_lock);
> >         INIT_WORK(&srv_path->close_work, rtrs_srv_close_work);
> > @@ -1818,7 +1816,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> >         struct rtrs_srv_sess *srv;
> >
> >         u16 version, con_num, cid;
> > -       u16 recon_cnt;
> >         int err = -ECONNRESET;
> >         bool alloc_path = false;
> >
> > @@ -1848,7 +1845,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> >                 pr_err("Incorrect cid: %d >= %d\n", cid, con_num);
> >                 goto reject_w_err;
> >         }
> > -       recon_cnt = le16_to_cpu(msg->recon_cnt);
> >         srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
> >         if (IS_ERR(srv)) {
> >                 err = PTR_ERR(srv);
> > @@ -1885,8 +1881,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> >                         goto reject_w_err;
> >                 }
> >         } else {
> > -               srv_path = __alloc_path(srv, cm_id, con_num, recon_cnt,
> > -                                   &msg->sess_uuid);
> > +               srv_path = __alloc_path(srv, cm_id, con_num, &msg->sess_uuid);
> >                 if (IS_ERR(srv_path)) {
> >                         mutex_unlock(&srv->paths_mutex);
> >                         put_srv(srv);
> > --
> > 2.31.1
> >

Re: [PATCH RFC 08/12] RDMA/rtrs: Kill recon_cnt from several structs

[Jinpu Wang]

On Tue, Nov 15, 2022 at 10:53 AM Jinpu Wang <jinpu.wang@ionos.com> wrote:
>
> On Tue, Nov 15, 2022 at 10:39 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
> >
> > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> > >
> > > Seems the only relevant comment about recon_cnt is,
> > >
> > > /*
> > >  * On every new session connections increase reconnect counter
> > >  * to avoid clashes with previous sessions not yet closed
> > >  * sessions on a server side.
> > >  */
> > >
> > > However, it is not clear how the recon_cnt avoid clashed at these places
> > > in the commit since no where checks it.
> >
> > It does seem redundant. This predates my time, so I don't know if
> > there was a change which removed the usage of this. I tried to search
> > in commit history, but couldn't.
> >
> > @Jinpu Your thoughts?
Sorry for sending an empty reply.
I checked a bit, and also can find a reason why the recon_cnt was
added, it is not checked on the server side.
I think it's ok to remove it. But we can simply remove the field in
rtrs_msg_conn_req, commented below.

> >
> > >
> > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> > > ---
> > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 8 --------
> > >  drivers/infiniband/ulp/rtrs/rtrs-pri.h | 3 ---
> > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 7 +------
> > >  3 files changed, 1 insertion(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > index 5ffc170dae8c..dcc8c041a141 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > @@ -1802,7 +1802,6 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con)
> > >                 .version = cpu_to_le16(RTRS_PROTO_VER),
> > >                 .cid = cpu_to_le16(con->c.cid),
> > >                 .cid_num = cpu_to_le16(clt_path->s.con_num),
> > > -               .recon_cnt = cpu_to_le16(clt_path->s.recon_cnt),
> > >         };
> > >         msg.first_conn = clt_path->for_new_clt ? FIRST_CONN : 0;
> > >         uuid_copy(&msg.sess_uuid, &clt_path->s.uuid);
> > > @@ -2336,13 +2335,6 @@ static int init_conns(struct rtrs_clt_path *clt_path)
> > >         unsigned int cid;
> > >         int err;
> > >
> > > -       /*
> > > -        * On every new session connections increase reconnect counter
> > > -        * to avoid clashes with previous sessions not yet closed
> > > -        * sessions on a server side.
> > > -        */
> > > -       clt_path->s.recon_cnt++;
> > > -
> > >         /* Establish all RDMA connections  */
> > >         for (cid = 0; cid < clt_path->s.con_num; cid++) {
> > >                 err = create_con(clt_path, cid);
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> > > index a2420eecaf5a..c4ddaeba1c59 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
> > > @@ -109,7 +109,6 @@ struct rtrs_path {
> > >         struct rtrs_con **con;
> > >         unsigned int            con_num;
> > >         unsigned int            irq_con_num;
> > > -       unsigned int            recon_cnt;
> > >         unsigned int            signal_interval;
> > >         struct rtrs_ib_dev      *dev;
> > >         int                     dev_ref;
> > > @@ -177,7 +176,6 @@ struct rtrs_sg_desc {
> > >   * @version:      RTRS protocol version
> > >   * @cid:          Current connection id
> > >   * @cid_num:      Number of connections per session
> > > - * @recon_cnt:    Reconnections counter
> > >   * @sess_uuid:    UUID of a session (path)
> > >   * @paths_uuid:           UUID of a group of sessions (paths)
> > >   *
> > > @@ -196,7 +194,6 @@ struct rtrs_msg_conn_req {
> > >         __le16          version;
> > >         __le16          cid;
> > >         __le16          cid_num;
> > > -       __le16          recon_cnt;
> > >         uuid_t          sess_uuid;
> > >         uuid_t          paths_uuid;
> > >         u8              first_conn : 1;
We can remove it from protocol, this break compatibility with older kernel.
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > index 4c883c57c2ef..e2ea09a8def7 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > @@ -1712,7 +1712,6 @@ static int create_con(struct rtrs_srv_path *srv_path,
> > >  static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
> > >                                            struct rdma_cm_id *cm_id,
> > >                                            unsigned int con_num,
> > > -                                          unsigned int recon_cnt,
> > >                                            const uuid_t *uuid)
> > >  {
> > >         struct rtrs_srv_path *srv_path;
> > > @@ -1768,7 +1767,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
> > >
> > >         srv_path->s.con_num = con_num;
> > >         srv_path->s.irq_con_num = con_num;
> > > -       srv_path->s.recon_cnt = recon_cnt;
> > >         uuid_copy(&srv_path->s.uuid, uuid);
> > >         spin_lock_init(&srv_path->state_lock);
> > >         INIT_WORK(&srv_path->close_work, rtrs_srv_close_work);
> > > @@ -1818,7 +1816,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> > >         struct rtrs_srv_sess *srv;
> > >
> > >         u16 version, con_num, cid;
> > > -       u16 recon_cnt;
> > >         int err = -ECONNRESET;
> > >         bool alloc_path = false;
> > >
> > > @@ -1848,7 +1845,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> > >                 pr_err("Incorrect cid: %d >= %d\n", cid, con_num);
> > >                 goto reject_w_err;
> > >         }
> > > -       recon_cnt = le16_to_cpu(msg->recon_cnt);
> > >         srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
> > >         if (IS_ERR(srv)) {
> > >                 err = PTR_ERR(srv);
> > > @@ -1885,8 +1881,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> > >                         goto reject_w_err;
> > >                 }
> > >         } else {
> > > -               srv_path = __alloc_path(srv, cm_id, con_num, recon_cnt,
> > > -                                   &msg->sess_uuid);
> > > +               srv_path = __alloc_path(srv, cm_id, con_num, &msg->sess_uuid);
> > >                 if (IS_ERR(srv_path)) {
> > >                         mutex_unlock(&srv->paths_mutex);
> > >                         put_srv(srv);
> > > --
> > > 2.31.1
> > >

Re: [PATCH RFC 10/12] RDMA/rtrs-srv: Remove paths_num

[Jinpu Wang]

On Tue, Nov 15, 2022 at 10:49 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
>
> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >
> > The paths_num is only increased by rtrs_rdma_connect -> __alloc_path
> > which is only one time thing, so is the decreasing of it given only
> > rtrs_srv_close_work -> del_path_from_srv, which means paths_num should
> > always be 1.
>
> It would actually go up to the number of paths a session will have in
> a multipath setup. It is the exact counter part of paths_num in the
> structure rtrs_clt_sess. But whereas on the client side, the number is
> used to access the path list for making decisions in multipathing IO
> like round-robin, etc. On the server side, I don't see the use of it.
> Maybe just for sanity checks.
>
> @Jinpu Any thoughts?
Yes, the idea is RTRS can have many paths, not only one, and you can
add/remove path at run time,
so not a one time thing.
>
> >
> > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 --------
> >  drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 -
> >  2 files changed, 9 deletions(-)
> >
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > index e2ea09a8def7..400cf8ae34a3 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > @@ -1437,8 +1437,6 @@ static void __add_path_to_srv(struct rtrs_srv_sess *srv,
> >                               struct rtrs_srv_path *srv_path)
> >  {
> >         list_add_tail(&srv_path->s.entry, &srv->paths_list);
> > -       srv->paths_num++;
> > -       WARN_ON(srv->paths_num >= MAX_PATHS_NUM);
> >  }
> >
> >  static void del_path_from_srv(struct rtrs_srv_path *srv_path)
> > @@ -1450,8 +1448,6 @@ static void del_path_from_srv(struct rtrs_srv_path *srv_path)
> >
> >         mutex_lock(&srv->paths_mutex);
> >         list_del(&srv_path->s.entry);
> > -       WARN_ON(!srv->paths_num);
> > -       srv->paths_num--;
> >         mutex_unlock(&srv->paths_mutex);
> >  }
> >
> > @@ -1719,10 +1715,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
> >         char str[NAME_MAX];
> >         struct rtrs_addr path;
> >
> > -       if (srv->paths_num >= MAX_PATHS_NUM) {
> > -               err = -ECONNRESET;
> > -               goto err;
> > -       }
> >         if (__is_path_w_addr_exists(srv, &cm_id->route.addr)) {
> >                 err = -EEXIST;
> >                 pr_err("Path with same addr exists\n");
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> > index eccc432b0715..8e4fcb578f49 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
> > @@ -100,7 +100,6 @@ struct rtrs_srv_sess {
> >         struct list_head        paths_list;
> >         int                     paths_up;
> >         struct mutex            paths_ev_mutex;
> > -       size_t                  paths_num;
> >         struct mutex            paths_mutex;
> >         uuid_t                  paths_uuid;
> >         refcount_t              refcount;
> > --
> > 2.31.1
> >

Re: [PATCH RFC 01/12] RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx

[Guoqing Jiang]

Hi Haris,

On 11/14/22 9:36 PM, Haris Iqbal wrote:
>
>>> The change was introduced due to  a bug report, you can follow the
>>> discussion here for the history and reason:
>>> https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/
>> Thanks for the link.
>>
>> I probably missed something but I don't know how rnbd_server module can be
>> initialized before cma module since we have the dependency chain as
>> follows.
> Hi Guoqing,
>
> One of the ways this was happening was, when one builds the RNBD/RTRS
> module into the kernel bzImage and then boots up the kernel. Depending
> on the module init sequence, if the RNBD/RTRS modules are picked
> before the RDMA one, then this issue would hit.

This is what I missed since I usually build RNBD with M.

> With the changes, RNBD/RTRS just register itself to ib. If any devices
> are present at that moment, for example when the module is modprob'ed
> later into the kernel, then .add gets called through
> ib_register_client. If no devices are present, for example in case if
> the module is built into the bzImage, and is inserted before RDMA
> module, then ib_register_client simply registers RTRS, and returns
> without calling .add
> Now, when RDMA gets initialized, and it detects devices, then .add is
> called for each device, which will land into rtrs_srv_add_one

Thanks for the details! It explains well why ib_register_client is added,
and rtrs_srv_rdma_init is guaranteed to be called once too. Will drop
this one.

Thanks,
Guoqings

Re: [PATCH RFC 02/12] RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler

[Guoqing Jiang]

On 11/14/22 10:39 PM, Haris Iqbal wrote:
> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang<guoqing.jiang@linux.dev>  wrote:
>> The RDMA_CM_EVENT_CONNECT_REQUEST is quite different to other types,
>> let's checking it separately at the beginning of routine, then we can
>> avoid the identation accordingly.
>>
>> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
>> ---
>>   drivers/infiniband/ulp/rtrs/rtrs-srv.c | 17 ++++++-----------
>>   1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> index 79504aaef0cc..2cc8b423bcaa 100644
>> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> @@ -1948,24 +1948,19 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>>   static int rtrs_srv_rdma_cm_handler(struct rdma_cm_id *cm_id,
>>                                       struct rdma_cm_event *ev)
>>   {
>> -       struct rtrs_srv_path *srv_path = NULL;
>> -       struct rtrs_path *s = NULL;
>> -
>> -       if (ev->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
>> -               struct rtrs_con *c = cm_id->context;
>> -
>> -               s = c->path;
>> -               srv_path = to_srv_path(s);
>> -       }
>> +       struct rtrs_con *c = cm_id->context;
>> +       struct rtrs_path *s = c->path;
>> +       struct rtrs_srv_path *srv_path = to_srv_path(s);
> This isn't correct for the RDMA_CM_EVENT_CONNECT_REQUEST event. At
> that moment, cm_id->context is still holding a pointer to struct
> rtrs_srv_ctx. Even though it never gets used for this event here, its
> not right IMO to dereference in this incorrect manner.

But rtrs_rdma_connect will deference the context again for connect request.

> How about we move the check for the RDMA_CM_EVENT_CONNECT_REQUEST
> event outside (just like you did), but let the pointers be NULL, and
> only dereference after the if condition for
> RDMA_CM_EVENT_CONNECT_REQUEST event?

Ok, will do it though it need more lines which I tried to avoid.

Thanks,
Guoqing

Re: [PATCH RFC 03/12] RDMA/rtrs-srv: Only close srv_path if it is just allocated

[Guoqing Jiang]

On 11/15/22 12:18 AM, Haris Iqbal wrote:
> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang<guoqing.jiang@linux.dev>  wrote:
>> RTRS creates several connections per nr_cpu_ids, it makes more sense
>> to only close the path when it just allocated.
>>
>> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
>> ---
>>   drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> index 2cc8b423bcaa..063082d29fc6 100644
>> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> @@ -1833,6 +1833,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>>          u16 version, con_num, cid;
>>          u16 recon_cnt;
>>          int err = -ECONNRESET;
>> +       bool alloc_path = false;
>>
>>          if (len < sizeof(*msg)) {
>>                  pr_err("Invalid RTRS connection request\n");
>> @@ -1906,6 +1907,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>>                          pr_err("RTRS server session allocation failed: %d\n", err);
>>                          goto reject_w_err;
>>                  }
>> +               alloc_path = true;
>>          }
>>          err = create_con(srv_path, cm_id, cid);
>>          if (err) {
>> @@ -1940,7 +1942,8 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
>>
>>   close_and_return_err:
>>          mutex_unlock(&srv->paths_mutex);
>> -       close_path(srv_path);
>> +       if (alloc_path)
>> +               close_path(srv_path);
> I think the way this is coded is, if there is a problem opening a
> connection in that srv_path, then it closes that srv_path gracefully,
> which results in all other connections in that path getting closed,
> and the IOs being failover'ed to the other path (in case of
> multipath), and then the client would trigger an auto reconnect.

Could it possible that IO is happening in the previous connection, then 
a later
failed connection try to close all the connections.

> @Jinpu can shed some more light, what would be the preferred course of
> action if there is a failure in one connection. To keep the current
> design or to avoid closing the path in case of a connection issue.

Frankly, I am less confident about this patch and seems it is a rare 
condition.
Will just drop it for now.

Thanks,
Guoqing

Re: [PATCH RFC 10/12] RDMA/rtrs-srv: Remove paths_num

[Guoqing Jiang]

On 11/15/22 6:01 PM, Jinpu Wang wrote:
> On Tue, Nov 15, 2022 at 10:49 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>>> The paths_num is only increased by rtrs_rdma_connect -> __alloc_path
>>> which is only one time thing, so is the decreasing of it given only
>>> rtrs_srv_close_work -> del_path_from_srv, which means paths_num should
>>> always be 1.
>> It would actually go up to the number of paths a session will have in
>> a multipath setup. It is the exact counter part of paths_num in the
>> structure rtrs_clt_sess. But whereas on the client side, the number is
>> used to access the path list for making decisions in multipathing IO
>> like round-robin, etc. On the server side, I don't see the use of it.
>> Maybe just for sanity checks.
>>
>> @Jinpu Any thoughts?
> Yes, the idea is RTRS can have many paths, not only one, and you can
> add/remove path at run time,
> so not a one time thing.

Got it, thanks for your review.

Guoqing

Re: [PATCH RFC 03/12] RDMA/rtrs-srv: Only close srv_path if it is just allocated

[Jinpu Wang]

On Tue, Nov 15, 2022 at 11:24 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 11/15/22 12:18 AM, Haris Iqbal wrote:
> > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang<guoqing.jiang@linux.dev>  wrote:
> >> RTRS creates several connections per nr_cpu_ids, it makes more sense
> >> to only close the path when it just allocated.
> >>
> >> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
> >> ---
> >>   drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> >> index 2cc8b423bcaa..063082d29fc6 100644
> >> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> >> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> >> @@ -1833,6 +1833,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> >>          u16 version, con_num, cid;
> >>          u16 recon_cnt;
> >>          int err = -ECONNRESET;
> >> +       bool alloc_path = false;
> >>
> >>          if (len < sizeof(*msg)) {
> >>                  pr_err("Invalid RTRS connection request\n");
> >> @@ -1906,6 +1907,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> >>                          pr_err("RTRS server session allocation failed: %d\n", err);
> >>                          goto reject_w_err;
> >>                  }
> >> +               alloc_path = true;
> >>          }
> >>          err = create_con(srv_path, cm_id, cid);
> >>          if (err) {
> >> @@ -1940,7 +1942,8 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> >>
> >>   close_and_return_err:
> >>          mutex_unlock(&srv->paths_mutex);
> >> -       close_path(srv_path);
> >> +       if (alloc_path)
> >> +               close_path(srv_path);
> > I think the way this is coded is, if there is a problem opening a
> > connection in that srv_path, then it closes that srv_path gracefully,
> > which results in all other connections in that path getting closed,
> > and the IOs being failover'ed to the other path (in case of
> > multipath), and then the client would trigger an auto reconnect.
>
> Could it possible that IO is happening in the previous connection, then
> a later
> failed connection try to close all the connections.

Yes, the idea is if any failure during rdma_connect, we close the full
path and expecting the other healthy path
to take over.
>
> > @Jinpu can shed some more light, what would be the preferred course of
> > action if there is a failure in one connection. To keep the current
> > design or to avoid closing the path in case of a connection issue.
>
> Frankly, I am less confident about this patch and seems it is a rare
> condition.
> Will just drop it for now.
>
> Thanks,
> Guoqing

Re: [PATCH RFC 08/12] RDMA/rtrs: Kill recon_cnt from several structs

[Guoqing Jiang]

On 11/15/22 5:58 PM, Jinpu Wang wrote:
> On Tue, Nov 15, 2022 at 10:53 AM Jinpu Wang <jinpu.wang@ionos.com> wrote:
>> On Tue, Nov 15, 2022 at 10:39 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
>>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>>>> Seems the only relevant comment about recon_cnt is,
>>>>
>>>> /*
>>>>   * On every new session connections increase reconnect counter
>>>>   * to avoid clashes with previous sessions not yet closed
>>>>   * sessions on a server side.
>>>>   */
>>>>
>>>> However, it is not clear how the recon_cnt avoid clashed at these places
>>>> in the commit since no where checks it.
>>> It does seem redundant. This predates my time, so I don't know if
>>> there was a change which removed the usage of this. I tried to search
>>> in commit history, but couldn't.
>>>
>>> @Jinpu Your thoughts?
> Sorry for sending an empty reply.
> I checked a bit, and also can find a reason why the recon_cnt was
> added, it is not checked on the server side.
> I think it's ok to remove it. But we can simply remove the field in
> rtrs_msg_conn_req, commented below.
>
>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>>>> ---
>>>>   drivers/infiniband/ulp/rtrs/rtrs-clt.c | 8 --------
>>>>   drivers/infiniband/ulp/rtrs/rtrs-pri.h | 3 ---
>>>>   drivers/infiniband/ulp/rtrs/rtrs-srv.c | 7 +------
>>>>   3 files changed, 1 insertion(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>> index 5ffc170dae8c..dcc8c041a141 100644
>>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>> @@ -1802,7 +1802,6 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con)
>>>>                  .version = cpu_to_le16(RTRS_PROTO_VER),
>>>>                  .cid = cpu_to_le16(con->c.cid),
>>>>                  .cid_num = cpu_to_le16(clt_path->s.con_num),
>>>> -               .recon_cnt = cpu_to_le16(clt_path->s.recon_cnt),
>>>>          };
>>>>          msg.first_conn = clt_path->for_new_clt ? FIRST_CONN : 0;
>>>>          uuid_copy(&msg.sess_uuid, &clt_path->s.uuid);
>>>> @@ -2336,13 +2335,6 @@ static int init_conns(struct rtrs_clt_path *clt_path)
>>>>          unsigned int cid;
>>>>          int err;
>>>>
>>>> -       /*
>>>> -        * On every new session connections increase reconnect counter
>>>> -        * to avoid clashes with previous sessions not yet closed
>>>> -        * sessions on a server side.
>>>> -        */
>>>> -       clt_path->s.recon_cnt++;
>>>> -
>>>>          /* Establish all RDMA connections  */
>>>>          for (cid = 0; cid < clt_path->s.con_num; cid++) {
>>>>                  err = create_con(clt_path, cid);
>>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
>>>> index a2420eecaf5a..c4ddaeba1c59 100644
>>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
>>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
>>>> @@ -109,7 +109,6 @@ struct rtrs_path {
>>>>          struct rtrs_con **con;
>>>>          unsigned int            con_num;
>>>>          unsigned int            irq_con_num;
>>>> -       unsigned int            recon_cnt;
>>>>          unsigned int            signal_interval;
>>>>          struct rtrs_ib_dev      *dev;
>>>>          int                     dev_ref;
>>>> @@ -177,7 +176,6 @@ struct rtrs_sg_desc {
>>>>    * @version:      RTRS protocol version
>>>>    * @cid:          Current connection id
>>>>    * @cid_num:      Number of connections per session
>>>> - * @recon_cnt:    Reconnections counter
>>>>    * @sess_uuid:    UUID of a session (path)
>>>>    * @paths_uuid:           UUID of a group of sessions (paths)
>>>>    *
>>>> @@ -196,7 +194,6 @@ struct rtrs_msg_conn_req {
>>>>          __le16          version;
>>>>          __le16          cid;
>>>>          __le16          cid_num;
>>>> -       __le16          recon_cnt;
>>>>          uuid_t          sess_uuid;
>>>>          uuid_t          paths_uuid;
>>>>          u8              first_conn : 1;
> We can remove it from protocol, this break compatibility with older kernel.

Can we bump up RTRS_PROTO_VER_MINOR for the compatibility? Otherwise
those structs are immutable forever.

Thanks,
Guoqing

Re: [PATCH RFC 05/12] RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg

[Jinpu Wang]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> We should check with nr_sgt, also the only successful case is that
> all sg elements are mapped, so make it explict.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 88eae0dcf87f..f3bf5bbb4377 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
>                 }
>                 nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt,
>                                   NULL, max_chunk_size);
> -               if (nr < 0 || nr < sgt->nents) {
> -                       err = nr < 0 ? nr : -EINVAL;
> +               if (nr != nr_sgt) {
> +                       err = -EINVAL;
but with this, the initial errno are lost, we only return EINVAL
>                         goto dereg_mr;
>                 }
>
> --
> 2.31.1
>

Re: [PATCH RFC 06/12] RDMA/rtrs-clt: Correct the checking of ib_map_mr_sg

[Jinpu Wang]

On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> We should check with count, also the only successful case is that
> all sg elements are mapped, so make it explict.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index 8546b8816524..5ffc170dae8c 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -1064,9 +1064,7 @@ static int rtrs_map_sg_fr(struct rtrs_clt_io_req *req, size_t count)
>
>         /* Align the MR to a 4K page size to match the block virt boundary */
>         nr = ib_map_mr_sg(req->mr, req->sglist, count, NULL, SZ_4K);
> -       if (nr < 0)
> -               return nr;
> -       if (nr < req->sg_cnt)
> +       if (nr != count)
>                 return -EINVAL;
same here, we lost errno from API.
>         ib_update_fast_reg_key(req->mr, ib_inc_rkey(req->mr->rkey));
>
> --
> 2.31.1
>

Re: [PATCH RFC 05/12] RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg

[Guoqing Jiang]

On 11/15/22 7:46 PM, Jinpu Wang wrote:
> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> We should check with nr_sgt, also the only successful case is that
>> all sg elements are mapped, so make it explict.
>>
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>> ---
>>   drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> index 88eae0dcf87f..f3bf5bbb4377 100644
>> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
>> @@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
>>                  }
>>                  nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt,
>>                                    NULL, max_chunk_size);
>> -               if (nr < 0 || nr < sgt->nents) {
>> -                       err = nr < 0 ? nr : -EINVAL;
>> +               if (nr != nr_sgt) {
>> +                       err = -EINVAL;
> but with this, the initial errno are lost, we only return EINVAL

OK, assume you mean 'nr' here, I can change it back as iser_memory did.
But seems the only negative value returned from ib_map_mr_sg is also
"-EINVAL" from ib_sg_to_pages after go through hw drivers.

BTW, I looked all call sites of ib_map_mr_sg, seems they have different
kind of checking.

1. if (ret < 0 || ret < nents)

2. if (ret < nents) or if (ret < 0)

3. if (ret != nents)

Thanks,
Guoqing

Re: [PATCH RFC 05/12] RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg

[Jinpu Wang]

On Wed, Nov 16, 2022 at 2:13 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 11/15/22 7:46 PM, Jinpu Wang wrote:
> > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >> We should check with nr_sgt, also the only successful case is that
> >> all sg elements are mapped, so make it explict.
> >>
> >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> >> ---
> >>   drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> >> index 88eae0dcf87f..f3bf5bbb4377 100644
> >> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> >> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> >> @@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path)
> >>                  }
> >>                  nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt,
> >>                                    NULL, max_chunk_size);
> >> -               if (nr < 0 || nr < sgt->nents) {
> >> -                       err = nr < 0 ? nr : -EINVAL;
> >> +               if (nr != nr_sgt) {
> >> +                       err = -EINVAL;
> > but with this, the initial errno are lost, we only return EINVAL
>
> OK, assume you mean 'nr' here, I can change it back as iser_memory did.
> But seems the only negative value returned from ib_map_mr_sg is also
> "-EINVAL" from ib_sg_to_pages after go through hw drivers.
Yes, I meant nr.
there is also cases
"
if (unlikely(!mr->device->map_mr_sg))
return -ENOSYS;
"
>
> BTW, I looked all call sites of ib_map_mr_sg, seems they have different
> kind of checking.
>
> 1. if (ret < 0 || ret < nents)
>
> 2. if (ret < nents) or if (ret < 0)
>
> 3. if (ret != nents)
Yes, I also checked them. :)
>
> Thanks,
> Guoqing

Re: [PATCH RFC 00/12] Misc changes for rtrs

[Leon Romanovsky]

On Sun, Nov 13, 2022 at 09:08:11AM +0800, Guoqing Jiang wrote:
> Hi,
> 
> Here are some changes for rtrs, please review them.
> 
> Thanks,
> Guoqing
> 
> Guoqing Jiang (12):
>   RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx
>   RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler
>   RDMA/rtrs-srv: Only close srv_path if it is just allocated
>   RDMA/rtrs-srv: refactor the handling of failure case in map_cont_bufs
>   RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg
>   RDMA/rtrs-clt: Correct the checking of ib_map_mr_sg
>   RDMA/rtrs-srv: Remove outdated comments from create_con
>   RDMA/rtrs: Kill recon_cnt from several structs
>   RDMA/rtrs: Clean up rtrs_rdma_dev_pd_ops
>   RDMA/rtrs-srv: Remove paths_num
>   RDMA/rtrs-srv: fix several issues in rtrs_srv_destroy_path_files
>   RDMA/rtrs-srv: Remove kobject_del from
>     rtrs_srv_destroy_once_sysfs_root_folders

Can you please resend already reviewed and non-controversial patches as a
standalone series, so we will be able to apply them?

Thanks

> 
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c       |  12 +-
>  drivers/infiniband/ulp/rtrs/rtrs-pri.h       |   6 -
>  drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c |  13 ++-
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 110 ++++++-------------
>  drivers/infiniband/ulp/rtrs/rtrs-srv.h       |   2 -
>  drivers/infiniband/ulp/rtrs/rtrs.c           |  22 +---
>  6 files changed, 47 insertions(+), 118 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH RFC 00/12] Misc changes for rtrs

[Guoqing Jiang]

On 11/17/22 5:52 PM, Leon Romanovsky wrote:
> On Sun, Nov 13, 2022 at 09:08:11AM +0800, Guoqing Jiang wrote:
>> Hi,
>>
>> Here are some changes for rtrs, please review them.
>>
>> Thanks,
>> Guoqing
>>
>> Guoqing Jiang (12):
>>    RDMA/rtrs-srv: Remove ib_dev_count from rtrs_srv_ib_ctx
>>    RDMA/rtrs-srv: Refactor rtrs_srv_rdma_cm_handler
>>    RDMA/rtrs-srv: Only close srv_path if it is just allocated
>>    RDMA/rtrs-srv: refactor the handling of failure case in map_cont_bufs
>>    RDMA/rtrs-srv: Correct the checking of ib_map_mr_sg
>>    RDMA/rtrs-clt: Correct the checking of ib_map_mr_sg
>>    RDMA/rtrs-srv: Remove outdated comments from create_con
>>    RDMA/rtrs: Kill recon_cnt from several structs
>>    RDMA/rtrs: Clean up rtrs_rdma_dev_pd_ops
>>    RDMA/rtrs-srv: Remove paths_num
>>    RDMA/rtrs-srv: fix several issues in rtrs_srv_destroy_path_files
>>    RDMA/rtrs-srv: Remove kobject_del from
>>      rtrs_srv_destroy_once_sysfs_root_folders
> Can you please resend already reviewed and non-controversial patches as a
> standalone series, so we will be able to apply them?

Sure, will send a new v2 series soon.

Thanks,
Guoqing