Sagi,

On 12/12/21 1:19 AM, Sagi Grimberg wrote:

External email: Use caution opening links or attachments

quoted

quoted

External email: Use caution opening links or attachments


On Fri, Dec 10, 2021 at 03:21:15AM -0800, Chaitanya Kulkarni wrote:

quoted

-     if (!(ctrl->quirks & NVME_QUIRK_SKIP_CID_GEN))
-             nvme_req(req)->genctr++;
        cmd->common.command_id = nvme_cid(req);
        trace_nvme_setup_cmd(req, cmd);
        return ret;

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 98d7627cfdce..2be0191e1a1f 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h

@@ -511,6 +511,8 @@ struct nvme_ctrl_ops {

   static inline u16 nvme_cid(struct request *rq)
   {
+     if (!(nvme_req(rq)->ctrl->quirks & NVME_QUIRK_SKIP_CID_GEN))
+             nvme_req(rq)->genctr++;
        return nvme_cid_install_genctr(nvme_req(rq)->genctr) | rq->tag;
   }

No, this will incrememnt the counter every time someone queries the
command id for this request, which happens in multiple places. The
counter can't be modified for the lifetime of the request.

Yes, the right thing to do here is to create a new stub for
CONFIG_NVME_DEBUG_USE_GENCTR and !CONFIG_NVME_DEBUG_USE_GENCTR
case and move the quick check in there, will send out V2.

Don't understand the global config option at all, distributions
will probably enable it anyways (as the default needs to be set
to Y anyways). However, this set was obviously not tested with nvme-tcp
because it breaks it completely. And it is also in general a bad
practice IMO to increment the genctr on every install. It needs to
be in a different helper.

Please make sure to test nvme-tcp on your v2.

Yes the nvme_cid() chanege is wrong, let me send a V2 tested on the
tcp and correct mentioned helper.

-ck