On Tue, Aug 04, 2020 at 02:57:52PM +0530, Kashyap Desai wrote:
> > >
> > > > However, it looks a bit
> > > > complicated, and I was thinking if one simpler approach can be
> > > > figured
> > > out.
> > >
> > > I was thinking your original approach is simple, but if you think some
> > > other simple approach I can test as part of these series.
> > > BTW, I am still not getting why you think your original approach is
> > > not good design.
> >
> > It is still not straightforward enough or simple enough for proving its
> > correctness, even though the implementation isn't complicated.
>
> Ming -
>
> I noted your comments.
>
> I have completed testing and this particular latest performance issue on
> Volume is outstanding.
> Currently it is 20-25% performance drop in IOPs and we want that to be
> closed before shared host tag is enabled for <megaraid_sas> driver.
> Just for my understanding - What will be the next steps on this ?
>
> I can validate any new approach/patch for this issue.
>
Hello,
What do you think of the following patch?
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c866a4f33871..49f0fc5c7a63 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -552,8 +552,24 @@ 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);
+ else {
+ /*
+ * smp_mb() implied in either rq->end_io or blk_mq_free_request
+ * is for ordering writing .device_busy in scsi_device_unbusy()
+ * and reading sdev->restarts.
+ */
+ int old = atomic_read(&sdev->restarts);
+
+ if (old) {
+ blk_mq_run_hw_queues(sdev->request_queue, true);
+
+ /*
+ * ->restarts has to be kept as non-zero if there is
+ * new budget contention comes.
+ */
+ atomic_cmpxchg(&sdev->restarts, old, 0);
+ }
+ }
}
/* Returns false when no more bytes to process, true if there are more */
@@ -1612,8 +1628,34 @@ static void scsi_mq_put_budget(struct request_queue *q)
static bool scsi_mq_get_budget(struct request_queue *q)
{
struct scsi_device *sdev = q->queuedata;
+ int ret = scsi_dev_queue_ready(q, sdev);
- return scsi_dev_queue_ready(q, sdev);
+ if (ret)
+ return true;
+
+ /*
+ * If all in-flight requests originated from this LUN are completed
+ * before setting .restarts, sdev->device_busy will be observed as
+ * zero, then blk_mq_delay_run_hw_queue() will dispatch this request
+ * soon. Otherwise, completion of one of these request will observe
+ * the .restarts flag, and the request queue will be run for handling
+ * this request, see scsi_end_request().
+ */
+ atomic_inc(&sdev->restarts);
+
+ /*
+ * Order writing .restarts and reading .device_busy, and make sure
+ * .restarts is visible to scsi_end_request(). Its pair is implied by
+ * __blk_mq_end_request() in scsi_end_request() for ordering
+ * writing .device_busy in scsi_device_unbusy() and reading .restarts.
+ *
+ */
+ smp_mb__after_atomic();
+
+ if (unlikely(atomic_read(&sdev->device_busy) == 0 &&
+ !scsi_device_blocked(sdev)))
+ blk_mq_delay_run_hw_queues(sdev->request_queue, SCSI_QUEUE_DELAY);
+ return false;
}
static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index bc5909033d13..1a5c9a3df6d6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -109,6 +109,7 @@ struct scsi_device {
atomic_t device_busy; /* commands actually active on LLDD */
atomic_t device_blocked; /* Device returned QUEUE_FULL. */
+ atomic_t restarts;
spinlock_t list_lock;
struct list_head starved_entry;
unsigned short queue_depth; /* How deep of a queue we want */
Thanks,
Ming
