Re: [PATCH] scsi: core: run queue in case of IO queueing failure

[Ming Lei <ming.lei@redhat.com>]

On Sat, Jul 18, 2020 at 01:22:27PM -0700, Bart Van Assche wrote:
> On 2020-07-08 06:14, Ming Lei wrote:
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 534b85e87c80..4d7fab9e8af9 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1694,6 +1694,16 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
> >  		 */
> >  		if (req->rq_flags & RQF_DONTPREP)
> >  			scsi_mq_uninit_cmd(cmd);
> > +
> > +		/*
> > +		 * Requests may be held in block layer queue because of
> > +		 * resource contention. We usually run queue in normal
> > +		 * completion for queuing these requests again. Block layer
> > +		 * will finish this failed request simply, run queue in case
> > +		 * of IO queueing failure so that requests can get chance to
> > +		 * be finished.
> > +		 */
> > +		scsi_run_queue(q);
> >  		break;
> >  	}
> >  	return ret;
> 
> So this patch causes blk_mq_run_hw_queues() to be called synchronously
> from inside blk_mq_run_hw_queues()? Wouldn't it be better to avoid such

OK, look this patch may risk to overflow stack.

> recursion and to run the queue asynchronously instead of synchronously
> from inside scsi_queue_rq()? The following code already exists in
> scsi_end_request():
> 
> 	blk_mq_run_hw_queues(q, true);

How about the following change?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b9adee0a9266..9798fbffe307 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -564,6 +564,15 @@ static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
 	scsi_uninit_cmd(cmd);
 }
 
+static void scsi_run_queue_async(struct scsi_device *sdev)
+{
+	if (scsi_target(sdev)->single_lun ||
+	    !list_empty(&sdev->host->starved_list))
+		kblockd_schedule_work(&sdev->requeue_work);
+	else
+		blk_mq_run_hw_queues(sdev->request_queue, true);
+}
+
 /* Returns false when no more bytes to process, true if there are more */
 static bool scsi_end_request(struct request *req, blk_status_t error,
 		unsigned int bytes)
@@ -608,11 +617,7 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
 
 	__blk_mq_end_request(req, error);
 
-	if (scsi_target(sdev)->single_lun ||
-	    !list_empty(&sdev->host->starved_list))
-		kblockd_schedule_work(&sdev->requeue_work);
-	else
-		blk_mq_run_hw_queues(q, true);
+	scsi_run_queue_async(sdev);
 
 	percpu_ref_put(&q->q_usage_counter);
 	return false;
@@ -1721,6 +1726,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 		 */
 		if (req->rq_flags & RQF_DONTPREP)
 			scsi_mq_uninit_cmd(cmd);
+		scsi_run_queue_async(sdev);
 		break;
 	}
 	return ret;


Thanks,
Ming