[PATCH 0/3] scsi: pollable 'state' attribute

[PATCH 0/3] scsi: pollable 'state' attribute

[Hannes Reinecke]

Hi all,

here's a small patchset to make the 'state' device attribute pollable.
It was supposed to be a small patch, but tnen hch suggested a rework
and I found a missing state transition.

So there you go.

Hannes Reinecke (3):
  scsi: allow state transition CREATED_BLOCK -> TRANSPORT_OFFLINE
  scsi_lib: rework scsi_internal_device_unblock_nowait()
  scsi: make 'state' device attribute pollable

 drivers/scsi/scsi_lib.c           | 22 ++++++++++++++++------
 drivers/scsi/scsi_transport_srp.c |  5 ++++-
 2 files changed, 20 insertions(+), 7 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] scsi: allow state transition CREATED_BLOCK -> TRANSPORT_OFFLINE

[Hannes Reinecke]

scsi_internal_device_unblock_nowait() allows a state transition
SDEV_CREATED_BLOCK -> SDEV_TRANSPORT_OFFLINE/SDEV_OFFLINE,
scsi_device_set_state() does not.
So add the missing state transition to scsi_device_set_state().

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 41c19c7..1ae531b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2599,6 +2599,7 @@ void scsi_exit_queue(void)
 		case SDEV_RUNNING:
 		case SDEV_QUIESCE:
 		case SDEV_BLOCK:
+		case SDEV_CREATED_BLOCK:
 			break;
 		default:
 			goto illegal;
-- 
1.8.5.6

[PATCH 2/3] scsi_lib: rework scsi_internal_device_unblock_nowait()

[Hannes Reinecke]

Rework scsi_internal_device_unblock_nowait() into using a
switch statement.
No functional changes.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1ae531b..035aa4c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3074,19 +3074,25 @@ int scsi_internal_device_unblock_nowait(struct scsi_device *sdev,
 	 * 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))
+	switch (sdev->sdev_state) {
+	case SDEV_BLOCK:
+	case SDEV_TRANSPORT_OFFLINE:
 		sdev->sdev_state = new_state;
-	else if (sdev->sdev_state == SDEV_CREATED_BLOCK) {
+		break;
+	case 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)
+		break;
+	case SDEV_TRANSPORT_OFFLINE:
+	case SDEV_CANCEL:
+	case SDEV_OFFLINE:
+		break;
+	default:
 		return -EINVAL;
-
+	}
 	scsi_start_queue(sdev);
 
 	return 0;
-- 
1.8.5.6

[PATCH 3/3] scsi: make 'state' device attribute pollable

[Hannes Reinecke]

While the 'state' attribute can (and will) change occasionally,
calling 'poll()' or 'select()' on it fails as sysfs is never
notified that the state has changed.
With this patch calling 'poll()' or 'select()' will work
properly.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c           | 3 +++
 drivers/scsi/scsi_transport_srp.c | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 035aa4c..3abef85 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2655,6 +2655,7 @@ void scsi_exit_queue(void)
 
 	}
 	sdev->sdev_state = state;
+	sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
 	return 0;
 
  illegal:
@@ -3078,6 +3079,7 @@ int scsi_internal_device_unblock_nowait(struct scsi_device *sdev,
 	case SDEV_BLOCK:
 	case SDEV_TRANSPORT_OFFLINE:
 		sdev->sdev_state = new_state;
+		sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
 		break;
 	case SDEV_CREATED_BLOCK:
 		if (new_state == SDEV_TRANSPORT_OFFLINE ||
@@ -3085,6 +3087,7 @@ int scsi_internal_device_unblock_nowait(struct scsi_device *sdev,
 			sdev->sdev_state = new_state;
 		else
 			sdev->sdev_state = SDEV_CREATED;
+		sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
 		break;
 	case SDEV_TRANSPORT_OFFLINE:
 	case SDEV_CANCEL:
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index f617021..698cc46 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -556,8 +556,11 @@ int srp_reconnect_rport(struct srp_rport *rport)
 		 */
 		shost_for_each_device(sdev, shost) {
 			mutex_lock(&sdev->state_mutex);
-			if (sdev->sdev_state == SDEV_OFFLINE)
+			if (sdev->sdev_state == SDEV_OFFLINE) {
 				sdev->sdev_state = SDEV_RUNNING;
+				sysfs_notify(&sdev->sdev_gendev.kobj,
+					     NULL, "state");
+			}
 			mutex_unlock(&sdev->state_mutex);
 		}
 	} else if (rport->state == SRP_RPORT_RUNNING) {
-- 
1.8.5.6

Re: [PATCH 1/3] scsi: allow state transition CREATED_BLOCK -> TRANSPORT_OFFLINE

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 2/3] scsi_lib: rework scsi_internal_device_unblock_nowait()

[Johannes Thumshirn]

switch()es are so much nicer to read :-)
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 3/3] scsi: make 'state' device attribute pollable

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/3] scsi: allow state transition CREATED_BLOCK -> TRANSPORT_OFFLINE

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] scsi_lib: rework scsi_internal_device_unblock_nowait()

[Christoph Hellwig]

On Thu, Aug 10, 2017 at 09:05:30AM +0200, Hannes Reinecke wrote:
> Rework scsi_internal_device_unblock_nowait() into using a
> switch statement.
> No functional changes.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_lib.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 1ae531b..035aa4c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3074,19 +3074,25 @@ int scsi_internal_device_unblock_nowait(struct scsi_device *sdev,
>  	 * 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))
> +	switch (sdev->sdev_state) {
> +	case SDEV_BLOCK:
> +	case SDEV_TRANSPORT_OFFLINE:
>  		sdev->sdev_state = new_state;
> -	else if (sdev->sdev_state == SDEV_CREATED_BLOCK) {
> +		break;
> +	case 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)
> +		break;
> +	case SDEV_TRANSPORT_OFFLINE:
> +	case SDEV_CANCEL:
> +	case SDEV_OFFLINE:
> +		break;
> +	default:
>  		return -EINVAL;

This changes ok by default to reject by default and instead lists
the ok states.  Which probably is the right thing to do for future
proofing against new states, so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] scsi: make 'state' device attribute pollable

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/3] scsi: allow state transition CREATED_BLOCK -> TRANSPORT_OFFLINE

[James Bottomley]

On Thu, 2017-08-10 at 09:05 +0200, Hannes Reinecke wrote:
> scsi_internal_device_unblock_nowait() allows a state transition
> SDEV_CREATED_BLOCK -> SDEV_TRANSPORT_OFFLINE/SDEV_OFFLINE,
> scsi_device_set_state() does not.
> So add the missing state transition to scsi_device_set_state().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_lib.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 41c19c7..1ae531b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2599,6 +2599,7 @@ void scsi_exit_queue(void)
>  		case SDEV_RUNNING:
>  		case SDEV_QUIESCE:
>  		case SDEV_BLOCK:
> +		case SDEV_CREATED_BLOCK:
>  			break;
>  		default:
>  			goto illegal;

This isn't quite good enough: a device that went CREATE_BLOCK ->
OFFLINE, the queue will still be stopped meaning I/O will pile up in it
forever and it won't restart when the device is brought online.  It we
need to set the queue running again so that the now offline device
errors all pending I/O.  It looks like this is a bug in BLOCK->OFFLINE
too.

We can't simply call scsi_start_queue() from within
scsi_device_set_state() because we may have a lock recursion problem,
so we might have to introduce a new state that allows the queue to be
restarted in the caller.

James

Re: [PATCH 2/3] scsi_lib: rework scsi_internal_device_unblock_nowait()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2888 bytes --]

Hi Hannes,

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on v4.13-rc4 next-20170810]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-pollable-state-attribute/20170811-071630
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   drivers/scsi/scsi_lib.c: In function 'scsi_internal_device_unblock_nowait':
>> drivers/scsi/scsi_lib.c:3089:2: error: duplicate case value
     case SDEV_TRANSPORT_OFFLINE:
     ^
>> drivers/scsi/scsi_lib.c:3079:2: error: previously used here
     case SDEV_TRANSPORT_OFFLINE:
     ^

vim +3089 drivers/scsi/scsi_lib.c

  3054	
  3055	/**
  3056	 * scsi_internal_device_unblock_nowait - resume a device after a block request
  3057	 * @sdev:	device to resume
  3058	 * @new_state:	state to set the device to after unblocking
  3059	 *
  3060	 * Restart the device queue for a previously suspended SCSI device. Does not
  3061	 * sleep.
  3062	 *
  3063	 * Returns zero if successful or a negative error code upon failure.
  3064	 *
  3065	 * Notes:
  3066	 * This routine transitions the device to the SDEV_RUNNING state or to one of
  3067	 * the offline states (which must be a legal transition) allowing the midlayer
  3068	 * to goose the queue for this device.
  3069	 */
  3070	int scsi_internal_device_unblock_nowait(struct scsi_device *sdev,
  3071						enum scsi_device_state new_state)
  3072	{
  3073		/*
  3074		 * Try to transition the scsi device to SDEV_RUNNING or one of the
  3075		 * offlined states and goose the device queue if successful.
  3076		 */
  3077		switch (sdev->sdev_state) {
  3078		case SDEV_BLOCK:
> 3079		case SDEV_TRANSPORT_OFFLINE:
  3080			sdev->sdev_state = new_state;
  3081			break;
  3082		case SDEV_CREATED_BLOCK:
  3083			if (new_state == SDEV_TRANSPORT_OFFLINE ||
  3084			    new_state == SDEV_OFFLINE)
  3085				sdev->sdev_state = new_state;
  3086			else
  3087				sdev->sdev_state = SDEV_CREATED;
  3088			break;
> 3089		case SDEV_TRANSPORT_OFFLINE:
  3090		case SDEV_CANCEL:
  3091		case SDEV_OFFLINE:
  3092			break;
  3093		default:
  3094			return -EINVAL;
  3095		}
  3096		scsi_start_queue(sdev);
  3097	
  3098		return 0;
  3099	}
  3100	EXPORT_SYMBOL_GPL(scsi_internal_device_unblock_nowait);
  3101	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50926 bytes --]