From: Ming Lei <ming.lei@redhat.com> To: Dongli Zhang <dongli.zhang@oracle.com> Cc: Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org, linux-nvme@lists.infradead.org, Christoph Hellwig <hch@lst.de>, Alan Adamson <alan.adamson@oracle.com>, Sagi Grimberg <sagi@grimberg.me>, Keith Busch <kbusch@kernel.org>, Max Gurtovoy <maxg@mellanox.com> Subject: Re: [PATCH 3/3] nvme-pci: make nvme reset more reliable Date: Tue, 26 May 2020 15:12:49 +0800 [thread overview] Message-ID: <20200526071249.GA874504@T590> (raw) In-Reply-To: <9c5ac1e0-b5ca-0f54-5ee3-fd630dbdb8d4@oracle.com> On Mon, May 25, 2020 at 10:01:18PM -0700, Dongli Zhang wrote: > > > On 5/20/20 4:56 AM, Ming Lei wrote: > > During waiting for in-flight IO completion in reset handler, timeout > > Does this indicate the window since nvme_start_queues() in nvme_reset_work(), > that is, just after the queues are unquiesced again? Right, nvme_start_queues() starts to dispatch requests again, and nvme_wait_freeze() waits completion of all these in-flight IOs. > > If v2 is required in the future, how about to mention the specific function to > that it would be much more easier to track the issue. Not sure it is needed, cause it is quite straightforward. > > > or controller failure still may happen, then the controller is deleted > > and all inflight IOs are failed. This way is too violent. > > > > Improve the reset handling by replacing nvme_wait_freeze with query > > & check controller. If all ns queues are frozen, the controller is reset > > successfully, otherwise check and see if the controller has been disabled. > > If yes, break from the current recovery and schedule a fresh new reset. > > > > This way avoids to failing IO & removing controller unnecessarily. > > > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Sagi Grimberg <sagi@grimberg.me> > > Cc: Keith Busch <kbusch@kernel.org> > > Cc: Max Gurtovoy <maxg@mellanox.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/nvme/host/pci.c | 37 ++++++++++++++++++++++++++++++------- > > 1 file changed, 30 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > > index ce0d1e79467a..b5aeed33a634 100644 > > --- a/drivers/nvme/host/pci.c > > +++ b/drivers/nvme/host/pci.c > > @@ -24,6 +24,7 @@ > > #include <linux/io-64-nonatomic-lo-hi.h> > > #include <linux/sed-opal.h> > > #include <linux/pci-p2pdma.h> > > +#include <linux/delay.h> > > > > #include "trace.h" > > #include "nvme.h" > > @@ -1235,9 +1236,6 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved) > > * shutdown, so we return BLK_EH_DONE. > > */ > > switch (dev->ctrl.state) { > > - case NVME_CTRL_CONNECTING: > > - nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING); > > - /* fall through */ > > case NVME_CTRL_DELETING: > > dev_warn_ratelimited(dev->ctrl.device, > > "I/O %d QID %d timeout, disable controller\n", > > @@ -2393,7 +2391,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) > > u32 csts = readl(dev->bar + NVME_REG_CSTS); > > > > if (dev->ctrl.state == NVME_CTRL_LIVE || > > - dev->ctrl.state == NVME_CTRL_RESETTING) { > > + dev->ctrl.state == NVME_CTRL_RESETTING || > > + dev->ctrl.state == NVME_CTRL_CONNECTING) { > > freeze = true; > > nvme_start_freeze(&dev->ctrl); > > } > > @@ -2504,12 +2503,29 @@ static void nvme_remove_dead_ctrl(struct nvme_dev *dev) > > nvme_put_ctrl(&dev->ctrl); > > } > > > > +static bool nvme_wait_freeze_and_check(struct nvme_dev *dev) > > +{ > > + bool frozen; > > + > > + while (true) { > > + frozen = nvme_frozen(&dev->ctrl); > > + if (frozen) > > + break; > > ... and how about to comment that the below is because of nvme timeout handler > as explained in another email (if v2 would be sent) so that it is not required > to query for "online_queues" with cscope :) > > > + if (!dev->online_queues) > > + break; > > + msleep(5); Fine. Thanks, Ming
WARNING: multiple messages have this Message-ID (diff)
From: Ming Lei <ming.lei@redhat.com> To: Dongli Zhang <dongli.zhang@oracle.com> Cc: Jens Axboe <axboe@kernel.dk>, Sagi Grimberg <sagi@grimberg.me>, linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, Alan Adamson <alan.adamson@oracle.com>, Keith Busch <kbusch@kernel.org>, Max Gurtovoy <maxg@mellanox.com>, Christoph Hellwig <hch@lst.de> Subject: Re: [PATCH 3/3] nvme-pci: make nvme reset more reliable Date: Tue, 26 May 2020 15:12:49 +0800 [thread overview] Message-ID: <20200526071249.GA874504@T590> (raw) In-Reply-To: <9c5ac1e0-b5ca-0f54-5ee3-fd630dbdb8d4@oracle.com> On Mon, May 25, 2020 at 10:01:18PM -0700, Dongli Zhang wrote: > > > On 5/20/20 4:56 AM, Ming Lei wrote: > > During waiting for in-flight IO completion in reset handler, timeout > > Does this indicate the window since nvme_start_queues() in nvme_reset_work(), > that is, just after the queues are unquiesced again? Right, nvme_start_queues() starts to dispatch requests again, and nvme_wait_freeze() waits completion of all these in-flight IOs. > > If v2 is required in the future, how about to mention the specific function to > that it would be much more easier to track the issue. Not sure it is needed, cause it is quite straightforward. > > > or controller failure still may happen, then the controller is deleted > > and all inflight IOs are failed. This way is too violent. > > > > Improve the reset handling by replacing nvme_wait_freeze with query > > & check controller. If all ns queues are frozen, the controller is reset > > successfully, otherwise check and see if the controller has been disabled. > > If yes, break from the current recovery and schedule a fresh new reset. > > > > This way avoids to failing IO & removing controller unnecessarily. > > > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Sagi Grimberg <sagi@grimberg.me> > > Cc: Keith Busch <kbusch@kernel.org> > > Cc: Max Gurtovoy <maxg@mellanox.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/nvme/host/pci.c | 37 ++++++++++++++++++++++++++++++------- > > 1 file changed, 30 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > > index ce0d1e79467a..b5aeed33a634 100644 > > --- a/drivers/nvme/host/pci.c > > +++ b/drivers/nvme/host/pci.c > > @@ -24,6 +24,7 @@ > > #include <linux/io-64-nonatomic-lo-hi.h> > > #include <linux/sed-opal.h> > > #include <linux/pci-p2pdma.h> > > +#include <linux/delay.h> > > > > #include "trace.h" > > #include "nvme.h" > > @@ -1235,9 +1236,6 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved) > > * shutdown, so we return BLK_EH_DONE. > > */ > > switch (dev->ctrl.state) { > > - case NVME_CTRL_CONNECTING: > > - nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING); > > - /* fall through */ > > case NVME_CTRL_DELETING: > > dev_warn_ratelimited(dev->ctrl.device, > > "I/O %d QID %d timeout, disable controller\n", > > @@ -2393,7 +2391,8 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) > > u32 csts = readl(dev->bar + NVME_REG_CSTS); > > > > if (dev->ctrl.state == NVME_CTRL_LIVE || > > - dev->ctrl.state == NVME_CTRL_RESETTING) { > > + dev->ctrl.state == NVME_CTRL_RESETTING || > > + dev->ctrl.state == NVME_CTRL_CONNECTING) { > > freeze = true; > > nvme_start_freeze(&dev->ctrl); > > } > > @@ -2504,12 +2503,29 @@ static void nvme_remove_dead_ctrl(struct nvme_dev *dev) > > nvme_put_ctrl(&dev->ctrl); > > } > > > > +static bool nvme_wait_freeze_and_check(struct nvme_dev *dev) > > +{ > > + bool frozen; > > + > > + while (true) { > > + frozen = nvme_frozen(&dev->ctrl); > > + if (frozen) > > + break; > > ... and how about to comment that the below is because of nvme timeout handler > as explained in another email (if v2 would be sent) so that it is not required > to query for "online_queues" with cscope :) > > > + if (!dev->online_queues) > > + break; > > + msleep(5); Fine. Thanks, Ming _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2020-05-26 7:13 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-20 11:56 [PATCH 0/3] blk-mq/nvme: improve nvme-pci reset handler Ming Lei 2020-05-20 11:56 ` Ming Lei 2020-05-20 11:56 ` [PATCH 1/3] blk-mq: add API of blk_mq_queue_frozen Ming Lei 2020-05-20 11:56 ` Ming Lei 2020-05-20 11:56 ` [PATCH 2/3] nvme: add nvme_frozen Ming Lei 2020-05-20 11:56 ` Ming Lei 2020-05-20 11:56 ` [PATCH 3/3] nvme-pci: make nvme reset more reliable Ming Lei 2020-05-20 11:56 ` Ming Lei 2020-05-20 17:10 ` Dongli Zhang 2020-05-20 17:10 ` Dongli Zhang 2020-05-20 17:27 ` Dongli Zhang 2020-05-20 17:27 ` Dongli Zhang 2020-05-20 17:52 ` Keith Busch 2020-05-20 17:52 ` Keith Busch 2020-05-21 2:33 ` Ming Lei 2020-05-21 2:33 ` Ming Lei 2020-05-26 5:01 ` Dongli Zhang 2020-05-26 5:01 ` Dongli Zhang 2020-05-26 7:12 ` Ming Lei [this message] 2020-05-26 7:12 ` Ming Lei 2020-05-26 2:55 ` [PATCH 0/3] blk-mq/nvme: improve nvme-pci reset handler Ming Lei 2020-05-26 2:55 ` Ming Lei 2020-05-27 18:09 ` Alan Adamson 2020-05-27 18:09 ` Alan Adamson 2020-05-27 18:52 ` Keith Busch 2020-05-27 18:52 ` Keith Busch 2020-05-28 1:36 ` Ming Lei 2020-05-28 1:36 ` Ming Lei
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200526071249.GA874504@T590 \ --to=ming.lei@redhat.com \ --cc=alan.adamson@oracle.com \ --cc=axboe@kernel.dk \ --cc=dongli.zhang@oracle.com \ --cc=hch@lst.de \ --cc=kbusch@kernel.org \ --cc=linux-block@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=maxg@mellanox.com \ --cc=sagi@grimberg.me \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.