[PATCH v3 0/6] Make SCSI device suspend and resume work reliably

[PATCH v3 0/6] Make SCSI device suspend and resume work reliably

[Bart Van Assche]

Hello Jens,

It is known that during the resume following a hibernate sometimes the
system hangs instead of coming up properly. This patch series fixes this
problem. This patch series is an alternative for Ming Lei's "[PATCH V5
0/10] block/scsi: safe SCSI quiescing" patch series. The advantages of
this patch series are:
- Easier to review because no new race conditions are introduced between
  queue freezing and blk_cleanup_queue(). As the discussion that followed
  Ming's patch series shows the correctness of the new code is hard to
  verify.
- No new freeze modes and hence no new freeze mode variables.

These patches have been tested on top of a merge of the block layer
for-next branch and Linus' master tree. Linus' master tree includes
patch "KVM: x86: Fix the NULL pointer parameter in check_cr_write()"
but the block layer for-next branch not yet.

Please consider these changes for kernel v4.15.

Thanks,

Bart.

Changes between v2 and v3:
- Made md kernel threads freezable.
- Changed the approach for quiescing SCSI devices again.
- Addressed Ming's review comments.

Changes compared to v1 of this patch series:
- Changed the approach and rewrote the patch series.

Bart Van Assche (6):
  md: Make md resync and reshape threads freezable
  block: Convert RQF_PREEMPT into REQ_PREEMPT
  block: Add the QUEUE_FLAG_PREEMPT_ONLY request queue flag
  scsi: Set QUEUE_FLAG_PREEMPT_ONLY while quiesced
  block: Make SCSI device suspend and resume work reliably
  scsi-mq: Reduce suspend latency

 block/blk-core.c          | 46 +++++++++++++++++++++++++++++++++++++++-------
 block/blk-mq-debugfs.c    |  2 +-
 block/blk-mq.c            |  4 ++--
 block/blk-timeout.c       |  2 +-
 drivers/ide/ide-atapi.c   |  3 +--
 drivers/ide/ide-io.c      |  2 +-
 drivers/ide/ide-pm.c      |  4 ++--
 drivers/md/md.c           | 21 +++++++++++++--------
 drivers/scsi/scsi_lib.c   | 36 ++++++++++++++++++++++++++----------
 fs/block_dev.c            |  4 ++--
 include/linux/blk_types.h |  6 ++++++
 include/linux/blkdev.h    | 10 ++++++----
 12 files changed, 100 insertions(+), 40 deletions(-)

-- 
2.14.1

[PATCH v3 1/6] md: Make md resync and reshape threads freezable

[Bart Van Assche]

Some people use the md driver on laptops and use the suspend and
resume functionality. Since it is essential that submitting of
new I/O requests stops before device quiescing starts, make the
md resync and reshape threads freezable.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Shaohua Li <shli@kernel.org>
Cc: linux-raid@vger.kernel.org
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/md/md.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 08fcaebc61bd..26a12bd0db65 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -66,6 +66,7 @@
 #include <linux/raid/md_u.h>
 #include <linux/slab.h>
 #include <linux/percpu-refcount.h>
+#include <linux/freezer.h>
 
 #include <trace/events/block.h>
 #include "md.h"
@@ -7424,6 +7425,7 @@ static int md_thread(void *arg)
 	 */
 
 	allow_signal(SIGKILL);
+	set_freezable();
 	while (!kthread_should_stop()) {
 
 		/* We need to wait INTERRUPTIBLE so that
@@ -7434,7 +7436,7 @@ static int md_thread(void *arg)
 		if (signal_pending(current))
 			flush_signals(current);
 
-		wait_event_interruptible_timeout
+		wait_event_freezable_timeout
 			(thread->wqueue,
 			 test_bit(THREAD_WAKEUP, &thread->flags)
 			 || kthread_should_stop() || kthread_should_park(),
@@ -8133,6 +8135,8 @@ void md_do_sync(struct md_thread *thread)
 		return;
 	}
 
+	set_freezable();
+
 	if (mddev_is_clustered(mddev)) {
 		ret = md_cluster_ops->resync_start(mddev);
 		if (ret)
@@ -8324,7 +8328,7 @@ void md_do_sync(struct md_thread *thread)
 		     mddev->curr_resync_completed > mddev->resync_max
 			    )) {
 			/* time to update curr_resync_completed */
-			wait_event(mddev->recovery_wait,
+			wait_event_freezable(mddev->recovery_wait,
 				   atomic_read(&mddev->recovery_active) == 0);
 			mddev->curr_resync_completed = j;
 			if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery) &&
@@ -8342,10 +8346,10 @@ void md_do_sync(struct md_thread *thread)
 			 * to avoid triggering warnings.
 			 */
 			flush_signals(current); /* just in case */
-			wait_event_interruptible(mddev->recovery_wait,
-						 mddev->resync_max > j
-						 || test_bit(MD_RECOVERY_INTR,
-							     &mddev->recovery));
+			wait_event_freezable(mddev->recovery_wait,
+					     mddev->resync_max > j ||
+					     test_bit(MD_RECOVERY_INTR,
+						      &mddev->recovery));
 		}
 
 		if (test_bit(MD_RECOVERY_INTR, &mddev->recovery))
@@ -8421,7 +8425,7 @@ void md_do_sync(struct md_thread *thread)
 				 * Give other IO more of a chance.
 				 * The faster the devices, the less we wait.
 				 */
-				wait_event(mddev->recovery_wait,
+				wait_event_freezable(mddev->recovery_wait,
 					   !atomic_read(&mddev->recovery_active));
 			}
 		}
@@ -8433,7 +8437,8 @@ void md_do_sync(struct md_thread *thread)
 	 * this also signals 'finished resyncing' to md_stop
 	 */
 	blk_finish_plug(&plug);
-	wait_event(mddev->recovery_wait, !atomic_read(&mddev->recovery_active));
+	wait_event_freezable(mddev->recovery_wait,
+			     !atomic_read(&mddev->recovery_active));
 
 	if (!test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
 	    !test_bit(MD_RECOVERY_INTR, &mddev->recovery) &&
-- 
2.14.1

Re: [PATCH v3 1/6] md: Make md resync and reshape threads freezable

[Ming Lei]

On Fri, Sep 22, 2017 at 03:14:00PM -0700, Bart Van Assche wrote:
> Some people use the md driver on laptops and use the suspend and
> resume functionality. Since it is essential that submitting of
> new I/O requests stops before device quiescing starts, make the
> md resync and reshape threads freezable.
> 

Why is this patch part of this patchset? I don't think
MD is special enough wrt. the SCSI quiesce issue.


-- 
Ming

Re: [PATCH v3 1/6] md: Make md resync and reshape threads freezable

[Bart Van Assche]

T24gTW9uLCAyMDE3LTA5LTI1IGF0IDEwOjM4ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
RnJpLCBTZXAgMjIsIDIwMTcgYXQgMDM6MTQ6MDBQTSAtMDcwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IFNvbWUgcGVvcGxlIHVzZSB0aGUgbWQgZHJpdmVyIG9uIGxhcHRvcHMgYW5kIHVz
ZSB0aGUgc3VzcGVuZCBhbmQNCj4gPiByZXN1bWUgZnVuY3Rpb25hbGl0eS4gU2luY2UgaXQgaXMg
ZXNzZW50aWFsIHRoYXQgc3VibWl0dGluZyBvZg0KPiA+IG5ldyBJL08gcmVxdWVzdHMgc3RvcHMg
YmVmb3JlIGRldmljZSBxdWllc2Npbmcgc3RhcnRzLCBtYWtlIHRoZQ0KPiA+IG1kIHJlc3luYyBh
bmQgcmVzaGFwZSB0aHJlYWRzIGZyZWV6YWJsZS4NCj4gDQo+IFdoeSBpcyB0aGlzIHBhdGNoIHBh
cnQgb2YgdGhpcyBwYXRjaHNldD8gSSBkb24ndCB0aGluaw0KPiBNRCBpcyBzcGVjaWFsIGVub3Vn
aCB3cnQuIHRoZSBTQ1NJIHF1aWVzY2UgaXNzdWUuDQoNCkJlY2F1c2UgdGhlIHNjb3BlIG9mIHRo
aXMgcGF0Y2ggc2VyaWVzIGlzIG5vdCBvbmx5IHRvIG1ha2UgU0NTSSBxdWllc2NlIHNhZmUNCmJ1
dCBhbHNvIHRvIGF2b2lkIHRoYXQgc3VzcGVuZCBhbmQgcmVzdW1lIGxvY2sgdXAgd2l0aCBhIHN0
b3JhZ2Ugc3RhY2sgdGhhdA0KdXNlcyB0aGUgbWQgZHJpdmVyLg0KDQpCYXJ0Lg==

Re: [PATCH v3 1/6] md: Make md resync and reshape threads freezable

[Ming Lei]

On Mon, Sep 25, 2017 at 04:22:29PM +0000, Bart Van Assche wrote:
> On Mon, 2017-09-25 at 10:38 +0800, Ming Lei wrote:
> > On Fri, Sep 22, 2017 at 03:14:00PM -0700, Bart Van Assche wrote:
> > > Some people use the md driver on laptops and use the suspend and
> > > resume functionality. Since it is essential that submitting of
> > > new I/O requests stops before device quiescing starts, make the
> > > md resync and reshape threads freezable.
> > 
> > Why is this patch part of this patchset? I don't think
> > MD is special enough wrt. the SCSI quiesce issue.
> 
> Because the scope of this patch series is not only to make SCSI quiesce safe
> but also to avoid that suspend and resume lock up with a storage stack that
> uses the md driver.

If SCSI quiesce is safe, there shouldn't be suspend/resume lockup on any
SCSI device wrt. quiesce, right?

Did you see Martin's report which is on BTRFS(RAID) instead of MD?

	https://marc.info/?l=linux-block&m=150634883816965&w=2

-- 
Ming

Re: [PATCH v3 1/6] md: Make md resync and reshape threads freezable

[Bart Van Assche]

T24gVHVlLCAyMDE3LTA5LTI2IGF0IDA2OjQ1ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
TW9uLCBTZXAgMjUsIDIwMTcgYXQgMDQ6MjI6MjlQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IE9uIE1vbiwgMjAxNy0wOS0yNSBhdCAxMDozOCArMDgwMCwgTWluZyBMZWkgd3Jv
dGU6DQo+ID4gPiBPbiBGcmksIFNlcCAyMiwgMjAxNyBhdCAwMzoxNDowMFBNIC0wNzAwLCBCYXJ0
IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiA+IFNvbWUgcGVvcGxlIHVzZSB0aGUgbWQgZHJpdmVy
IG9uIGxhcHRvcHMgYW5kIHVzZSB0aGUgc3VzcGVuZCBhbmQNCj4gPiA+ID4gcmVzdW1lIGZ1bmN0
aW9uYWxpdHkuIFNpbmNlIGl0IGlzIGVzc2VudGlhbCB0aGF0IHN1Ym1pdHRpbmcgb2YNCj4gPiA+
ID4gbmV3IEkvTyByZXF1ZXN0cyBzdG9wcyBiZWZvcmUgZGV2aWNlIHF1aWVzY2luZyBzdGFydHMs
IG1ha2UgdGhlDQo+ID4gPiA+IG1kIHJlc3luYyBhbmQgcmVzaGFwZSB0aHJlYWRzIGZyZWV6YWJs
ZS4NCj4gPiA+IA0KPiA+ID4gV2h5IGlzIHRoaXMgcGF0Y2ggcGFydCBvZiB0aGlzIHBhdGNoc2V0
PyBJIGRvbid0IHRoaW5rDQo+ID4gPiBNRCBpcyBzcGVjaWFsIGVub3VnaCB3cnQuIHRoZSBTQ1NJ
IHF1aWVzY2UgaXNzdWUuDQo+ID4gDQo+ID4gQmVjYXVzZSB0aGUgc2NvcGUgb2YgdGhpcyBwYXRj
aCBzZXJpZXMgaXMgbm90IG9ubHkgdG8gbWFrZSBTQ1NJIHF1aWVzY2Ugc2FmZQ0KPiA+IGJ1dCBh
bHNvIHRvIGF2b2lkIHRoYXQgc3VzcGVuZCBhbmQgcmVzdW1lIGxvY2sgdXAgd2l0aCBhIHN0b3Jh
Z2Ugc3RhY2sgdGhhdA0KPiA+IHVzZXMgdGhlIG1kIGRyaXZlci4NCj4gDQo+IElmIFNDU0kgcXVp
ZXNjZSBpcyBzYWZlLCB0aGVyZSBzaG91bGRuJ3QgYmUgc3VzcGVuZC9yZXN1bWUgbG9ja3VwIG9u
IGFueQ0KPiBTQ1NJIGRldmljZSB3cnQuIHF1aWVzY2UsIHJpZ2h0Pw0KDQpQbGVhc2UgcmVyZWFk
IG15IGUtbWFpbC4gWW91IG1pc3JlYWQgaXQuIEkgZGlkbid0IHdyaXRlIHRoYXQgU0NTSSBxdWll
c2NlIGlzIHNhZmUuDQoNCkJhcnQu

[PATCH v3 2/6] block: Convert RQF_PREEMPT into REQ_PREEMPT

[Bart Van Assche]

This patch does not change any functionality but makes the
REQ_PREEMPT flag available to blk_get_request(). A later patch
will add code to blk_get_request() that checks the REQ_PREEMPT
flag. Note: the IDE sense_rq request is allocated statically so
there is no blk_get_request() call that corresponds to this
request.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 block/blk-mq-debugfs.c    |  2 +-
 drivers/ide/ide-atapi.c   |  3 +--
 drivers/ide/ide-io.c      |  2 +-
 drivers/ide/ide-pm.c      |  4 ++--
 drivers/scsi/scsi_lib.c   | 11 ++++++-----
 include/linux/blk_types.h |  6 ++++++
 include/linux/blkdev.h    |  3 ---
 7 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 980e73095643..62ac248a4984 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -266,6 +266,7 @@ static const char *const cmd_flag_name[] = {
 	CMD_FLAG_NAME(BACKGROUND),
 	CMD_FLAG_NAME(NOUNMAP),
 	CMD_FLAG_NAME(NOWAIT),
+	CMD_FLAG_NAME(PREEMPT),
 };
 #undef CMD_FLAG_NAME
 
@@ -279,7 +280,6 @@ static const char *const rqf_name[] = {
 	RQF_NAME(MIXED_MERGE),
 	RQF_NAME(MQ_INFLIGHT),
 	RQF_NAME(DONTPREP),
-	RQF_NAME(PREEMPT),
 	RQF_NAME(COPY_USER),
 	RQF_NAME(FAILED),
 	RQF_NAME(QUIET),
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 14d1e7d9a1d6..1258739d5fa1 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -211,9 +211,8 @@ void ide_prep_sense(ide_drive_t *drive, struct request *rq)
 	}
 
 	sense_rq->rq_disk = rq->rq_disk;
-	sense_rq->cmd_flags = REQ_OP_DRV_IN;
+	sense_rq->cmd_flags = REQ_OP_DRV_IN | REQ_PREEMPT;
 	ide_req(sense_rq)->type = ATA_PRIV_SENSE;
-	sense_rq->rq_flags |= RQF_PREEMPT;
 
 	req->cmd[0] = GPCMD_REQUEST_SENSE;
 	req->cmd[4] = cmd_len;
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 3a234701d92c..06ffccd0fb10 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -539,7 +539,7 @@ void do_ide_request(struct request_queue *q)
 		 */
 		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) &&
 		    ata_pm_request(rq) == 0 &&
-		    (rq->rq_flags & RQF_PREEMPT) == 0) {
+		    (rq->cmd_flags & REQ_PREEMPT) == 0) {
 			/* there should be no pending command at this point */
 			ide_unlock_port(hwif);
 			goto plug_device;
diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
index 544f02d673ca..f8d2709dcd56 100644
--- a/drivers/ide/ide-pm.c
+++ b/drivers/ide/ide-pm.c
@@ -89,9 +89,9 @@ int generic_ide_resume(struct device *dev)
 	}
 
 	memset(&rqpm, 0, sizeof(rqpm));
-	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
+	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN | REQ_PREEMPT,
+			     __GFP_RECLAIM);
 	ide_req(rq)->type = ATA_PRIV_PM_RESUME;
-	rq->rq_flags |= RQF_PREEMPT;
 	rq->special = &rqpm;
 	rqpm.pm_step = IDE_PM_START_RESUME;
 	rqpm.pm_state = PM_EVENT_ON;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9cf6a80fe297..62f905b22821 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -253,8 +253,9 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	int ret = DRIVER_ERROR << 24;
 
 	req = blk_get_request(sdev->request_queue,
-			data_direction == DMA_TO_DEVICE ?
-			REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, __GFP_RECLAIM);
+			      (data_direction == DMA_TO_DEVICE ?
+			       REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN) | REQ_PREEMPT,
+			      __GFP_RECLAIM);
 	if (IS_ERR(req))
 		return ret;
 	rq = scsi_req(req);
@@ -268,7 +269,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	rq->retries = retries;
 	req->timeout = timeout;
 	req->cmd_flags |= flags;
-	req->rq_flags |= rq_flags | RQF_QUIET | RQF_PREEMPT;
+	req->rq_flags |= rq_flags | RQF_QUIET;
 
 	/*
 	 * head injection *required* here otherwise quiesce won't work
@@ -1301,7 +1302,7 @@ scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 			/*
 			 * If the devices is blocked we defer normal commands.
 			 */
-			if (!(req->rq_flags & RQF_PREEMPT))
+			if (!(req->cmd_flags & REQ_PREEMPT))
 				ret = BLKPREP_DEFER;
 			break;
 		default:
@@ -1310,7 +1311,7 @@ scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 			 * special commands.  In particular any user initiated
 			 * command is not allowed.
 			 */
-			if (!(req->rq_flags & RQF_PREEMPT))
+			if (!(req->cmd_flags & REQ_PREEMPT))
 				ret = BLKPREP_KILL;
 			break;
 		}
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index a2d2aa709cef..b6b6bf44e462 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -229,6 +229,11 @@ enum req_flag_bits {
 	__REQ_NOUNMAP,		/* do not free blocks when zeroing */
 
 	__REQ_NOWAIT,           /* Don't wait if request will block */
+	/*
+	 * set for "ide_preempt" requests and also for requests for which the
+	 * SCSI "quiesce" state must be ignored.
+	 */
+	__REQ_PREEMPT,
 	__REQ_NR_BITS,		/* stops here */
 };
 
@@ -248,6 +253,7 @@ enum req_flag_bits {
 
 #define REQ_NOUNMAP		(1ULL << __REQ_NOUNMAP)
 #define REQ_NOWAIT		(1ULL << __REQ_NOWAIT)
+#define REQ_PREEMPT		(1ULL << __REQ_PREEMPT)
 
 #define REQ_FAILFAST_MASK \
 	(REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 460294bb0fa5..cd26901a6e25 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -96,9 +96,6 @@ typedef __u32 __bitwise req_flags_t;
 #define RQF_MQ_INFLIGHT		((__force req_flags_t)(1 << 6))
 /* don't call prep for this one */
 #define RQF_DONTPREP		((__force req_flags_t)(1 << 7))
-/* set for "ide_preempt" requests and also for requests for which the SCSI
-   "quiesce" state must be ignored. */
-#define RQF_PREEMPT		((__force req_flags_t)(1 << 8))
 /* contains copies of user pages */
 #define RQF_COPY_USER		((__force req_flags_t)(1 << 9))
 /* vaguely specified driver internal error.  Ignored by the block layer */
-- 
2.14.1

[PATCH v3 3/6] block: Add the QUEUE_FLAG_PREEMPT_ONLY request queue flag

[Bart Van Assche]

This flag will be used in the next patch to let the block layer
core know whether or not a SCSI request queue has been quiesced.
A quiesced SCSI queue namely only processes RQF_PREEMPT requests.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 block/blk-core.c       | 13 +++++++++++++
 include/linux/blkdev.h |  5 +++++
 2 files changed, 18 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index aebe676225e6..45cf3f56a730 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -346,6 +346,19 @@ void blk_sync_queue(struct request_queue *q)
 }
 EXPORT_SYMBOL(blk_sync_queue);
 
+void blk_set_preempt_only(struct request_queue *q, bool preempt_only)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	if (preempt_only)
+		queue_flag_set(QUEUE_FLAG_PREEMPT_ONLY, q);
+	else
+		queue_flag_clear(QUEUE_FLAG_PREEMPT_ONLY, q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+}
+EXPORT_SYMBOL(blk_set_preempt_only);
+
 /**
  * __blk_run_queue_uncond - run a queue whether or not it has been stopped
  * @q:	The queue to run
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index cd26901a6e25..5bd87599eed0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -627,6 +627,7 @@ struct request_queue {
 #define QUEUE_FLAG_REGISTERED  26	/* queue has been registered to a disk */
 #define QUEUE_FLAG_SCSI_PASSTHROUGH 27	/* queue supports SCSI commands */
 #define QUEUE_FLAG_QUIESCED    28	/* queue has been quiesced */
+#define QUEUE_FLAG_PREEMPT_ONLY	29	/* only process REQ_PREEMPT requests */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\
@@ -731,6 +732,10 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 	((rq)->cmd_flags & (REQ_FAILFAST_DEV|REQ_FAILFAST_TRANSPORT| \
 			     REQ_FAILFAST_DRIVER))
 #define blk_queue_quiesced(q)	test_bit(QUEUE_FLAG_QUIESCED, &(q)->queue_flags)
+#define blk_queue_preempt_only(q)				\
+	test_bit(QUEUE_FLAG_PREEMPT_ONLY, &(q)->queue_flags)
+
+extern void blk_set_preempt_only(struct request_queue *q, bool preempt_only);
 
 static inline bool blk_account_rq(struct request *rq)
 {
-- 
2.14.1

[PATCH v3 4/6] scsi: Set QUEUE_FLAG_PREEMPT_ONLY while quiesced

[Bart Van Assche]

Make the quiesce state visible to the block layer for the next
patch in this series.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_lib.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 62f905b22821..ca84fd2d93ea 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2927,19 +2927,22 @@ static void scsi_wait_for_queuecommand(struct scsi_device *sdev)
 int
 scsi_device_quiesce(struct scsi_device *sdev)
 {
+	struct request_queue *q = sdev->request_queue;
 	int err;
 
 	mutex_lock(&sdev->state_mutex);
 	err = scsi_device_set_state(sdev, SDEV_QUIESCE);
+	if (err == 0)
+		blk_set_preempt_only(q, true);
 	mutex_unlock(&sdev->state_mutex);
 
 	if (err)
 		return err;
 
-	scsi_run_queue(sdev->request_queue);
+	scsi_run_queue(q);
 	while (atomic_read(&sdev->device_busy)) {
 		msleep_interruptible(200);
-		scsi_run_queue(sdev->request_queue);
+		scsi_run_queue(q);
 	}
 	return 0;
 }
@@ -2962,8 +2965,10 @@ void scsi_device_resume(struct scsi_device *sdev)
 	 */
 	mutex_lock(&sdev->state_mutex);
 	if (sdev->sdev_state == SDEV_QUIESCE &&
-	    scsi_device_set_state(sdev, SDEV_RUNNING) == 0)
+	    scsi_device_set_state(sdev, SDEV_RUNNING) == 0) {
+		blk_set_preempt_only(sdev->request_queue, false);
 		scsi_run_queue(sdev->request_queue);
+	}
 	mutex_unlock(&sdev->state_mutex);
 }
 EXPORT_SYMBOL(scsi_device_resume);
-- 
2.14.1

[PATCH v3 5/6] block: Make SCSI device suspend and resume work reliably

[Bart Van Assche]

It is essential during suspend and resume that neither the filesystem
state nor the filesystem metadata in RAM changes. This is why while
the hibernation image is being written or restored that SCSI devices
are quiesced. The SCSI core quiesces devices through scsi_device_quiesce()
and scsi_device_resume(). In the SDEV_QUIESCE state execution of
non-preempt requests is deferred. This is realized by returning
BLKPREP_DEFER from inside scsi_prep_state_check() for quiesced SCSI
devices. Avoid that a full queue prevents power management requests
to be submitted by deferring allocation of non-preempt requests for
devices in the quiesced state. This patch has been tested by running
the following commands and by verifying that after resume the fio job
is still running:

for d in /sys/class/block/sd*[a-z]; do
  hcil=$(readlink "$d/device")
  hcil=${hcil#../../../}
  echo 4 > "$d/queue/nr_requests"
  echo 1 > "/sys/class/scsi_device/$hcil/device/queue_depth"
done
bdev=$(readlink /dev/disk/by-uuid/5217d83f-213e-4b42-b86e-20013325ba6c)
bdev=${bdev#../../}
hcil=$(readlink "/sys/block/$bdev/device")
hcil=${hcil#../../../}
fio --name="$bdev" --filename="/dev/$bdev" --buffered=0 --bs=512 --rw=randread \
  --ioengine=libaio --numjobs=4 --iodepth=16 --iodepth_batch=1 --thread \
  --loops=$((2**31)) &
pid=$!
sleep 1
systemctl hibernate
sleep 10
kill $pid

Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
References: "I/O hangs after resuming from suspend-to-ram" (https://marc.info/?l=linux-block&m=150340235201348).
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 block/blk-core.c       | 37 ++++++++++++++++++++++++++++---------
 block/blk-mq.c         |  4 ++--
 block/blk-timeout.c    |  2 +-
 fs/block_dev.c         |  4 ++--
 include/linux/blkdev.h |  2 +-
 5 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 45cf3f56a730..971825bd4462 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -351,10 +351,12 @@ void blk_set_preempt_only(struct request_queue *q, bool preempt_only)
 	unsigned long flags;
 
 	spin_lock_irqsave(q->queue_lock, flags);
-	if (preempt_only)
+	if (preempt_only) {
 		queue_flag_set(QUEUE_FLAG_PREEMPT_ONLY, q);
-	else
+	} else {
 		queue_flag_clear(QUEUE_FLAG_PREEMPT_ONLY, q);
+		wake_up_all(&q->mq_freeze_wq);
+	}
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 EXPORT_SYMBOL(blk_set_preempt_only);
@@ -773,13 +775,29 @@ struct request_queue *blk_alloc_queue(gfp_t gfp_mask)
 }
 EXPORT_SYMBOL(blk_alloc_queue);
 
-int blk_queue_enter(struct request_queue *q, bool nowait)
+/**
+ * blk_queue_enter() - try to increase q->q_usage_counter
+ * @q: request queue pointer
+ * @nowait: if the queue is frozen, do not wait until it is unfrozen
+ * @preempt: if QUEUE_FLAG_PREEMPT_ONLY has been set, do not wait until that
+ *	flag has been cleared
+ */
+int blk_queue_enter(struct request_queue *q, bool nowait, bool preempt)
 {
 	while (true) {
 		int ret;
 
-		if (percpu_ref_tryget_live(&q->q_usage_counter))
-			return 0;
+		if (percpu_ref_tryget_live(&q->q_usage_counter)) {
+			/*
+			 * Ensure read order of q_usage_counter and the
+			 * PREEMPT_ONLY queue flag.
+			 */
+			smp_rmb();
+			if (preempt || !blk_queue_preempt_only(q))
+				return 0;
+			else
+				percpu_ref_put(&q->q_usage_counter);
+		}
 
 		if (nowait)
 			return -EBUSY;
@@ -794,7 +812,8 @@ int blk_queue_enter(struct request_queue *q, bool nowait)
 		smp_rmb();
 
 		ret = wait_event_interruptible(q->mq_freeze_wq,
-				!atomic_read(&q->mq_freeze_depth) ||
+				(atomic_read(&q->mq_freeze_depth) == 0 &&
+				 (preempt || !blk_queue_preempt_only(q))) ||
 				blk_queue_dying(q));
 		if (blk_queue_dying(q))
 			return -ENODEV;
@@ -2172,6 +2191,7 @@ blk_qc_t generic_make_request(struct bio *bio)
 	 */
 	struct bio_list bio_list_on_stack[2];
 	blk_qc_t ret = BLK_QC_T_NONE;
+	const bool nowait = bio->bi_opf & REQ_NOWAIT;
 
 	if (!generic_make_request_checks(bio))
 		goto out;
@@ -2211,7 +2231,7 @@ blk_qc_t generic_make_request(struct bio *bio)
 	do {
 		struct request_queue *q = bio->bi_disk->queue;
 
-		if (likely(blk_queue_enter(q, bio->bi_opf & REQ_NOWAIT) == 0)) {
+		if (likely(blk_queue_enter(q, nowait, false) == 0)) {
 			struct bio_list lower, same;
 
 			/* Create a fresh bio_list for all subordinate requests */
@@ -2236,8 +2256,7 @@ blk_qc_t generic_make_request(struct bio *bio)
 			bio_list_merge(&bio_list_on_stack[0], &same);
 			bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
 		} else {
-			if (unlikely(!blk_queue_dying(q) &&
-					(bio->bi_opf & REQ_NOWAIT)))
+			if (unlikely(!blk_queue_dying(q) && nowait))
 				bio_wouldblock_error(bio);
 			else
 				bio_io_error(bio);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 98a18609755e..ddaa0b444652 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -390,7 +390,7 @@ struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op,
 	struct request *rq;
 	int ret;
 
-	ret = blk_queue_enter(q, flags & BLK_MQ_REQ_NOWAIT);
+	ret = blk_queue_enter(q, flags & BLK_MQ_REQ_NOWAIT, op & REQ_PREEMPT);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -429,7 +429,7 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
 	if (hctx_idx >= q->nr_hw_queues)
 		return ERR_PTR(-EIO);
 
-	ret = blk_queue_enter(q, true);
+	ret = blk_queue_enter(q, true, op & REQ_PREEMPT);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index 17ec83bb0900..0dfdc975473a 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -134,7 +134,7 @@ void blk_timeout_work(struct work_struct *work)
 	struct request *rq, *tmp;
 	int next_set = 0;
 
-	if (blk_queue_enter(q, true))
+	if (blk_queue_enter(q, true, true))
 		return;
 	spin_lock_irqsave(q->queue_lock, flags);
 
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 93d088ffc05c..e9ca45087a40 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -674,7 +674,7 @@ int bdev_read_page(struct block_device *bdev, sector_t sector,
 	if (!ops->rw_page || bdev_get_integrity(bdev))
 		return result;
 
-	result = blk_queue_enter(bdev->bd_queue, false);
+	result = blk_queue_enter(bdev->bd_queue, false, false);
 	if (result)
 		return result;
 	result = ops->rw_page(bdev, sector + get_start_sect(bdev), page, false);
@@ -710,7 +710,7 @@ int bdev_write_page(struct block_device *bdev, sector_t sector,
 
 	if (!ops->rw_page || bdev_get_integrity(bdev))
 		return -EOPNOTSUPP;
-	result = blk_queue_enter(bdev->bd_queue, false);
+	result = blk_queue_enter(bdev->bd_queue, false, false);
 	if (result)
 		return result;
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5bd87599eed0..a025597c378a 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -964,7 +964,7 @@ extern int scsi_cmd_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			 struct scsi_ioctl_command __user *);
 
-extern int blk_queue_enter(struct request_queue *q, bool nowait);
+extern int blk_queue_enter(struct request_queue *q, bool nowait, bool preempt);
 extern void blk_queue_exit(struct request_queue *q);
 extern void blk_start_queue(struct request_queue *q);
 extern void blk_start_queue_async(struct request_queue *q);
-- 
2.14.1

Re: [PATCH v3 5/6] block: Make SCSI device suspend and resume work reliably

[Ming Lei]

On Fri, Sep 22, 2017 at 03:14:04PM -0700, Bart Van Assche wrote:
> It is essential during suspend and resume that neither the filesystem
> state nor the filesystem metadata in RAM changes. This is why while
> the hibernation image is being written or restored that SCSI devices
> are quiesced. The SCSI core quiesces devices through scsi_device_quiesce()
> and scsi_device_resume(). In the SDEV_QUIESCE state execution of
> non-preempt requests is deferred. This is realized by returning
> BLKPREP_DEFER from inside scsi_prep_state_check() for quiesced SCSI
> devices. Avoid that a full queue prevents power management requests
> to be submitted by deferring allocation of non-preempt requests for
> devices in the quiesced state. This patch has been tested by running
> the following commands and by verifying that after resume the fio job
> is still running:
> 
> for d in /sys/class/block/sd*[a-z]; do
>   hcil=$(readlink "$d/device")
>   hcil=${hcil#../../../}
>   echo 4 > "$d/queue/nr_requests"
>   echo 1 > "/sys/class/scsi_device/$hcil/device/queue_depth"
> done
> bdev=$(readlink /dev/disk/by-uuid/5217d83f-213e-4b42-b86e-20013325ba6c)
> bdev=${bdev#../../}
> hcil=$(readlink "/sys/block/$bdev/device")
> hcil=${hcil#../../../}
> fio --name="$bdev" --filename="/dev/$bdev" --buffered=0 --bs=512 --rw=randread \
>   --ioengine=libaio --numjobs=4 --iodepth=16 --iodepth_batch=1 --thread \
>   --loops=$((2**31)) &
> pid=$!
> sleep 1
> systemctl hibernate
> sleep 10
> kill $pid
> 
> Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> References: "I/O hangs after resuming from suspend-to-ram" (https://marc.info/?l=linux-block&m=150340235201348).
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  block/blk-core.c       | 37 ++++++++++++++++++++++++++++---------
>  block/blk-mq.c         |  4 ++--
>  block/blk-timeout.c    |  2 +-
>  fs/block_dev.c         |  4 ++--
>  include/linux/blkdev.h |  2 +-
>  5 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 45cf3f56a730..971825bd4462 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -351,10 +351,12 @@ void blk_set_preempt_only(struct request_queue *q, bool preempt_only)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(q->queue_lock, flags);
> -	if (preempt_only)
> +	if (preempt_only) {
>  		queue_flag_set(QUEUE_FLAG_PREEMPT_ONLY, q);
> -	else
> +	} else {
>  		queue_flag_clear(QUEUE_FLAG_PREEMPT_ONLY, q);
> +		wake_up_all(&q->mq_freeze_wq);
> +	}
>  	spin_unlock_irqrestore(q->queue_lock, flags);
>  }
>  EXPORT_SYMBOL(blk_set_preempt_only);
> @@ -773,13 +775,29 @@ struct request_queue *blk_alloc_queue(gfp_t gfp_mask)
>  }
>  EXPORT_SYMBOL(blk_alloc_queue);
>  
> -int blk_queue_enter(struct request_queue *q, bool nowait)
> +/**
> + * blk_queue_enter() - try to increase q->q_usage_counter
> + * @q: request queue pointer
> + * @nowait: if the queue is frozen, do not wait until it is unfrozen
> + * @preempt: if QUEUE_FLAG_PREEMPT_ONLY has been set, do not wait until that
> + *	flag has been cleared
> + */
> +int blk_queue_enter(struct request_queue *q, bool nowait, bool preempt)
>  {
>  	while (true) {
>  		int ret;
>  
> -		if (percpu_ref_tryget_live(&q->q_usage_counter))
> -			return 0;
> +		if (percpu_ref_tryget_live(&q->q_usage_counter)) {
> +			/*
> +			 * Ensure read order of q_usage_counter and the
> +			 * PREEMPT_ONLY queue flag.
> +			 */
> +			smp_rmb();
> +			if (preempt || !blk_queue_preempt_only(q))
> +				return 0;
> +			else
> +				percpu_ref_put(&q->q_usage_counter);
> +		}

Now you introduce one smp_rmb() and test on preempt flag on
blk-mq's fast path, which should have been avoided, so I
think this way is worse than my patchset.

On some systems(even a system with SCSI, or system without
SCSI), SCSI quiesce may never be used at all, so it is unfair
to introduce the cost in fast path for this system.

We can avoid that, why not do that?

-- 
Ming

Re: [PATCH v3 5/6] block: Make SCSI device suspend and resume work reliably

[Bart Van Assche]

T24gTW9uLCAyMDE3LTA5LTI1IGF0IDEwOjI2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
RnJpLCBTZXAgMjIsIDIwMTcgYXQgMDM6MTQ6MDRQTSAtMDcwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+ICtpbnQgYmxrX3F1ZXVlX2VudGVyKHN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxLCBi
b29sIG5vd2FpdCwgYm9vbCBwcmVlbXB0KQ0KPiA+ICB7DQo+ID4gIAl3aGlsZSAodHJ1ZSkgew0K
PiA+ICAJCWludCByZXQ7DQo+ID4gIA0KPiA+IC0JCWlmIChwZXJjcHVfcmVmX3RyeWdldF9saXZl
KCZxLT5xX3VzYWdlX2NvdW50ZXIpKQ0KPiA+IC0JCQlyZXR1cm4gMDsNCj4gPiArCQlpZiAocGVy
Y3B1X3JlZl90cnlnZXRfbGl2ZSgmcS0+cV91c2FnZV9jb3VudGVyKSkgew0KPiA+ICsJCQkvKg0K
PiA+ICsJCQkgKiBFbnN1cmUgcmVhZCBvcmRlciBvZiBxX3VzYWdlX2NvdW50ZXIgYW5kIHRoZQ0K
PiA+ICsJCQkgKiBQUkVFTVBUX09OTFkgcXVldWUgZmxhZy4NCj4gPiArCQkJICovDQo+ID4gKwkJ
CXNtcF9ybWIoKTsNCj4gPiArCQkJaWYgKHByZWVtcHQgfHwgIWJsa19xdWV1ZV9wcmVlbXB0X29u
bHkocSkpDQo+ID4gKwkJCQlyZXR1cm4gMDsNCj4gPiArCQkJZWxzZQ0KPiA+ICsJCQkJcGVyY3B1
X3JlZl9wdXQoJnEtPnFfdXNhZ2VfY291bnRlcik7DQo+ID4gKwkJfQ0KPiANCj4gTm93IHlvdSBp
bnRyb2R1Y2Ugb25lIHNtcF9ybWIoKSBhbmQgdGVzdCBvbiBwcmVlbXB0IGZsYWcgb24NCj4gYmxr
LW1xJ3MgZmFzdCBwYXRoLCB3aGljaCBzaG91bGQgaGF2ZSBiZWVuIGF2b2lkZWQsIHNvIEkNCj4g
dGhpbmsgdGhpcyB3YXkgaXMgd29yc2UgdGhhbiBteSBwYXRjaHNldC4NCg0KU28gdGhhdCBtZWFu
cyB0aGF0IHlvdSBoYXZlIG5vdCBub3RpY2VkIHRoYXQgaXQgaXMgc2FmZSB0byBsZWF2ZSBvdXQg
dGhhdA0Kc21wX3JtcCgpIGNhbGwgYmVjYXVzZSBibGstbXEgcXVldWUgZnJlZXppbmcgYW5kIHVu
ZnJlZXppbmcgd2FpdHMgZm9yIGEgZ3JhY2UNCnBlcmlvZCBhbmQgaGVuY2Ugd2FpdHMgdW50aWwg
YWxsIENQVXMgaGF2ZSBleGVjdXRlZCBhIGZ1bGwgbWVtb3J5IGJhcnJpZXI/DQoNCkJhcnQu

Re: [PATCH v3 5/6] block: Make SCSI device suspend and resume work reliably

[Ming Lei]

On Mon, Sep 25, 2017 at 04:20:20PM +0000, Bart Van Assche wrote:
> On Mon, 2017-09-25 at 10:26 +0800, Ming Lei wrote:
> > On Fri, Sep 22, 2017 at 03:14:04PM -0700, Bart Van Assche wrote:
> > > +int blk_queue_enter(struct request_queue *q, bool nowait, bool preempt)
> > >  {
> > >  	while (true) {
> > >  		int ret;
> > >  
> > > -		if (percpu_ref_tryget_live(&q->q_usage_counter))
> > > -			return 0;
> > > +		if (percpu_ref_tryget_live(&q->q_usage_counter)) {
> > > +			/*
> > > +			 * Ensure read order of q_usage_counter and the
> > > +			 * PREEMPT_ONLY queue flag.
> > > +			 */
> > > +			smp_rmb();
> > > +			if (preempt || !blk_queue_preempt_only(q))
> > > +				return 0;
> > > +			else
> > > +				percpu_ref_put(&q->q_usage_counter);
> > > +		}
> > 
> > Now you introduce one smp_rmb() and test on preempt flag on
> > blk-mq's fast path, which should have been avoided, so I
> > think this way is worse than my patchset.
> 
> So that means that you have not noticed that it is safe to leave out that
> smp_rmp() call because blk-mq queue freezing and unfreezing waits for a grace
> period and hence waits until all CPUs have executed a full memory barrier?

No, memory barrier has to be pair, this barrier has to be there, another
one before unfreeze/freeze can be removed because it is implied inside
freeze/unfreeze.

Actually it should have been smp_mb() between writing the percpu-refcount
and reading preempt_only flag, otherwise if the two op is reordered,
queue freeze/unfreeze may see the counter is zero, and this normal I/O
still can be observed even after queue is freezed and SCSI is put into
quiesced.

-- 
Ming

Re: [PATCH v3 5/6] block: Make SCSI device suspend and resume work reliably

[Bart Van Assche]

T24gVHVlLCAyMDE3LTA5LTI2IGF0IDA2OjUxICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
TW9uLCBTZXAgMjUsIDIwMTcgYXQgMDQ6MjA6MjBQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IFNvIHRoYXQgbWVhbnMgdGhhdCB5b3UgaGF2ZSBub3Qgbm90aWNlZCB0aGF0IGl0
IGlzIHNhZmUgdG8gbGVhdmUgb3V0IHRoYXQNCj4gPiBzbXBfcm1wKCkgY2FsbCBiZWNhdXNlIGJs
ay1tcSBxdWV1ZSBmcmVlemluZyBhbmQgdW5mcmVlemluZyB3YWl0cyBmb3IgYSBncmFjZQ0KPiA+
IHBlcmlvZCBhbmQgaGVuY2Ugd2FpdHMgdW50aWwgYWxsIENQVXMgaGF2ZSBleGVjdXRlZCBhIGZ1
bGwgbWVtb3J5IGJhcnJpZXI/DQo+IA0KPiBObywgbWVtb3J5IGJhcnJpZXIgaGFzIHRvIGJlIHBh
aXIsIHRoaXMgYmFycmllciBoYXMgdG8gYmUgdGhlcmUsIGFub3RoZXINCj4gb25lIGJlZm9yZSB1
bmZyZWV6ZS9mcmVlemUgY2FuIGJlIHJlbW92ZWQgYmVjYXVzZSBpdCBpcyBpbXBsaWVkIGluc2lk
ZQ0KPiBmcmVlemUvdW5mcmVlemUuDQo+IA0KPiBBY3R1YWxseSBpdCBzaG91bGQgaGF2ZSBiZWVu
IHNtcF9tYigpIGJldHdlZW4gd3JpdGluZyB0aGUgcGVyY3B1LXJlZmNvdW50DQo+IGFuZCByZWFk
aW5nIHByZWVtcHRfb25seSBmbGFnLCBvdGhlcndpc2UgaWYgdGhlIHR3byBvcCBpcyByZW9yZGVy
ZWQsDQo+IHF1ZXVlIGZyZWV6ZS91bmZyZWV6ZSBtYXkgc2VlIHRoZSBjb3VudGVyIGlzIHplcm8s
IGFuZCB0aGlzIG5vcm1hbCBJL08NCj4gc3RpbGwgY2FuIGJlIG9ic2VydmVkIGV2ZW4gYWZ0ZXIg
cXVldWUgaXMgZnJlZXplZCBhbmQgU0NTSSBpcyBwdXQgaW50bw0KPiBxdWllc2NlZC4NCg0KVGhh
dCdzIHdyb25nLiBCb3RoIG1lbW9yeSBiYXJyaWVycyBjYW4gYmUgbGVmdCBvdXQuIEZyZWV6aW5n
ICsgdW5mcmVlemluZyBhDQpibG9jayBsYXllciBxdWV1ZSBjYXVzZXMgY2FsbF9yY3UoKSB0byBi
ZSBpbnZva2VkIGFuZCBhbHNvIG1ha2VzIHRoZSBjb2RlDQp0aGF0IHBlcmZvcm1zIHRoZSBmcmVl
emluZyArIHVuZnJlZXppbmcgdG8gd2FpdCB1bnRpbCBhIGdyYWNlIHBlcmlvZCBoYXMNCm9jY3Vy
cmVkIG9uIGFsbCBDUFVzLiBJbiBvdGhlciB3b3JkcywgdW50aWwgYWxsIENQVXMgaGF2ZSBwZXJm
b3JtZWQgYSBmdWxsDQptZW1vcnkgYmFycmllci4gVGhpcyBpcyB3aHkgYm90aCBtZW1vcnkgYmFy
cmllcnMgY2FuIGJlIGxlZnQgb3V0LiBBIHF1b3RlDQpmcm9tICJUaGUgUkNVLWJhcnJpZXIgbWVu
YWdlcmllIiAoVGhlIFJDVS1iYXJyaWVyIG1lbmFnZXJpZSk6DQoiVGhlcmUgYXJlIG1hbnkgaXNz
dWVzIHdpdGggZXhjZXNzaXZlIHVzZSBvZiBtZW1vcnkgYmFycmllcnMsIG9uZSBvZiB3aGljaA0K
aXMgdGhlaXIgb3ZlcmhlYWQuIEl0IHR1cm5zIG91dCB0aGF0IFJDVSBjYW4gYmUgc3Vic3RpdHV0
ZWQgZm9yIG1lbW9yeQ0KYmFycmllcnMgaW4gYSBncmVhdCBtYW55IGNhc2VzLCBhbmQgdGhpcyBz
dWJzdGl0dXRpb24gYWxsb3dzIG9uZSBvZiB0aGUNCm1lbW9yeSBiYXJyaWVycyBvZiB0aGUgcGFp
ciB0byBiZSByZW1vdmVkLiBVbmZvcnR1bmF0ZWx5LCBpbiBtb3N0IGNhc2VzLCB0aGUNCm90aGVy
IG1lbW9yeSBiYXJyaWVyIG11c3QgYmUgcmVwbGFjZWQgYnkgYW4gZXhwZW5zaXZlIGdyYWNlLXBl
cmlvZC13YWl0DQpwcmltaXRpdmUgc3VjaCBhcyBzeW5jaHJvbml6ZV9yY3UoKSwgYnV0IHRoZXJl
IGFyZSBhIGZldyBzcGVjaWFsIGNhc2VzIHdoZXJlDQpzeW5jaHJvbml6ZV9yY3UoKSBpcyBub3Qg
bmVlZGVkLiINCg0KQmFydC4=

[PATCH v3 6/6] scsi-mq: Reduce suspend latency

[Bart Van Assche]

Avoid that it can take 200 ms too long to wait for requests to
finish. Note: blk_mq_freeze_queue() uses a wait queue to wait
for ongoing requests to finish.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_lib.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ca84fd2d93ea..c5e66bb0c94d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2939,10 +2939,20 @@ scsi_device_quiesce(struct scsi_device *sdev)
 	if (err)
 		return err;
 
-	scsi_run_queue(q);
-	while (atomic_read(&sdev->device_busy)) {
-		msleep_interruptible(200);
+	if (q->mq_ops) {
+		/*
+		 * Ensure write order of the PREEMPT_ONLY queue flag and
+		 * q_usage_counter. See also blk_queue_enter().
+		 */
+		smp_wmb();
+		blk_mq_freeze_queue(q);
+		blk_mq_unfreeze_queue(q);
+	} else {
 		scsi_run_queue(q);
+		while (atomic_read(&sdev->device_busy)) {
+			msleep_interruptible(200);
+			scsi_run_queue(q);
+		}
 	}
 	return 0;
 }
-- 
2.14.1

Re: [PATCH v3 6/6] scsi-mq: Reduce suspend latency

[Ming Lei]

On Fri, Sep 22, 2017 at 03:14:05PM -0700, Bart Van Assche wrote:
> Avoid that it can take 200 ms too long to wait for requests to
> finish. Note: blk_mq_freeze_queue() uses a wait queue to wait
> for ongoing requests to finish.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_lib.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ca84fd2d93ea..c5e66bb0c94d 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2939,10 +2939,20 @@ scsi_device_quiesce(struct scsi_device *sdev)
>  	if (err)
>  		return err;
>  
> -	scsi_run_queue(q);
> -	while (atomic_read(&sdev->device_busy)) {
> -		msleep_interruptible(200);
> +	if (q->mq_ops) {
> +		/*
> +		 * Ensure write order of the PREEMPT_ONLY queue flag and
> +		 * q_usage_counter. See also blk_queue_enter().
> +		 */
> +		smp_wmb();
> +		blk_mq_freeze_queue(q);
> +		blk_mq_unfreeze_queue(q);
> +	} else {
>  		scsi_run_queue(q);
> +		while (atomic_read(&sdev->device_busy)) {
> +			msleep_interruptible(200);
> +			scsi_run_queue(q);
> +		}

Again, your patch doesn't fix scsi quiesce issue
on block legacy, either on suspend path or revalidate
path of transport spi.

-- 
Ming

Re: [PATCH v3 0/6] Make SCSI device suspend and resume work reliably

[Ming Lei]

On Sat, Sep 23, 2017 at 6:13 AM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> Hello Jens,
>
> It is known that during the resume following a hibernate sometimes the
> system hangs instead of coming up properly. This patch series fixes this
> problem. This patch series is an alternative for Ming Lei's "[PATCH V5
> 0/10] block/scsi: safe SCSI quiescing" patch series. The advantages of
> this patch series are:

No, your patch doesn't fix scsi quiesce on block legacy, so not an alternative
of my patchset at all.

> - Easier to review because no new race conditions are introduced between
>   queue freezing and blk_cleanup_queue(). As the discussion that followed
>   Ming's patch series shows the correctness of the new code is hard to
>   verify.

I don't agree that my code is hard to verify. I have replied all your comments,
and the only thing you pay special attention to is that the race between preempt
quiesce and blk_cleanup_queue():

    - that is simply not a race
    - we have depended on drivers(legacy or blk-mq) to handle request correctly
   after queue is set as dying for long long time


-- 
Ming Lei

Re: [PATCH v3 0/6] Make SCSI device suspend and resume work reliably

[Bart Van Assche]

T24gTW9uLCAyMDE3LTA5LTI1IGF0IDEwOjM2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
U2F0LCBTZXAgMjMsIDIwMTcgYXQgNjoxMyBBTSwgQmFydCBWYW4gQXNzY2hlIDxiYXJ0LnZhbmFz
c2NoZUB3ZGMuY29tPiB3cm90ZToNCj4gPiBJdCBpcyBrbm93biB0aGF0IGR1cmluZyB0aGUgcmVz
dW1lIGZvbGxvd2luZyBhIGhpYmVybmF0ZSBzb21ldGltZXMgdGhlDQo+ID4gc3lzdGVtIGhhbmdz
IGluc3RlYWQgb2YgY29taW5nIHVwIHByb3Blcmx5LiBUaGlzIHBhdGNoIHNlcmllcyBmaXhlcyB0
aGlzDQo+ID4gcHJvYmxlbS4gVGhpcyBwYXRjaCBzZXJpZXMgaXMgYW4gYWx0ZXJuYXRpdmUgZm9y
IE1pbmcgTGVpJ3MgIltQQVRDSCBWNQ0KPiA+IDAvMTBdIGJsb2NrL3Njc2k6IHNhZmUgU0NTSSBx
dWllc2NpbmciIHBhdGNoIHNlcmllcy4gVGhlIGFkdmFudGFnZXMgb2YNCj4gPiB0aGlzIHBhdGNo
IHNlcmllcyBhcmU6DQo+IA0KPiBObywgeW91ciBwYXRjaCBkb2Vzbid0IGZpeCBzY3NpIHF1aWVz
Y2Ugb24gYmxvY2sgbGVnYWN5LCBzbyBub3QgYW4gYWx0ZXJuYXRpdmUNCj4gb2YgbXkgcGF0Y2hz
ZXQgYXQgYWxsLg0KDQpUaGlzIHBhdGNoIHNlcmllcyBkZWZpbml0ZWx5IGlzIGFuIGFsdGVybmF0
aXZlIGZvciBibGstbXEvc2NzaS1tcS4gQW5kIGFzIHlvdQ0Ka25vdyBteSBhcHByb2FjaCBjYW4g
YmUgZXh0ZW5kZWQgZWFzaWx5IHRvIHRoZSBsZWdhY3kgU0NTSSBjb3JlIGJ5IGFkZGluZw0KYmxr
X3F1ZXVlX2VudGVyKCkgLyBibGtfcXVldWVfZXhpdCgpIGNhbGxzIHdoZXJlIG5lY2Vzc2FyeSBp
biB0aGUgbGVnYWN5IGJsb2NrDQpsYXllci4gSSBoYXZlIG5vdCBkb25lIHRoaXMgYmVjYXVzZSB0
aGUgYnVnIHJlcG9ydCB3YXMgYWdhaW5zdCBzY3NpLW1xIGFuZCBub3QNCmFnYWluc3QgdGhlIGxl
Z2FjeSBTQ1NJIGNvcmUuIEFkZGl0aW9uYWxseSwgc2luY2UgdGhlIGxlZ2FjeSBibG9jayBsYXll
ciBhbmQNClNDU0kgY29yZSBhcmUgb24gdGhlaXIgd2F5IG91dCBJIGRpZCBub3Qgd2FudCB0byBz
cGVuZCB0aW1lIG9uIG1vZGlmeWluZyB0aGVzZS4NCg0KQmFydC4=

Re: [PATCH v3 0/6] Make SCSI device suspend and resume work reliably

[hch]

On Mon, Sep 25, 2017 at 04:17:27PM +0000, Bart Van Assche wrote:
> This patch series definitely is an alternative for blk-mq/scsi-mq. And as you
> know my approach can be extended easily to the legacy SCSI core by adding
> blk_queue_enter() / blk_queue_exit() calls where necessary in the legacy block
> layer. I have not done this because the bug report was against scsi-mq and not
> against the legacy SCSI core. Additionally, since the legacy block layer and
> SCSI core are on their way out I did not want to spend time on modifying these.

For now we'll have to fix both paths to have equal functionality.  Even
if scsi might be on it's way out there still are many drivers that
use the legacy request path, and with mmc we'll probably grow another
dual block layer driver soon, although hopefully the transition
period will be much shorter.

Re: [PATCH v3 0/6] Make SCSI device suspend and resume work reliably

[Ming Lei]

On Mon, Sep 25, 2017 at 04:17:27PM +0000, Bart Van Assche wrote:
> On Mon, 2017-09-25 at 10:36 +0800, Ming Lei wrote:
> > On Sat, Sep 23, 2017 at 6:13 AM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> > > It is known that during the resume following a hibernate sometimes the
> > > system hangs instead of coming up properly. This patch series fixes this
> > > problem. This patch series is an alternative for Ming Lei's "[PATCH V5
> > > 0/10] block/scsi: safe SCSI quiescing" patch series. The advantages of
> > > this patch series are:
> > 
> > No, your patch doesn't fix scsi quiesce on block legacy, so not an alternative
> > of my patchset at all.
> 
> This patch series definitely is an alternative for blk-mq/scsi-mq. And as you
> know my approach can be extended easily to the legacy SCSI core by adding
> blk_queue_enter() / blk_queue_exit() calls where necessary in the legacy block
> layer. I have not done this because the bug report was against scsi-mq and not
> against the legacy SCSI core. Additionally, since the legacy block layer and
> SCSI core are on their way out I did not want to spend time on modifying these.

Let me show you the legacy report and verification:

	https://www.spinics.net/lists/linux-block/msg17237.html

If you have transport_spi device at hand, the issue can be reproduced
in several minutes by the following way:

	- set nr_request of this disk as 4
	- while true; do
		trigger revalidate once in 5 seconds
		meantime run heavy/background concurrent I/O

-- 
Ming