[PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Ming Lei]

Hi,

This patches removes the expensive atomic opeation on host-wide counter
of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
15% with this change in IO test over scsi_debug.


Ming Lei (3):
  scsi: introduce scsi_host_busy()
  scsi: read host_busy via scsi_host_busy()
  scsi: avoid to hold host-wide counter of host_busy for scsi_mq

 drivers/scsi/advansys.c                   |  8 ++++----
 drivers/scsi/hosts.c                      | 32 +++++++++++++++++++++++++++++++
 drivers/scsi/libsas/sas_scsi_host.c       |  4 ++--
 drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c       |  4 ++--
 drivers/scsi/qlogicpti.c                  |  2 +-
 drivers/scsi/scsi.c                       |  2 +-
 drivers/scsi/scsi_error.c                 |  6 +++---
 drivers/scsi/scsi_lib.c                   | 23 ++++++++++++++++------
 drivers/scsi/scsi_sysfs.c                 |  2 +-
 include/scsi/scsi_host.h                  |  1 +
 11 files changed, 65 insertions(+), 21 deletions(-)


Cc: Omar Sandoval <osandov@fb.com>,
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
Cc: James Bottomley <james.bottomley@hansenpartnership.com>,
Cc: Christoph Hellwig <hch@lst.de>,
Cc: Don Brace <don.brace@microsemi.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Laurence Oberman <loberman@redhat.com>

-- 
2.9.5

[PATCH 1/3] scsi: introduce scsi_host_busy()

[Ming Lei]

This patch introduces SCSI middle layer API of scsi_host_busy() for
drivers to read the host-wide counter of scsi_host->host_busy.

Cc: Omar Sandoval <osandov@fb.com>,
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
Cc: James Bottomley <james.bottomley@hansenpartnership.com>,
Cc: Christoph Hellwig <hch@lst.de>,
Cc: Don Brace <don.brace@microsemi.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/hosts.c     | 10 ++++++++++
 include/scsi/scsi_host.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 7649d63a1b8d..69beb30205f1 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -565,6 +565,16 @@ struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
 EXPORT_SYMBOL(scsi_host_get);
 
 /**
+ * scsi_host_busy - Return the host busy counter
+ * @shost:	Pointer to Scsi_Host to inc.
+ **/
+int scsi_host_busy(struct Scsi_Host *shost)
+{
+	return atomic_read(&shost->host_busy);
+}
+EXPORT_SYMBOL(scsi_host_busy);
+
+/**
  * scsi_host_put - dec a Scsi_Host ref count
  * @shost:	Pointer to Scsi_Host to dec.
  **/
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 12f454cb6f61..44ab89268f30 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -758,6 +758,7 @@ extern void scsi_scan_host(struct Scsi_Host *);
 extern void scsi_rescan_device(struct device *);
 extern void scsi_remove_host(struct Scsi_Host *);
 extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
+extern int scsi_host_busy(struct Scsi_Host *shost);
 extern void scsi_host_put(struct Scsi_Host *t);
 extern struct Scsi_Host *scsi_host_lookup(unsigned short);
 extern const char *scsi_host_state_name(enum scsi_host_state);
-- 
2.9.5

[PATCH 2/3] scsi: read host_busy via scsi_host_busy()

[Ming Lei]

No functional change, just replace the direct read of scsi_host->host_busy
with scsi_host_busy().

Cc: Omar Sandoval <osandov@fb.com>,
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
Cc: James Bottomley <james.bottomley@hansenpartnership.com>,
Cc: Christoph Hellwig <hch@lst.de>,
Cc: Don Brace <don.brace@microsemi.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/advansys.c                   | 8 ++++----
 drivers/scsi/libsas/sas_scsi_host.c       | 4 ++--
 drivers/scsi/megaraid/megaraid_sas_base.c | 2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c       | 4 ++--
 drivers/scsi/qlogicpti.c                  | 2 +-
 drivers/scsi/scsi.c                       | 2 +-
 drivers/scsi/scsi_error.c                 | 6 +++---
 drivers/scsi/scsi_sysfs.c                 | 2 +-
 8 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 24e57e770432..38ee024cbfc7 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -2416,8 +2416,8 @@ static void asc_prt_scsi_host(struct Scsi_Host *s)
 	struct asc_board *boardp = shost_priv(s);
 
 	printk("Scsi_Host at addr 0x%p, device %s\n", s, dev_name(boardp->dev));
-	printk(" host_busy %u, host_no %d,\n",
-	       atomic_read(&s->host_busy), s->host_no);
+	printk(" host_busy %d, host_no %d,\n",
+	       scsi_host_busy(s), s->host_no);
 
 	printk(" base 0x%lx, io_port 0x%lx, irq %d,\n",
 	       (ulong)s->base, (ulong)s->io_port, boardp->irq);
@@ -3182,8 +3182,8 @@ static void asc_prt_driver_conf(struct seq_file *m, struct Scsi_Host *shost)
 		shost->host_no);
 
 	seq_printf(m,
-		   " host_busy %u, max_id %u, max_lun %llu, max_channel %u\n",
-		   atomic_read(&shost->host_busy), shost->max_id,
+		   " host_busy %d, max_id %u, max_lun %llu, max_channel %u\n",
+		   scsi_host_busy(shost), shost->max_id,
 		   shost->max_lun, shost->max_channel);
 
 	seq_printf(m,
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index ceab5e5c41c2..33229348dcb6 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -759,7 +759,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost)
 	spin_unlock_irq(shost->host_lock);
 
 	SAS_DPRINTK("Enter %s busy: %d failed: %d\n",
-		    __func__, atomic_read(&shost->host_busy), shost->host_failed);
+		    __func__, scsi_host_busy(shost), shost->host_failed);
 	/*
 	 * Deal with commands that still have SAS tasks (i.e. they didn't
 	 * complete via the normal sas_task completion mechanism),
@@ -801,7 +801,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost)
 		goto retry;
 
 	SAS_DPRINTK("--- Exit %s: busy: %d failed: %d tries: %d\n",
-		    __func__, atomic_read(&shost->host_busy),
+		    __func__, scsi_host_busy(shost),
 		    shost->host_failed, tries);
 }
 
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index b89c6e6c0589..008a3bdfa948 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -2822,7 +2822,7 @@ static int megasas_reset_bus_host(struct scsi_cmnd *scmd)
 		"SCSI command pointer: (%p)\t SCSI host state: %d\t"
 		" SCSI host busy: %d\t FW outstanding: %d\n",
 		scmd, scmd->device->host->shost_state,
-		atomic_read((atomic_t *)&scmd->device->host->host_busy),
+		scsi_host_busy(scmd->device->host),
 		atomic_read(&instance->fw_outstanding));
 
 	/*
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 61f93a134956..b7325355df8e 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3260,7 +3260,7 @@ _base_recovery_check(struct MPT3SAS_ADAPTER *ioc)
 	 * See _wait_for_commands_to_complete() call with regards to this code.
 	 */
 	if (ioc->shost_recovery && ioc->pending_io_count) {
-		ioc->pending_io_count = atomic_read(&ioc->shost->host_busy);
+		ioc->pending_io_count = scsi_host_busy(ioc->shost);
 		if (ioc->pending_io_count == 0)
 			wake_up(&ioc->reset_wq);
 	}
@@ -6658,7 +6658,7 @@ mpt3sas_wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc)
 		return;
 
 	/* pending command count */
-	ioc->pending_io_count = atomic_read(&ioc->shost->host_busy);
+	ioc->pending_io_count = scsi_host_busy(ioc->shost);
 
 	if (!ioc->pending_io_count)
 		return;
diff --git a/drivers/scsi/qlogicpti.c b/drivers/scsi/qlogicpti.c
index cec9a14982e6..aaaaca685d65 100644
--- a/drivers/scsi/qlogicpti.c
+++ b/drivers/scsi/qlogicpti.c
@@ -959,7 +959,7 @@ static inline void update_can_queue(struct Scsi_Host *host, u_int in_ptr, u_int
 	/* Temporary workaround until bug is found and fixed (one bug has been found
 	   already, but fixing it makes things even worse) -jj */
 	int num_free = QLOGICPTI_REQ_QUEUE_LEN - REQ_QUEUE_DEPTH(in_ptr, out_ptr) - 64;
-	host->can_queue = atomic_read(&host->host_busy) + num_free;
+	host->can_queue = scsi_host_busy(host) + num_free;
 	host->sg_tablesize = QLOGICPTI_MAX_SG(num_free);
 }
 
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 4c60c260c5da..a15d078321a2 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -167,7 +167,7 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
 			if (level > 3)
 				scmd_printk(KERN_INFO, cmd,
 					    "scsi host busy %d failed %d\n",
-					    atomic_read(&cmd->device->host->host_busy),
+					    scsi_host_busy(cmd->device->host),
 					    cmd->device->host->host_failed);
 		}
 	}
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 946039117bf4..bc06e465dc56 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -65,7 +65,7 @@ void scsi_eh_wakeup(struct Scsi_Host *shost)
 {
 	lockdep_assert_held(shost->host_lock);
 
-	if (atomic_read(&shost->host_busy) == shost->host_failed) {
+	if (scsi_host_busy(shost) == shost->host_failed) {
 		trace_scsi_eh_wakeup(shost);
 		wake_up_process(shost->ehandler);
 		SCSI_LOG_ERROR_RECOVERY(5, shost_printk(KERN_INFO, shost,
@@ -2152,7 +2152,7 @@ int scsi_error_handler(void *data)
 			break;
 
 		if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
-		    shost->host_failed != atomic_read(&shost->host_busy)) {
+		    shost->host_failed != scsi_host_busy(shost)) {
 			SCSI_LOG_ERROR_RECOVERY(1,
 				shost_printk(KERN_INFO, shost,
 					     "scsi_eh_%d: sleeping\n",
@@ -2167,7 +2167,7 @@ int scsi_error_handler(void *data)
 				     "scsi_eh_%d: waking up %d/%d/%d\n",
 				     shost->host_no, shost->host_eh_scheduled,
 				     shost->host_failed,
-				     atomic_read(&shost->host_busy)));
+				     scsi_host_busy(shost)));
 
 		/*
 		 * We have a host that is failing for some reason.  Figure out
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 1e36c9a9ad17..016a1c7930a8 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -382,7 +382,7 @@ static ssize_t
 show_host_busy(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct Scsi_Host *shost = class_to_shost(dev);
-	return snprintf(buf, 20, "%d\n", atomic_read(&shost->host_busy));
+	return snprintf(buf, 20, "%d\n", scsi_host_busy(shost));
 }
 static DEVICE_ATTR(host_busy, S_IRUGO, show_host_busy, NULL);
 
-- 
2.9.5

[PATCH 3/3] scsi: avoid to hold host-wide counter of host_busy for scsi_mq

[Ming Lei]

It isn't necessary to check the host depth in scsi_queue_rq() any more
since it has been respected by blk-mq before calling scsi_queue_rq() via
getting driver tag.

Lots of LUNs may attach to same host, and per-host IOPS may reach millions
level, so we should avoid to this expensive atomic operations on the
hostwide counter in IO path.

This patch implemens scsi_host_busy() via blk_mq_tagset_busy_iter() for
reading the count of busy IOs for scsi_mq.

It is observed that IOPS is increased by 15% in IO test on scsi_debug
(32 LUNs, 32 submit queues, 1024 can_queue, libaio/dio) in one
dual-socket system.

Cc: Omar Sandoval <osandov@fb.com>,
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
Cc: James Bottomley <james.bottomley@hansenpartnership.com>,
Cc: Christoph Hellwig <hch@lst.de>,
Cc: Don Brace <don.brace@microsemi.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/hosts.c    | 24 +++++++++++++++++++++++-
 drivers/scsi/scsi_lib.c | 23 +++++++++++++++++------
 2 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 69beb30205f1..ad56e2b10ac8 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -564,13 +564,35 @@ struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(scsi_host_get);
 
+struct scsi_host_mq_in_flight {
+	int cnt;
+};
+
+static void scsi_host_check_in_flight(struct request *rq, void *data,
+		bool reserved)
+{
+	struct scsi_host_mq_in_flight *in_flight = data;
+
+	if (blk_mq_request_started(rq))
+		in_flight->cnt++;
+}
+
 /**
  * scsi_host_busy - Return the host busy counter
  * @shost:	Pointer to Scsi_Host to inc.
  **/
 int scsi_host_busy(struct Scsi_Host *shost)
 {
-	return atomic_read(&shost->host_busy);
+	struct scsi_host_mq_in_flight in_flight = {
+		.cnt = 0,
+	};
+
+	if (!shost->use_blk_mq)
+		return atomic_read(&shost->host_busy);
+
+	blk_mq_tagset_busy_iter(&shost->tag_set, scsi_host_check_in_flight,
+			&in_flight);
+	return in_flight.cnt;
 }
 EXPORT_SYMBOL(scsi_host_busy);
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0dfec0dedd5e..dc437c642934 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -345,7 +345,8 @@ static void scsi_dec_host_busy(struct Scsi_Host *shost)
 	unsigned long flags;
 
 	rcu_read_lock();
-	atomic_dec(&shost->host_busy);
+	if (!shost->use_blk_mq)
+		atomic_dec(&shost->host_busy);
 	if (unlikely(scsi_host_in_recovery(shost))) {
 		spin_lock_irqsave(shost->host_lock, flags);
 		if (shost->host_failed || shost->host_eh_scheduled)
@@ -444,7 +445,12 @@ static inline bool scsi_target_is_busy(struct scsi_target *starget)
 
 static inline bool scsi_host_is_busy(struct Scsi_Host *shost)
 {
-	if (shost->can_queue > 0 &&
+	/*
+	 * blk-mq can handle host queue busy efficiently via host-wide driver
+	 * tag allocation
+	 */
+
+	if (!shost->use_blk_mq && shost->can_queue > 0 &&
 	    atomic_read(&shost->host_busy) >= shost->can_queue)
 		return true;
 	if (atomic_read(&shost->host_blocked) > 0)
@@ -1539,9 +1545,12 @@ static inline int scsi_host_queue_ready(struct request_queue *q,
 	if (scsi_host_in_recovery(shost))
 		return 0;
 
-	busy = atomic_inc_return(&shost->host_busy) - 1;
+	if (!shost->use_blk_mq)
+		busy = atomic_inc_return(&shost->host_busy) - 1;
+	else
+		busy = 0;
 	if (atomic_read(&shost->host_blocked) > 0) {
-		if (busy)
+		if (busy || scsi_host_busy(shost))
 			goto starved;
 
 		/*
@@ -1555,7 +1564,7 @@ static inline int scsi_host_queue_ready(struct request_queue *q,
 				     "unblocking host at zero depth\n"));
 	}
 
-	if (shost->can_queue > 0 && busy >= shost->can_queue)
+	if (!shost->use_blk_mq && shost->can_queue > 0 && busy >= shost->can_queue)
 		goto starved;
 	if (shost->host_self_blocked)
 		goto starved;
@@ -1641,7 +1650,9 @@ static void scsi_kill_request(struct request *req, struct request_queue *q)
 	 * with the locks as normal issue path does.
 	 */
 	atomic_inc(&sdev->device_busy);
-	atomic_inc(&shost->host_busy);
+
+	if (!shost->use_blk_mq)
+		atomic_inc(&shost->host_busy);
 	if (starget->can_queue > 0)
 		atomic_inc(&starget->target_busy);
 
-- 
2.9.5

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTIwIGF0IDE0OjU3ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gVGhp
cyBwYXRjaGVzIHJlbW92ZXMgdGhlIGV4cGVuc2l2ZSBhdG9taWMgb3BlYXRpb24gb24gaG9zdC13
aWRlIGNvdW50ZXINCj4gb2YgLmhvc3RfYnVzeSBmb3Igc2NzaS1tcSwgYW5kIGl0IGlzIG9ic2Vy
dmVkIHRoYXQgSU9QUyBjYW4gYmUgaW5jcmVhc2VkIGJ5DQo+IDE1JSB3aXRoIHRoaXMgY2hhbmdl
IGluIElPIHRlc3Qgb3ZlciBzY3NpX2RlYnVnLg0KPiANCj4gDQo+IE1pbmcgTGVpICgzKToNCj4g
ICBzY3NpOiBpbnRyb2R1Y2Ugc2NzaV9ob3N0X2J1c3koKQ0KPiAgIHNjc2k6IHJlYWQgaG9zdF9i
dXN5IHZpYSBzY3NpX2hvc3RfYnVzeSgpDQo+ICAgc2NzaTogYXZvaWQgdG8gaG9sZCBob3N0LXdp
ZGUgY291bnRlciBvZiBob3N0X2J1c3kgZm9yIHNjc2lfbXENCj4gDQo+ICBkcml2ZXJzL3Njc2kv
YWR2YW5zeXMuYyAgICAgICAgICAgICAgICAgICB8ICA4ICsrKystLS0tDQo+ICBkcml2ZXJzL3Nj
c2kvaG9zdHMuYyAgICAgICAgICAgICAgICAgICAgICB8IDMyICsrKysrKysrKysrKysrKysrKysr
KysrKysrKysrKysNCj4gIGRyaXZlcnMvc2NzaS9saWJzYXMvc2FzX3Njc2lfaG9zdC5jICAgICAg
IHwgIDQgKystLQ0KPiAgZHJpdmVycy9zY3NpL21lZ2FyYWlkL21lZ2FyYWlkX3Nhc19iYXNlLmMg
fCAgMiArLQ0KPiAgZHJpdmVycy9zY3NpL21wdDNzYXMvbXB0M3Nhc19iYXNlLmMgICAgICAgfCAg
NCArKy0tDQo+ICBkcml2ZXJzL3Njc2kvcWxvZ2ljcHRpLmMgICAgICAgICAgICAgICAgICB8ICAy
ICstDQo+ICBkcml2ZXJzL3Njc2kvc2NzaS5jICAgICAgICAgICAgICAgICAgICAgICB8ICAyICst
DQo+ICBkcml2ZXJzL3Njc2kvc2NzaV9lcnJvci5jICAgICAgICAgICAgICAgICB8ICA2ICsrKy0t
LQ0KPiAgZHJpdmVycy9zY3NpL3Njc2lfbGliLmMgICAgICAgICAgICAgICAgICAgfCAyMyArKysr
KysrKysrKysrKysrLS0tLS0tDQo+ICBkcml2ZXJzL3Njc2kvc2NzaV9zeXNmcy5jICAgICAgICAg
ICAgICAgICB8ICAyICstDQo+ICBpbmNsdWRlL3Njc2kvc2NzaV9ob3N0LmggICAgICAgICAgICAg
ICAgICB8ICAxICsNCj4gIDExIGZpbGVzIGNoYW5nZWQsIDY1IGluc2VydGlvbnMoKyksIDIxIGRl
bGV0aW9ucygtKVwNCg0KSGVsbG8gTWluZywNCg0KRnJvbSB0aGUgTUFJTlRBSU5FUlMgZmlsZToN
Cg0KU0NTSSBTVUJTWVNURU0NCk06ICAgICAgIkphbWVzIEUuSi4gQm90dG9tbGV5IiA8amVqYkBs
aW51eC52bmV0LmlibS5jb20+DQpUOiAgICAgIGdpdCBnaXQ6Ly9naXQua2VybmVsLm9yZy9wdWIv
c2NtL2xpbnV4L2tlcm5lbC9naXQvamVqYi9zY3NpLmdpdA0KTTogICAgICAiTWFydGluIEsuIFBl
dGVyc2VuIiA8bWFydGluLnBldGVyc2VuQG9yYWNsZS5jb20+DQpUOiAgICAgIGdpdCBnaXQ6Ly9n
aXQua2VybmVsLm9yZy9wdWIvc2NtL2xpbnV4L2tlcm5lbC9naXQvbWtwL3Njc2kuZ2l0DQpMOiAg
ICAgIGxpbnV4LXNjc2lAdmdlci5rZXJuZWwub3JnDQpTOiAgICAgIE1haW50YWluZWQNCkY6ICAg
ICAgRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL3Njc2kvDQpGOiAgICAgIGRyaXZl
cnMvc2NzaS8NCkY6ICAgICAgaW5jbHVkZS9zY3NpLw0KDQpIZW5jZSBteSBzdXJwcmlzZSB3aGVu
IEkgc2F3IHRoYXQgeW91IHNlbnQgdGhpcyBwYXRjaCBzZXJpZXMgdG8gSmVucyBpbnN0ZWFkDQpv
ZiBKYW1lcyBhbmQgTWFydGluPw0KDQpCYXJ0Lg==

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Jens Axboe]

On 4/27/18 9:31 AM, Bart Van Assche wrote:
> On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
>> This patches removes the expensive atomic opeation on host-wide counter
>> of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
>> 15% with this change in IO test over scsi_debug.
>>
>>
>> Ming Lei (3):
>>   scsi: introduce scsi_host_busy()
>>   scsi: read host_busy via scsi_host_busy()
>>   scsi: avoid to hold host-wide counter of host_busy for scsi_mq
>>
>>  drivers/scsi/advansys.c                   |  8 ++++----
>>  drivers/scsi/hosts.c                      | 32 +++++++++++++++++++++++++++++++
>>  drivers/scsi/libsas/sas_scsi_host.c       |  4 ++--
>>  drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
>>  drivers/scsi/mpt3sas/mpt3sas_base.c       |  4 ++--
>>  drivers/scsi/qlogicpti.c                  |  2 +-
>>  drivers/scsi/scsi.c                       |  2 +-
>>  drivers/scsi/scsi_error.c                 |  6 +++---
>>  drivers/scsi/scsi_lib.c                   | 23 ++++++++++++++++------
>>  drivers/scsi/scsi_sysfs.c                 |  2 +-
>>  include/scsi/scsi_host.h                  |  1 +
>>  11 files changed, 65 insertions(+), 21 deletions(-)\
> 
> Hello Ming,
> 
> From the MAINTAINERS file:
> 
> SCSI SUBSYSTEM
> M:      "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
> T:      git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
> M:      "Martin K. Petersen" <martin.petersen@oracle.com>
> T:      git git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
> L:      linux-scsi@vger.kernel.org
> S:      Maintained
> F:      Documentation/devicetree/bindings/scsi/
> F:      drivers/scsi/
> F:      include/scsi/
> 
> Hence my surprise when I saw that you sent this patch series to Jens instead
> of James and Martin?

Martin and James are both on the CC as well. For what it's worth, the patch
seems like a good approach to me. To handle the case that Hannes was concerned
about (older drivers doing internal command issue), I would suggest that those
drivers get instrumented to include a inc/dec of the host busy count for
internal commands that bypass the normal tagging. That means the mq case needs
to be

blk_mq_tagset_busy_iter(&shost->tag_set, scsi_host_check_in_flight,
			&in_flight);
return in_flight.cnt + atomic_read(&shost->host_busy);

The atomic read is basically free, once we get rid of the dirty of that
variable on each IO.

-- 
Jens Axboe

Re: [PATCH 1/3] scsi: introduce scsi_host_busy()

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTIwIGF0IDE0OjU3ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gVGhp
cyBwYXRjaCBpbnRyb2R1Y2VzIFNDU0kgbWlkZGxlIGxheWVyIEFQSSBvZiBzY3NpX2hvc3RfYnVz
eSgpIGZvcg0KPiBkcml2ZXJzIHRvIHJlYWQgdGhlIGhvc3Qtd2lkZSBjb3VudGVyIG9mIHNjc2lf
aG9zdC0+aG9zdF9idXN5Lg0KDQpUaGlzIHBhdGNoIGludHJvZHVjZXMgYSBmdW5jdGlvbiB0aGF0
IGhhcyBubyBjYWxsZXJzIHNvIEkgdGhpbmsgdGhpcyBwYXRjaA0Kc2hvdWxkIGJlIGNvbWJpbmVk
IHdpdGggcGF0Y2ggMi8zIGZyb20gdGhpcyBzZXJpZXMgaW50byBhIHNpbmdsZSBwYXRjaC4NCg0K
QmFydC4NCg0KDQo=

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTI3IGF0IDA5OjM5IC0wNjAwLCBKZW5zIEF4Ym9lIHdyb3RlOg0KPiBi
bGtfbXFfdGFnc2V0X2J1c3lfaXRlcigmc2hvc3QtPnRhZ19zZXQsIHNjc2lfaG9zdF9jaGVja19p
bl9mbGlnaHQsDQo+IAkJCSZpbl9mbGlnaHQpOw0KPiByZXR1cm4gaW5fZmxpZ2h0LmNudCArIGF0
b21pY19yZWFkKCZzaG9zdC0+aG9zdF9idXN5KTsNCj4gDQo+IFRoZSBhdG9taWMgcmVhZCBpcyBi
YXNpY2FsbHkgZnJlZSwgb25jZSB3ZSBnZXQgcmlkIG9mIHRoZSBkaXJ0eSBvZiB0aGF0DQo+IHZh
cmlhYmxlIG9uIGVhY2ggSU8uDQoNCkhlbGxvIEplbnMsDQoNCldoYXQgbWFrZXMgeW91IHRoaW5r
IHRoYXQgIiArIGF0b21pY19yZWFkKCZzaG9zdC0+aG9zdF9idXN5KSIgaXMgbmVjZXNzYXJ5Pw0K
SSBhbSBub3QgYXdhcmUgb2YgYW55IGNvZGUgb3V0c2lkZSB0aGUgU0NTSSBjb3JlIHRoYXQgbW9k
aWZpZXMgdGhlIGhvc3RfYnVzeQ0KbWVtYmVyLg0KDQpUaGFua3MsDQoNCkJhcnQuDQoNCg0KDQo=

Re: [PATCH 2/3] scsi: read host_busy via scsi_host_busy()

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTIwIGF0IDE0OjU3ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gIHNo
b3dfaG9zdF9idXN5KHN0cnVjdCBkZXZpY2UgKmRldiwgc3RydWN0IGRldmljZV9hdHRyaWJ1dGUg
KmF0dHIsIGNoYXIgKmJ1ZikNCj4gIHsNCj4gIAlzdHJ1Y3QgU2NzaV9Ib3N0ICpzaG9zdCA9IGNs
YXNzX3RvX3Nob3N0KGRldik7DQo+IC0JcmV0dXJuIHNucHJpbnRmKGJ1ZiwgMjAsICIlZFxuIiwg
YXRvbWljX3JlYWQoJnNob3N0LT5ob3N0X2J1c3kpKTsNCj4gKwlyZXR1cm4gc25wcmludGYoYnVm
LCAyMCwgIiVkXG4iLCBzY3NpX2hvc3RfYnVzeShzaG9zdCkpOw0KPiAgfQ0KPiAgc3RhdGljIERF
VklDRV9BVFRSKGhvc3RfYnVzeSwgU19JUlVHTywgc2hvd19ob3N0X2J1c3ksIE5VTEwpOw0KDQpU
aGUgIiwgMjAiIHBhcnQgaXMgY2FyZ28tY3VsdCBwcm9ncmFtbWluZy4gU2luY2UgeW91IGhhdmUg
dG8gdG91Y2ggdGhpcyBjb2RlLA0KcGxlYXNlIGVpdGhlciB1c2UgInNwcmludGYoYnVmLCAuLi4p
IiBvciB1c2UgInNjbnByaW50ZihidWYsIFBBR0VfU0laRSwgLi4uKSIuDQoNClRoYW5rcywNCg0K
QmFydC4NCg0KDQoNCg==

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Laurence Oberman]

On Fri, 2018-04-27 at 15:48 +0000, Bart Van Assche wrote:
> On Fri, 2018-04-27 at 09:39 -0600, Jens Axboe wrote:
> > blk_mq_tagset_busy_iter(&shost->tag_set, scsi_host_check_in_flight,
> > 			&in_flight);
> > return in_flight.cnt + atomic_read(&shost->host_busy);
> > 
> > The atomic read is basically free, once we get rid of the dirty of
> > that
> > variable on each IO.
> 
> Hello Jens,
> 
> What makes you think that " + atomic_read(&shost->host_busy)" is
> necessary?
> I am not aware of any code outside the SCSI core that modifies the
> host_busy
> member.
> 
> Thanks,
> 
> Bart.
> 
> 
> 

As part of testing latest upstream in MQ and non-MQ I intend to test
this patch series fully on actual hardware
F/C 8G to memory backed array LUNS and of course SRP/RDMA
I have started working on this and will report back.
Thanks
Laurence

Re: [PATCH 3/3] scsi: avoid to hold host-wide counter of host_busy for scsi_mq

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTIwIGF0IDE0OjU3ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gK3N0
cnVjdCBzY3NpX2hvc3RfbXFfaW5fZmxpZ2h0IHsNCj4gKwlpbnQgY250Ow0KPiArfTsNCj4gKw0K
PiArc3RhdGljIHZvaWQgc2NzaV9ob3N0X2NoZWNrX2luX2ZsaWdodChzdHJ1Y3QgcmVxdWVzdCAq
cnEsIHZvaWQgKmRhdGEsDQo+ICsJCWJvb2wgcmVzZXJ2ZWQpDQo+ICt7DQo+ICsJc3RydWN0IHNj
c2lfaG9zdF9tcV9pbl9mbGlnaHQgKmluX2ZsaWdodCA9IGRhdGE7DQo+ICsNCj4gKwlpZiAoYmxr
X21xX3JlcXVlc3Rfc3RhcnRlZChycSkpDQo+ICsJCWluX2ZsaWdodC0+Y250Kys7DQo+ICt9DQo+
ICsNCj4gIC8qKg0KPiAgICogc2NzaV9ob3N0X2J1c3kgLSBSZXR1cm4gdGhlIGhvc3QgYnVzeSBj
b3VudGVyDQo+ICAgKiBAc2hvc3Q6CVBvaW50ZXIgdG8gU2NzaV9Ib3N0IHRvIGluYy4NCj4gICAq
Ki8NCj4gIGludCBzY3NpX2hvc3RfYnVzeShzdHJ1Y3QgU2NzaV9Ib3N0ICpzaG9zdCkNCj4gIHsN
Cj4gLQlyZXR1cm4gYXRvbWljX3JlYWQoJnNob3N0LT5ob3N0X2J1c3kpOw0KPiArCXN0cnVjdCBz
Y3NpX2hvc3RfbXFfaW5fZmxpZ2h0IGluX2ZsaWdodCA9IHsNCj4gKwkJLmNudCA9IDAsDQo+ICsJ
fTsNCj4gKw0KPiArCWlmICghc2hvc3QtPnVzZV9ibGtfbXEpDQo+ICsJCXJldHVybiBhdG9taWNf
cmVhZCgmc2hvc3QtPmhvc3RfYnVzeSk7DQo+ICsNCj4gKwlibGtfbXFfdGFnc2V0X2J1c3lfaXRl
cigmc2hvc3QtPnRhZ19zZXQsIHNjc2lfaG9zdF9jaGVja19pbl9mbGlnaHQsDQo+ICsJCQkmaW5f
ZmxpZ2h0KTsNCj4gKwlyZXR1cm4gaW5fZmxpZ2h0LmNudDsNCj4gIH0NCj4gIEVYUE9SVF9TWU1C
T0woc2NzaV9ob3N0X2J1c3kpOw0KDQpUaGlzIHBhdGNoIGludHJvZHVjZXMgYSBzdWJ0bGUgYmVo
YXZpb3IgY2hhbmdlIHRoYXQgaGFzIG5vdCBiZWVuIGV4cGxhaW5lZA0KaW4gdGhlIGNvbW1pdCBt
ZXNzYWdlLiBJZiBhIFNDU0kgcmVxdWVzdCBnZXRzIHJlcXVldWVkIHRoYXQgcmVzdWx0cyBpbiBh
DQpkZWNyZWFzZSBvZiB0aGUgLmhvc3RfYnVzeSBjb3VudGVyIGJ5IHNjc2lfZGV2aWNlX3VuYnVz
eSgpIGJlZm9yZSB0aGUgcmVxdWVzdA0KaXMgcmVxdWV1ZWQgYW5kIGFuIGluY3JlYXNlIG9mIHRo
ZSBob3N0X2J1c3kgY291bnRlciB3aGVuIHNjc2lfcXVldWVfcnEoKSBpcw0KY2FsbGVkIGFnYWlu
LiBEdXJpbmcgdGhhdCB0aW1lIHN1Y2ggcmVxdWVzdHMgaGF2ZSB0aGUgc3RhdGUgTVFfUlFfQ09N
UExFVEUgYW5kDQpoZW5jZSBibGtfbXFfcmVxdWVzdF9zdGFydGVkKCkgd2lsbCByZXR1cm4gdHJ1
ZSBhbmQgc2NzaV9ob3N0X2NoZWNrX2luX2ZsaWdodCgpDQp3aWxsIGluY2x1ZGUgdGhlc2UgcmVx
dWVzdHMuIEluIG90aGVyIHdvcmRzLCB0aGlzIHBhdGNoIGludHJvZHVjZXMgYSBzdWJ0bGUNCmJl
aGF2aW9yIGNoYW5nZSB0aGF0IGhhcyBub3QgYmVlbiBleHBsYWluZWQgaW4gdGhlIGNvbW1pdCBt
ZXNzYWdlLiBIZW5jZSBJJ20NCmRvdWJ0IHRoYXQgdGhpcyBjaGFuZ2UgaXMgY29ycmVjdC4NCg0K
QmFydC4NCg0KDQoNCg==

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Jens Axboe]

On 4/27/18 9:48 AM, Bart Van Assche wrote:
> On Fri, 2018-04-27 at 09:39 -0600, Jens Axboe wrote:
>> blk_mq_tagset_busy_iter(&shost->tag_set, scsi_host_check_in_flight,
>> 			&in_flight);
>> return in_flight.cnt + atomic_read(&shost->host_busy);
>>
>> The atomic read is basically free, once we get rid of the dirty of that
>> variable on each IO.
> 
> Hello Jens,
> 
> What makes you think that " + atomic_read(&shost->host_busy)" is necessary?
> I am not aware of any code outside the SCSI core that modifies the host_busy
> member.

It's a (scalable) hack to count those as well. Going forward they should be
converted to just using reserved tags, of course.

-- 
Jens Axboe

Re: [PATCH 2/3] scsi: read host_busy via scsi_host_busy()

[Ming Lei]

On Fri, Apr 27, 2018 at 03:51:46PM +0000, Bart Van Assche wrote:
> On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
> >  show_host_busy(struct device *dev, struct device_attribute *attr, char *buf)
> >  {
> >  	struct Scsi_Host *shost = class_to_shost(dev);
> > -	return snprintf(buf, 20, "%d\n", atomic_read(&shost->host_busy));
> > +	return snprintf(buf, 20, "%d\n", scsi_host_busy(shost));
> >  }
> >  static DEVICE_ATTR(host_busy, S_IRUGO, show_host_busy, NULL);
> 
> The ", 20" part is cargo-cult programming. Since you have to touch this code,
> please either use "sprintf(buf, ...)" or use "scnprintf(buf, PAGE_SIZE, ...)".

This patch is only to replace atomic_read(&shost->host_busy) with
scsi_host_busy(shost) which returns 'int' too, so nothing related
with snprintf(buf, 20,..).

No mention the string with 20 length is enough to hold integer, so it
isn't needed too. 

So I don't see any reason to do that in this patch.

Thanks, 
Ming

Re: [PATCH 3/3] scsi: avoid to hold host-wide counter of host_busy for scsi_mq

[Ming Lei]

On Fri, Apr 27, 2018 at 04:16:48PM +0000, Bart Van Assche wrote:
> On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
> > +struct scsi_host_mq_in_flight {
> > +	int cnt;
> > +};
> > +
> > +static void scsi_host_check_in_flight(struct request *rq, void *data,
> > +		bool reserved)
> > +{
> > +	struct scsi_host_mq_in_flight *in_flight = data;
> > +
> > +	if (blk_mq_request_started(rq))
> > +		in_flight->cnt++;
> > +}
> > +
> >  /**
> >   * scsi_host_busy - Return the host busy counter
> >   * @shost:	Pointer to Scsi_Host to inc.
> >   **/
> >  int scsi_host_busy(struct Scsi_Host *shost)
> >  {
> > -	return atomic_read(&shost->host_busy);
> > +	struct scsi_host_mq_in_flight in_flight = {
> > +		.cnt = 0,
> > +	};
> > +
> > +	if (!shost->use_blk_mq)
> > +		return atomic_read(&shost->host_busy);
> > +
> > +	blk_mq_tagset_busy_iter(&shost->tag_set, scsi_host_check_in_flight,
> > +			&in_flight);
> > +	return in_flight.cnt;
> >  }
> >  EXPORT_SYMBOL(scsi_host_busy);
> 
> This patch introduces a subtle behavior change that has not been explained
> in the commit message. If a SCSI request gets requeued that results in a
> decrease of the .host_busy counter by scsi_device_unbusy() before the request
> is requeued and an increase of the host_busy counter when scsi_queue_rq() is
> called again. During that time such requests have the state MQ_RQ_COMPLETE and
> hence blk_mq_request_started() will return true and scsi_host_check_in_flight()

No, __blk_mq_requeue_request() will change the rq state into MQ_RQ_IDLE,
so such issue you worried about, please look at scsi_mq_requeue_cmd(),
which calls blk_mq_requeue_request(), which puts driver tag and updates
rq's state to IDLE.

> will include these requests. In other words, this patch introduces a subtle
> behavior change that has not been explained in the commit message. Hence I'm
> doubt that this change is correct.

As I explained above, no such issue.


Thanks,
Ming

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Ming Lei]

On Fri, Apr 27, 2018 at 09:39:47AM -0600, Jens Axboe wrote:
> On 4/27/18 9:31 AM, Bart Van Assche wrote:
> > On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
> >> This patches removes the expensive atomic opeation on host-wide counter
> >> of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
> >> 15% with this change in IO test over scsi_debug.
> >>
> >>
> >> Ming Lei (3):
> >>   scsi: introduce scsi_host_busy()
> >>   scsi: read host_busy via scsi_host_busy()
> >>   scsi: avoid to hold host-wide counter of host_busy for scsi_mq
> >>
> >>  drivers/scsi/advansys.c                   |  8 ++++----
> >>  drivers/scsi/hosts.c                      | 32 +++++++++++++++++++++++++++++++
> >>  drivers/scsi/libsas/sas_scsi_host.c       |  4 ++--
> >>  drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
> >>  drivers/scsi/mpt3sas/mpt3sas_base.c       |  4 ++--
> >>  drivers/scsi/qlogicpti.c                  |  2 +-
> >>  drivers/scsi/scsi.c                       |  2 +-
> >>  drivers/scsi/scsi_error.c                 |  6 +++---
> >>  drivers/scsi/scsi_lib.c                   | 23 ++++++++++++++++------
> >>  drivers/scsi/scsi_sysfs.c                 |  2 +-
> >>  include/scsi/scsi_host.h                  |  1 +
> >>  11 files changed, 65 insertions(+), 21 deletions(-)\
> > 
> > Hello Ming,
> > 
> > From the MAINTAINERS file:
> > 
> > SCSI SUBSYSTEM
> > M:      "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
> > T:      git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
> > M:      "Martin K. Petersen" <martin.petersen@oracle.com>
> > T:      git git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
> > L:      linux-scsi@vger.kernel.org
> > S:      Maintained
> > F:      Documentation/devicetree/bindings/scsi/
> > F:      drivers/scsi/
> > F:      include/scsi/
> > 
> > Hence my surprise when I saw that you sent this patch series to Jens instead
> > of James and Martin?
> 
> Martin and James are both on the CC as well. For what it's worth, the patch
> seems like a good approach to me. To handle the case that Hannes was concerned
> about (older drivers doing internal command issue), I would suggest that those
> drivers get instrumented to include a inc/dec of the host busy count for
> internal commands that bypass the normal tagging. That means the mq case needs
> to be
> 
> blk_mq_tagset_busy_iter(&shost->tag_set, scsi_host_check_in_flight,
> 			&in_flight);
> return in_flight.cnt + atomic_read(&shost->host_busy);
> 
> The atomic read is basically free, once we get rid of the dirty of that
> variable on each IO.

Actually the internal command isn't submitted via normal IO path, then
it won't be completed via the normal completion path(soft_irq_done, or
.timeout), so handling internal command doesn't touch the scsi generic
counter of .host_busy.

I have talked with Hannes a bit about this at LSFMM, looks he agreed too.

Thanks,
Ming

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Bart Van Assche]

On 04/20/18 00:00, Ming Lei wrote:
> This patches removes the expensive atomic opeation on host-wide counter
> of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
> 15% with this change in IO test over scsi_debug.

Hello Ming,

Do you want to resend these patches to Martin or do you perhaps that I 
do that myself?

Thanks,

Bart.

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Ming Lei]

On Fri, Jun 22, 2018 at 08:29:20AM -0700, Bart Van Assche wrote:
> On 04/20/18 00:00, Ming Lei wrote:
> > This patches removes the expensive atomic opeation on host-wide counter
> > of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
> > 15% with this change in IO test over scsi_debug.
> 
> Hello Ming,
> 
> Do you want to resend these patches to Martin or do you perhaps that I do
> that myself?

Hi Bart,

I will resend it later.

thanks,
Ming