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

[Jinpu Wang <jinpu.wang@cloud.ionos.com>]

On Tue, Dec 31, 2019 at 12:53 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2019-12-30 02:29, Jack Wang wrote:
> > + * InfiniBand Transport Layer
>
> InfiniBand or RDMA?
will fix.
>
> > +MODULE_DESCRIPTION("RTRS Client");
>
> Please spell out RTRS in full.
ok.

>
> > +static const struct rtrs_ib_dev_pool_ops dev_pool_ops;
>
> Can this forward declaration be avoided?
I don't see how to do it easily.

>
> > +static struct rtrs_ib_dev_pool dev_pool = {
> > +     .ops = &dev_pool_ops
> > +};
>
> Can this structure be declared 'const'?
No, it's not const, we also initialize it in rtrs_ib_dev_pool_init
>
> > +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?
will check.
>
> > +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.
We don't use RDMA READ, it's requested RDMA read meaning, server side will do
RDMA write to the buffers.
>
> What does "server forgot to send INV" mean?
Means server side malfunctional/server panic/etc, server didnot sent
SEND_WITH_INV WR,
so client have to do local invalidate.
>
> 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.
OK, thanks