On Tue, Apr 12, 2016 at 04:52:34PM -0700, Bart Van Assche wrote:
> Please clarify the comment above this function. The way that comment is
> written seems to contradict the code for iWARP writes.
Thanks, fixed.
>> + count++;
>> + } else {
>> + reg->inv_wr.next = NULL;
>> + }
>> +
>> + ret = ib_map_mr_sg(reg->mr, sg, nents, offset, PAGE_SIZE);
>> + if (ret < nents) {
>> + ib_mr_pool_put(qp, &qp->rdma_mrs, reg->mr);
>> + return -EINVAL;
>> + }
>
> The above code assumes that the length of each sg list element is lower
> than or equal to mr->page_size. I think this is something that should be
> documented since the block layer has to be configured explicitly to ensure
> this.
It shouldn't assume that - ib_map_mr_sg just uses PAGE_SIZE as
the MR granularuty. Note that the block layer isn't involved for any of
the users of this function - it's used on the target sides of iSER, SRP,
and (out of tree for a few more weeks) NVMe over fabrics. All of them
only provide 4k segments so while I think this code should handle larger
segments, there is no way to verify that until we have consumers that
provide larger segments. I think SCST had some allocator that could
use larger pages, and Pure Storage mention they had LIO changes to
use large pages as well, so if this comes up I think we should be
able to support it without too much effort.
>
>> + u32 pages_per_mr = rdma_rw_fr_page_list_len(qp->pd->device);
>> + int i, j, ret = 0, count = 0;
>> +
>> + ctx->nr_ops = (sg_cnt + pages_per_mr - 1) / pages_per_mr;
>> + ctx->reg = kcalloc(ctx->nr_ops, sizeof(*ctx->reg), GFP_KERNEL);
>> + if (!ctx->reg) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + for (i = 0; i < ctx->nr_ops; i++) {
>> + struct rdma_rw_reg_ctx *prev = i ? &ctx->reg[i - 1] : NULL;
>> + struct rdma_rw_reg_ctx *reg = &ctx->reg[i];
>> + u32 nents = min(sg_cnt, pages_per_mr);
>
> The same min(sg_cnt, pages_per_mr) computation occurs here and in
> rdma_rw_init_one_mr(). Is there a way to avoid that duplication?
It's just a single min statement, so coming up with an inline function
to wrap seems a little too much overhead to me. If you have a better
suggestion I'd be happy to look into it.
>> +#define RDMA_RW_SINGLE_WR 0
>> +#define RDMA_RW_MULTI_WR 1
>> +#define RDMA_RW_MR 2
>
> The above constants are only used in the rw.c source file. Do we need these
> constants in the header file or can these be moved into source file rw.c?
I have moved them, and converted the constants to an enum while I was at
it.
Thanks for the review!
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
