Re: [PATCH 4/9] nvme-pci: refactor nvme_pci_use_sgls

[Keith Busch <kbusch@kernel.org>]

On Tue, Jun 10, 2025 at 07:06:42AM +0200, Christoph Hellwig wrote:
>  static inline bool nvme_pci_metadata_use_sgls(struct request *req)
>  {
>  	return req->nr_integrity_segments > 1 ||
>  		nvme_req(req)->flags & NVME_REQ_USERCMD;
>  }
>  
> -static inline bool nvme_pci_use_sgls(struct nvme_dev *dev, struct request *req,
> -				     int nseg)
> +static inline enum nvme_use_sgl nvme_pci_use_sgls(struct nvme_dev *dev,
> +		struct request *req)
>  {
>  	struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
> -	unsigned int avg_seg_size;
>  
> -	avg_seg_size = DIV_ROUND_UP(blk_rq_payload_bytes(req), nseg);
> +	if (nvmeq->qid && nvme_ctrl_sgl_supported(&dev->ctrl)) {
> +		if (nvme_req(req)->flags & NVME_REQ_USERCMD)
> +			return SGL_FORCED;
> +		if (nvme_pci_metadata_use_sgls(req))
> +			return SGL_FORCED;

nvme_pci_metadata_use_sgls() already handles checking for
NVME_REQ_USERCMD flagged commands, so I don't think you need both of
these conditions to return FORCED.

> @@ -886,7 +897,9 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
>  		goto out_free_sg;
>  	}
>  
> -	if (nvme_pci_use_sgls(dev, req, iod->sgt.nents))
> +	if (use_sgl == SGL_FORCED ||
> +	    (use_sgl == SGL_SUPPORTED &&
> +	     (!sgl_threshold || nvme_pci_avg_seg_size(req) < sgl_threshold)))

This looks backwards for deciding to use sgls in the non-forced case.
Shouldn't it be:

	     (sgl_threshold && nvme_pci_avg_seg_size(req) >= sgl_threshold)))

?

>  		ret = nvme_pci_setup_sgls(nvmeq, req, &cmnd->rw);
>  	else
>  		ret = nvme_pci_setup_prps(nvmeq, req, &cmnd->rw);