linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: keith.busch@intel.com (Keith Busch)
Subject: [PATCH 3/4] NVMe: Surprise removal fixes
Date: Mon, 8 Feb 2016 18:38:23 +0000	[thread overview]
Message-ID: <20160208183823.GA29157@localhost.localdomain> (raw)
In-Reply-To: <20160208181640.GD13126@infradead.org>

On Mon, Feb 08, 2016@10:16:40AM -0800, Christoph Hellwig wrote:
> On Wed, Feb 03, 2016@09:05:42AM -0700, Keith Busch wrote:
> > +		blk_mq_freeze_queue_start(ns->queue);
> > +		blk_set_queue_dying(ns->queue);
> >  		blk_mq_abort_requeue_list(ns->queue);
> > +		blk_mq_start_hw_queues(ns->queue);
> >  	}
> 
> Do we really still need all this magic if ->queue_rq returns a failure
> if the queue is dying?

This is far from perfect. Let me try explaining what's happening, then
I hope to abandon this patch and do it correctly. :)

The test does buffered IO writes with 'dd'. If I yank the drive,
'dd' continues until writing the device's capacity. The device is
gone, so there is no place to write dirty pages. The capcity exceeds
available memory, so 'dd' will wait even though everything it tries to
write fails. It consumes all available memory and the system becomes
noticably slower.

Ending that process is not what this patch accomplishes. It just lets
del_gendisk and blk_cleanup_queue complete by not allowing processes
to continue entering the queue by freezing first. Driver unbind doesn't
complete without this.

But there is a possible deadlock here. If you do an orderly removal
and surprise removal immediatly after, the device will initially be
considered capable of io and let dirty data sync. The hot removal will
make that impossible, but the orderly removal is holding the namespace
mutex, so no one can cleanup the namespace's queue.

Anyway, I wish to withdraw this patch. I will send an alternate this
week using state machine driven logic that should fix the deadlock unless
there is a different proposal sooner.

  reply	other threads:[~2016-02-08 18:38 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-03 16:05 [PATCH 0/4] NVMe: Surprise removal fixes Keith Busch
2016-02-03 16:05 ` [PATCH 1/4] NVMe: Fix io incapable return values Keith Busch
2016-02-04 15:55   ` Sagi Grimberg
2016-02-08 18:10   ` Christoph Hellwig
2016-02-03 16:05 ` [PATCH 2/4] NVMe: Sync stopped queues with block layer Keith Busch
2016-02-04 15:57   ` Sagi Grimberg
2016-02-04 16:00     ` Keith Busch
2016-02-08 18:11   ` Christoph Hellwig
2016-02-03 16:05 ` [PATCH 3/4] NVMe: Surprise removal fixes Keith Busch
2016-02-08 18:16   ` Christoph Hellwig
2016-02-08 18:38     ` Keith Busch [this message]
2016-02-08 23:48       ` Keith Busch
2016-02-03 16:05 ` [PATCH 4/4] blk-mq: End unstarted requests on dying queue Keith Busch
2016-02-04 15:59   ` Sagi Grimberg
2016-02-08 18:13   ` Christoph Hellwig
2016-02-04  3:25 ` [PATCH 0/4] NVMe: Surprise removal fixes Wenbo Wang
2016-02-04 15:52   ` Keith Busch
2016-02-04 15:55 ` Sagi Grimberg

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=20160208183823.GA29157@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).