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

[许春光]

Chaitanya Kulkarni <chaitanyak@xxxxxxxxxx> 于2024年4月30日周二 14:59写道：
>
> 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 ...
>

My pleasure, after reviewed, if no more doubt, I will try to do it, thanks

> -ck
>
> [1] https://github.com/osandov/blktests
>