Re: [PATCH] nvme-fabrics: use reserved tag for reg read/write command

[Chaitanya Kulkarni]

On 4/29/2024 10:10 PM, 许春光 wrote:
> Chaitanya Kulkarni <chaitanyak@xxxxxxxxxx> 于2024年4月30日周二 11:47写道：
>>
>> On 4/29/2024 7:17 PM, brookxu.cn wrote:
>>> From: Chunguang Xu <chunguang.xu@xxxxxxxxxx>
>>>
>>> In some scenarios, if too many commands are issued by nvme command in
>>> the same time by user tasks, this may exhaust all tags of admin_q. If
>>> a reset (nvme reset or IO timeout) occurs before these commands finish,
>>> reconnect routine may fail to update nvme regs due to insufficient tags,
>>> which will cause kernel hang forever. In order to workaround this issue,
>>> maybe we can let reg_read32()/reg_read64()/reg_write32() use reserved
>>> tags. This maybe safe for nvmf:
>>>
>>> 1. For the disable ctrl path,  we will not issue connect command
>>> 2. For the enable ctrl / fw activate path, since connect and reg_xx()
>>>      are called serially.
>>>
>>
>> Given the complexity of the scenario described above, is it possible to
>> write a script for this scenario that will trigger this and submit to
>> blktest ? not that this is a blocker to get this patch reviewed, but
>> believe it is needed in long run, WDYT ?
> 
> Thanks for you reply, I can easily reproduce it in my VMs by following steps:
> STEP 1. In order to reduce the complexity of reproduction, I reduce
> NVME_AQ_MQ_TAG_DEPTH from 31 to 8
> 
> STEP 2. Create a nvme device by NVMe over tcp, such as following command:
> nvme connect -t tcp -a 192.168.122.20 -s 4420 -n
> nqn.2014-08.org.nvmexpress.mytest
> 
> STEP 3. Buind and run the c++ program issues nvme commands as followed:
> #include <sys/types.h>
> #include <signal.h>
> #include <unistd.h>
> #include <vector>
> #include <set>
> #include <stdlib.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> 
> const int concurrency = 64;
> std::set<pid_t> chlds;
> 
> int __exit = 0;
> void  sigint_proc(int signo)
> {
>          __exit = 1;
> }
> 
> int main(int argc, char **argv)
> {
>          signal(SIGINT, sigint_proc);
> 
>          for (auto i  = 0; i < concurrency; i++) {
>                  auto pid = fork();
>                  if (!pid) {
>                          while (true) {
>                                  system("nvme list -o json 2>&1 > /dev/null");
>                          }
>                  }
> 
>                  chlds.insert(pid);
>          }
> 
>          while (!__exit) {
>                  if (chlds.empty())
>                          break;
> 
>                  for (auto pid : chlds) {
>                          int wstatus, ret;
>                          ret = waitpid(pid, &wstatus, WNOWAIT);
>                          if (ret > 0) {
>                                  chlds.erase(pid);
>                                  break;
>                          }
>                  }
>                  usleep(1000);
>          }
> 
>          // exit
>          for (auto pid : chlds)
>                  kill(pid, SIGKILL);
> 
>          return 0;
> }
> 
> STEP 4. Open a new console, running the followed command:
> while [ true ]; do nvme reset /dev/nvme0; sleep `echo "$RANDOM%1" | bc`; done
> 
> We will reproduce this issue soon.
>>

cool, can you please submit a blktest [1] for this ? I'm not sure if we 
have any coverage for this scenario ...

-ck

[1] https://github.com/osandov/blktests