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);
