Messages in this thread | | | Date | Mon, 17 May 2021 16:58:05 +0200 | From | Daniel Wagner <> | Subject | Re: [PATCH v2] nvme-tcp: Check if request has started before processing it |
| |
Hi Sagi,
On Fri, May 07, 2021 at 04:22:30PM -0700, Sagi Grimberg wrote: > Yea, maybe something like this?
I did give this a spin with blktests (loopback device) and on real hardware (FC). Seems to do work fine. Just two things.
> +/* > + * nvme command_id is constructed as such: > + * | xxxx | xxxxxxxxxxxx | > + * gen request tag > + */ > +#define nvme_cid_install_genctr(gen) ((gen & 0xf) << 12) > +#define nvme_genctr_from_cid(cid) ((cid & 0xf000) >> 12) > +#define nvme_tag_from_cid(cid) (cid & 0xfff) > + > +static inline u16 nvme_cid(struct request *rq) > +{ > + return nvme_cid_install_genctr(nvme_req(rq)->genctr++) | rq->tag; > +}
- return nvme_cid_install_genctr(nvme_req(rq)->genctr++) | rq->tag; + nvme_req(rq)->genctr = ++nvme_req(rq)->genctr & 0xf; + return nvme_cid_install_genctr(nvme_req(rq)->genctr) | rq->tag;
The first issue, it really needs prefix increment if you want to write it in one line. And it should store only the first 4 bits. nvme_find_rq() would complain with 0x0 != 0x10 after the first overflow.
> +static inline struct request *nvme_find_rq(struct blk_mq_tags *tags, > + u16 command_id) > +{ > + u8 genctr = nvme_genctr_from_cid(command_id); > + u16 tag = nvme_tag_from_cid(command_id); > + struct request *rq; > + > + rq = blk_mq_tag_to_rq(tags, tag); > + if (unlikely(!rq)) { > + pr_err("could not locate request for tag %#x\n", > + tag); > + return NULL; > + } > + if (unlikely(nvme_req(rq)->genctr != genctr)) { > + dev_err(nvme_req(rq)->ctrl->device, > + "request %#x genctr mismatch (got %#x expected > %#x)\n", > + tag, nvme_req(rq)->genctr, genctr);
- tag, nvme_req(rq)->genctr, genctr); + tag, genctr, nvme_req(rq)->genctr);
The arguments are in the wrong order. Got me a bit confused.
Are you going to send out a proper patch? I'd like to move things forward and could offer to do some more testing if needed.
Thanks, Daniel
| |