Re: [PATCHv2 for-next 2/5] RDMA/rtrs-srv: Rename rtrs_srv_sess to rtrs_srv_path

[Guoqing Jiang <guoqing.jiang@linux.dev>]

On 1/3/22 9:33 PM, Jack Wang wrote:

> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -269,9 +269,9 @@ static int create_sess(struct rtrs_srv *rtrs)
>   	char sessname[NAME_MAX];

It is pathname with the change.

>   	int err;
>   
> -	err = rtrs_srv_get_sess_name(rtrs, sessname, sizeof(sessname));
> +	err = rtrs_srv_get_path_name(rtrs, sessname, sizeof(sessname));
>   	if (err) {
> -		pr_err("rtrs_srv_get_sess_name(%s): %d\n", sessname, err);
> +		pr_err("rtrs_srv_get_path_name(%s): %d\n", sessname, err);
>   
>   		return err;
>   	}

[ ... ]

> -void close_sess(struct rtrs_srv_sess *sess)
> +void close_path(struct rtrs_srv_path *srv_path)

I guess close_srv_path is better if it has counterpart in rtrs-clt.c.

[ ... ]

>   		rtrs_err_rl(s,
>   			    "Sending I/O response failed,  session %s is disconnected, sess state %s\n",

With the change, the above should print path instead of session.

> -			    kobject_name(&sess->kobj),
> -			    rtrs_srv_state_str(sess->state));
> +			    kobject_name(&srv_path->kobj),
> +			    rtrs_srv_state_str(srv_path->state));
>   		goto out;
>   	}
>   	if (always_invalidate) {
> -		struct rtrs_srv_mr *mr = &sess->mrs[id->msg_id];
> +		struct rtrs_srv_mr *mr = &srv_path->mrs[id->msg_id];
>   
>   		ib_update_fast_reg_key(mr->mr, ib_inc_rkey(mr->mr->rkey));
>   	}
>   	if (atomic_sub_return(1, &con->c.sq_wr_avail) < 0) {
>   		rtrs_err(s, "IB send queue full:*sess*=%s cid=%d\n",
> -			 kobject_name(&sess->kobj),
> +			 kobject_name(&srv_path->kobj),

Ditto.

>   			 con->c.cid);
>   		atomic_add(1, &con->c.sq_wr_avail);
>   		spin_lock(&con->rsp_wr_wait_lock);
> @@ -524,11 +527,11 @@ bool rtrs_srv_resp_rdma(struct rtrs_srv_op *id, int status)
>   
>   	if (err) {
>   		rtrs_err_rl(s, "IO response failed: %d:*sess*=%s\n", err,
> -			    kobject_name(&sess->kobj));
> -		close_sess(sess);
> +			    kobject_name(&srv_path->kobj));
> +		close_path(srv_path);

Ditto.

[ ... ]

> @@ -754,7 +757,7 @@ static bool exist_sessname(struct rtrs_srv_ctx *ctx,
>   			   const char *sessname, const uuid_t *path_uuid)
>   {
>   	struct rtrs_srv *srv;
> -	struct rtrs_srv_sess *sess;
> +	struct rtrs_srv_path *srv_path;
>   	bool found = false;
>   
>   	mutex_lock(&ctx->srv_mutex);
> @@ -767,9 +770,9 @@ static bool exist_sessname(struct rtrs_srv_ctx *ctx,
>   			continue;
>   		}
>   
> -		list_for_each_entry(sess, &srv->paths_list, s.entry) {
> -			if (strlen(sess->s.sessname) == strlen(sessname) &&
> -			    !strcmp(sess->s.sessname, sessname)) {
> +		list_for_each_entry(srv_path, &srv->paths_list, s.entry) {
> +			if (strlen(srv_path->s.sessname) == strlen(sessname) &&
> +			    !strcmp(srv_path->s.sessname, sessname)) {

I am wondering if the sessname should be replaced with pathname, pls 
double check.
Does it make sense to add the definitions about "path" and "session" 
somewhere such
as in README?

[ ... ]

>   
> -static int post_recv_sess(struct rtrs_srv_sess *sess)
> +static int post_recv_sess(struct rtrs_srv_path *srv_path)

post_recv_path?

>   {
> -	struct rtrs_srv *srv = sess->srv;
> -	struct rtrs_path *s = &sess->s;
> +	struct rtrs_srv *srv = srv_path->srv;
> +	struct rtrs_path *s = &srv_path->s;
>   	size_t q_size;
>   	int err, cid;
>   
> -	for (cid = 0; cid < sess->s.con_num; cid++) {
> +	for (cid = 0; cid < srv_path->s.con_num; cid++) {
>   		if (cid == 0)
>   			q_size = SERVICE_CON_QUEUE_DEPTH;
>   		else
>   			q_size = srv->queue_depth;
>   
> -		err = post_recv_io(to_srv_con(sess->s.con[cid]), q_size);
> +		err = post_recv_io(to_srv_con(srv_path->s.con[cid]), q_size);
>   		if (err) {
>   			rtrs_err(s, "post_recv_io(), err: %d\n", err);
>   			return err;

[ .. ]

>   /**
> - * rtrs_srv_get_sess_name() - Get rtrs_srv peer hostname.
> + * rtrs_srv_get_path_name() - Get rtrs_srv peer hostname.
>    * @srv:	Session
>    * @sessname:	Sessname buffer
>    * @len:	Length of sessname buffer
>    */
> -int rtrs_srv_get_sess_name(struct rtrs_srv *srv, char *sessname, size_t len)
> +int rtrs_srv_get_path_name(struct rtrs_srv *srv, char *sessname, size_t len)

The "sessname" argument is actually for path, right?

>   {
> -	struct rtrs_srv_sess *sess;
> +	struct rtrs_srv_path *srv_path;
>   	int err = -ENOTCONN;
>   
>   	mutex_lock(&srv->paths_mutex);
> -	list_for_each_entry(sess, &srv->paths_list, s.entry) {
> -		if (sess->state != RTRS_SRV_CONNECTED)
> +	list_for_each_entry(srv_path, &srv->paths_list, s.entry) {
> +		if (srv_path->state != RTRS_SRV_CONNECTED)
>   			continue;
> -		strscpy(sessname, sess->s.sessname,
> -		       min_t(size_t, sizeof(sess->s.sessname), len));
> +		strscpy(sessname, srv_path->s.sessname,
> +			min_t(size_t, sizeof(srv_path->s.sessname), len));
>   		err = 0;
>   		break;
>   	}
> @@ -1307,7 +1313,7 @@ int rtrs_srv_get_sess_name(struct rtrs_srv *srv, char *sessname, size_t len)
>   
>   	return err;
>   }
> -EXPORT_SYMBOL(rtrs_srv_get_sess_name);
> +EXPORT_SYMBOL(rtrs_srv_get_path_name);

[ ... ]

Thanks,
Guoqing