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

Bart Van Assche
2019-12-30T23:53:14Z

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.