Re: [PATCH v6 06/25] rtrs: client: main functionality

[Bart Van Assche <bvanassche@acm.org>]

On 2019-12-30 02:29, Jack Wang wrote:
> + * InfiniBand Transport Layer

InfiniBand or RDMA?

> +MODULE_DESCRIPTION("RTRS Client");

Please spell out RTRS in full.

> +static const struct rtrs_ib_dev_pool_ops dev_pool_ops;

Can this forward declaration be avoided?

> +static struct rtrs_ib_dev_pool dev_pool = {
> +	.ops = &dev_pool_ops
> +};

Can this structure be declared 'const'?

> +static inline struct rtrs_permit *
> +__rtrs_get_permit(struct rtrs_clt *clt, enum rtrs_clt_con_type con_type)
> +{
> +	size_t max_depth = clt->queue_depth;
> +	struct rtrs_permit *permit;
> +	int cpu, bit;
> +
> +	cpu = get_cpu();
> +	do {
> +		bit = find_first_zero_bit(clt->permits_map, max_depth);
> +		if (unlikely(bit >= max_depth)) {
> +			put_cpu();
> +			return NULL;
> +		}
> +
> +	} while (unlikely(test_and_set_bit_lock(bit, clt->permits_map)));
> +	put_cpu();

Are the get_cpu() and put_cpu() calls around this loop useful? If not,
please remove these calls. Otherwise please add a comment that explains
the purpose of these calls.

An additional question: is it possible to replace the above loop with an
sbitmap_get() call?

> +static void complete_rdma_req(struct rtrs_clt_io_req *req, int errno,
> +			      bool notify, bool can_wait)
> +{
> +	struct rtrs_clt_con *con = req->con;
> +	struct rtrs_clt_sess *sess;
> +	int err;
> +
> +	if (WARN_ON(!req->in_use))
> +		return;
> +	if (WARN_ON(!req->con))
> +		return;
> +	sess = to_clt_sess(con->c.sess);
> +
> +	if (req->sg_cnt) {
> +		if (unlikely(req->dir == DMA_FROM_DEVICE && req->need_inv)) {
> +			/*
> +			 * We are here to invalidate RDMA read requests
> +			 * ourselves.  In normal scenario server should
> +			 * send INV for all requested RDMA reads, but
> +			 * we are here, thus two things could happen:
> +			 *
> +			 *    1.  this is failover, when errno != 0
> +			 *        and can_wait == 1,
> +			 *
> +			 *    2.  something totally bad happened and
> +			 *        server forgot to send INV, so we
> +			 *        should do that ourselves.
> +			 */

Please document in the protocol documentation when RDMA reads are used.

What does "server forgot to send INV" mean?

Additionally, if I remember correctly Jason considers it very important
that invalidation happens from the submitting context because otherwise
the RDMA retry mechanism can't work.

> +static void process_io_rsp(struct rtrs_clt_sess *sess, u32 msg_id,
> +			   s16 errno, bool w_inval)
> +{
> +	struct rtrs_clt_io_req *req;
> +
> +	if (WARN_ON(msg_id >= sess->queue_depth))
> +		return;
> +
> +	req = &sess->reqs[msg_id];
> +	/* Drop need_inv if server responsed with invalidation */
> +	req->need_inv &= !w_inval;
> +	complete_rdma_req(req, errno, true, false);
> +}

Please document the meaning of the "w_inval" argument. Please also fix
the spelling of "responsed".

Thanks,

Bart.