DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 26E192145D
MIME-Version: 1.0
References: <20181119153707.10832.42881.stgit@manet.1015granger.net> <20181119154607.10832.92558.stgit@manet.1015granger.net>
In-Reply-To: <20181119154607.10832.92558.stgit@manet.1015granger.net>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Mon, 19 Nov 2018 12:47:23 -0500
Message-ID: <CAN-5tyG26W+TjTsjpTVvF3RJOGCnD8L79dCbRqKTm5F1V3fuxw@mail.gmail.com>
Subject: Re: [PATCH v1 4/4] xprtrdma: Plant XID in on-the-wire RDMA offset (FRWR)
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     linux-rdma <linux-rdma@vger.kernel.org>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Mon, Nov 19, 2018 at 10:46 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> Place the associated RPC transaction's XID in the upper 32 bits of
> each RDMA segment's rdma_offset field. These bits are currently
> always zero.
>
> There are two reasons to do this:
>
> - The R_key only has 8 bits that are different from registration to
>   registration. The XID adds more uniqueness to each RDMA segment to
>   reduce the likelihood of a software bug on the server reading from
>   or writing into memory it's not supposed to.
>
> - On-the-wire RDMA Read and Write operations do not otherwise carry
>   any identifier that matches them up to an RPC. The XID in the
>   upper 32 bits will act as an eye-catcher in network captures.

Is this just an "eye-catcher" or do you have plans to use it in
wireshark? If the latter, then can we really do that? while a linux
implementation may do that, other (or even possibly future linux)
implementation might not do this. Can we justify changing the
wireshark logic for it?

> Suggested-by: Tom Talpey <ttalpey@microsoft.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/xprtrdma/frwr_ops.c  |    3 ++-
>  net/sunrpc/xprtrdma/rpc_rdma.c  |    6 +++---
>  net/sunrpc/xprtrdma/xprt_rdma.h |    2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c
> index 49b314d..3b260d2 100644
> --- a/net/sunrpc/xprtrdma/frwr_ops.c
> +++ b/net/sunrpc/xprtrdma/frwr_ops.c
> @@ -344,7 +344,7 @@
>   */
>  static struct rpcrdma_mr_seg *
>  frwr_op_map(struct rpcrdma_xprt *r_xprt, struct rpcrdma_mr_seg *seg,
> -           int nsegs, bool writing, struct rpcrdma_mr **out)
> +           int nsegs, bool writing, u32 xid, struct rpcrdma_mr **out)
>  {
>         struct rpcrdma_ia *ia = &r_xprt->rx_ia;
>         bool holes_ok = ia->ri_mrtype == IB_MR_TYPE_SG_GAPS;
> @@ -398,6 +398,7 @@
>         if (unlikely(n != mr->mr_nents))
>                 goto out_mapmr_err;
>
> +       ibmr->iova |= ((u64)cpu_to_be32(xid)) << 32;
>         key = (u8)(ibmr->rkey & 0x000000FF);
>         ib_update_fast_reg_key(ibmr, ++key);
>
> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
> index 9f53e02..89a2db2 100644
> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
> @@ -357,7 +357,7 @@ static bool rpcrdma_results_inline(struct rpcrdma_xprt *r_xprt,
>
>         do {
>                 seg = r_xprt->rx_ia.ri_ops->ro_map(r_xprt, seg, nsegs,
> -                                                  false, &mr);
> +                                                  false, rqst->rq_xid, &mr);
>                 if (IS_ERR(seg))
>                         return PTR_ERR(seg);
>                 rpcrdma_mr_push(mr, &req->rl_registered);
> @@ -415,7 +415,7 @@ static bool rpcrdma_results_inline(struct rpcrdma_xprt *r_xprt,
>         nchunks = 0;
>         do {
>                 seg = r_xprt->rx_ia.ri_ops->ro_map(r_xprt, seg, nsegs,
> -                                                  true, &mr);
> +                                                  true, rqst->rq_xid, &mr);
>                 if (IS_ERR(seg))
>                         return PTR_ERR(seg);
>                 rpcrdma_mr_push(mr, &req->rl_registered);
> @@ -473,7 +473,7 @@ static bool rpcrdma_results_inline(struct rpcrdma_xprt *r_xprt,
>         nchunks = 0;
>         do {
>                 seg = r_xprt->rx_ia.ri_ops->ro_map(r_xprt, seg, nsegs,
> -                                                  true, &mr);
> +                                                  true, rqst->rq_xid, &mr);
>                 if (IS_ERR(seg))
>                         return PTR_ERR(seg);
>                 rpcrdma_mr_push(mr, &req->rl_registered);
> diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h
> index a13ccb6..2ae1ee2 100644
> --- a/net/sunrpc/xprtrdma/xprt_rdma.h
> +++ b/net/sunrpc/xprtrdma/xprt_rdma.h
> @@ -472,7 +472,7 @@ struct rpcrdma_memreg_ops {
>         struct rpcrdma_mr_seg *
>                         (*ro_map)(struct rpcrdma_xprt *,
>                                   struct rpcrdma_mr_seg *, int, bool,
> -                                 struct rpcrdma_mr **);
> +                                 u32, struct rpcrdma_mr **);
>         int             (*ro_send)(struct rpcrdma_ia *ia,
>                                    struct rpcrdma_req *req);
>         void            (*ro_reminv)(struct rpcrdma_rep *rep,
>