Re: [PATCH 01/18] nvme: add missing unmaps in nvme_queue_rq

["Busch, Keith" <keith.busch@intel.com>]

On Tue, Oct 20, 2015 at 01:04:24PM +0300, Sagi Grimberg wrote:
> On 10/16/2015 8:58 AM, Christoph Hellwig wrote:
> >  			sg_init_table(iod->meta_sg, 1);
> >  			if (blk_rq_map_integrity_sg(
> >-					req->q, req->bio, iod->meta_sg) != 1)
> >+					req->q, req->bio, iod->meta_sg) != 1) {
> >+				dma_unmap_sg(dev->dev, iod->sg, iod->nents,
> >+						dma_dir);
> >  				goto error_cmd;
> >+			}
> 
> This is not related to the patch itself. But this condition seems bogus
> to me. We passed meta_sg that consists of a single entry. If we
> happened to map more than a single entry we're already in trouble as we
> overrun meta_sg (modified the iod->sg pointer).
> 
> I think a WARN_ON_ONCE statement is more suitable here (which should
> probably come as a separate patch).

We are in trouble if it maps more than 1, but I think the condition
here is intended to guard against 0 rather than > 1. We should already
be ensured it won't be > 1 from a previous check.

Based on the implementation of blk_rq_map_integrity_sg and the functions
earlier setup, I don't think we can ever see 0 here either.