[PATCH 0/1] Change path checker for IPR devices

[PATCH 0/1] Change path checker for IPR devices

[wenxiong]

We would like to change path checker to directio for IBM IPR devices.

Thanks for your help!
wendy
-- 

[PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[wenxiong]

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

We would like to change path checker to directio for IBM IPR devices.

Signed-off-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
---
 libmultipath/hwtable.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/libmultipath/hwtable.c
===================================================================
--- a/libmultipath/hwtable.c	2014-09-23 17:39:27.000000000 -0500
+++ b/libmultipath/hwtable.c	2014-09-23 17:40:31.419667000 -0500
@@ -630,7 +630,7 @@ static struct hwentry default_hw[] = {
 		.pgfailback    = -FAILBACK_IMMEDIATE,
 		.rr_weight     = RR_WEIGHT_NONE,
 		.no_path_retry = NO_PATH_RETRY_UNDEF,
-		.checker_name  = TUR,
+		.checker_name  = DIRECTIO,
 		.prio_name     = PRIO_ALUA,
 		.prio_args     = NULL,
 	},

-- 

Re: [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[Hannes Reinecke]

On 09/24/2014 09:57 PM, wenxiong@linux.vnet.ibm.com wrote:
> We would like to change path checker to directio for IBM IPR devices.
> 
> Signed-off-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
> ---
>  libmultipath/hwtable.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/libmultipath/hwtable.c
> ===================================================================
> --- a/libmultipath/hwtable.c	2014-09-23 17:39:27.000000000 -0500
> +++ b/libmultipath/hwtable.c	2014-09-23 17:40:31.419667000 -0500
> @@ -630,7 +630,7 @@ static struct hwentry default_hw[] = {
>  		.pgfailback    = -FAILBACK_IMMEDIATE,
>  		.rr_weight     = RR_WEIGHT_NONE,
>  		.no_path_retry = NO_PATH_RETRY_UNDEF,
> -		.checker_name  = TUR,
> +		.checker_name  = DIRECTIO,
>  		.prio_name     = PRIO_ALUA,
>  		.prio_args     = NULL,
>  	},
> 
> -- -- dm-devel mailing list dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
Any particular reason for doing so?

Using 'directio' has the drawback that it might stall under higher
load, as it's being added to the _tail_ of the request queue.
TUR is using the 'SG_IO' ioctl, which is added to the head of the
request_queue and hence won't be stalled ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[Brian King]

On 09/25/2014 01:29 AM, Hannes Reinecke wrote:
> On 09/24/2014 09:57 PM, wenxiong@linux.vnet.ibm.com wrote:
>> We would like to change path checker to directio for IBM IPR devices.
>>
>> Signed-off-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
>> ---
>>  libmultipath/hwtable.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: b/libmultipath/hwtable.c
>> ===================================================================
>> --- a/libmultipath/hwtable.c	2014-09-23 17:39:27.000000000 -0500
>> +++ b/libmultipath/hwtable.c	2014-09-23 17:40:31.419667000 -0500
>> @@ -630,7 +630,7 @@ static struct hwentry default_hw[] = {
>>  		.pgfailback    = -FAILBACK_IMMEDIATE,
>>  		.rr_weight     = RR_WEIGHT_NONE,
>>  		.no_path_retry = NO_PATH_RETRY_UNDEF,
>> -		.checker_name  = TUR,
>> +		.checker_name  = DIRECTIO,
>>  		.prio_name     = PRIO_ALUA,
>>  		.prio_args     = NULL,
>>  	},
>>
>> -- -- dm-devel mailing list dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
> Any particular reason for doing so?
> 
> Using 'directio' has the drawback that it might stall under higher
> load, as it's being added to the _tail_ of the request queue.
> TUR is using the 'SG_IO' ioctl, which is added to the head of the
> request_queue and hence won't be stalled ...

The issue we've run into started when this patch started making its
way into distros:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/scsi/scsi_error.c?id=14216561e164671ce147458653b1fea06a4ada1e

That changed the behaviour for user initiated TUR commands. After an ipr
adapter gets reset, all disk array devices require a start unit command
to be issued to them before they will accept commands. So, with the SCSI
EH change, we now end up in a scenario with dual ipr adapters where the
TUR getting issued from the health checker returns with a Not Ready response
and since SCSI EH no longer triggers the Start Unit in this scenario,
the path never recovers.

The alternative solution would be to change the TUR path checker in multipath-tools
to issue a Start Unit if it sees a 02/04/02.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [dm-devel] [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[Christoph Hellwig]

On Thu, Sep 25, 2014 at 11:47:42AM -0500, Brian King wrote:
> The issue we've run into started when this patch started making its
> way into distros:
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/scsi/scsi_error.c?id=14216561e164671ce147458653b1fea06a4ada1e
> 
> That changed the behaviour for user initiated TUR commands. After an ipr
> adapter gets reset, all disk array devices require a start unit command
> to be issued to them before they will accept commands. So, with the SCSI
> EH change, we now end up in a scenario with dual ipr adapters where the
> TUR getting issued from the health checker returns with a Not Ready response
> and since SCSI EH no longer triggers the Start Unit in this scenario,
> the path never recovers.
> 
> The alternative solution would be to change the TUR path checker in multipath-tools
> to issue a Start Unit if it sees a 02/04/02.

Or we could fix up the check introduced by the commit, with something
ala:

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a2c3d3d..7228d9e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -459,13 +459,18 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	if (! scsi_command_normalize_sense(scmd, &sshdr))
 		return FAILED;	/* no valid sense data */
 
-	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
+	if (scmd->cmnd[0] == TEST_UNIT_READY &&
+	    scmd->request->cmd_type == REQ_TYPE_FS &&
+	    scmd->scsi_done != scsi_eh_done) {
 		/*
 		 * nasty: for mid-layer issued TURs, we need to return the
 		 * actual sense data without any recovery attempt.  For eh
-		 * issued ones, we need to try to recover and interpret
+		 * issued ones, we need to try to recover and interpret,
+		 * and for pass through TURs we just need to stay out of the
+		 * way, so that the device handlers can do the right thing.
 		 */
 		return SUCCESS;
+	}
 
 	scsi_report_sense(sdev, &sshdr);
 

> 
> Thanks,
> 
> Brian
> 
> -- 
> Brian King
> Power Linux I/O
> IBM Linux Technology Center
> 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
---end quoted text---

Re: [dm-devel] [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[wenxiong]

Quoting Christoph Hellwig <hch@infradead.org>:

> On Thu, Sep 25, 2014 at 11:47:42AM -0500, Brian King wrote:
>> The issue we've run into started when this patch started making its
>> way into distros:
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/scsi/scsi_error.c?id=14216561e164671ce147458653b1fea06a4ada1e
>>
>> That changed the behaviour for user initiated TUR commands. After an ipr
>> adapter gets reset, all disk array devices require a start unit command
>> to be issued to them before they will accept commands. So, with the SCSI
>> EH change, we now end up in a scenario with dual ipr adapters where the
>> TUR getting issued from the health checker returns with a Not Ready response
>> and since SCSI EH no longer triggers the Start Unit in this scenario,
>> the path never recovers.
>>
>> The alternative solution would be to change the TUR path checker in  
>> multipath-tools
>> to issue a Start Unit if it sees a 02/04/02.
>
> Or we could fix up the check introduced by the commit, with something
> ala:
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index a2c3d3d..7228d9e 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -459,13 +459,18 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>  	if (! scsi_command_normalize_sense(scmd, &sshdr))
>  		return FAILED;	/* no valid sense data */
>
> -	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
> +	if (scmd->cmnd[0] == TEST_UNIT_READY &&
> +	    scmd->request->cmd_type == REQ_TYPE_FS &&
> +	    scmd->scsi_done != scsi_eh_done) {
>  		/*
>  		 * nasty: for mid-layer issued TURs, we need to return the
>  		 * actual sense data without any recovery attempt.  For eh
> -		 * issued ones, we need to try to recover and interpret
> +		 * issued ones, we need to try to recover and interpret,
> +		 * and for pass through TURs we just need to stay out of the
> +		 * way, so that the device handlers can do the right thing.
>  		 */
>  		return SUCCESS;
> +	}
>
>  	scsi_report_sense(sdev, &sshdr);
>
>

Hi Christoph,

We have verified above patch in our test group system yesterday and  
today. It works fine with their testcases.

Thanks,
Wendy


>>
>> Thanks,
>>
>> Brian
>>
>> --
>> Brian King
>> Power Linux I/O
>> IBM Linux Technology Center
>>
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
> ---end quoted text---
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[Christoph Hellwig]

Unfortunately the patch wasn't quite correct - all TEST_UNIT_READY
commands are sent as BLOCK_PC, so this would basically revert James'
original fix for the SATL case.

Am I right to assume you only need the call to scsi_dh->check_sense and
not the rest of the handling for the multipath path checker?  If that's
the case something like the patch below sould work:

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 5db8454..399c1c8 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -459,14 +459,6 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 	if (! scsi_command_normalize_sense(scmd, &sshdr))
 		return FAILED;	/* no valid sense data */
 
-	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
-		/*
-		 * nasty: for mid-layer issued TURs, we need to return the
-		 * actual sense data without any recovery attempt.  For eh
-		 * issued ones, we need to try to recover and interpret
-		 */
-		return SUCCESS;
-
 	scsi_report_sense(sdev, &sshdr);
 
 	if (scsi_sense_is_deferred(&sshdr))
@@ -482,6 +474,14 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		/* handler does not care. Drop down to default handling */
 	}
 
+	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
+		/*
+		 * nasty: for mid-layer issued TURs, we need to return the
+		 * actual sense data without any recovery attempt.  For eh
+		 * issued ones, we need to try to recover and interpret
+		 */
+		return SUCCESS;
+
 	/*
 	 * Previous logic looked for FILEMARK, EOM or ILI which are
 	 * mainly associated with tapes and returned SUCCESS.

Re: [dm-devel] [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[Brian King]

On 10/01/2014 07:51 AM, Christoph Hellwig wrote:
> Unfortunately the patch wasn't quite correct - all TEST_UNIT_READY
> commands are sent as BLOCK_PC, so this would basically revert James'
> original fix for the SATL case.
> 
> Am I right to assume you only need the call to scsi_dh->check_sense and
> not the rest of the handling for the multipath path checker?  If that's
> the case something like the patch below sould work:

This would work if we also duplicated the 02/04/02 K/C/Q check in alua_check_sense
handler.

Wendy - can you try my patch below, along with Christoph's latest patch here
and see if that resolves the issue?

Thanks,

Brian

> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 5db8454..399c1c8 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -459,14 +459,6 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>  	if (! scsi_command_normalize_sense(scmd, &sshdr))
>  		return FAILED;	/* no valid sense data */
> 
> -	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
> -		/*
> -		 * nasty: for mid-layer issued TURs, we need to return the
> -		 * actual sense data without any recovery attempt.  For eh
> -		 * issued ones, we need to try to recover and interpret
> -		 */
> -		return SUCCESS;
> -
>  	scsi_report_sense(sdev, &sshdr);
> 
>  	if (scsi_sense_is_deferred(&sshdr))
> @@ -482,6 +474,14 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>  		/* handler does not care. Drop down to default handling */
>  	}
> 
> +	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
> +		/*
> +		 * nasty: for mid-layer issued TURs, we need to return the
> +		 * actual sense data without any recovery attempt.  For eh
> +		 * issued ones, we need to try to recover and interpret
> +		 */
> +		return SUCCESS;
> +
>  	/*
>  	 * Previous logic looked for FILEMARK, EOM or ILI which are
>  	 * mainly associated with tapes and returned SUCCESS.
> 



Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 drivers/scsi/device_handler/scsi_dh_alua.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff -puN drivers/scsi/device_handler/scsi_dh_alua.c~alua_allow_restart drivers/scsi/device_handler/scsi_dh_alua.c
--- linux/drivers/scsi/device_handler/scsi_dh_alua.c~alua_allow_restart	2014-10-06 10:19:16.184798305 -0500
+++ linux-bjking1/drivers/scsi/device_handler/scsi_dh_alua.c	2014-10-06 10:20:35.743165951 -0500
@@ -474,6 +474,13 @@ static int alua_check_sense(struct scsi_
 			 * LUN Not Ready -- Offline
 			 */
 			return SUCCESS;
+		if (sdev->allow_restart &&
+		    (sense_hdr->asc == 0x04) && (sense_hdr->ascq == 0x02))
+			/*
+			 * if the device is not started, we need to wake
+			 * the error handler to start the motor
+			 */
+			return FAILED;
 		break;
 	case UNIT_ATTENTION:
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
_

Re: [dm-devel] [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[wenxiong]

Quoting Brian King <brking@linux.vnet.ibm.com>:

> On 10/01/2014 07:51 AM, Christoph Hellwig wrote:
>> Unfortunately the patch wasn't quite correct - all TEST_UNIT_READY
>> commands are sent as BLOCK_PC, so this would basically revert James'
>> original fix for the SATL case.
>>
>> Am I right to assume you only need the call to scsi_dh->check_sense and
>> not the rest of the handling for the multipath path checker?  If that's
>> the case something like the patch below sould work:
>
> This would work if we also duplicated the 02/04/02 K/C/Q check in  
> alua_check_sense
> handler.
>
> Wendy - can you try my patch below, along with Christoph's latest patch here
> and see if that resolves the issue?
>
> Thanks,
>
> Brian
>
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 5db8454..399c1c8 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -459,14 +459,6 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>>  	if (! scsi_command_normalize_sense(scmd, &sshdr))
>>  		return FAILED;	/* no valid sense data */
>>
>> -	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
>> -		/*
>> -		 * nasty: for mid-layer issued TURs, we need to return the
>> -		 * actual sense data without any recovery attempt.  For eh
>> -		 * issued ones, we need to try to recover and interpret
>> -		 */
>> -		return SUCCESS;
>> -
>>  	scsi_report_sense(sdev, &sshdr);
>>
>>  	if (scsi_sense_is_deferred(&sshdr))
>> @@ -482,6 +474,14 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
>>  		/* handler does not care. Drop down to default handling */
>>  	}
>>
>> +	if (scmd->cmnd[0] == TEST_UNIT_READY && scmd->scsi_done != scsi_eh_done)
>> +		/*
>> +		 * nasty: for mid-layer issued TURs, we need to return the
>> +		 * actual sense data without any recovery attempt.  For eh
>> +		 * issued ones, we need to try to recover and interpret
>> +		 */
>> +		return SUCCESS;
>> +
>>  	/*
>>  	 * Previous logic looked for FILEMARK, EOM or ILI which are
>>  	 * mainly associated with tapes and returned SUCCESS.
>>
>
>
>
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
> ---
>
>  drivers/scsi/device_handler/scsi_dh_alua.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff -puN  
> drivers/scsi/device_handler/scsi_dh_alua.c~alua_allow_restart  
> drivers/scsi/device_handler/scsi_dh_alua.c
> ---  
> linux/drivers/scsi/device_handler/scsi_dh_alua.c~alua_allow_restart	2014-10-06 10:19:16.184798305  
> -0500
> +++  
> linux-bjking1/drivers/scsi/device_handler/scsi_dh_alua.c	2014-10-06  
> 10:20:35.743165951 -0500
> @@ -474,6 +474,13 @@ static int alua_check_sense(struct scsi_
>  			 * LUN Not Ready -- Offline
>  			 */
>  			return SUCCESS;
> +		if (sdev->allow_restart &&
> +		    (sense_hdr->asc == 0x04) && (sense_hdr->ascq == 0x02))
> +			/*
> +			 * if the device is not started, we need to wake
> +			 * the error handler to start the motor
> +			 */
> +			return FAILED;
>  		break;
>  	case UNIT_ATTENTION:
>  		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
> _
>
> --

Sorry it took some time since we need to re-config the systems for this test.

With Christoph's new patch only, still saw the failure.
With Christoph's new patch + Brian's patch, works fine, didn't see the  
failure.


Thanks,
Wendy


> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 1/1] multipath-tools: Change path checker for IBM IPR devices

[Christoph Hellwig]

On Mon, Oct 06, 2014 at 05:50:32PM -0400, wenxiong@linux.vnet.ibm.com wrote:
> Sorry it took some time since we need to re-config the systems for this test.
> 
> With Christoph's new patch only, still saw the failure.
> With Christoph's new patch + Brian's patch, works fine, didn't see the
> failure.

Can one of you send me a tested series with both patches?

Thanks!