From: Sagi Grimberg <sagig@dev.mellanox.co.il> To: Wenbo Wang <wenbo.wang@memblaze.com>, Jens Axboe <axboe@fb.com>, Wenbo Wang <mail_weber_wang@163.com>, "keith.busch@intel.com" <keith.busch@intel.com> Cc: "Wenwei.Tao" <wenwei.tao@memblaze.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org> Subject: Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended Date: Tue, 2 Feb 2016 14:41:37 +0200 [thread overview] Message-ID: <56B0A401.30306@dev.mellanox.co.il> (raw) In-Reply-To: <BJXPR01MB19940BA759DBBADC6BF6BD2E0DF0@BJXPR01MB199.CHNPR01.prod.partner.outlook.cn> > Jens, > > I did the following test to validate the issue. > > 1. Modify code as below to increase the chance of races. > Add 10s delay after nvme_dev_unmap() in nvme_dev_disable() > Add 10s delay before __nvme_submit_cmd() > 2. Run dd and at the same time, echo 1 to reset_controller to trigger device reset. Finally kernel crashes due to accessing unmapped door bell register. > > Following is the execution order of the two code paths: > __blk_mq_run_hw_queue > Test BLK_MQ_S_STOPPED > nvme_dev_disable() > nvme_stop_queues() <-- set BLK_MQ_S_STOPPED > nvme_dev_unmap(dev) <-- unmap door bell > nvme_queue_rq() > Touch door bell <-- panic here First of all, I think we need to cancel all inflight requests before nvme_dev_unmap. With my patches that move I/O termination to the nvme core ([PATCH v1 0/3] Move active IO termination to the core) the change needed is: -- diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index e921165..2288bdb 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1890,10 +1890,11 @@ static void nvme_dev_shutdown(struct nvme_dev *dev) nvme_shutdown_ctrl(&dev->ctrl); nvme_disable_queue(dev, 0); } - nvme_dev_unmap(dev); blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_io, dev); blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_io, dev); + + nvme_dev_unmap(dev); } static int nvme_setup_prp_pools(struct nvme_dev *dev) -- But still we need a way to wait out for all active queue_rq to end. It seems like we need to maintain the request_fn_active for blk-mq and provide an API (blk_mq_wait_for_active_requests ?) that waits for it to drop to zero. Thoughts?
WARNING: multiple messages have this Message-ID (diff)
From: sagig@dev.mellanox.co.il (Sagi Grimberg) Subject: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended Date: Tue, 2 Feb 2016 14:41:37 +0200 [thread overview] Message-ID: <56B0A401.30306@dev.mellanox.co.il> (raw) In-Reply-To: <BJXPR01MB19940BA759DBBADC6BF6BD2E0DF0@BJXPR01MB199.CHNPR01.prod.partner.outlook.cn> > Jens, > > I did the following test to validate the issue. > > 1. Modify code as below to increase the chance of races. > Add 10s delay after nvme_dev_unmap() in nvme_dev_disable() > Add 10s delay before __nvme_submit_cmd() > 2. Run dd and at the same time, echo 1 to reset_controller to trigger device reset. Finally kernel crashes due to accessing unmapped door bell register. > > Following is the execution order of the two code paths: > __blk_mq_run_hw_queue > Test BLK_MQ_S_STOPPED > nvme_dev_disable() > nvme_stop_queues() <-- set BLK_MQ_S_STOPPED > nvme_dev_unmap(dev) <-- unmap door bell > nvme_queue_rq() > Touch door bell <-- panic here First of all, I think we need to cancel all inflight requests before nvme_dev_unmap. With my patches that move I/O termination to the nvme core ([PATCH v1 0/3] Move active IO termination to the core) the change needed is: -- diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index e921165..2288bdb 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1890,10 +1890,11 @@ static void nvme_dev_shutdown(struct nvme_dev *dev) nvme_shutdown_ctrl(&dev->ctrl); nvme_disable_queue(dev, 0); } - nvme_dev_unmap(dev); blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_io, dev); blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_io, dev); + + nvme_dev_unmap(dev); } static int nvme_setup_prp_pools(struct nvme_dev *dev) -- But still we need a way to wait out for all active queue_rq to end. It seems like we need to maintain the request_fn_active for blk-mq and provide an API (blk_mq_wait_for_active_requests ?) that waits for it to drop to zero. Thoughts?
next prev parent reply other threads:[~2016-02-02 12:41 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-01 15:42 [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended Wenbo Wang 2016-02-01 15:42 ` Wenbo Wang 2016-02-01 15:59 ` Busch, Keith 2016-02-01 15:59 ` Busch, Keith 2016-02-01 16:17 ` Wenbo Wang 2016-02-01 16:17 ` Wenbo Wang 2016-02-01 16:54 ` Jens Axboe 2016-02-01 16:54 ` Jens Axboe 2016-02-02 7:15 ` Wenbo Wang 2016-02-02 12:41 ` Sagi Grimberg [this message] 2016-02-02 12:41 ` Sagi Grimberg 2016-02-02 14:27 ` Keith Busch 2016-02-02 14:27 ` Keith Busch 2016-02-02 14:33 ` Sagi Grimberg 2016-02-02 14:33 ` Sagi Grimberg 2016-02-02 14:46 ` Keith Busch 2016-02-02 14:46 ` Keith Busch 2016-02-02 17:20 ` Sagi Grimberg 2016-02-02 17:20 ` Sagi Grimberg 2016-02-03 0:49 ` Wenbo Wang 2016-02-02 17:25 ` Keith Busch 2016-02-02 17:25 ` Keith Busch 2016-02-03 0:19 ` Wenbo Wang 2016-02-03 14:41 ` Keith Busch 2016-02-03 14:41 ` Keith Busch 2016-02-03 16:35 ` Wenbo Wang 2016-02-03 16:38 ` Keith Busch 2016-02-03 16:38 ` Keith Busch 2016-02-06 14:32 ` Wenbo Wang 2016-02-07 13:41 ` Sagi Grimberg 2016-02-07 13:41 ` Sagi Grimberg 2016-02-08 15:01 ` Keith Busch 2016-02-08 15:01 ` Keith Busch 2016-02-09 11:22 ` Wenbo Wang 2016-02-09 22:55 ` Keith Busch 2016-02-09 22:55 ` Keith Busch
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=56B0A401.30306@dev.mellanox.co.il \ --to=sagig@dev.mellanox.co.il \ --cc=axboe@fb.com \ --cc=keith.busch@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=mail_weber_wang@163.com \ --cc=wenbo.wang@memblaze.com \ --cc=wenwei.tao@memblaze.com \ /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.