Re: [PATCH 07/16] nvme-rdma: Add metadata/T10-PI support

[James Smart <james.smart@broadcom.com>]

On 12/2/2019 6:48 AM, Max Gurtovoy wrote:
> @@ -1215,34 +1283,115 @@ static int nvme_rdma_map_sg_single(struct nvme_rdma_queue *queue,
>   	return 0;
>   }
>   
> -static int nvme_rdma_map_sg_fr(struct nvme_rdma_queue *queue,
> -		struct nvme_rdma_request *req, struct nvme_command *c,
> -		int count)
> +#ifdef CONFIG_BLK_DEV_INTEGRITY
> +static void nvme_rdma_set_diff_domain(struct nvme_command *cmd, struct bio *bio,
> +		struct ib_sig_domain *domain, struct request *rq)
>   {
> -	struct nvme_keyed_sgl_desc *sg = &c->common.dptr.ksgl;
> -	int nr;
> +	struct blk_integrity *bi = blk_get_integrity(bio->bi_disk);
> +	struct nvme_ns *ns = rq->rq_disk->private_data;
> +
> +	WARN_ON(bi == NULL);
>   
> -	req->mr = ib_mr_pool_get(queue->qp, &queue->qp->rdma_mrs);
> -	if (WARN_ON_ONCE(!req->mr))
> -		return -EAGAIN;
> +	domain->sig_type = IB_SIG_TYPE_T10_DIF;
> +	domain->sig.dif.bg_type = IB_T10DIF_CRC;
> +	domain->sig.dif.pi_interval = 1 << bi->interval_exp;
> +	domain->sig.dif.ref_tag = le32_to_cpu(cmd->rw.reftag);
>   
>   	/*
> -	 * Align the MR to a 4K page size to match the ctrl page size and
> -	 * the block virtual boundary.
> +	 * At the moment we hard code those, but in the future
> +	 * we will take them from cmd.
>   	 */
> -	nr = ib_map_mr_sg(req->mr, req->data_sgl.sg_table.sgl, count, NULL,
> -			  SZ_4K);
> -	if (unlikely(nr < count)) {
> -		ib_mr_pool_put(queue->qp, &queue->qp->rdma_mrs, req->mr);
> -		req->mr = NULL;
> -		if (nr < 0)
> -			return nr;
> -		return -EINVAL;
> +	domain->sig.dif.apptag_check_mask = 0xffff;
> +	domain->sig.dif.app_escape = true;
> +	domain->sig.dif.ref_escape = true;
> +	if (ns->pi_type != NVME_NS_DPS_PI_TYPE3)
> +		domain->sig.dif.ref_remap = true;
> +}
> +
>

On a per-io basis, there needs to be specific descriptions of the DIF 
information to program the port hardware.  Things such as block size, 
type, and so. I see this routine using a mix of the bio that is 
associated with the original request as well as the namespace pointer to 
get this info.    To me the reaching into the bio, as well as the 
locating of the ns structures are reaching into the other layers too much.

Wouldn't we be better off with with the core layer doing all the 
reaching and setting up a pi structure in the nvme_request with this 
information ?   replace has_pi with this pi struct and 
"nvme_req(rq)->pi.pi_type == 0" is equivalent to has_pi ?   If we didn't 
want to replicate the PI info, then nvme_request can simply add a 
pointer to the ns, and the ns can be looked at explicitly to gather the 
attributes.

Thoughts ?

-- james


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme