Skip to contentSkip to conversation

SCSI core patches for kernel 3.12

PATCH In review Bart Van Assche 4 participants 29 messages Aug 20, 2013 – Oct 2, 2013 SCSI

lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

The individual patches are. 0001-Introduce-scsi_device_being_removed.patch 0002-Rework-scsi_internal_device_unblock.patch 0003-Avoid-re-enabling-I-O-after-the-transport-became-off.patch 0004-Disallow-changing-the-device-state-via-sysfs-into-de.patch 0005-Micro-optimize-scsi_request_fn.patch 0006-Rename-scsi_get_command-and-scsi_put_command.patch 0007-Micro-optimize-scsi_next_command.patch

author
#1 lore raw

PATCH 1/7 Introduce scsi_device_being_removed()

Introduce the helper function scsi_device_being_removed(). This patch does not change any functionality.

Signed-off-by Bart Van Assche <[email protected]>
Acked-by Hannes Reinecke <[email protected]>
Cc Mike Christie <[email protected]>
---
 drivers/scsi/device_handler/scsi_dh.c |    7 ++-----
 include/scsi/scsi_device.h            |    5 +++++
 2 files changed, 7 insertions(+), 5 deletions(-)
drivers/scsi/device_handler/scsi_dh.c +2 -5
index 33e422e..78b3ddb 100644
@@ -156,8 +156,7 @@ store_dh_state(struct device *dev, struct device_attribute *attr,
 	struct scsi_device_handler *scsi_dh;
 	int err = -EINVAL;
 
-	if (sdev->sdev_state == SDEV_CANCEL ||
-	    sdev->sdev_state == SDEV_DEL)
+	if (scsi_device_being_removed(sdev))
 		return -ENODEV;
 
 	if (!sdev->scsi_dh_data) {

@@ -400,9 +399,7 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 	if (sdev->scsi_dh_data)
 		scsi_dh = sdev->scsi_dh_data->scsi_dh;
 	dev = get_device(&sdev->sdev_gendev);
-	if (!scsi_dh || !dev ||
-	    sdev->sdev_state == SDEV_CANCEL ||
-	    sdev->sdev_state == SDEV_DEL)
+	if (!scsi_dh || !dev || scsi_device_being_removed(sdev))
 		err = SCSI_DH_NOSYS;
 	if (sdev->sdev_state == SDEV_OFFLINE)
 		err = SCSI_DH_DEV_OFFLINED;
include/scsi/scsi_device.h +5 -0
index a44954c..69540bf 100644
@@ -455,6 +455,11 @@ static inline int scsi_device_created(struct scsi_device *sdev)
 	return sdev->sdev_state == SDEV_CREATED ||
 		sdev->sdev_state == SDEV_CREATED_BLOCK;
 }
+static inline int scsi_device_being_removed(struct scsi_device *sdev)
+{
+	return sdev->sdev_state == SDEV_CANCEL ||
+		sdev->sdev_state == SDEV_DEL;
+}
 
 /* accessor functions for the SCSI parameters */
 static inline int scsi_device_sync(struct scsi_device *sdev)
Signature · 1 line
1.7.10.4
author
#3 lore raw

PATCH 2/7 Rework scsi_internal_device_unblock()

In reply to Bart Van Assche

Except for scsi_internal_device_unblock() all SCSI device state changes happen via scsi_device_set_state(). Modify scsi_internal_device_unblock() such that it uses scsi_device_set_state() to change the device state. This requires modifying scsi_device_set_state() such that it allows the transition from SDEV_CREATED_BLOCK to the SDEV_OFFLINE and SDEV_TRANSPORT_OFFLINE states.

Note: since the SDEV_CREATED_BLOCK to SDEV_{TRANSPORT_,}OFFLINE transition is now allowed, direct scsi_device_set_state() calls that change the device state from SDEV_CREATED_BLOCK into SDEV_*OFFLINE will now proceed instead of being rejected. As far as I have been able to verify this behavior change is fine for all upstream SCSI transport drivers and SCSI LLDs.

Signed-off-by Bart Van Assche <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Roland Dreier <[email protected]>
---
 drivers/scsi/scsi_lib.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)
drivers/scsi/scsi_lib.c +11 -18
index 124392f..9eb05a7 100644
@@ -2147,6 +2147,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 		case SDEV_RUNNING:
 		case SDEV_QUIESCE:
 		case SDEV_BLOCK:
+		case SDEV_CREATED_BLOCK:
 			break;
 		default:
 			goto illegal;

@@ -2501,29 +2502,21 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 {
 	struct request_queue *q = sdev->request_queue; 
 	unsigned long flags;
+	int res;
 
 	/*
 	 * Try to transition the scsi device to SDEV_RUNNING or one of the
 	 * offlined states and goose the device queue if successful.
 	 */
-	if ((sdev->sdev_state == SDEV_BLOCK) ||
-	    (sdev->sdev_state == SDEV_TRANSPORT_OFFLINE))
-		sdev->sdev_state = new_state;
-	else if (sdev->sdev_state == SDEV_CREATED_BLOCK) {
-		if (new_state == SDEV_TRANSPORT_OFFLINE ||
-		    new_state == SDEV_OFFLINE)
-			sdev->sdev_state = new_state;
-		else
-			sdev->sdev_state = SDEV_CREATED;
-	} else if (sdev->sdev_state != SDEV_CANCEL &&
-		 sdev->sdev_state != SDEV_OFFLINE)
-		return -EINVAL;
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	blk_start_queue(q);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	return 0;
+	if (sdev->sdev_state == SDEV_CREATED_BLOCK && new_state == SDEV_RUNNING)
+		new_state = SDEV_CREATED;
+	res = scsi_device_set_state(sdev, new_state);
+	if (!scsi_device_blocked(sdev)) {
+		spin_lock_irqsave(q->queue_lock, flags);
+		blk_start_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+	return res;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_unblock);
Signature · 1 line
1.7.10.4
author
#5 lore raw

PATCH 3/7 Avoid re-enabling I/O after the transport became offline

In reply to Bart Van Assche

Functions like sd_shutdown() use scsi_execute_req() and hence set the REQ_PREEMPT flag. Requests for which this flag is set are passed to the LLD queuecommand callback for devices that are in the state SDEV_CANCEL. This means that the scsi_device_set_state(sdev, SDEV_CANCEL) call in __scsi_remove_device() will reenable I/O for offline devices.

Avoid this by introducing a new SCSI device state SDEV_CANCEL_OFFLINE. This state is reached once deleting an offline device starts. Allow the SDEV_{TRANSPORT_,}OFFLINE to SDEV_CANCEL_OFFLINE and SDEV_CANCEL_OFFLINE to SDEV_DEL transitions. Disallow the SDEV_{TRANSPORT_,}OFFLINE to SDEV_CANCEL transitions.

Note: this patch does not affect Fibre Channel LLD drivers since these drivers invoke fc_remote_port_chkready() before submitting a SCSI request to the HBA. That function already prevents commands to be submitted to the HBA for SCSI devices in state SDEV_CANCEL if the transport is offline.

Signed-off-by Bart Van Assche <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
 drivers/scsi/scsi_lib.c    |   12 +++++++++++-
 drivers/scsi/scsi_sysfs.c  |    4 +++-
 include/scsi/scsi_device.h |    4 ++++
 3 files changed, 18 insertions(+), 2 deletions(-)
drivers/scsi/scsi_lib.c +11 -1
index 9eb05a7..362855d 100644
@@ -1232,6 +1232,7 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 		switch (sdev->sdev_state) {
 		case SDEV_OFFLINE:
 		case SDEV_TRANSPORT_OFFLINE:
+		case SDEV_CANCEL_OFFLINE:
 			/*
 			 * If the device is offline we refuse to process any
 			 * commands.  The device must be brought online

@@ -2178,9 +2179,17 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 		case SDEV_CREATED:
 		case SDEV_RUNNING:
 		case SDEV_QUIESCE:
+		case SDEV_BLOCK:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
+	case SDEV_CANCEL_OFFLINE:
+		switch (oldstate) {
 		case SDEV_OFFLINE:
 		case SDEV_TRANSPORT_OFFLINE:
-		case SDEV_BLOCK:
 			break;
 		default:
 			goto illegal;

@@ -2194,6 +2203,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 		case SDEV_OFFLINE:
 		case SDEV_TRANSPORT_OFFLINE:
 		case SDEV_CANCEL:
+		case SDEV_CANCEL_OFFLINE:
 		case SDEV_CREATED_BLOCK:
 			break;
 		default:
drivers/scsi/scsi_sysfs.c +3 -1
index 7e50061..9a5cde9 100644
@@ -32,6 +32,7 @@ static const struct {
 	{ SDEV_CREATED, "created" },
 	{ SDEV_RUNNING, "running" },
 	{ SDEV_CANCEL, "cancel" },
+	{ SDEV_CANCEL_OFFLINE, "cancel-offline" },
 	{ SDEV_DEL, "deleted" },
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },

@@ -985,7 +986,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	struct device *dev = &sdev->sdev_gendev;
 
 	if (sdev->is_visible) {
-		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
+		    scsi_device_set_state(sdev, SDEV_CANCEL_OFFLINE) != 0)
 			return;
 
 		bsg_unregister_queue(sdev->request_queue);
include/scsi/scsi_device.h +4 -0
index 69540bf..f0aad47 100644
@@ -35,6 +35,8 @@ enum scsi_device_state {
 				 * All commands allowed */
 	SDEV_CANCEL,		/* beginning to delete device
 				 * Only error handler commands allowed */
+	SDEV_CANCEL_OFFLINE,	/* beginning to delete offline device
+				 * No commands allowed */
 	SDEV_DEL,		/* device deleted 
 				 * no commands allowed */
 	SDEV_QUIESCE,		/* Device quiescent.  No block commands

@@ -443,6 +445,7 @@ static inline int scsi_device_online(struct scsi_device *sdev)
 {
 	return (sdev->sdev_state != SDEV_OFFLINE &&
 		sdev->sdev_state != SDEV_TRANSPORT_OFFLINE &&
+		sdev->sdev_state != SDEV_CANCEL_OFFLINE &&
 		sdev->sdev_state != SDEV_DEL);
 }
 static inline int scsi_device_blocked(struct scsi_device *sdev)

@@ -458,6 +461,7 @@ static inline int scsi_device_created(struct scsi_device *sdev)
 static inline int scsi_device_being_removed(struct scsi_device *sdev)
 {
 	return sdev->sdev_state == SDEV_CANCEL ||
+		sdev->sdev_state == SDEV_CANCEL_OFFLINE ||
 		sdev->sdev_state == SDEV_DEL;
 }
Signature · 1 line
1.7.10.4
author
#7 lore raw

PATCH 4/7 Disallow changing the device state via sysfs into "deleted"

In reply to Bart Van Assche

Changing the state of a SCSI device via sysfs into "cancel", "cancel-offline" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this.

Signed-off-by Bart Van Assche <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc David Milburn <[email protected]>
---
 drivers/scsi/scsi_sysfs.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/scsi/scsi_sysfs.c +3 -4
index 9a5cde9..c6276ec 100644
@@ -635,10 +635,9 @@ store_state_field(struct device *dev, struct device_attribute *attr,
 			break;
 		}
 	}
-	if (!state)
-		return -EINVAL;
-
-	if (scsi_device_set_state(sdev, state))
+	if (state == 0 || state == SDEV_CANCEL ||
+	    state == SDEV_CANCEL_OFFLINE || state == SDEV_DEL ||
+	    scsi_device_set_state(sdev, state) != 0)
 		return -EINVAL;
 	return count;
 }
Signature · 1 line
1.7.10.4
Reviewed-by on v1 4/7
#8 lore raw

Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"

Quoted 3 lines · Bart Van Assche

Changing the state of a SCSI device via sysfs into "cancel", "cancel-offline" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this.

Looks good modulo a minor style nipick below.

Reviewed-by Christoph Hellwig <[email protected]>

drivers/scsi/scsi_sysfs.c store_state_field(struct device *dev, struct device_attribute *attr, · line 641

Quoted 4 lines
drivers/scsi/scsi_sysfs.c +3 -0
+	if (state == 0 || state == SDEV_CANCEL ||
+	    state == SDEV_CANCEL_OFFLINE || state == SDEV_DEL ||
+	    scsi_device_set_state(sdev, state) != 0)
 		return -EINVAL;

Doing a switch on the state would be a lot more readable here:

	switch (state) {
	case 0:
	case SDEV_CANCEL:
	case SDEV_CANCEL_OFFLINE:
	case SDEV_DEL:
		ret = -EINVAL;
		break;
	default:
		ret = scsi_device_set_state(sdev, state);
	}

	if (ret)
		return ret;
Quoted 2 lines

return count;
}

author
#9 lore raw

Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"

Quoted 16 lines · Christoph Hellwig
Quoted 3 lines · Bart Van Assche

Changing the state of a SCSI device via sysfs into "cancel", "cancel-offline" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this.

Looks good modulo a minor style nipick below.

Reviewed-by Christoph Hellwig <[email protected]>

drivers/scsi/scsi_sysfs.c store_state_field(struct device *dev, struct device_attribute *attr, · line 641

Quoted 4 lines
drivers/scsi/scsi_sysfs.c +3 -0
+	if (state == 0 || state == SDEV_CANCEL ||
+	    state == SDEV_CANCEL_OFFLINE || state == SDEV_DEL ||
+	    scsi_device_set_state(sdev, state) != 0)
  		return -EINVAL;

Doing a switch on the state would be a lot more readable here:

Quoted 15 lines
	switch (state) {
	case 0:
	case SDEV_CANCEL:
	case SDEV_CANCEL_OFFLINE:
	case SDEV_DEL:
		ret = -EINVAL;
		break;
	default:
		ret = scsi_device_set_state(sdev, state);
	}

	if (ret)
		return ret;
Quoted 2 lines
  	return count;
  }

Do you agree with changing switch (state) into switch ((int)state) ? Without that additional change gcc reports the following warning:

drivers/scsi/scsi_sysfs.c: In function ‘store_state_field’:
drivers/scsi/scsi_sysfs.c:640:2: warning: case value ‘0’ not in enumerated type ‘enum scsi_device_state’ [-Wswitch]

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw

Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"

Quoted 6 lines · Bart Van Assche

Do you agree with changing switch (state) into switch ((int)state) ? Without that additional change gcc reports the following warning:

drivers/scsi/scsi_sysfs.c: In function ?store_state_field?:
drivers/scsi/scsi_sysfs.c:640:2: warning: case value ?0? not in enumerated type ?enum scsi_device_state? [-Wswitch]

Either that, or add a SDEV_INVALID_STATE = 0 value to the enum. That variant seems a little more elegant.

author
#11 lore raw

Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"

Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Bart Van Assche

Do you agree with changing switch (state) into switch ((int)state) ? Without that additional change gcc reports the following warning:

drivers/scsi/scsi_sysfs.c: In function ?store_state_field?:
drivers/scsi/scsi_sysfs.c:640:2: warning: case value ?0? not in enumerated type ?enum scsi_device_state? [-Wswitch]

Either that, or add a SDEV_INVALID_STATE = 0 value to the enum. That variant seems a little more elegant.

Hmm ... I think the second option would require to add an additional case in sdev_set_state() to avoid a compiler warning. This is something James objected against (on June 24, see also http://thread.gmane.org/gmane.linux.scsi/82572/focus=82576).

Bart.

#12 lore raw

Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"

Quoted 4 lines · Bart Van Assche

Hmm ... I think the second option would require to add an additional case in sdev_set_state() to avoid a compiler warning. This is something James objected against (on June 24, see also
http://thread.gmane.org/gmane.linux.scsi/82572/focus=82576).

In that case the cast version seems fine to me.

author
#13 lore raw

PATCH 5/7 Micro-optimize scsi_request_fn()

In reply to Bart Van Assche

SCSI devices may only be removed by calling scsi_remove_device(). That function must invoke blk_cleanup_queue() before the final put of sdev->sdev_gendev. Since blk_cleanup_queue() waits for the block queue to drain and then tears it down, scsi_request_fn cannot be active anymore after blk_cleanup_queue() has returned and hence the get_device()/put_device() pair in scsi_request_fn is unnecessary.

Signed-off-by Bart Van Assche <[email protected]>
Acked-by Tejun Heo <[email protected]>
Reviewed-by Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
 drivers/scsi/scsi_lib.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/scsi/scsi_lib.c +3 -11
index 362855d..905ac00 100644
@@ -1544,16 +1544,14 @@ static void scsi_softirq_done(struct request *rq)
  * Lock status: IO request lock assumed to be held when called.
  */
 static void scsi_request_fn(struct request_queue *q)
+	__releases(q->queue_lock)
+	__acquires(q->queue_lock)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost;
 	struct scsi_cmnd *cmd;
 	struct request *req;
 
-	if(!get_device(&sdev->sdev_gendev))
-		/* We must be tearing the block queue down already */
-		return;
-
 	/*
 	 * To start with, we keep looping until the queue is empty, or until
 	 * the host is no longer able to accept any more requests.

@@ -1642,7 +1640,7 @@ static void scsi_request_fn(struct request_queue *q)
 			goto out_delay;
 	}
 
-	goto out;
+	return;
 
  not_ready:
 	spin_unlock_irq(shost->host_lock);

@@ -1661,12 +1659,6 @@ static void scsi_request_fn(struct request_queue *q)
 out_delay:
 	if (sdev->device_busy == 0)
 		blk_delay_queue(q, SCSI_QUEUE_DELAY);
-out:
-	/* must be careful here...if we trigger the ->remove() function
-	 * we cannot be holding the q lock */
-	spin_unlock_irq(q->queue_lock);
-	put_device(&sdev->sdev_gendev);
-	spin_lock_irq(q->queue_lock);
 }
 
 u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
Signature · 1 line
1.7.10.4
author
#15 lore raw

PATCH 6/7 Rename scsi_get_command() and scsi_put_command()

In reply to Bart Van Assche

Make it explicit which variants of scsi_get_command() and scsi_put_command() manipulate the SCSI device reference count.

This patch does not change any functionality.

Signed-off-by Bart Van Assche <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
 drivers/scsi/scsi.c         |   23 ++++++++++++-----------
 drivers/scsi/scsi_error.c   |    2 +-
 drivers/scsi/scsi_lib.c     |   10 +++++-----
 drivers/scsi/scsi_tgt_lib.c |    2 +-
 include/scsi/scsi_cmnd.h    |    8 ++++----
 5 files changed, 23 insertions(+), 22 deletions(-)
drivers/scsi/scsi.c +12 -11
index eaa808e..3d3eb74 100644
@@ -281,13 +281,14 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 EXPORT_SYMBOL_GPL(__scsi_get_command);
 
 /**
- * scsi_get_command - Allocate and setup a scsi command block
+ * scsi_get_command_and_dev() - Allocate and setup a scsi command block
  * @dev: parent scsi device
  * @gfp_mask: allocator flags
  *
  * Returns:	The allocated scsi command structure.
  */
-struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, gfp_t gfp_mask)
+struct scsi_cmnd *scsi_get_command_and_dev(struct scsi_device *dev,
+					   gfp_t gfp_mask)
 {
 	struct scsi_cmnd *cmd;
 

@@ -311,16 +312,16 @@ struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, gfp_t gfp_mask)
 
 	return cmd;
 }
-EXPORT_SYMBOL(scsi_get_command);
+EXPORT_SYMBOL(scsi_get_command_and_dev);
 
 /**
- * __scsi_put_command - Free a struct scsi_cmnd
+ * __scsi_put_command_and_dev() - Free a struct scsi_cmnd
  * @shost: dev->host
  * @cmd: Command to free
  * @dev: parent scsi device
  */
-void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
-			struct device *dev)
+void __scsi_put_command_and_dev(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
+				struct device *dev)
 {
 	unsigned long flags;
 

@@ -337,17 +338,17 @@ void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
 
 	put_device(dev);
 }
-EXPORT_SYMBOL(__scsi_put_command);
+EXPORT_SYMBOL(__scsi_put_command_and_dev);
 
 /**
- * scsi_put_command - Free a scsi command block
+ * scsi_put_command_and_dev() - Free a scsi command block
  * @cmd: command block to free
  *
  * Returns:	Nothing.
  *
  * Notes:	The command must not belong to any lists.
  */
-void scsi_put_command(struct scsi_cmnd *cmd)
+void scsi_put_command_and_dev(struct scsi_cmnd *cmd)
 {
 	struct scsi_device *sdev = cmd->device;
 	unsigned long flags;

@@ -358,9 +359,9 @@ void scsi_put_command(struct scsi_cmnd *cmd)
 	list_del_init(&cmd->list);
 	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-	__scsi_put_command(cmd->device->host, cmd, &sdev->sdev_gendev);
+	__scsi_put_command_and_dev(cmd->device->host, cmd, &sdev->sdev_gendev);
 }
-EXPORT_SYMBOL(scsi_put_command);
+EXPORT_SYMBOL(scsi_put_command_and_dev);
 
 static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
 {
drivers/scsi/scsi_error.c +1 -1
index 2150596..4a1462a 100644
@@ -1995,7 +1995,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	if (scsi_autopm_get_host(shost) < 0)
 		return FAILED;
 
-	scmd = scsi_get_command(dev, GFP_KERNEL);
+	scmd = scsi_get_command_and_dev(dev, GFP_KERNEL);
 	blk_rq_init(NULL, &req);
 	scmd->request = &req;
drivers/scsi/scsi_lib.c +5 -5
index 905ac00..fc18403 100644
@@ -116,7 +116,7 @@ static void scsi_unprep_request(struct request *req)
 	blk_unprep_request(req);
 	req->special = NULL;
 
-	scsi_put_command(cmd);
+	scsi_put_command_and_dev(cmd);
 }
 
 /**

@@ -545,7 +545,7 @@ void scsi_next_command(struct scsi_cmnd *cmd)
 	/* need to hold a reference on the device before we let go of the cmd */
 	get_device(&sdev->sdev_gendev);
 
-	scsi_put_command(cmd);
+	scsi_put_command_and_dev(cmd);
 	scsi_run_queue(q);
 
 	/* ok to remove device now */

@@ -1110,7 +1110,7 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 err_exit:
 	scsi_release_buffers(cmd);
 	cmd->request->special = NULL;
-	scsi_put_command(cmd);
+	scsi_put_command_and_dev(cmd);
 	return error;
 }
 EXPORT_SYMBOL(scsi_init_io);

@@ -1121,7 +1121,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 	struct scsi_cmnd *cmd;
 
 	if (!req->special) {
-		cmd = scsi_get_command(sdev, GFP_ATOMIC);
+		cmd = scsi_get_command_and_dev(sdev, GFP_ATOMIC);
 		if (unlikely(!cmd))
 			return NULL;
 		req->special = cmd;

@@ -1286,7 +1286,7 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 		if (req->special) {
 			struct scsi_cmnd *cmd = req->special;
 			scsi_release_buffers(cmd);
-			scsi_put_command(cmd);
+			scsi_put_command_and_dev(cmd);
 			req->special = NULL;
 		}
 		break;
drivers/scsi/scsi_tgt_lib.c +1 -1
index 84a1fdf..79d18c5 100644
@@ -155,7 +155,7 @@ void scsi_host_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 	__blk_put_request(q, rq);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-	__scsi_put_command(shost, cmd, &shost->shost_gendev);
+	__scsi_put_command_and_dev(shost, cmd, &shost->shost_gendev);
 }
 EXPORT_SYMBOL_GPL(scsi_host_put_command);
include/scsi/scsi_cmnd.h +4 -4
index de5f5d8..3649eef 100644
@@ -138,11 +138,11 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
-extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
+extern struct scsi_cmnd *scsi_get_command_and_dev(struct scsi_device *, gfp_t);
 extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);
-extern void scsi_put_command(struct scsi_cmnd *);
-extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *,
-			       struct device *);
+extern void scsi_put_command_and_dev(struct scsi_cmnd *);
+extern void __scsi_put_command_and_dev(struct Scsi_Host *, struct scsi_cmnd *,
+				       struct device *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
Signature · 1 line
1.7.10.4
author
#16 lore raw

PATCH 7/7 Micro-optimize scsi_next_command()

In reply to Bart Van Assche

Eliminate a get_device() / put_device() pair from scsi_next_command(). Both are atomic operations hence removing these slightly improves performance.

Signed-off-by Bart Van Assche <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
 drivers/scsi/scsi.c         |   34 ++++++++++++++++++++--------------
 drivers/scsi/scsi_lib.c     |    7 +------
 drivers/scsi/scsi_tgt_lib.c |    3 ++-
 include/scsi/scsi_cmnd.h    |    4 ++--
 4 files changed, 25 insertions(+), 23 deletions(-)
drivers/scsi/scsi.c +20 -14
index 3d3eb74..721f3cc 100644
@@ -315,13 +315,11 @@ struct scsi_cmnd *scsi_get_command_and_dev(struct scsi_device *dev,
 EXPORT_SYMBOL(scsi_get_command_and_dev);
 
 /**
- * __scsi_put_command_and_dev() - Free a struct scsi_cmnd
+ * __scsi_put_command() - Free a struct scsi_cmnd but keep the sdev reference
  * @shost: dev->host
  * @cmd: Command to free
- * @dev: parent scsi device
  */
-void __scsi_put_command_and_dev(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
-				struct device *dev)
+void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 {
 	unsigned long flags;
 

@@ -335,10 +333,24 @@ void __scsi_put_command_and_dev(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
 
 	if (likely(cmd != NULL))
 		scsi_pool_free_command(shost->cmd_pool, cmd);
+}
+EXPORT_SYMBOL(__scsi_put_command);
 
-	put_device(dev);
+/**
+ * scsi_put_command() - put the command but keep the sdev reference
+ */
+void scsi_put_command(struct scsi_cmnd *cmd)
+{
+	unsigned long flags;
+
+	/* serious error if the command hasn't come from a device list */
+	spin_lock_irqsave(&cmd->device->list_lock, flags);
+	BUG_ON(list_empty(&cmd->list));
+	list_del_init(&cmd->list);
+	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
+
+	__scsi_put_command(cmd->device->host, cmd);
 }
-EXPORT_SYMBOL(__scsi_put_command_and_dev);
 
 /**
  * scsi_put_command_and_dev() - Free a scsi command block

@@ -351,15 +363,9 @@ EXPORT_SYMBOL(__scsi_put_command_and_dev);
 void scsi_put_command_and_dev(struct scsi_cmnd *cmd)
 {
 	struct scsi_device *sdev = cmd->device;
-	unsigned long flags;
-
-	/* serious error if the command hasn't come from a device list */
-	spin_lock_irqsave(&cmd->device->list_lock, flags);
-	BUG_ON(list_empty(&cmd->list));
-	list_del_init(&cmd->list);
-	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-	__scsi_put_command_and_dev(cmd->device->host, cmd, &sdev->sdev_gendev);
+	scsi_put_command(cmd);
+	put_device(&sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_put_command_and_dev);
drivers/scsi/scsi_lib.c +1 -6
index fc18403..f382f7d 100644
@@ -542,13 +542,8 @@ void scsi_next_command(struct scsi_cmnd *cmd)
 	struct scsi_device *sdev = cmd->device;
 	struct request_queue *q = sdev->request_queue;
 
-	/* need to hold a reference on the device before we let go of the cmd */
-	get_device(&sdev->sdev_gendev);
-
-	scsi_put_command_and_dev(cmd);
+	scsi_put_command(cmd);
 	scsi_run_queue(q);
-
-	/* ok to remove device now */
 	put_device(&sdev->sdev_gendev);
 }
drivers/scsi/scsi_tgt_lib.c +2 -1
index 79d18c5..e51add0 100644
@@ -155,7 +155,8 @@ void scsi_host_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 	__blk_put_request(q, rq);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-	__scsi_put_command_and_dev(shost, cmd, &shost->shost_gendev);
+	__scsi_put_command(shost, cmd);
+	put_device(&shost->shost_gendev);
 }
 EXPORT_SYMBOL_GPL(scsi_host_put_command);
include/scsi/scsi_cmnd.h +2 -2
index 3649eef..f2eb304 100644
@@ -141,8 +141,8 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 extern struct scsi_cmnd *scsi_get_command_and_dev(struct scsi_device *, gfp_t);
 extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);
 extern void scsi_put_command_and_dev(struct scsi_cmnd *);
-extern void __scsi_put_command_and_dev(struct Scsi_Host *, struct scsi_cmnd *,
-				       struct device *);
+extern void scsi_put_command(struct scsi_cmnd *);
+extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
Signature · 1 line
1.7.10.4
#17 lore raw

Re: [PATCH 7/7] Micro-optimize scsi_next_command()

Quoted 3 lines · Bart Van Assche

Eliminate a get_device() / put_device() pair from scsi_next_command(). Both are atomic operations hence removing these slightly improves performance.

The same should also be applied to scsi_requeue_command, while it might not matter for performance it'll make the code a lot more readable, especially when also merging scsi_unprep_request into scsi_requeue_command. See the patch I'll send out for this ASAP.

I'm also not sure if there's a point in keeping the get/_put_command_and_dev variants. There's very few callers left and just opencoding the device reference manipulation might be cleaner.

#18 lore raw

[PATCH 8/7] scsi: cleanup scsi_requeue_command

Avoid a spurious device get/put cycle by using scsi_put_command and folding scsi_unprep_request into scsi_requeue_command.

Signed-off-by Christoph Hellwig <[email protected]>
drivers/scsi/scsi_lib.c +3 -30
index 0fc7970..2c30df0 100644
@@ -97,28 +97,6 @@ EXPORT_SYMBOL_GPL(scsi_unregister_acpi_bus_type);
  */
 #define SCSI_QUEUE_DELAY	3
 
-/*
- * Function:	scsi_unprep_request()
- *
- * Purpose:	Remove all preparation done for a request, including its
- *		associated scsi_cmnd, so that it can be requeued.
- *
- * Arguments:	req	- request to unprepare
- *
- * Lock status:	Assumed that no locks are held upon entry.
- *
- * Returns:	Nothing.
- */
-static void scsi_unprep_request(struct request *req)
-{
-	struct scsi_cmnd *cmd = req->special;
-
-	blk_unprep_request(req);
-	req->special = NULL;
-
-	scsi_put_command_and_dev(cmd);
-}
-
 /**
  * __scsi_queue_insert - private queue insertion
  * @cmd: The SCSI command being requeued

@@ -519,16 +497,11 @@ static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
 	struct request *req = cmd->request;
 	unsigned long flags;
 
-	/*
-	 * We need to hold a reference on the device to avoid the queue being
-	 * killed after the unlock and before scsi_run_queue is invoked which
-	 * may happen because scsi_unprep_request() puts the command which
-	 * releases its reference on the device.
-	 */
-	get_device(&sdev->sdev_gendev);
 
 	spin_lock_irqsave(q->queue_lock, flags);
-	scsi_unprep_request(req);
+	blk_unprep_request(req);
+	req->special = NULL;
+	scsi_put_command(cmd);
 	blk_requeue_request(q, req);
 	spin_unlock_irqrestore(q->queue_lock, flags);
#19 lore raw
In reply to Bart Van Assche
Quoted 3 lines · Bart Van Assche

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

Perhaps it would be useful to know what the performance improvement actually is..? Eg: fio numbers before and after.

Other than that, patch #7 to drop the unnecessary get_device -> put_device is nice. I've been intentionally leaving this overhead out of scsi-mq thus far, and it's useful to know that this extra case (according to this series) is not even required.

--nab

Quoted 13 lines

The individual patches are. 0001-Introduce-scsi_device_being_removed.patch 0002-Rework-scsi_internal_device_unblock.patch 0003-Avoid-re-enabling-I-O-after-the-transport-became-off.patch 0004-Disallow-changing-the-device-state-via-sysfs-into-de.patch 0005-Micro-optimize-scsi_request_fn.patch 0006-Rename-scsi_get_command-and-scsi_put_command.patch 0007-Micro-optimize-scsi_next_command.patch

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#20 lore raw
Quoted 6 lines · Nicholas A. Bellinger
Quoted 2 lines · Bart Van Assche

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

Perhaps it would be useful to know what the performance improvement actually is..? Eg: fio numbers before and after.

The optimizations in this patch series are micro-optimizations. Their performance impact is small but measurable. Peak IOPS results are improved by about 1% by each of these performance improvements for a low-latency transport.

Bart.

#21 lore raw
Quoted 5 lines · Bart Van Assche
Quoted 4 lines · Nicholas A. Bellinger
Quoted 2 lines · Bart Van Assche

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

Quoted 8 lines
Quoted 2 lines

Perhaps it would be useful to know what the performance improvement actually is..? Eg: fio numbers before and after.

The optimizations in this patch series are micro-optimizations. Their performance impact is small but measurable. Peak IOPS results are improved by about 1% by each of these performance improvements for a low-latency transport.

So that is 250K to 252.5K per LUN, or what..?

--nab

author
#22 lore raw
Quoted 9 lines · Nicholas A. Bellinger
Quoted 8 lines · Bart Van Assche
Quoted 6 lines · Nicholas A. Bellinger
Quoted 2 lines · Bart Van Assche

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

Perhaps it would be useful to know what the performance improvement actually is..? Eg: fio numbers before and after.

Quoted 6 lines
Quoted 4 lines

The optimizations in this patch series are micro-optimizations. Their performance impact is small but measurable. Peak IOPS results are improved by about 1% by each of these performance improvements for a low-latency transport.

So that is 250K to 252.5K per LUN, or what..?

The exact numbers depend on the number of CPU's in the initiator system, the number of CPU's in the target system, the transport type, HCA model, SCSI target stack configuration, target storage medium etc. If you want to verify yourself the impact of this patch without all these dependencies, that's possible by running a test against the scsi_debug driver.

Bart.

#23 lore raw
Quoted 15 lines · Bart Van Assche
Quoted 14 lines · Nicholas A. Bellinger
Quoted 12 lines · Bart Van Assche
Quoted 6 lines · Nicholas A. Bellinger
Quoted 2 lines · Bart Van Assche

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

Perhaps it would be useful to know what the performance improvement actually is..? Eg: fio numbers before and after.

The optimizations in this patch series are micro-optimizations. Their performance impact is small but measurable. Peak IOPS results are improved by about 1% by each of these performance improvements for a low-latency transport.

Quoted 9 lines
Quoted 1 line

So that is 250K to 252.5K per LUN, or what..?

The exact numbers depend on the number of CPU's in the initiator system, the number of CPU's in the target system, the transport type, HCA model, SCSI target stack configuration, target storage medium etc. If you want to verify yourself the impact of this patch without all these dependencies, that's possible by running a test against the scsi_debug driver.

<yawn>, I only care about the performance against upstream code, so that would mean scsi_debug here. Typically the onus of demonstrating a performance improvement is on the patch submitter (eg: not the reviewer).

But it would be at least useful to know the actual benefit with results as an incremental step, short of avoiding this code entirely for scsi-mq.

--nab

#24 lore raw
Quoted 4 lines · Nicholas A. Bellinger

<yawn>, I only care about the performance against upstream code, so that would mean scsi_debug here. Typically the onus of demonstrating a performance improvement is on the patch submitter (eg: not the reviewer).

Do you really care? If the patchset introduced a lot of code or ugliness I might agreee to your above statement, but it actually makes the code more obvious, simpler and also fixes some small issues so I don't think we'll need an exact performance improvement to justify it.

Quoted 3 lines

But it would be at least useful to know the actual benefit with results as an incremental step, short of avoiding this code entirely for scsi-mq.

I don't think you can avoid the device put/get entirely for that case. I've been looking over the mq code a bit more lately, and in a few places it just seems to try to cut a few too many corners. To get it out of the prototype status we'll need to make sure all edge cases are handled correctly.

#25 lore raw
Quoted 11 lines · Christoph Hellwig
Quoted 4 lines · Nicholas A. Bellinger

<yawn>, I only care about the performance against upstream code, so that would mean scsi_debug here. Typically the onus of demonstrating a performance improvement is on the patch submitter (eg: not the reviewer).

Do you really care? If the patchset introduced a lot of code or ugliness I might agreee to your above statement, but it actually makes the code more obvious, simpler and also fixes some small issues so I don't think we'll need an exact performance improvement to justify it.

Of course not. I'm curious 1) how fast Bart has a SCSI client going using scsi_request_fn(), and if it's a measurable difference 2) what that difference is.

Quoted 8 lines
Quoted 4 lines

But it would be at least useful to know the actual benefit with results as an incremental step, short of avoiding this code entirely for scsi-mq.

I don't think you can avoid the device put/get entirely for that case. I've been looking over the mq code a bit more lately, and in a few places it just seems to try to cut a few too many corners.

Yes, it's a prototype!

Quoted 3 lines

To get it out of the prototype status we'll need to make sure all edge cases are handled correctly.

Ohhh, yes. I'm not claiming that it's anything beyond a very early alpha at this stage, and have no plans for a mainline push anytime before 2014 arrives, before scsi_error.c supports per-device context recovery, or before hell freezes over.

Which ever comes first.. ;)

--nab

#26 lore raw
In reply to Bart Van Assche

James, and comments on this?

Quoted 13 lines · Bart Van Assche

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

The individual patches are. 0001-Introduce-scsi_device_being_removed.patch 0002-Rework-scsi_internal_device_unblock.patch 0003-Avoid-re-enabling-I-O-after-the-transport-became-off.patch 0004-Disallow-changing-the-device-state-via-sysfs-into-de.patch 0005-Micro-optimize-scsi_request_fn.patch 0006-Rename-scsi_get_command-and-scsi_put_command.patch 0007-Micro-optimize-scsi_next_command.patch

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---end quoted text---

#27 lore raw

And I thought the world was ending when Linus started top posting ...

Quoted 1 line · Christoph Hellwig

James, and comments on this?

You mean apart from the fact that it's a long series which seems to self confess to providing not much value and therefore not rating itself high on the review list?

So if I get the gist, the only real value is removing a pair of get/put calls in the critical path, so produce a single patch doing that and it will all go a lot faster than having to review seven code movement and pointless rename patches.

James

Quoted 23 lines · Bart Van Assche
Quoted 13 lines

This patch series consists of four patches that address device removal issues and three patches that improve performance of the SCSI mid-layer.

The individual patches are. 0001-Introduce-scsi_device_being_removed.patch 0002-Rework-scsi_internal_device_unblock.patch 0003-Avoid-re-enabling-I-O-after-the-transport-became-off.patch 0004-Disallow-changing-the-device-state-via-sysfs-into-de.patch 0005-Micro-optimize-scsi_request_fn.patch 0006-Rename-scsi_get_command-and-scsi_put_command.patch 0007-Micro-optimize-scsi_next_command.patch

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

---end quoted text---
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#28 lore raw
Quoted 1 line · James Bottomley

And I thought the world was ending when Linus started top posting ...

I think it's quite usual style if you ask a bytestander to comment on a whole mail - there's not specific context to quote in that case.

Quoted 3 lines

You mean apart from the fact that it's a long series which seems to self confess to providing not much value and therefore not rating itself high on the review list?

It seem generally useful, and has been out for over a month. For any reasonable maintained subsystem even a patch at the end of the review list should have gotten a comment from the maintainer at this point, even if it's a "go away" in the worst case.