* [PATCH 1/7] Introduce scsi_device_being_removed()
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
@ 2013-08-20 12:06 ` Bart Van Assche
2013-09-01 16:43 ` Christoph Hellwig
2013-08-20 12:07 ` [PATCH 2/7] Rework scsi_internal_device_unblock() Bart Van Assche
` (7 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 12:06 UTC (permalink / raw)
To: James Bottomley; +Cc: Mike Christie, Hannes Reinecke, David Milburn, linux-scsi
Introduce the helper function scsi_device_being_removed(). This patch
does not change any functionality.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Hannes Reinecke <hare@suse.de>
Cc: Mike Christie <michaelc@cs.wisc.edu>
---
drivers/scsi/device_handler/scsi_dh.c | 7 ++-----
include/scsi/scsi_device.h | 5 +++++
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 33e422e..78b3ddb 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -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;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index a44954c..69540bf 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -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)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 2/7] Rework scsi_internal_device_unblock()
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
2013-08-20 12:06 ` [PATCH 1/7] Introduce scsi_device_being_removed() Bart Van Assche
@ 2013-08-20 12:07 ` Bart Van Assche
2013-09-01 16:44 ` Christoph Hellwig
2013-08-20 12:08 ` [PATCH 3/7] Avoid re-enabling I/O after the transport became offline Bart Van Assche
` (6 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 12:07 UTC (permalink / raw)
To: James Bottomley
Cc: Mike Christie, Hannes Reinecke, David Milburn, linux-scsi,
Roland Dreier
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 <bvanassche@acm.org>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Roland Dreier <roland@kernel.org>
---
drivers/scsi/scsi_lib.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 124392f..9eb05a7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -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);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 3/7] Avoid re-enabling I/O after the transport became offline
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
2013-08-20 12:06 ` [PATCH 1/7] Introduce scsi_device_being_removed() Bart Van Assche
2013-08-20 12:07 ` [PATCH 2/7] Rework scsi_internal_device_unblock() Bart Van Assche
@ 2013-08-20 12:08 ` Bart Van Assche
2013-09-01 16:46 ` Christoph Hellwig
2013-08-20 12:08 ` [PATCH 4/7] Disallow changing the device state via sysfs into "deleted" Bart Van Assche
` (5 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 12:08 UTC (permalink / raw)
To: James Bottomley; +Cc: Mike Christie, Hannes Reinecke, David Milburn, linux-scsi
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 <bvanassche@acm.org>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
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(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9eb05a7..362855d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -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:
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 7e50061..9a5cde9 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -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);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 69540bf..f0aad47 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -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;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
` (2 preceding siblings ...)
2013-08-20 12:08 ` [PATCH 3/7] Avoid re-enabling I/O after the transport became offline Bart Van Assche
@ 2013-08-20 12:08 ` Bart Van Assche
2013-09-01 16:49 ` Christoph Hellwig
2013-08-20 12:09 ` [PATCH 5/7] Micro-optimize scsi_request_fn() Bart Van Assche
` (4 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 12:08 UTC (permalink / raw)
To: James Bottomley; +Cc: Mike Christie, Hannes Reinecke, David Milburn, linux-scsi
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 <bvanassche@acm.org>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
Cc: David Milburn <dmilburn@redhat.com>
---
drivers/scsi/scsi_sysfs.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 9a5cde9..c6276ec 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -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;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"
2013-08-20 12:08 ` [PATCH 4/7] Disallow changing the device state via sysfs into "deleted" Bart Van Assche
@ 2013-09-01 16:49 ` Christoph Hellwig
2013-09-02 18:59 ` Bart Van Assche
0 siblings, 1 reply; 29+ messages in thread
From: Christoph Hellwig @ 2013-09-01 16:49 UTC (permalink / raw)
To: Bart Van Assche
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On Tue, Aug 20, 2013 at 02:08:39PM +0200, Bart Van Assche wrote:
> 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 <hch@lst.de>
> + 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;
> return count;
> }
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"
2013-09-01 16:49 ` Christoph Hellwig
@ 2013-09-02 18:59 ` Bart Van Assche
2013-09-02 19:06 ` Christoph Hellwig
0 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-09-02 18:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On 09/01/13 18:49, Christoph Hellwig wrote:
> On Tue, Aug 20, 2013 at 02:08:39PM +0200, Bart Van Assche wrote:
>> 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 <hch@lst.de>
>
>> + 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;
>> 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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"
2013-09-02 18:59 ` Bart Van Assche
@ 2013-09-02 19:06 ` Christoph Hellwig
2013-09-02 19:12 ` Bart Van Assche
0 siblings, 1 reply; 29+ messages in thread
From: Christoph Hellwig @ 2013-09-02 19:06 UTC (permalink / raw)
To: Bart Van Assche
Cc: Christoph Hellwig, James Bottomley, Mike Christie,
Hannes Reinecke, David Milburn, linux-scsi
On Mon, Sep 02, 2013 at 08:59:30PM +0200, Bart Van Assche wrote:
> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"
2013-09-02 19:06 ` Christoph Hellwig
@ 2013-09-02 19:12 ` Bart Van Assche
2013-09-02 19:17 ` Christoph Hellwig
0 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-09-02 19:12 UTC (permalink / raw)
To: Christoph Hellwig
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On 09/02/13 21:06, Christoph Hellwig wrote:
> On Mon, Sep 02, 2013 at 08:59:30PM +0200, Bart Van Assche wrote:
>> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/7] Disallow changing the device state via sysfs into "deleted"
2013-09-02 19:12 ` Bart Van Assche
@ 2013-09-02 19:17 ` Christoph Hellwig
0 siblings, 0 replies; 29+ messages in thread
From: Christoph Hellwig @ 2013-09-02 19:17 UTC (permalink / raw)
To: Bart Van Assche
Cc: Christoph Hellwig, James Bottomley, Mike Christie,
Hannes Reinecke, David Milburn, linux-scsi
On Mon, Sep 02, 2013 at 09:12:15PM +0200, Bart Van Assche wrote:
> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 5/7] Micro-optimize scsi_request_fn()
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
` (3 preceding siblings ...)
2013-08-20 12:08 ` [PATCH 4/7] Disallow changing the device state via sysfs into "deleted" Bart Van Assche
@ 2013-08-20 12:09 ` Bart Van Assche
2013-09-01 16:50 ` Christoph Hellwig
2013-08-20 12:09 ` [PATCH 6/7] Rename scsi_get_command() and scsi_put_command() Bart Van Assche
` (3 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 12:09 UTC (permalink / raw)
To: James Bottomley; +Cc: Mike Christie, Hannes Reinecke, David Milburn, linux-scsi
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 <bvanassche@acm.org>
Acked-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
drivers/scsi/scsi_lib.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 362855d..905ac00 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -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)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 6/7] Rename scsi_get_command() and scsi_put_command()
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
` (4 preceding siblings ...)
2013-08-20 12:09 ` [PATCH 5/7] Micro-optimize scsi_request_fn() Bart Van Assche
@ 2013-08-20 12:09 ` Bart Van Assche
2013-08-20 12:10 ` [PATCH 7/7] Micro-optimize scsi_next_command() Bart Van Assche
` (2 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 12:09 UTC (permalink / raw)
To: James Bottomley; +Cc: Mike Christie, Hannes Reinecke, David Milburn, linux-scsi
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 <bvanassche@acm.org>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
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(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index eaa808e..3d3eb74 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -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)
{
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 2150596..4a1462a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -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;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 905ac00..fc18403 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -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;
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index 84a1fdf..79d18c5 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -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);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index de5f5d8..3649eef 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -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,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 7/7] Micro-optimize scsi_next_command()
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
` (5 preceding siblings ...)
2013-08-20 12:09 ` [PATCH 6/7] Rename scsi_get_command() and scsi_put_command() Bart Van Assche
@ 2013-08-20 12:10 ` Bart Van Assche
2013-09-01 17:06 ` Christoph Hellwig
2013-08-20 16:11 ` SCSI core patches for kernel 3.12 Nicholas A. Bellinger
2013-10-02 7:40 ` Christoph Hellwig
8 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 12:10 UTC (permalink / raw)
To: James Bottomley; +Cc: Mike Christie, Hannes Reinecke, David Milburn, linux-scsi
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 <bvanassche@acm.org>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
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(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 3d3eb74..721f3cc 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -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);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fc18403..f382f7d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -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);
}
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index 79d18c5..e51add0 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -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);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3649eef..f2eb304 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -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,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 7/7] Micro-optimize scsi_next_command()
2013-08-20 12:10 ` [PATCH 7/7] Micro-optimize scsi_next_command() Bart Van Assche
@ 2013-09-01 17:06 ` Christoph Hellwig
2013-09-01 17:08 ` [PATCH 8/7] scsi: cleanup scsi_requeue_command Christoph Hellwig
0 siblings, 1 reply; 29+ messages in thread
From: Christoph Hellwig @ 2013-09-01 17:06 UTC (permalink / raw)
To: Bart Van Assche
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On Tue, Aug 20, 2013 at 02:10:41PM +0200, Bart Van Assche wrote:
> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 8/7] scsi: cleanup scsi_requeue_command
2013-09-01 17:06 ` Christoph Hellwig
@ 2013-09-01 17:08 ` Christoph Hellwig
0 siblings, 0 replies; 29+ messages in thread
From: Christoph Hellwig @ 2013-09-01 17:08 UTC (permalink / raw)
To: Bart Van Assche
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
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 <hch@lst.de>
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0fc7970..2c30df0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -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);
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
` (6 preceding siblings ...)
2013-08-20 12:10 ` [PATCH 7/7] Micro-optimize scsi_next_command() Bart Van Assche
@ 2013-08-20 16:11 ` Nicholas A. Bellinger
2013-08-20 16:15 ` Bart Van Assche
2013-10-02 7:40 ` Christoph Hellwig
8 siblings, 1 reply; 29+ messages in thread
From: Nicholas A. Bellinger @ 2013-08-20 16:11 UTC (permalink / raw)
To: Bart Van Assche
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On Tue, 2013-08-20 at 14:05 +0200, Bart Van Assche wrote:
> 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
> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: SCSI core patches for kernel 3.12
2013-08-20 16:11 ` SCSI core patches for kernel 3.12 Nicholas A. Bellinger
@ 2013-08-20 16:15 ` Bart Van Assche
2013-08-20 17:04 ` Nicholas A. Bellinger
0 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 16:15 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On 08/20/13 18:11, Nicholas A. Bellinger wrote:
> On Tue, 2013-08-20 at 14:05 +0200, Bart Van Assche wrote:
>> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-08-20 16:15 ` Bart Van Assche
@ 2013-08-20 17:04 ` Nicholas A. Bellinger
2013-08-20 18:00 ` Bart Van Assche
0 siblings, 1 reply; 29+ messages in thread
From: Nicholas A. Bellinger @ 2013-08-20 17:04 UTC (permalink / raw)
To: Bart Van Assche
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On Tue, 2013-08-20 at 18:15 +0200, Bart Van Assche wrote:
> On 08/20/13 18:11, Nicholas A. Bellinger wrote:
> > On Tue, 2013-08-20 at 14:05 +0200, Bart Van Assche wrote:
> >> 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.
>
So that is 250K to 252.5K per LUN, or what..?
--nab
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-08-20 17:04 ` Nicholas A. Bellinger
@ 2013-08-20 18:00 ` Bart Van Assche
2013-08-20 20:38 ` Nicholas A. Bellinger
0 siblings, 1 reply; 29+ messages in thread
From: Bart Van Assche @ 2013-08-20 18:00 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On 08/20/13 19:04, Nicholas A. Bellinger wrote:
> On Tue, 2013-08-20 at 18:15 +0200, Bart Van Assche wrote:
>> On 08/20/13 18:11, Nicholas A. Bellinger wrote:
>>> On Tue, 2013-08-20 at 14:05 +0200, Bart Van Assche wrote:
>>>> 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.
>
> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-08-20 18:00 ` Bart Van Assche
@ 2013-08-20 20:38 ` Nicholas A. Bellinger
2013-08-21 6:48 ` Christoph Hellwig
0 siblings, 1 reply; 29+ messages in thread
From: Nicholas A. Bellinger @ 2013-08-20 20:38 UTC (permalink / raw)
To: Bart Van Assche
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
On Tue, 2013-08-20 at 20:00 +0200, Bart Van Assche wrote:
> On 08/20/13 19:04, Nicholas A. Bellinger wrote:
> > On Tue, 2013-08-20 at 18:15 +0200, Bart Van Assche wrote:
> >> On 08/20/13 18:11, Nicholas A. Bellinger wrote:
> >>> On Tue, 2013-08-20 at 14:05 +0200, Bart Van Assche wrote:
> >>>> 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.
> >
> > 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-08-20 20:38 ` Nicholas A. Bellinger
@ 2013-08-21 6:48 ` Christoph Hellwig
2013-08-21 7:35 ` Nicholas A. Bellinger
0 siblings, 1 reply; 29+ messages in thread
From: Christoph Hellwig @ 2013-08-21 6:48 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Bart Van Assche, James Bottomley, Mike Christie, Hannes Reinecke,
David Milburn, linux-scsi
On Tue, Aug 20, 2013 at 01:38:04PM -0700, Nicholas A. Bellinger wrote:
> <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.
> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-08-21 6:48 ` Christoph Hellwig
@ 2013-08-21 7:35 ` Nicholas A. Bellinger
0 siblings, 0 replies; 29+ messages in thread
From: Nicholas A. Bellinger @ 2013-08-21 7:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bart Van Assche, James Bottomley, Mike Christie, Hannes Reinecke,
David Milburn, linux-scsi
On Tue, 2013-08-20 at 23:48 -0700, Christoph Hellwig wrote:
> On Tue, Aug 20, 2013 at 01:38:04PM -0700, Nicholas A. Bellinger wrote:
> > <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.
>
> > 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!
> 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-08-20 12:05 SCSI core patches for kernel 3.12 Bart Van Assche
` (7 preceding siblings ...)
2013-08-20 16:11 ` SCSI core patches for kernel 3.12 Nicholas A. Bellinger
@ 2013-10-02 7:40 ` Christoph Hellwig
2013-10-02 15:15 ` James Bottomley
8 siblings, 1 reply; 29+ messages in thread
From: Christoph Hellwig @ 2013-10-02 7:40 UTC (permalink / raw)
To: Bart Van Assche
Cc: James Bottomley, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
James, and comments on this?
On Tue, Aug 20, 2013 at 02:05:45PM +0200, Bart Van Assche wrote:
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
---end quoted text---
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: SCSI core patches for kernel 3.12
2013-10-02 7:40 ` Christoph Hellwig
@ 2013-10-02 15:15 ` James Bottomley
2013-10-02 16:17 ` Christoph Hellwig
0 siblings, 1 reply; 29+ messages in thread
From: James Bottomley @ 2013-10-02 15:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bart Van Assche, Mike Christie, Hannes Reinecke, David Milburn,
linux-scsi
And I thought the world was ending when Linus started top posting ...
On Wed, 2013-10-02 at 00:40 -0700, Christoph Hellwig wrote:
> 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
> On Tue, Aug 20, 2013 at 02:05:45PM +0200, Bart Van Assche wrote:
> > 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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: SCSI core patches for kernel 3.12
2013-10-02 15:15 ` James Bottomley
@ 2013-10-02 16:17 ` Christoph Hellwig
0 siblings, 0 replies; 29+ messages in thread
From: Christoph Hellwig @ 2013-10-02 16:17 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, Bart Van Assche, Mike Christie,
Hannes Reinecke, David Milburn, linux-scsi
On Wed, Oct 02, 2013 at 03:15:20PM +0000, James Bottomley wrote:
> 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.
> 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.
^ permalink raw reply [flat|nested] 29+ messages in thread