Re: [PATCH 2/3] nvme-core: move gencounter check into nvme_cid()

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

Sagi,

On 12/12/21 1:19 AM, Sagi Grimberg wrote:
> External email: Use caution opening links or attachments
> 
> 
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Fri, Dec 10, 2021 at 03:21:15AM -0800, Chaitanya Kulkarni wrote:
>>>> -     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