Re: [PATCH 0/4] nvme patches for 6.3

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Fri, 2023-02-10 at 08:37 -0700, Keith Busch wrote:
> On Fri, Feb 10, 2023 at 08:24:46AM -0700, Keith Busch wrote:
> > On Fri, Feb 10, 2023 at 03:34:09PM +0100, Niklas Schnelle wrote:
> > > Hi Christoph, Hi Keith,
> > > 
> > > It looks like this series causes crashes on s390x.
> > > With current linux-next-20230210 and a Samsung PM173X I get the
> > > below[0] crash. Reverting patches 1-3 makes the NVMe work again. I
> > > tried reverting just patch 3 and 2/3 but this results in crashes as
> > > well and as far as I can see patches 2/3 depend on each other. Not
> > > entirely sure what's going on but patch 3 mentions padding to the cache
> > > line size and our 256 byte cache lines are definitely unusual. I didn't
> > > see any obvious place where this would break things though. I did debug
> > > that in the crashing nvme_unmap_data() iod->nr_allocations is -1 and
> > > iod->use_sgl is true which is weird since it looks to me like iod-
> > > > nr_allocations should only be -1 if sg_list couldn't be allocated from
> > > the pool.
> > 
> > Thanks for the notice.
> > 
> > I think the driver must have received a request with multiple physical
> > segments, but the dma mapping collapsed it to 1. In that case, the driver won't
> > use the "simple" sgl, but we don't allocate a descriptor either, so we need to
> > account for that. I'll send a fix shortly.
> 
> This should fix it:
> 
> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index a331fbfa9a667..6e8fcbf9306d2 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -553,14 +553,16 @@ static void nvme_unmap_data(struct nvme_dev *dev, struct request *req)
>  
>  	dma_unmap_sgtable(dev->dev, &iod->sgt, rq_dma_dir(req), 0);
>  
> -	if (iod->nr_allocations == 0)
> +	if (iod->nr_allocations == 0) {
>  		dma_pool_free(dev->prp_small_pool, iod->list[0].sg_list,
>  			      iod->first_dma);
> -	else if (iod->use_sgl)
> -		dma_pool_free(dev->prp_page_pool, iod->list[0].sg_list,
> -			      iod->first_dma);
> -	else
> -		nvme_free_prps(dev, req);
> +	} else if (iod->nr_allocations > 0) {
> +		if (iod->use_sgl)
> +			dma_pool_free(dev->prp_page_pool, iod->list[0].sg_list,
> +				      iod->first_dma);
> +		else
> +			nvme_free_prps(dev, req);
> +	}
>  	mempool_free(iod->sgt.sgl, dev->iod_mempool);
>  }
>  
> --

Thanks, yes that seems to have been it and with the above patch applied
on top of linux-next-20230210 the crash disappears and my standard fio
script runs fine too.

So if I understand things correctly possibly thanks to "nvme-pci: use
mapped entries for sgl decision" a collapsed DMA mapping lands us in
the entries == 1 case in nvme_pci_setup_sgls() so no extra mapping is
needed and thus the dma_unmap_sgtable() is the only unmapping we need
in nvme_unmap_data()? Does that mean that previously this case might
have used prps and thus now needs fewer mapping operations?

Either way, feel free to add my:

Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>