On Thu, Apr 23, 2020 at 12:22:27PM +0300, Max Gurtovoy wrote:
>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>> index e38f8f7..23cc77e 100644
>>> --- a/drivers/nvme/host/rdma.c
>>> +++ b/drivers/nvme/host/rdma.c
>>> @@ -67,6 +67,9 @@ struct nvme_rdma_request {
>>> struct ib_cqe reg_cqe;
>>> struct nvme_rdma_queue *queue;
>>> struct nvme_rdma_sgl data_sgl;
>>> + /* Metadata (T10-PI) support */
>>> + struct nvme_rdma_sgl *md_sgl;
>>> + bool use_md;
>> Do we need a use_md flag vs just using md_sgl as a boolean and/or
>> using blk_integrity_rq?
>
> md_sgl will be used if we'll get a blk request with blk_integrity (memory
> domain).
>
> use_md will be responsible for wire domain.
>
> so instead of this bool we can check in any place (after prev commit
> changes):
>
> "
>
> if (queue->pi_support && nvme_ns_has_pi(ns))
> req->use_md = c.common.opcode == nvme_cmd_write ||
> c.common.opcode == nvme_cmd_read;
>
> "
>
> And this is less readable IMO.
It would obviously have to go into a little helper, but I really hate
adding lots of little fields caching easily derived information. There
are a few exception, for example if we really need to not touch too
many cache lines. Do you have a git tree with your current code? That
might be a little easier to follow than the various patches, maybe
I can think of something better.
>>> + if (blk_integrity_rq(rq)) {
>>> + memset(req->md_sgl, 0, sizeof(struct nvme_rdma_sgl));
>> Why do we need this memset?
>
> just good practice we took from drivers/scsi/scsi_lib.c.
>
> It's not a must and I can remove it if needed and test it.
I think (and please double check) that we initialize all three fields
anyway, so the memset should not be needed.
