linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: keith.busch@intel.com (Keith Busch)
Subject: [PATCH V6 11/11] nvme: pci: support nested EH
Date: Wed, 16 May 2018 08:12:42 -0600	[thread overview]
Message-ID: <20180516141242.GA20119@localhost.localdomain> (raw)
In-Reply-To: <20180516040313.13596-12-ming.lei@redhat.com>

Hi Ming,

I'm sorry, but I am frankly not on board with introducing yet
another work-queue into this driver for handling this situation. The
fact you missed syncing with this queue in the surprise remove case,
introducing various use-after-free conditions, just demonstrates exactly
how over-complicated this approach is. That, and the forced controller
state transtions is yet another way surprise removal will break as its
depending on the state machine to prevent certain transitions.

The driver is already in a work queue context when it becomes aware of
corrective action being necessary. Seriously, simply syncing these in the
reset convers nearly all conditions you're concered with, most of which
will be obviated if Bart's blk-mq timeout rework is added. The only case
that isn't covered is if IO stops when renumbering the hardware contexts
(unlikely as that is), and that's easily fixable just moving that into
the scan_work.

As far as blktests block/011 is concerned, I think this needs to be
rethought considering what it's actually showing us. The fact the
pci driver provides such an easy way to not only muck with PCI config
register *and* internal kernel structures out from under a driver that's
bound to it is insane.  If PCI really wants to provide this sysfs entry,
it really ought to notify bound drivers that this is occuring, similar
to the 'reset' sysfs.

Anyway, there is merit to some of your earlier patches. In particular,
specifically patches 2, 4, and 5.  On the timing out the host memory
releasing (patch 2), I would just rather see this as a generic API,
though:

  http://lists.infradead.org/pipermail/linux-nvme/2018-January/015313.html
  http://lists.infradead.org/pipermail/linux-nvme/2018-January/015314.html

  reply	other threads:[~2018-05-16 14:12 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-16  4:03 [PATCH V6 00/11] nvme: pci: fix & improve timeout handling Ming Lei
2018-05-16  4:03 ` [PATCH V6 01/11] block: introduce blk_quiesce_timeout() and blk_unquiesce_timeout() Ming Lei
2018-05-16  4:03 ` [PATCH V6 02/11] nvme: pci: cover timeout for admin commands running in EH Ming Lei
2018-05-24 15:39   ` Keith Busch
2018-05-16  4:03 ` [PATCH V6 03/11] nvme: pci: unquiesce admin queue after controller is shutdown Ming Lei
2018-05-16  4:03 ` [PATCH V6 04/11] nvme: pci: set nvmeq->cq_vector after alloc cq/sq Ming Lei
2018-05-16  4:03 ` [PATCH V6 05/11] nvme: pci: only wait freezing if queue is frozen Ming Lei
2018-05-16  4:03 ` [PATCH V6 06/11] nvme: pci: freeze queue in nvme_dev_disable() in case of error recovery Ming Lei
2018-05-16  4:03 ` [PATCH V6 07/11] nvme: pci: prepare for supporting error recovery from resetting context Ming Lei
2018-05-16  4:03 ` [PATCH V6 08/11] nvme: pci: move error handling out of nvme_reset_dev() Ming Lei
2018-05-16  4:03 ` [PATCH V6 09/11] nvme: pci: don't unfreeze queue until controller state updating succeeds Ming Lei
2018-05-16  4:03 ` [PATCH V6 10/11] nvme: core: introduce nvme_force_change_ctrl_state() Ming Lei
2018-05-16  4:03 ` [PATCH V6 11/11] nvme: pci: support nested EH Ming Lei
2018-05-16 14:12   ` Keith Busch [this message]
2018-05-16 23:10     ` Ming Lei
2018-05-17  2:20       ` Keith Busch
2018-05-17  8:41         ` Christoph Hellwig
2018-05-17 14:20           ` Keith Busch
2018-05-17 14:23             ` Johannes Thumshirn
2018-05-18 16:28               ` Keith Busch
2018-05-22  7:35                 ` Johannes Thumshirn
2018-05-18  0:20         ` Ming Lei
2018-05-18  1:01           ` Ming Lei
2018-05-18 13:57           ` Keith Busch
2018-05-18 16:58             ` Jens Axboe
2018-05-18 22:26             ` Ming Lei
2018-05-18 23:45               ` Keith Busch
2018-05-18 23:51                 ` 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=20180516141242.GA20119@localhost.localdomain \
    --to=keith.busch@intel.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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).