[PATCH v2 0/4] Make SCSI device suspend and resume work reliably

[PATCH v2 0/4] 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:
- No new freeze states and hence no new freeze state variables.
- 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.

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 compared to v1 of this patch series:
- Changed the approach and rewrote the patch series.


Bart Van Assche (4):
  block: Convert RQF_PREEMPT into REQ_PREEMPT
  block: Add the QUEUE_PREEMPT_ONLY request queue flag
  block, scsi: Make SCSI device suspend and resume work reliably
  scsi-mq: Reduce suspend latency

 block/blk-core.c          | 26 +++++++++++++++++++++++---
 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   | 31 +++++++++++++++++++++----------
 include/linux/blk_types.h |  6 ++++++
 include/linux/blkdev.h    |  8 +++++---
 8 files changed, 60 insertions(+), 22 deletions(-)

-- 
2.14.1

[PATCH v2 1/4] 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 f6097b89d5d3..6db8247577a0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -245,8 +245,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);
@@ -260,7 +261,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
@@ -1271,7 +1272,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:
@@ -1280,7 +1281,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 v2 2/4] block: Add the QUEUE_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 d709c0e3a2ac..1ac337712bbd 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 v2 3/4] block, scsi: 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 slowing down 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"
  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=psync --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        | 13 ++++++++++---
 drivers/scsi/scsi_lib.c | 11 ++++++++---
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1ac337712bbd..6a190dd998aa 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1429,11 +1429,18 @@ struct request *blk_get_request(struct request_queue *q, unsigned int op,
 				gfp_t gfp_mask)
 {
 	struct request *req;
+	const bool may_sleep = gfp_mask & __GFP_DIRECT_RECLAIM;
+
+	if (unlikely(blk_queue_preempt_only(q) && !(op & REQ_PREEMPT))) {
+		if (may_sleep)
+			msleep(100);
+		else
+			return ERR_PTR(-EBUSY);
+	}
 
 	if (q->mq_ops) {
-		req = blk_mq_alloc_request(q, op,
-			(gfp_mask & __GFP_DIRECT_RECLAIM) ?
-				0 : BLK_MQ_REQ_NOWAIT);
+		req = blk_mq_alloc_request(q, op, may_sleep ?
+					   0 : BLK_MQ_REQ_NOWAIT);
 		if (!IS_ERR(req) && q->mq_ops->initialize_rq_fn)
 			q->mq_ops->initialize_rq_fn(req);
 	} else {
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6db8247577a0..e76fd6e89a81 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2889,19 +2889,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;
 }
@@ -2924,8 +2927,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

Re: [PATCH v2 3/4] block, scsi: Make SCSI device suspend and resume work reliably

[Ming Lei]

On Thu, Sep 21, 2017 at 02:22:54PM -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 slowing down 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"
>   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=psync --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        | 13 ++++++++++---
>  drivers/scsi/scsi_lib.c | 11 ++++++++---
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1ac337712bbd..6a190dd998aa 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1429,11 +1429,18 @@ struct request *blk_get_request(struct request_queue *q, unsigned int op,
>  				gfp_t gfp_mask)
>  {
>  	struct request *req;
> +	const bool may_sleep = gfp_mask & __GFP_DIRECT_RECLAIM;
> +
> +	if (unlikely(blk_queue_preempt_only(q) && !(op & REQ_PREEMPT))) {

The flag is set with queue_lock, but checked without any lock, do you
think it is safe in this way?

Also this flag isn't checked in normal I/O path, but you unfreeze
queue during scsi_device_quiesce(), then any normal I/O can come
from that time.

> +		if (may_sleep)
> +			msleep(100);

This is definitely a hack, why do you introduce the msleep()?
why is it 100? instead of other delay?

> +		else
> +			return ERR_PTR(-EBUSY);
> +	}
>  
>  	if (q->mq_ops) {
> -		req = blk_mq_alloc_request(q, op,
> -			(gfp_mask & __GFP_DIRECT_RECLAIM) ?
> -				0 : BLK_MQ_REQ_NOWAIT);
> +		req = blk_mq_alloc_request(q, op, may_sleep ?
> +					   0 : BLK_MQ_REQ_NOWAIT);
>  		if (!IS_ERR(req) && q->mq_ops->initialize_rq_fn)
>  			q->mq_ops->initialize_rq_fn(req);
>  	} else {
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 6db8247577a0..e76fd6e89a81 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2889,19 +2889,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;
>  }
> @@ -2924,8 +2927,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
> 

-- 
Ming

Re: [PATCH v2 3/4] block, scsi: Make SCSI device suspend and resume work reliably

[Bart Van Assche]

T24gRnJpLCAyMDE3LTA5LTIyIGF0IDA2OjA0ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
VGh1LCBTZXAgMjEsIDIwMTcgYXQgMDI6MjI6NTRQTSAtMDcwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IC0tLSBhL2Jsb2NrL2Jsay1jb3JlLmMNCj4gPiArKysgYi9ibG9jay9ibGstY29y
ZS5jDQo+ID4gQEAgLTE0MjksMTEgKzE0MjksMTggQEAgc3RydWN0IHJlcXVlc3QgKmJsa19nZXRf
cmVxdWVzdChzdHJ1Y3QgcmVxdWVzdF9xdWV1ZSAqcSwgdW5zaWduZWQgaW50IG9wLA0KPiA+ICAJ
CQkJZ2ZwX3QgZ2ZwX21hc2spDQo+ID4gIHsNCj4gPiAgCXN0cnVjdCByZXF1ZXN0ICpyZXE7DQo+
ID4gKwljb25zdCBib29sIG1heV9zbGVlcCA9IGdmcF9tYXNrICYgX19HRlBfRElSRUNUX1JFQ0xB
SU07DQo+ID4gKw0KPiA+ICsJaWYgKHVubGlrZWx5KGJsa19xdWV1ZV9wcmVlbXB0X29ubHkocSkg
JiYgIShvcCAmIFJFUV9QUkVFTVBUKSkpIHsNCj4gDQo+IFRoZSBmbGFnIGlzIHNldCB3aXRoIHF1
ZXVlX2xvY2ssIGJ1dCBjaGVja2VkIHdpdGhvdXQgYW55IGxvY2ssIGRvIHlvdQ0KPiB0aGluayBp
dCBpcyBzYWZlIGluIHRoaXMgd2F5Pw0KPiANCj4gQWxzbyB0aGlzIGZsYWcgaXNuJ3QgY2hlY2tl
ZCBpbiBub3JtYWwgSS9PIHBhdGgsIGJ1dCB5b3UgdW5mcmVlemUNCj4gcXVldWUgZHVyaW5nIHNj
c2lfZGV2aWNlX3F1aWVzY2UoKSwgdGhlbiBhbnkgbm9ybWFsIEkvTyBjYW4gY29tZQ0KPiBmcm9t
IHRoYXQgdGltZS4NCg0KSSB3aWxsIGFkZHJlc3MgYm90aCBjb21tZW50cyBpbiB0aGUgbmV4dCB2
ZXJzaW9uIG9mIHRoaXMgcGF0Y2ggc2VyaWVzLg0KDQpCYXJ0Lg==

[PATCH v2 4/4] scsi-mq: Reduce suspend latency

[Bart Van Assche]

Avoid that it can take 200 ms too long to wait for ongoing 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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e76fd6e89a81..34e5f0f95d01 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2901,10 +2901,15 @@ 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) {
+		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 v2 4/4] scsi-mq: Reduce suspend latency

[Ming Lei]

On Thu, Sep 21, 2017 at 02:22:55PM -0700, Bart Van Assche wrote:
> Avoid that it can take 200 ms too long to wait for ongoing 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index e76fd6e89a81..34e5f0f95d01 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2901,10 +2901,15 @@ 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) {
> +		blk_mq_freeze_queue(q);
> +		blk_mq_unfreeze_queue(q);

As I commented in another patch, you don't check the 'preempt only'
flag in normal I/O path, then from now on, any I/O can come.


> +	} else {
>  		scsi_run_queue(q);
> +		while (atomic_read(&sdev->device_busy)) {
> +			msleep_interruptible(200);
> +			scsi_run_queue(q);
> +		}

Are you sure only blk-mq need to drain queue? We need
to do that for block legacy too.


-- 
Ming

Re: [PATCH v2 4/4] scsi-mq: Reduce suspend latency

[Bart Van Assche]

T24gRnJpLCAyMDE3LTA5LTIyIGF0IDA2OjA2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
VGh1LCBTZXAgMjEsIDIwMTcgYXQgMDI6MjI6NTVQTSAtMDcwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+ICsJfSBlbHNlIHsNCj4gPiAgCQlzY3NpX3J1bl9xdWV1ZShxKTsNCj4gPiArCQl3
aGlsZSAoYXRvbWljX3JlYWQoJnNkZXYtPmRldmljZV9idXN5KSkgew0KPiA+ICsJCQltc2xlZXBf
aW50ZXJydXB0aWJsZSgyMDApOw0KPiA+ICsJCQlzY3NpX3J1bl9xdWV1ZShxKTsNCj4gPiArCQl9
DQo+IA0KPiBBcmUgeW91IHN1cmUgb25seSBibGstbXEgbmVlZCB0byBkcmFpbiBxdWV1ZT8gV2Ug
bmVlZA0KPiB0byBkbyB0aGF0IGZvciBibG9jayBsZWdhY3kgdG9vLg0KDQpUaGUgY29kZSBhYm92
ZSB5b3VyIGNvbW1lbnQgZHJhaW5zIHRoZSBxdWV1ZSBmb3IgdGhlIGxlZ2FjeSBibG9jayBsYXll
ci4NCg0KQmFydC4=

Re: [PATCH v2 4/4] scsi-mq: Reduce suspend latency

[Ming Lei]

On Thu, Sep 21, 2017 at 10:43:26PM +0000, Bart Van Assche wrote:
> On Fri, 2017-09-22 at 06:06 +0800, Ming Lei wrote:
> > On Thu, Sep 21, 2017 at 02:22:55PM -0700, Bart Van Assche wrote:
> > > +	} else {
> > >  		scsi_run_queue(q);
> > > +		while (atomic_read(&sdev->device_busy)) {
> > > +			msleep_interruptible(200);
> > > +			scsi_run_queue(q);
> > > +		}
> > 
> > Are you sure only blk-mq need to drain queue? We need
> > to do that for block legacy too.
> 
> The code above your comment drains the queue for the legacy block layer.

That is just draining the requests dispatched to SCSI layer, and there
might be lots of requests in block I/O scheduler queue or requeue or
whatever.

-- 
Ming

Re: [PATCH v2 4/4] scsi-mq: Reduce suspend latency

[Bart Van Assche]

T24gRnJpLCAyMDE3LTA5LTIyIGF0IDA3OjI1ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
VGh1LCBTZXAgMjEsIDIwMTcgYXQgMTA6NDM6MjZQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IE9uIEZyaSwgMjAxNy0wOS0yMiBhdCAwNjowNiArMDgwMCwgTWluZyBMZWkgd3Jv
dGU6DQo+ID4gPiBPbiBUaHUsIFNlcCAyMSwgMjAxNyBhdCAwMjoyMjo1NVBNIC0wNzAwLCBCYXJ0
IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiA+ICsJfSBlbHNlIHsNCj4gPiA+ID4gIAkJc2NzaV9y
dW5fcXVldWUocSk7DQo+ID4gPiA+ICsJCXdoaWxlIChhdG9taWNfcmVhZCgmc2Rldi0+ZGV2aWNl
X2J1c3kpKSB7DQo+ID4gPiA+ICsJCQltc2xlZXBfaW50ZXJydXB0aWJsZSgyMDApOw0KPiA+ID4g
PiArCQkJc2NzaV9ydW5fcXVldWUocSk7DQo+ID4gPiA+ICsJCX0NCj4gPiA+IA0KPiA+ID4gQXJl
IHlvdSBzdXJlIG9ubHkgYmxrLW1xIG5lZWQgdG8gZHJhaW4gcXVldWU/IFdlIG5lZWQNCj4gPiA+
IHRvIGRvIHRoYXQgZm9yIGJsb2NrIGxlZ2FjeSB0b28uDQo+ID4gDQo+ID4gVGhlIGNvZGUgYWJv
dmUgeW91ciBjb21tZW50IGRyYWlucyB0aGUgcXVldWUgZm9yIHRoZSBsZWdhY3kgYmxvY2sgbGF5
ZXIuDQo+IA0KPiBUaGF0IGlzIGp1c3QgZHJhaW5pbmcgdGhlIHJlcXVlc3RzIGRpc3BhdGNoZWQg
dG8gU0NTSSBsYXllciwgYW5kIHRoZXJlDQo+IG1pZ2h0IGJlIGxvdHMgb2YgcmVxdWVzdHMgaW4g
YmxvY2sgSS9PIHNjaGVkdWxlciBxdWV1ZSBvciByZXF1ZXVlIG9yDQo+IHdoYXRldmVyLg0KDQpU
aGVyZSBpcyBubyByZXF1ZXVlIGxpc3QgZm9yIHRoZSBsZWdhY3kgYmxvY2sgbGF5ZXIuIFRoZXJl
IGlzIG9ubHkgYSByZXF1ZXVlDQpsaXN0IGZvciBibGstbXEuDQoNCldhaXRpbmcgZm9yIEkvTyBy
ZXF1ZXN0cyB0aGF0IGFyZSBpbiBzY2hlZHVsZXIgcXVldWVzIGlzIG5vdCB0aGUgcHVycG9zZSBv
Zg0Kc2NzaV9xdWllc2NlX2RldmljZSgpLiBUaGUgcHVycG9zZSBvZiB0aGF0IGZ1bmN0aW9uIGlz
IHRvIHdhaXQgZm9yIHJlcXVlc3RzDQp0aGF0IGhhdmUgYWxyZWFkeSBiZWVuIHN0YXJ0ZWQuIFRo
ZSBzZGV2LT5kZXZpY2VfYnVzeSBjb3VudGVyIHJlcHJlc2VudHMgdGhlDQpudW1iZXIgb2Ygc3Rh
cnRlZCByZXF1ZXN0cyBzbyB3YWl0aW5nIHVudGlsIHRoYXQgY291bnRlciBoYXMgcmVhY2hlZCB6
ZXJvIGlzDQpzdWZmaWNpZW50Lg0KDQpCYXJ0Lg==

Re: [PATCH v2 4/4] scsi-mq: Reduce suspend latency

[Ming Lei]

On Thu, Sep 21, 2017 at 11:32:33PM +0000, Bart Van Assche wrote:
> On Fri, 2017-09-22 at 07:25 +0800, Ming Lei wrote:
> > On Thu, Sep 21, 2017 at 10:43:26PM +0000, Bart Van Assche wrote:
> > > On Fri, 2017-09-22 at 06:06 +0800, Ming Lei wrote:
> > > > On Thu, Sep 21, 2017 at 02:22:55PM -0700, Bart Van Assche wrote:
> > > > > +	} else {
> > > > >  		scsi_run_queue(q);
> > > > > +		while (atomic_read(&sdev->device_busy)) {
> > > > > +			msleep_interruptible(200);
> > > > > +			scsi_run_queue(q);
> > > > > +		}
> > > > 
> > > > Are you sure only blk-mq need to drain queue? We need
> > > > to do that for block legacy too.
> > > 
> > > The code above your comment drains the queue for the legacy block layer.
> > 
> > That is just draining the requests dispatched to SCSI layer, and there
> > might be lots of requests in block I/O scheduler queue or requeue or
> > whatever.
> 
> There is no requeue list for the legacy block layer. There is only a requeue
> list for blk-mq.
> 
> Waiting for I/O requests that are in scheduler queues is not the purpose of
> scsi_quiesce_device(). The purpose of that function is to wait for requests
> that have already been started. The sdev->device_busy counter represents the
> number of started requests so waiting until that counter has reached zero is
> sufficient.

Then that is simply not enough since the issue is that request pool can
be used up easily when SCSI device is quiesced, then no request is left
for RQF_PREEMPT(PM), and I/O hang is triggered.

It has been discussed for long time, I am sorry you still don't
understand it.

-- 
Ming

Re: [PATCH v2 4/4] scsi-mq: Reduce suspend latency

[Bart Van Assche]

T24gRnJpLCAyMDE3LTA5LTIyIGF0IDA3OjUzICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gVGhl
biB0aGF0IGlzIHNpbXBseSBub3QgZW5vdWdoIHNpbmNlIHRoZSBpc3N1ZSBpcyB0aGF0IHJlcXVl
c3QgcG9vbCBjYW4NCj4gYmUgdXNlZCB1cCBlYXNpbHkgd2hlbiBTQ1NJIGRldmljZSBpcyBxdWll
c2NlZCwgdGhlbiBubyByZXF1ZXN0IGlzIGxlZnQNCj4gZm9yIFJRRl9QUkVFTVBUKFBNKSwgYW5k
IEkvTyBoYW5nIGlzIHRyaWdnZXJlZC4NCj4gDQo+IEl0IGhhcyBiZWVuIGRpc2N1c3NlZCBmb3Ig
bG9uZyB0aW1lLCBJIGFtIHNvcnJ5IHlvdSBzdGlsbCBkb24ndA0KPiB1bmRlcnN0YW5kIGl0Lg0K
DQpJJ20gcmVhbGx5IHNvcnJ5IHRoYXQgeW91IGRvbid0IHVuZGVyc3RhbmQgdGhlIGludGVudGlv
biBvZiB0aGlzIHBhdGNoIHNlcmllcy4NCkFueXdheSwgbGV0J3MgY2xvc2UgdGhlIGRpc2N1c3Np
b24gaGVyZSBhbmQgY29udGludWUgb25jZSB0aGUgbmV4dCB2ZXJzaW9uIG9mDQp0aGlzIHBhdGNo
IHNlcmllcyBoYXMgYmVlbiBwb3N0ZWQuDQoNCkJhcnQu

Re: [PATCH v2 4/4] scsi-mq: Reduce suspend latency

[Ming Lei]

On Thu, Sep 21, 2017 at 11:56:33PM +0000, Bart Van Assche wrote:
> On Fri, 2017-09-22 at 07:53 +0800, Ming Lei wrote:
> > Then that is simply not enough since the issue is that request pool can
> > be used up easily when SCSI device is quiesced, then no request is left
> > for RQF_PREEMPT(PM), and I/O hang is triggered.
> > 
> > It has been discussed for long time, I am sorry you still don't
> > understand it.
> 
> I'm really sorry that you don't understand the intention of this patch series.
> Anyway, let's close the discussion here and continue once the next version of
> this patch series has been posted.

So you mean you don't want to fix Cathy's issue? which was reported from
one production system of our customer.

And definitely belongs to same kind of SCSI quiesce issue.

-- 
Ming