Received-SPF: pass (google.com: domain of linux-kernel+bounces-163614-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240430021753.385089-1-brookxu.cn@gmail.com> <8379e4a9-3114-4d35-9d1f-81ca61351b85@nvidia.com>
 <CADtkEechcseDatdzoPDrYBapvQ5iTKo3E-tsdiwj2B7euLLwnQ@mail.gmail.com> <c5495a06-1edb-4097-ab77-280955d14a56@nvidia.com>
In-Reply-To: <c5495a06-1edb-4097-ab77-280955d14a56@nvidia.com>
From: =?UTF-8?B?6K645pil5YWJ?= <brookxu.cn@gmail.com>
Date: Tue, 30 Apr 2024 16:59:00 +0800
Message-ID: <CADtkEefA_eLKTDOJo7GcoaYGhWXbyyA6WCTE-Z1aMpA7bpwRvg@mail.gmail.com>
Subject: Re: [PATCH] nvme-fabrics: use reserved tag for reg read/write command
To: Chaitanya Kulkarni <chaitanyak@nvidia.com>
Cc: "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>, "hch@lst.de" <hch@lst.de>, 
	"kbusch@kernel.org" <kbusch@kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "axboe@kernel.dk" <axboe@kernel.dk>, 
	"sagi@grimberg.me" <sagi@grimberg.me>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Chaitanya Kulkarni <chaitanyak@nvidia.com> =E4=BA=8E2024=E5=B9=B44=E6=9C=88=
30=E6=97=A5=E5=91=A8=E4=BA=8C 14:59=E5=86=99=E9=81=93=EF=BC=9A
>
> On 4/29/2024 10:10 PM, =E8=AE=B8=E6=98=A5=E5=85=89 wrote:
> > Chaitanya Kulkarni <chaitanyak@nvidia.com> =E4=BA=8E2024=E5=B9=B44=E6=
=9C=8830=E6=97=A5=E5=91=A8=E4=BA=8C 11:47=E5=86=99=E9=81=93=EF=BC=9A
> >>
> >> On 4/29/2024 7:17 PM, brookxu.cn wrote:
> >>> From: Chunguang Xu <chunguang.xu@shopee.com>
> >>>
> >>> 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 finis=
h,
> >>> reconnect routine may fail to update nvme regs due to insufficient ta=
gs,
> >>> which will cause kernel hang forever. In order to workaround this iss=
ue,
> >>> 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 t=
o
> >> 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 comman=
d:
> > 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 =3D 64;
> > std::set<pid_t> chlds;
> >
> > int __exit =3D 0;
> > void  sigint_proc(int signo)
> > {
> >          __exit =3D 1;
> > }
> >
> > int main(int argc, char **argv)
> > {
> >          signal(SIGINT, sigint_proc);
> >
> >          for (auto i  =3D 0; i < concurrency; i++) {
> >                  auto pid =3D 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 =3D 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
>