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.
