Duplicate tag error with 5.2

[benh@kernel.crashing.org (Benjamin Herrenschmidt)]

On Fri, 2019-07-19@00:39 +0000, Damien Le Moal wrote:
> On 2019/07/18 16:40, Benjamin Herrenschmidt wrote:
> > On Thu, 2019-07-18@07:13 +0000, Damien Le Moal wrote:
> > > > I can trigger the problem easily now running smartctl -c /dev/nvme0n1
> > > > and doing a bit of IOs. It seems to happen when the IO and Admin queue
> > > > use the same tag.
> > > 
> > > So isn't it that you are getting a completion cqe for an admin queue command in
> > > an IO completion queue ? Or the reverse ? Given how weird this NVMe device seems
> > > to be, it may be a possibility. In addition to the command ID (tag), if you
> > > print the cqe queue ID (le16_to_cpu(cqe->sq_id)), what do you see ?
> > 
> > Ah I can add code to validate that it's coming into the right queue,
> > good idea.
> 
> If the completion really shows up into the wrong queue, a fix may be simply to
> hack this code in nvme_handle_cqe():
> 
> 	req = blk_mq_tag_to_rq(*nvmeq->tags, cqe->command_id);
> 	trace_nvme_sq(req, cqe->sq_head, nvmeq->sq_tail);
> 	nvme_end_request(req, cqe->status, cqe->result);
> 
> to use a different nvmeq pointer, that is the one that corresponds to cqe->sq_id
> queue used for submission, which would lead to the correct tagset to be
> referenced and suppress the false "duplicate" tag issue. Locking of queues/hctx
> may need careful checking with such a change though.

But if the completion arrived in the wrong queue, wouldn't we be
missing the completions for admin requests and thus having all sort of
issues ?

Things are now solid with those two changes I've done locally, I'll
send a tentative patche:

 - Offset all tags in the IO queue with 32 so they don't overlap (I do
it at the point of writing into the submission queue, and undo it when
processing the CQs).

 - Reduce the IO queue depth by 32

Without the latter, I occasionally (but more rarely) still got the
error, but always with tags > 127. I suspect that not only Apple
implementation actually *uses* the tags we specify for their own
internal tracking, but they also only support values 0...127.

With those two changes it's been solid. Thankfully the resulting quirk
is reasonably simple and self contained to pci.c. I'll clean things up
and post.

Cheers,
Ben.