Hi Chuck,
On 06/20/2016 12:08 PM, Chuck Lever wrote:
> ib_unmap_fmr() takes a list of FMRs to unmap. However, it does not
> remove the FMRs from this list as it processes them. Other
> ib_unmap_fmr() call sites are careful to remove FMRs from the list
> after ib_unmap_fmr() returns.
>
> Since commit 7c7a5390dc6c8 ("xprtrdma: Add ro_unmap_sync method for FMR")
> fmr_op_unmap_sync passes more than one FMR to ib_unmap_fmr(), but
> it didn't bother to remove the FMRs from that list once the call was
> complete.
>
> I've noticed some instability that could be related to list
> tangling by the new fmr_op_unmap_sync() logic. In an abundance
> of caution, add some defensive logic to clean up properly after
> ib_unmap_fmr().
>
> Fixes: 7c7a5390dc6c8 ("xprtrdma: Add ro_unmap_sync method for FMR")
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> net/sunrpc/xprtrdma/fmr_ops.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/fmr_ops.c b/net/sunrpc/xprtrdma/fmr_ops.c
> index 6326ebe..958c792 100644
> --- a/net/sunrpc/xprtrdma/fmr_ops.c
> +++ b/net/sunrpc/xprtrdma/fmr_ops.c
> @@ -63,9 +63,12 @@ static int
> __fmr_unmap(struct rpcrdma_mw *mw)
> {
> LIST_HEAD(l);
> + int rc;
>
> list_add(&mw->fmr.fmr->list, &l);
> - return ib_unmap_fmr(&l);
> + rc = ib_unmap_fmr(&l);
> + list_del_init(&mw->fmr.fmr->list);
> + return rc;
> }
>
> /* Deferred reset of a single FMR. Generate a fresh rkey by
> @@ -149,6 +152,7 @@ fmr_op_init(struct rpcrdma_xprt *r_xprt)
> r->fmr.fmr = ib_alloc_fmr(pd, mr_access_flags, &fmr_attr);
> if (IS_ERR(r->fmr.fmr))
> goto out_fmr_err;
> + INIT_LIST_HEAD(&r->fmr.fmr->list);
I'm a little surprised to see that ib_alloc_fmr() isn't initializing this list. Do you know of any plans to add the INIT_LIST_HEAD() into the infiniband code, that way it's always initialized when an ib_fmr is allocated?
Anna
>
> r->mw_xprt = r_xprt;
> list_add(&r->mw_list, &buf->rb_mws);
> @@ -267,7 +271,7 @@ fmr_op_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
> seg = &req->rl_segments[i];
> mw = seg->rl_mw;
>
> - list_add(&mw->fmr.fmr->list, &unmap_list);
> + list_add_tail(&mw->fmr.fmr->list, &unmap_list);
>
> i += seg->mr_nsegs;
> }
> @@ -280,7 +284,9 @@ fmr_op_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
> */
> for (i = 0, nchunks = req->rl_nchunks; nchunks; nchunks--) {
> seg = &req->rl_segments[i];
> + mw = seg->rl_mw;
>
> + list_del_init(&mw->fmr.fmr->list);
> __fmr_dma_unmap(r_xprt, seg);
> rpcrdma_put_mw(r_xprt, seg->rl_mw);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
