[PATCH v2 0/8] scsi: Support to handle Intermittent errors

[PATCH v2 0/8] scsi: Support to handle Intermittent errors

[Muneendra]

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

This patch adds a support to prevent retries of all the pending/inflight
io's after an abort succeeds on a particular device when transport
connectivity to the device is encountering intermittent errors.

Intermittent connectivity is a condition that can be detected by transport
fabric notifications. A service can monitor the ELS notifications and
take action on all the outstanding io's of a scsi device at that instant.

This feature is intended to be used when the device is part of a multipath
environment. When the service detects the poor connectivity, the multipath
path can be placed in a marginal path group and ignored further io
operations.

After placing a path in the marginal path group,the daemon sets the
port_state to Marginal which sets bit in scmd->state for all the
outstanding io's on that particular device with the new sysfs interface
provided in this patch.This prevent retries of all the pending/inflight
io's if an io hits a scsi timeout which inturn issues an abort.
On Abort succeeds on a marginal path the io will be immediately retried on 
another active path.On abort fails then the things escalates to existing
target reset sg interface recovery process.

Below is the interface provided to set the port state to Marginal
and Online.

echo "Marginal" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
echo "Online" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state


The patches were cut against  5.10/scsi-queue tree

---
v2:
Added new error code DID_TRANSPORT_MARGINAL to handle marginal errors.
Added a new rport_state FC_PORTSTATE_MARGINAL and also added a new
sysfs interface port_state to set the port_state to marginal.
Added the support in lpfc to handle the marginal state.


Muneendra (8):
  scsi: Added a new definition in scsi_cmnd.h
  scsi: Added a new error code in scsi.h
  scsi: Clear state bit SCMD_NORETRIES_ABORT of scsi_cmd before start
    request
  scsi: No retries on abort success
  scsi: Added routine to set/clear SCMD_NORETRIES_ABORT bit for
    outstanding io on scsi_dev
  scsi_transport_fc: Added a new rport state FC_PORTSTATE_MARGINAL
  scsi_transport_fc: Added a new sysfs attribute port_state
  lpfc: Added support to handle marginal state

 drivers/scsi/lpfc/lpfc_scsi.c    |   6 ++
 drivers/scsi/scsi_error.c        |  86 +++++++++++++++++++
 drivers/scsi/scsi_lib.c          |   2 +
 drivers/scsi/scsi_priv.h         |   2 +
 drivers/scsi/scsi_transport_fc.c | 140 +++++++++++++++++++++++++++----
 include/scsi/scsi.h              |   1 +
 include/scsi/scsi_cmnd.h         |   3 +
 include/scsi/scsi_transport_fc.h |  24 ++++++
 8 files changed, 246 insertions(+), 18 deletions(-)

-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

[PATCH v2 1/8] scsi: Added a new definition in scsi_cmnd.h

[Muneendra]

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

Added a new definition SCMD_NORETRIES_ABORT in scsi_cmnd.h

The SCMD_NORETRIES_ABORT defines the third bit postion
in scmd->state

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
Modified the commit log
---
 include/scsi/scsi_cmnd.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index e76bac4d14c5..e1883fee7659 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -64,6 +64,9 @@ struct scsi_pointer {
 /* for scmd->state */
 #define SCMD_STATE_COMPLETE	0
 #define SCMD_STATE_INFLIGHT	1
+#define SCMD_NORETRIES_ABORT	2 /* If this bit is set then there won't be any
+				   * retries of scmd on abort success
+				   */
 
 struct scsi_cmnd {
 	struct scsi_request req;
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 1/8] scsi: Added a new definition in scsi_cmnd.h

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Added a new definition SCMD_NORETRIES_ABORT in scsi_cmnd.h
> 
> The SCMD_NORETRIES_ABORT defines the third bit postion
> in scmd->state
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> Modified the commit log
> ---
>   include/scsi/scsi_cmnd.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index e76bac4d14c5..e1883fee7659 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -64,6 +64,9 @@ struct scsi_pointer {
>   /* for scmd->state */
>   #define SCMD_STATE_COMPLETE	0
>   #define SCMD_STATE_INFLIGHT	1
> +#define SCMD_NORETRIES_ABORT	2 /* If this bit is set then there won't be any
> +				   * retries of scmd on abort success
> +				   */
>   
>   struct scsi_cmnd {
>   	struct scsi_request req;
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH v2 2/8] scsi: Added a new error code in scsi.h

[Muneendra]

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

Added a new error code DID_TRANSPORT_MARGINAL to handle marginal
errors in scsi.h

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
New patch
---
 include/scsi/scsi.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 5339baadc082..5b287ad8b727 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -159,6 +159,7 @@ static inline int scsi_is_wlun(u64 lun)
 				 * paths might yield different results */
 #define DID_ALLOC_FAILURE 0x12  /* Space allocation on the device failed */
 #define DID_MEDIUM_ERROR  0x13  /* Medium error */
+#define DID_TRANSPORT_MARGINAL 0x14 /* Transport marginal errors */
 #define DRIVER_OK       0x00	/* Driver status                           */
 
 /*
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 2/8] scsi: Added a new error code in scsi.h

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Added a new error code DID_TRANSPORT_MARGINAL to handle marginal
> errors in scsi.h
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> New patch
> ---
>   include/scsi/scsi.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 5339baadc082..5b287ad8b727 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -159,6 +159,7 @@ static inline int scsi_is_wlun(u64 lun)
>   				 * paths might yield different results */
>   #define DID_ALLOC_FAILURE 0x12  /* Space allocation on the device failed */
>   #define DID_MEDIUM_ERROR  0x13  /* Medium error */
> +#define DID_TRANSPORT_MARGINAL 0x14 /* Transport marginal errors */
>   #define DRIVER_OK       0x00	/* Driver status                           */
>   
>   /*
> 
This should include the second hunk from the next patch to indicate how 
it'll be used.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH v2 3/8] scsi: Clear state bit SCMD_NORETRIES_ABORT of scsi_cmd before start request

[Muneendra]

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

Clearing the SCMD_NORETRIES_ABORT bit in state flag before
blk_mq_start_request.

Added a code in scsi_result_to_blk_status to translate
a new error DID_TRANSPORT_MARGINAL to the corresponding blk_status_t
i.e BLK_STS_TRANSPORT

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
Made changes in scsi_result_to_blk_status to support DID_TRANSPORT_MARGINAL
---
 drivers/scsi/scsi_lib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f0ee11dc07e4..da95ae8b572f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -643,6 +643,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 			return BLK_STS_OK;
 		return BLK_STS_IOERR;
 	case DID_TRANSPORT_FAILFAST:
+	case DID_TRANSPORT_MARGINAL:
 		return BLK_STS_TRANSPORT;
 	case DID_TARGET_FAILURE:
 		set_host_byte(cmd, DID_OK);
@@ -1689,6 +1690,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 		req->rq_flags |= RQF_DONTPREP;
 	} else {
 		clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
+		clear_bit(SCMD_NORETRIES_ABORT, &cmd->state);
 		blk_mq_start_request(req);
 	}
 
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 3/8] scsi: Clear state bit SCMD_NORETRIES_ABORT of scsi_cmd before start request

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Clearing the SCMD_NORETRIES_ABORT bit in state flag before
> blk_mq_start_request.
> 
> Added a code in scsi_result_to_blk_status to translate
> a new error DID_TRANSPORT_MARGINAL to the corresponding blk_status_t
> i.e BLK_STS_TRANSPORT
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> Made changes in scsi_result_to_blk_status to support DID_TRANSPORT_MARGINAL
> ---
>   drivers/scsi/scsi_lib.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f0ee11dc07e4..da95ae8b572f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -643,6 +643,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
>   			return BLK_STS_OK;
>   		return BLK_STS_IOERR;
>   	case DID_TRANSPORT_FAILFAST:
> +	case DID_TRANSPORT_MARGINAL:
>   		return BLK_STS_TRANSPORT;
>   	case DID_TARGET_FAILURE:
>   		set_host_byte(cmd, DID_OK);
> @@ -1689,6 +1690,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   		req->rq_flags |= RQF_DONTPREP;
>   	} else {
>   		clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
> +		clear_bit(SCMD_NORETRIES_ABORT, &cmd->state);
>   		blk_mq_start_request(req);
>   	}
>   
> 
Please split off this patch and merge the first part with the next 
patch, and the second hunk with the previous one.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH v2 4/8] scsi: No retries on abort success

[Muneendra]

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

Made an additional check in scsi_noretry_cmd to verify whether user has
decided not to do retries on abort success by setting the
SCMD_NORETRIES_ABORT bit

If SCMD_NORETRIES_ABORT bit is set we are making sure there won't be any
retries done on the same path and also setting the host byte as
DID_TRANSPORT_MARGINAL so that the error can be propogated as recoverable
transport error to the blk layers.

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
set the hostbyte as DID_TRANSPORT_MARGINAL instead of
DID_TRANSPORT_FAILFAST.
---
 drivers/scsi/scsi_error.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 5f3726abed78..3f14ea10d5da 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1748,6 +1748,16 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd)
 		return 0;
 
 check_type:
+	/*
+	 * Check whether caller has decided not to do retries on
+	 * abort success by setting the SCMD_NORETRIES_ABORT bit
+	 */
+	if ((test_bit(SCMD_NORETRIES_ABORT, &scmd->state)) &&
+		(scmd->request->cmd_flags & REQ_FAILFAST_TRANSPORT)) {
+		set_host_byte(scmd, DID_TRANSPORT_MARGINAL);
+		return 1;
+	}
+
 	/*
 	 * assume caller has checked sense and determined
 	 * the check condition was retryable.
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 4/8] scsi: No retries on abort success

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Made an additional check in scsi_noretry_cmd to verify whether user has
> decided not to do retries on abort success by setting the
> SCMD_NORETRIES_ABORT bit
> 
> If SCMD_NORETRIES_ABORT bit is set we are making sure there won't be any
> retries done on the same path and also setting the host byte as
> DID_TRANSPORT_MARGINAL so that the error can be propogated as recoverable
> transport error to the blk layers.
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> set the hostbyte as DID_TRANSPORT_MARGINAL instead of
> DID_TRANSPORT_FAILFAST.
> ---
>   drivers/scsi/scsi_error.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 5f3726abed78..3f14ea10d5da 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1748,6 +1748,16 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd)
>   		return 0;
>   
>   check_type:
> +	/*
> +	 * Check whether caller has decided not to do retries on
> +	 * abort success by setting the SCMD_NORETRIES_ABORT bit
> +	 */
> +	if ((test_bit(SCMD_NORETRIES_ABORT, &scmd->state)) &&
> +		(scmd->request->cmd_flags & REQ_FAILFAST_TRANSPORT)) {
> +		set_host_byte(scmd, DID_TRANSPORT_MARGINAL);
> +		return 1;
> +	}
> +
>   	/*
>   	 * assume caller has checked sense and determined
>   	 * the check condition was retryable.
> 
As indicated, the first part of the previous patch should be merged with 
this patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH v2 5/8] scsi: Added routine to set/clear SCMD_NORETRIES_ABORT bit for outstanding io on scsi_dev

[Muneendra]

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

Added a new routine scsi_chg_noretries_abort_io_device().
This functions accepts two arguments Scsi_device and an integer(set).

When set is passed as 1
this routine will set SCMD_NORETRIES_ABORT bit in
scmd->state for all the pending io's on the scsi device associated
with target port.

When set is passed as 0
This routine  will clear SCMD_NORETRIES_ABORT bit in
scmd->state for all the pending io's on the scsi device associated
with target port.

Export the symbol so the routine can be called by scsi_transport_fc.c

Added new function declaration scsi_chg_noretries_abort_io_device in
scsi_priv.h

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
Renamed the below functions as
scsi_set_noretries_abort_io_device ->scsi_chg_noretries_abort_io_device
__scsi_set_noretries_abort_io_device->__scsi_set_noretries_abort_io_device
which accepts the value as an arg to set/clear the SCMD_NORETRIES_ABORT bit
---
 drivers/scsi/scsi_error.c | 76 +++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_priv.h  |  2 ++
 2 files changed, 78 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 3f14ea10d5da..c0943f08b469 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -271,6 +271,82 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
 	call_rcu(&scmd->rcu, scsi_eh_inc_host_failed);
 }
 
+static bool
+scsi_clear_noretries_abort_io(struct request *rq, void *priv, bool reserved)
+{
+	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
+	struct scsi_device *sdev = scmd->device;
+
+	/* only clear SCMD_NORETRIES_ABORT on ios on a specific sdev */
+	if (sdev != priv)
+		return true;
+
+	/*Clear the SCMD_NORETRIES_ABORT bit*/
+	if (READ_ONCE(rq->state) == MQ_RQ_IN_FLIGHT)
+		clear_bit(SCMD_NORETRIES_ABORT, &scmd->state);
+	return true;
+}
+
+static bool
+scsi_set_noretries_abort_io(struct request *rq, void *priv, bool reserved)
+{
+	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
+	struct scsi_device *sdev = scmd->device;
+
+	/* only set SCMD_NORETRIES_ABORT on ios on a specific sdev */
+	if (sdev != priv)
+		return true;
+	/* we don't want this command reissued on abort success
+	 * so set SCMD_NORETRIES_ABORT bit to ensure it
+	 * won't get reissued
+	 */
+	if (READ_ONCE(rq->state) == MQ_RQ_IN_FLIGHT)
+		set_bit(SCMD_NORETRIES_ABORT, &scmd->state);
+	return true;
+}
+
+static int
+__scsi_chg_noretries_abort_io_device(struct scsi_device *sdev, int set)
+{
+
+	if (sdev->sdev_state != SDEV_RUNNING)
+		return -EINVAL;
+
+	if (blk_queue_init_done(sdev->request_queue)) {
+
+		blk_mq_quiesce_queue(sdev->request_queue);
+
+		if (set)
+			blk_mq_tagset_busy_iter(&sdev->host->tag_set,
+					scsi_set_noretries_abort_io, sdev);
+		else
+			blk_mq_tagset_busy_iter(&sdev->host->tag_set,
+				scsi_clear_noretries_abort_io, sdev);
+
+		blk_mq_unquiesce_queue(sdev->request_queue);
+	}
+	return 0;
+}
+
+/*
+ * scsi_chg_noretries_abort_io_device - set/clear the SCMD_NORETRIES_ABORT
+ * bit for all the pending io's on a device
+ * @sdev:	scsi_device
+ * @set: indicates to clear (0) or set (1) the SCMD_NORETRIES_ABORT flag
+ */
+int
+scsi_chg_noretries_abort_io_device(struct scsi_device *sdev, int set)
+{
+	struct Scsi_Host *shost = sdev->host;
+	int ret  = -EINVAL;
+
+	mutex_lock(&shost->scan_mutex);
+	ret = __scsi_chg_noretries_abort_io_device(sdev, set);
+	mutex_unlock(&shost->scan_mutex);
+	return ret;
+}
+EXPORT_SYMBOL(scsi_chg_noretries_abort_io_device);
+
 /**
  * scsi_times_out - Timeout function for normal scsi commands.
  * @req:	request that is timing out.
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index d12ada035961..aba98a3294ce 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -81,6 +81,8 @@ void scsi_eh_ready_devs(struct Scsi_Host *shost,
 int scsi_eh_get_sense(struct list_head *work_q,
 		      struct list_head *done_q);
 int scsi_noretry_cmd(struct scsi_cmnd *scmd);
+extern int scsi_chg_noretries_abort_io_device(struct scsi_device *sdev,
+			int set);
 
 /* scsi_lib.c */
 extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 5/8] scsi: Added routine to set/clear SCMD_NORETRIES_ABORT bit for outstanding io on scsi_dev

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Added a new routine scsi_chg_noretries_abort_io_device().
> This functions accepts two arguments Scsi_device and an integer(set).
> 
> When set is passed as 1
> this routine will set SCMD_NORETRIES_ABORT bit in
> scmd->state for all the pending io's on the scsi device associated
> with target port.
> 
> When set is passed as 0
> This routine  will clear SCMD_NORETRIES_ABORT bit in
> scmd->state for all the pending io's on the scsi device associated
> with target port.
> 
> Export the symbol so the routine can be called by scsi_transport_fc.c
> 
> Added new function declaration scsi_chg_noretries_abort_io_device in
> scsi_priv.h
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> Renamed the below functions as
> scsi_set_noretries_abort_io_device ->scsi_chg_noretries_abort_io_device
> __scsi_set_noretries_abort_io_device->__scsi_set_noretries_abort_io_device
> which accepts the value as an arg to set/clear the SCMD_NORETRIES_ABORT bit
> ---
>   drivers/scsi/scsi_error.c | 76 +++++++++++++++++++++++++++++++++++++++
>   drivers/scsi/scsi_priv.h  |  2 ++
>   2 files changed, 78 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 3f14ea10d5da..c0943f08b469 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -271,6 +271,82 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
>   	call_rcu(&scmd->rcu, scsi_eh_inc_host_failed);
>   }
>   
> +static bool
> +scsi_clear_noretries_abort_io(struct request *rq, void *priv, bool reserved)
> +{
> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
> +	struct scsi_device *sdev = scmd->device;
> +
> +	/* only clear SCMD_NORETRIES_ABORT on ios on a specific sdev */
> +	if (sdev != priv)
> +		return true;
> +
> +	/*Clear the SCMD_NORETRIES_ABORT bit*/
> +	if (READ_ONCE(rq->state) == MQ_RQ_IN_FLIGHT)
> +		clear_bit(SCMD_NORETRIES_ABORT, &scmd->state);
> +	return true;
> +}
> +
> +static bool
> +scsi_set_noretries_abort_io(struct request *rq, void *priv, bool reserved)
> +{
> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
> +	struct scsi_device *sdev = scmd->device;
> +
> +	/* only set SCMD_NORETRIES_ABORT on ios on a specific sdev */
> +	if (sdev != priv)
> +		return true;
> +	/* we don't want this command reissued on abort success
> +	 * so set SCMD_NORETRIES_ABORT bit to ensure it
> +	 * won't get reissued
> +	 */
> +	if (READ_ONCE(rq->state) == MQ_RQ_IN_FLIGHT)
> +		set_bit(SCMD_NORETRIES_ABORT, &scmd->state);
> +	return true;
> +}
> +

Do we really care about the 'in flight' parameter?
Why not set it on every command?

And meanwhile we do have scsi command iterators (scsi_host_busy_iter);
maybe one should be using them here.

> +static int
> +__scsi_chg_noretries_abort_io_device(struct scsi_device *sdev, int set)
> +{
> +
> +	if (sdev->sdev_state != SDEV_RUNNING)
> +		return -EINVAL;
> +
> +	if (blk_queue_init_done(sdev->request_queue)) {
> +
> +		blk_mq_quiesce_queue(sdev->request_queue);
> +
> +		if (set)
> +			blk_mq_tagset_busy_iter(&sdev->host->tag_set,
> +					scsi_set_noretries_abort_io, sdev);
> +		else
> +			blk_mq_tagset_busy_iter(&sdev->host->tag_set,
> +				scsi_clear_noretries_abort_io, sdev);
> +
> +		blk_mq_unquiesce_queue(sdev->request_queue);
> +	}
> +	return 0;
> +}

'set' is a boolean, so why not use the 'boolean' type here?
And I'm not sure if we need to check for 'blk_queue_init_done()'; surely 
that's always the case for SDEV_RUNNING?

> +
> +/*
> + * scsi_chg_noretries_abort_io_device - set/clear the SCMD_NORETRIES_ABORT
> + * bit for all the pending io's on a device
> + * @sdev:	scsi_device
> + * @set: indicates to clear (0) or set (1) the SCMD_NORETRIES_ABORT flag
> + */
> +int
> +scsi_chg_noretries_abort_io_device(struct scsi_device *sdev, int set)
> +{
> +	struct Scsi_Host *shost = sdev->host;
> +	int ret  = -EINVAL;
> +
> +	mutex_lock(&shost->scan_mutex);
> +	ret = __scsi_chg_noretries_abort_io_device(sdev, set);
> +	mutex_unlock(&shost->scan_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL(scsi_chg_noretries_abort_io_device);
> +
>   /**
>    * scsi_times_out - Timeout function for normal scsi commands.
>    * @req:	request that is timing out.
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index d12ada035961..aba98a3294ce 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -81,6 +81,8 @@ void scsi_eh_ready_devs(struct Scsi_Host *shost,
>   int scsi_eh_get_sense(struct list_head *work_q,
>   		      struct list_head *done_q);
>   int scsi_noretry_cmd(struct scsi_cmnd *scmd);
> +extern int scsi_chg_noretries_abort_io_device(struct scsi_device *sdev,
> +			int set);
>   
>   /* scsi_lib.c */
>   extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
> 
Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH v2 6/8] scsi_transport_fc: Added a new rport state FC_PORTSTATE_MARGINAL

[Muneendra]

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

Added a new rport state FC_PORTSTATE_MARGINAL.

Made changes in fc_remote_port_chkready function to treat marginal and
online as same

Added a new inline function fc_rport_chkmarginal_set_noretries
which will set the SCMD_NORETRIES_ABORT bit in cmd->state if rport state
is marginal.

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
New patch
---
 include/scsi/scsi_transport_fc.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index 1c7dd35cb7a0..ee99c6ca7e45 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -14,6 +14,7 @@
 #include <linux/bsg-lib.h>
 #include <asm/unaligned.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_netlink.h>
 #include <scsi/scsi_host.h>
 
@@ -67,6 +68,7 @@ enum fc_port_state {
 	FC_PORTSTATE_ERROR,
 	FC_PORTSTATE_LOOPBACK,
 	FC_PORTSTATE_DELETED,
+	FC_PORTSTATE_MARGINAL,
 };
 
 
@@ -383,6 +385,7 @@ struct fc_starget_attrs {	/* aka fc_target_attrs */
 	u64 node_name;
 	u64 port_name;
 	u32 port_id;
+	enum fc_port_state port_state;
 };
 
 #define fc_starget_node_name(x) \
@@ -391,6 +394,8 @@ struct fc_starget_attrs {	/* aka fc_target_attrs */
 	(((struct fc_starget_attrs *)&(x)->starget_data)->port_name)
 #define fc_starget_port_id(x) \
 	(((struct fc_starget_attrs *)&(x)->starget_data)->port_id)
+#define fc_starget_port_state(x) \
+	(((struct fc_starget_attrs *)&(x)->starget_data)->port_state)
 
 #define starget_to_rport(s)			\
 	scsi_is_fc_rport(s->dev.parent) ? dev_to_rport(s->dev.parent) : NULL
@@ -723,6 +728,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
 
 	switch (rport->port_state) {
 	case FC_PORTSTATE_ONLINE:
+	case FC_PORTSTATE_MARGINAL:
 		if (rport->roles & FC_PORT_ROLE_FCP_TARGET)
 			result = 0;
 		else if (rport->flags & FC_RPORT_DEVLOSS_PENDING)
@@ -743,6 +749,24 @@ fc_remote_port_chkready(struct fc_rport *rport)
 	return result;
 }
 
+/**
+ * fc_rport_chkmarginal_set_noretries - Set the SCMD_NORETRIES_ABORT bit
+ * in cmd->state if port state is marginal prior to initiating
+ * io to the port.
+ * @rport:	remote port to be checked
+ * @scmd:	scsi_cmd
+ **/
+static inline void
+fc_rport_chkmarginal_set_noretries(struct fc_rport *rport, struct scsi_cmnd *cmd)
+{
+	if ((rport->port_state == FC_PORTSTATE_MARGINAL) &&
+		 (cmd->request->cmd_flags & REQ_FAILFAST_TRANSPORT))
+		set_bit(SCMD_NORETRIES_ABORT, &cmd->state);
+	else
+		clear_bit(SCMD_NORETRIES_ABORT, &cmd->state);
+
+}
+
 static inline u64 wwn_to_u64(const u8 *wwn)
 {
 	return get_unaligned_be64(wwn);
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 6/8] scsi_transport_fc: Added a new rport state FC_PORTSTATE_MARGINAL

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Added a new rport state FC_PORTSTATE_MARGINAL.
> 
> Made changes in fc_remote_port_chkready function to treat marginal and
> online as same
> 
> Added a new inline function fc_rport_chkmarginal_set_noretries
> which will set the SCMD_NORETRIES_ABORT bit in cmd->state if rport state
> is marginal.
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> New patch
> ---
>   include/scsi/scsi_transport_fc.h | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> index 1c7dd35cb7a0..ee99c6ca7e45 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -14,6 +14,7 @@
>   #include <linux/bsg-lib.h>
>   #include <asm/unaligned.h>
>   #include <scsi/scsi.h>
> +#include <scsi/scsi_cmnd.h>
>   #include <scsi/scsi_netlink.h>
>   #include <scsi/scsi_host.h>
>   
> @@ -67,6 +68,7 @@ enum fc_port_state {
>   	FC_PORTSTATE_ERROR,
>   	FC_PORTSTATE_LOOPBACK,
>   	FC_PORTSTATE_DELETED,
> +	FC_PORTSTATE_MARGINAL,
>   };
>   
>   
> @@ -383,6 +385,7 @@ struct fc_starget_attrs {	/* aka fc_target_attrs */
>   	u64 node_name;
>   	u64 port_name;
>   	u32 port_id;
> +	enum fc_port_state port_state;
>   };
>   
>   #define fc_starget_node_name(x) \
> @@ -391,6 +394,8 @@ struct fc_starget_attrs {	/* aka fc_target_attrs */
>   	(((struct fc_starget_attrs *)&(x)->starget_data)->port_name)
>   #define fc_starget_port_id(x) \
>   	(((struct fc_starget_attrs *)&(x)->starget_data)->port_id)
> +#define fc_starget_port_state(x) \
> +	(((struct fc_starget_attrs *)&(x)->starget_data)->port_state)
>   
>   #define starget_to_rport(s)			\
>   	scsi_is_fc_rport(s->dev.parent) ? dev_to_rport(s->dev.parent) : NULL

This should be moved to the next patch.

> @@ -723,6 +728,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
>   
>   	switch (rport->port_state) {
>   	case FC_PORTSTATE_ONLINE:
> +	case FC_PORTSTATE_MARGINAL:
>   		if (rport->roles & FC_PORT_ROLE_FCP_TARGET)
>   			result = 0;
>   		else if (rport->flags & FC_RPORT_DEVLOSS_PENDING)
> @@ -743,6 +749,24 @@ fc_remote_port_chkready(struct fc_rport *rport)
>   	return result;
>   }
>   
> +/**
> + * fc_rport_chkmarginal_set_noretries - Set the SCMD_NORETRIES_ABORT bit
> + * in cmd->state if port state is marginal prior to initiating
> + * io to the port.
> + * @rport:	remote port to be checked
> + * @scmd:	scsi_cmd
> + **/
> +static inline void
> +fc_rport_chkmarginal_set_noretries(struct fc_rport *rport, struct scsi_cmnd *cmd)
> +{
> +	if ((rport->port_state == FC_PORTSTATE_MARGINAL) &&
> +		 (cmd->request->cmd_flags & REQ_FAILFAST_TRANSPORT))
> +		set_bit(SCMD_NORETRIES_ABORT, &cmd->state);
> +	else
> +		clear_bit(SCMD_NORETRIES_ABORT, &cmd->state);
> +
> +}
> +
>   static inline u64 wwn_to_u64(const u8 *wwn)
>   {
>   	return get_unaligned_be64(wwn);
> 
??? Where is this function used ?
I thought this was done with the iterators in the previous patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Muneendra]

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

Added a new sysfs attribute port_state under fc_transport/target*/

With this new interface the user can move the port_state from
Marginal -> Online and Online->Marginal.

On Marginal :This interface will set SCMD_NORETRIES_ABORT bit in
scmd->state for all the pending io's on the scsi device associated
with target port.

On Online :This interface will clear SCMD_NORETRIES_ABORT bit in
scmd->state for all the pending io's on the scsi device associated
with target port.

Below is the interface provided to set the port state to Marginal
and Online.

echo "Marginal" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
echo "Online" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state

Also made changes in fc_remote_port_delete,fc_user_scan_tgt,
fc_timeout_deleted_rport functions  to handle the new rport state
FC_PORTSTATE_MARGINAL.

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
Changed from a noretries_abort attribute under fc_transport/target*/ to
port_state for changing the port_state to a marginal state
---
 drivers/scsi/scsi_transport_fc.c | 140 +++++++++++++++++++++++++++----
 1 file changed, 122 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 2ff7f06203da..6fe2463c5a68 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -142,20 +142,23 @@ fc_enum_name_search(host_event_code, fc_host_event_code,
 static struct {
 	enum fc_port_state	value;
 	char			*name;
+	int			matchlen;
 } fc_port_state_names[] = {
-	{ FC_PORTSTATE_UNKNOWN,		"Unknown" },
-	{ FC_PORTSTATE_NOTPRESENT,	"Not Present" },
-	{ FC_PORTSTATE_ONLINE,		"Online" },
-	{ FC_PORTSTATE_OFFLINE,		"Offline" },
-	{ FC_PORTSTATE_BLOCKED,		"Blocked" },
-	{ FC_PORTSTATE_BYPASSED,	"Bypassed" },
-	{ FC_PORTSTATE_DIAGNOSTICS,	"Diagnostics" },
-	{ FC_PORTSTATE_LINKDOWN,	"Linkdown" },
-	{ FC_PORTSTATE_ERROR,		"Error" },
-	{ FC_PORTSTATE_LOOPBACK,	"Loopback" },
-	{ FC_PORTSTATE_DELETED,		"Deleted" },
+	{ FC_PORTSTATE_UNKNOWN,		"Unknown", 7},
+	{ FC_PORTSTATE_NOTPRESENT,	"Not Present", 11 },
+	{ FC_PORTSTATE_ONLINE,		"Online", 6 },
+	{ FC_PORTSTATE_OFFLINE,		"Offline", 7 },
+	{ FC_PORTSTATE_BLOCKED,		"Blocked", 7 },
+	{ FC_PORTSTATE_BYPASSED,	"Bypassed", 8 },
+	{ FC_PORTSTATE_DIAGNOSTICS,	"Diagnostics", 11 },
+	{ FC_PORTSTATE_LINKDOWN,	"Linkdown", 8 },
+	{ FC_PORTSTATE_ERROR,		"Error", 5 },
+	{ FC_PORTSTATE_LOOPBACK,	"Loopback", 8 },
+	{ FC_PORTSTATE_DELETED,		"Deleted", 7 },
+	{ FC_PORTSTATE_MARGINAL,	"Marginal", 8 },
 };
 fc_enum_name_search(port_state, fc_port_state, fc_port_state_names)
+fc_enum_name_match(port_state, fc_port_state, fc_port_state_names)
 #define FC_PORTSTATE_MAX_NAMELEN	20
 
 
@@ -306,7 +309,7 @@ static void fc_scsi_scan_rport(struct work_struct *work);
  * Attribute counts pre object type...
  * Increase these values if you add attributes
  */
-#define FC_STARGET_NUM_ATTRS 	3
+#define FC_STARGET_NUM_ATTRS	4
 #define FC_RPORT_NUM_ATTRS	10
 #define FC_VPORT_NUM_ATTRS	9
 #define FC_HOST_NUM_ATTRS	29
@@ -358,10 +361,12 @@ static int fc_target_setup(struct transport_container *tc, struct device *dev,
 		fc_starget_node_name(starget) = rport->node_name;
 		fc_starget_port_name(starget) = rport->port_name;
 		fc_starget_port_id(starget) = rport->port_id;
+		fc_starget_port_state(starget) = rport->port_state;
 	} else {
 		fc_starget_node_name(starget) = -1;
 		fc_starget_port_name(starget) = -1;
 		fc_starget_port_id(starget) = -1;
+		fc_starget_port_state(starget) = FC_PORTSTATE_UNKNOWN;
 	}
 
 	return 0;
@@ -995,6 +1000,93 @@ static FC_DEVICE_ATTR(rport, fast_io_fail_tmo, S_IRUGO | S_IWUSR,
 /*
  * FC SCSI Target Attribute Management
  */
+static void scsi_target_chg_noretries_abort(struct scsi_target *starget, int set)
+{
+	struct scsi_device *sdev, *tmp;
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry_safe(sdev, tmp, &starget->devices, same_target_siblings) {
+		if (sdev->sdev_state == SDEV_DEL)
+			continue;
+
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		if (scsi_device_get(sdev))
+			continue;
+
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		scsi_chg_noretries_abort_io_device(sdev, set);
+		spin_lock_irqsave(shost->host_lock, flags);
+		scsi_device_put(sdev);
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+}
+
+/*
+ * Sets port_state to Marginal/Online.
+ * On Marginal it Sets  no retries on abort in scmd->state for all
+ * outstanding io of all the scsi_devs
+ * This only allows ONLINE->MARGINAL and MARGINAL->ONLINE
+ */
+static ssize_t fc_target_set_marginal_state(struct device *dev,
+						struct device_attribute *attr,
+						const char *buf, size_t count)
+{
+	struct scsi_target *starget = transport_class_to_starget(dev);
+	struct fc_rport *rport = starget_to_rport(starget);
+	enum fc_port_state port_state;
+	int ret = 0;
+
+	ret = get_fc_port_state_match(buf, &port_state);
+
+	if (port_state == FC_PORTSTATE_MARGINAL) {
+
+		/*
+		 * Change the state to marginal only if the
+		 * current rport state is Online
+		 * Allow only Online->marginal
+		 */
+		if (rport->port_state == FC_PORTSTATE_ONLINE) {
+			rport->port_state = port_state;
+			scsi_target_chg_noretries_abort(starget, 1);
+		}
+
+	} else if (port_state == FC_PORTSTATE_ONLINE) {
+		/*
+		 * Change the state to Online only if the
+		 * current rport state is Marginal
+		 * Allow only  MArginal->Online
+		 */
+
+		if (rport->port_state == FC_PORTSTATE_MARGINAL) {
+			rport->port_state = port_state;
+			scsi_target_chg_noretries_abort(starget, 0);
+		}
+
+
+	} else
+		return -EINVAL;
+	return count;
+}
+
+static ssize_t
+fc_target_show_port_state(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	const char *name;
+	struct scsi_target *starget = transport_class_to_starget(dev);
+	struct fc_rport *rport = starget_to_rport(starget);
+
+	name = get_fc_port_state_name(rport->port_state);
+	if (!name)
+		return -EINVAL;
+
+	return snprintf(buf, 20, "%s\n", name);
+}
+
+static FC_DEVICE_ATTR(starget, port_state, 0444 | 0200,
+		fc_target_show_port_state, fc_target_set_marginal_state);
 
 /*
  * Note: in the target show function we recognize when the remote
@@ -1037,6 +1129,13 @@ static FC_DEVICE_ATTR(starget, field, S_IRUGO,			\
 	if (i->f->show_starget_##field)					\
 		count++
 
+#define SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(field)			\
+do {									\
+	i->private_starget_attrs[count] = device_attr_starget_##field; \
+	i->starget_attrs[count] = &i->private_starget_attrs[count];	\
+	count++;							\
+} while (0)
+
 #define SETUP_STARGET_ATTRIBUTE_RW(field)				\
 	i->private_starget_attrs[count] = device_attr_starget_##field; \
 	if (!i->f->set_starget_##field) {				\
@@ -2095,7 +2194,8 @@ fc_user_scan_tgt(struct Scsi_Host *shost, uint channel, uint id, u64 lun)
 		if (rport->scsi_target_id == -1)
 			continue;
 
-		if (rport->port_state != FC_PORTSTATE_ONLINE)
+		if ((rport->port_state != FC_PORTSTATE_ONLINE) &&
+			(rport->port_state != FC_PORTSTATE_MARGINAL))
 			continue;
 
 		if ((channel == rport->channel) &&
@@ -2198,7 +2298,7 @@ fc_attach_transport(struct fc_function_template *ft)
 	SETUP_STARGET_ATTRIBUTE_RD(node_name);
 	SETUP_STARGET_ATTRIBUTE_RD(port_name);
 	SETUP_STARGET_ATTRIBUTE_RD(port_id);
-
+	SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(port_state);
 	BUG_ON(count > FC_STARGET_NUM_ATTRS);
 
 	i->starget_attrs[count] = NULL;
@@ -2958,7 +3058,8 @@ fc_remote_port_delete(struct fc_rport  *rport)
 
 	spin_lock_irqsave(shost->host_lock, flags);
 
-	if (rport->port_state != FC_PORTSTATE_ONLINE) {
+	if ((rport->port_state != FC_PORTSTATE_ONLINE) &&
+		(rport->port_state != FC_PORTSTATE_MARGINAL)) {
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		return;
 	}
@@ -3100,7 +3201,8 @@ fc_timeout_deleted_rport(struct work_struct *work)
 	 * target, validate it still is. If not, tear down the
 	 * scsi_target on it.
 	 */
-	if ((rport->port_state == FC_PORTSTATE_ONLINE) &&
+	if (((rport->port_state == FC_PORTSTATE_ONLINE) ||
+		(rport->port_state == FC_PORTSTATE_MARGINAL)) &&
 	    (rport->scsi_target_id != -1) &&
 	    !(rport->roles & FC_PORT_ROLE_FCP_TARGET)) {
 		dev_printk(KERN_ERR, &rport->dev,
@@ -3243,7 +3345,8 @@ fc_scsi_scan_rport(struct work_struct *work)
 	struct fc_internal *i = to_fc_internal(shost->transportt);
 	unsigned long flags;
 
-	if ((rport->port_state == FC_PORTSTATE_ONLINE) &&
+	if (((rport->port_state == FC_PORTSTATE_ONLINE) ||
+		(rport->port_state == FC_PORTSTATE_ONLINE)) &&
 	    (rport->roles & FC_PORT_ROLE_FCP_TARGET) &&
 	    !(i->f->disable_target_scan)) {
 		scsi_scan_target(&rport->dev, rport->channel,
@@ -3747,7 +3850,8 @@ static blk_status_t fc_bsg_rport_prep(struct fc_rport *rport)
 	    !(rport->flags & FC_RPORT_FAST_FAIL_TIMEDOUT))
 		return BLK_STS_RESOURCE;
 
-	if (rport->port_state != FC_PORTSTATE_ONLINE)
+	if ((rport->port_state != FC_PORTSTATE_ONLINE) &&
+		(rport->port_state != FC_PORTSTATE_MARGINAL))
 		return BLK_STS_IOERR;
 
 	return BLK_STS_OK;
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Added a new sysfs attribute port_state under fc_transport/target*/
> 
> With this new interface the user can move the port_state from
> Marginal -> Online and Online->Marginal.
> 
> On Marginal :This interface will set SCMD_NORETRIES_ABORT bit in
> scmd->state for all the pending io's on the scsi device associated
> with target port.
> 
> On Online :This interface will clear SCMD_NORETRIES_ABORT bit in
> scmd->state for all the pending io's on the scsi device associated
> with target port.
> 
> Below is the interface provided to set the port state to Marginal
> and Online.
> 
> echo "Marginal" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
> echo "Online" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
> 
> Also made changes in fc_remote_port_delete,fc_user_scan_tgt,
> fc_timeout_deleted_rport functions  to handle the new rport state
> FC_PORTSTATE_MARGINAL.
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> Changed from a noretries_abort attribute under fc_transport/target*/ to
> port_state for changing the port_state to a marginal state
> ---
>   drivers/scsi/scsi_transport_fc.c | 140 +++++++++++++++++++++++++++----
>   1 file changed, 122 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 2ff7f06203da..6fe2463c5a68 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -142,20 +142,23 @@ fc_enum_name_search(host_event_code, fc_host_event_code,
>   static struct {
>   	enum fc_port_state	value;
>   	char			*name;
> +	int			matchlen;
>   } fc_port_state_names[] = {
> -	{ FC_PORTSTATE_UNKNOWN,		"Unknown" },
> -	{ FC_PORTSTATE_NOTPRESENT,	"Not Present" },
> -	{ FC_PORTSTATE_ONLINE,		"Online" },
> -	{ FC_PORTSTATE_OFFLINE,		"Offline" },
> -	{ FC_PORTSTATE_BLOCKED,		"Blocked" },
> -	{ FC_PORTSTATE_BYPASSED,	"Bypassed" },
> -	{ FC_PORTSTATE_DIAGNOSTICS,	"Diagnostics" },
> -	{ FC_PORTSTATE_LINKDOWN,	"Linkdown" },
> -	{ FC_PORTSTATE_ERROR,		"Error" },
> -	{ FC_PORTSTATE_LOOPBACK,	"Loopback" },
> -	{ FC_PORTSTATE_DELETED,		"Deleted" },
> +	{ FC_PORTSTATE_UNKNOWN,		"Unknown", 7},
> +	{ FC_PORTSTATE_NOTPRESENT,	"Not Present", 11 },
> +	{ FC_PORTSTATE_ONLINE,		"Online", 6 },
> +	{ FC_PORTSTATE_OFFLINE,		"Offline", 7 },
> +	{ FC_PORTSTATE_BLOCKED,		"Blocked", 7 },
> +	{ FC_PORTSTATE_BYPASSED,	"Bypassed", 8 },
> +	{ FC_PORTSTATE_DIAGNOSTICS,	"Diagnostics", 11 },
> +	{ FC_PORTSTATE_LINKDOWN,	"Linkdown", 8 },
> +	{ FC_PORTSTATE_ERROR,		"Error", 5 },
> +	{ FC_PORTSTATE_LOOPBACK,	"Loopback", 8 },
> +	{ FC_PORTSTATE_DELETED,		"Deleted", 7 },
> +	{ FC_PORTSTATE_MARGINAL,	"Marginal", 8 },
>   };
>   fc_enum_name_search(port_state, fc_port_state, fc_port_state_names)
> +fc_enum_name_match(port_state, fc_port_state, fc_port_state_names)
>   #define FC_PORTSTATE_MAX_NAMELEN	20
>   
>   
> @@ -306,7 +309,7 @@ static void fc_scsi_scan_rport(struct work_struct *work);
>    * Attribute counts pre object type...
>    * Increase these values if you add attributes
>    */
> -#define FC_STARGET_NUM_ATTRS 	3
> +#define FC_STARGET_NUM_ATTRS	4
>   #define FC_RPORT_NUM_ATTRS	10
>   #define FC_VPORT_NUM_ATTRS	9
>   #define FC_HOST_NUM_ATTRS	29
> @@ -358,10 +361,12 @@ static int fc_target_setup(struct transport_container *tc, struct device *dev,
>   		fc_starget_node_name(starget) = rport->node_name;
>   		fc_starget_port_name(starget) = rport->port_name;
>   		fc_starget_port_id(starget) = rport->port_id;
> +		fc_starget_port_state(starget) = rport->port_state;
>   	} else {
>   		fc_starget_node_name(starget) = -1;
>   		fc_starget_port_name(starget) = -1;
>   		fc_starget_port_id(starget) = -1;
> +		fc_starget_port_state(starget) = FC_PORTSTATE_UNKNOWN;
>   	}
>   
>   	return 0;
> @@ -995,6 +1000,93 @@ static FC_DEVICE_ATTR(rport, fast_io_fail_tmo, S_IRUGO | S_IWUSR,
>   /*
>    * FC SCSI Target Attribute Management
>    */
> +static void scsi_target_chg_noretries_abort(struct scsi_target *starget, int set)
> +{
> +	struct scsi_device *sdev, *tmp;
> +	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	list_for_each_entry_safe(sdev, tmp, &starget->devices, same_target_siblings) {
> +		if (sdev->sdev_state == SDEV_DEL)
> +			continue;
> +
> +		spin_unlock_irqrestore(shost->host_lock, flags);
> +		if (scsi_device_get(sdev))
> +			continue;
> +
> +		spin_unlock_irqrestore(shost->host_lock, flags);
> +		scsi_chg_noretries_abort_io_device(sdev, set);
> +		spin_lock_irqsave(shost->host_lock, flags);
> +		scsi_device_put(sdev);
> +	}
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +}
> +
> +/*
> + * Sets port_state to Marginal/Online.
> + * On Marginal it Sets  no retries on abort in scmd->state for all
> + * outstanding io of all the scsi_devs
> + * This only allows ONLINE->MARGINAL and MARGINAL->ONLINE
> + */
> +static ssize_t fc_target_set_marginal_state(struct device *dev,
> +						struct device_attribute *attr,
> +						const char *buf, size_t count)
> +{
> +	struct scsi_target *starget = transport_class_to_starget(dev);
> +	struct fc_rport *rport = starget_to_rport(starget);
> +	enum fc_port_state port_state;
> +	int ret = 0;
> +
> +	ret = get_fc_port_state_match(buf, &port_state);
> +
> +	if (port_state == FC_PORTSTATE_MARGINAL) {
> +
> +		/*
> +		 * Change the state to marginal only if the
> +		 * current rport state is Online
> +		 * Allow only Online->marginal
> +		 */
> +		if (rport->port_state == FC_PORTSTATE_ONLINE) {
> +			rport->port_state = port_state;
> +			scsi_target_chg_noretries_abort(starget, 1);
> +		}
> +
> +	} else if (port_state == FC_PORTSTATE_ONLINE) {
> +		/*
> +		 * Change the state to Online only if the
> +		 * current rport state is Marginal
> +		 * Allow only  MArginal->Online
> +		 */
> +
> +		if (rport->port_state == FC_PORTSTATE_MARGINAL) {
> +			rport->port_state = port_state;
> +			scsi_target_chg_noretries_abort(starget, 0);
> +		}
> +
> +
> +	} else
> +		return -EINVAL;
> +	return count;
> +}
> +
> +static ssize_t
> +fc_target_show_port_state(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	const char *name;
> +	struct scsi_target *starget = transport_class_to_starget(dev);
> +	struct fc_rport *rport = starget_to_rport(starget);
> +
> +	name = get_fc_port_state_name(rport->port_state);
> +	if (!name)
> +		return -EINVAL;
> +
> +	return snprintf(buf, 20, "%s\n", name);
> +}
> +
> +static FC_DEVICE_ATTR(starget, port_state, 0444 | 0200,
> +		fc_target_show_port_state, fc_target_set_marginal_state);
>   
>   /*
>    * Note: in the target show function we recognize when the remote
> @@ -1037,6 +1129,13 @@ static FC_DEVICE_ATTR(starget, field, S_IRUGO,			\
>   	if (i->f->show_starget_##field)					\
>   		count++
>   
> +#define SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(field)			\
> +do {									\
> +	i->private_starget_attrs[count] = device_attr_starget_##field; \
> +	i->starget_attrs[count] = &i->private_starget_attrs[count];	\
> +	count++;							\
> +} while (0)
> +
>   #define SETUP_STARGET_ATTRIBUTE_RW(field)				\
>   	i->private_starget_attrs[count] = device_attr_starget_##field; \
>   	if (!i->f->set_starget_##field) {				\
> @@ -2095,7 +2194,8 @@ fc_user_scan_tgt(struct Scsi_Host *shost, uint channel, uint id, u64 lun)
>   		if (rport->scsi_target_id == -1)
>   			continue;
>   
> -		if (rport->port_state != FC_PORTSTATE_ONLINE)
> +		if ((rport->port_state != FC_PORTSTATE_ONLINE) &&
> +			(rport->port_state != FC_PORTSTATE_MARGINAL))
>   			continue;
>   
>   		if ((channel == rport->channel) &&

This really should be moved to the previous patch.

> @@ -2198,7 +2298,7 @@ fc_attach_transport(struct fc_function_template *ft)
>   	SETUP_STARGET_ATTRIBUTE_RD(node_name);
>   	SETUP_STARGET_ATTRIBUTE_RD(port_name);
>   	SETUP_STARGET_ATTRIBUTE_RD(port_id);
> -
> +	SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(port_state);
>   	BUG_ON(count > FC_STARGET_NUM_ATTRS);
>   
>   	i->starget_attrs[count] = NULL;

Why did you move it to be a 'starget' attribute?
I would have thought it should be an 'rport' attribute, seeing that it's 
intrinsic to the fc transport class.

> @@ -2958,7 +3058,8 @@ fc_remote_port_delete(struct fc_rport  *rport)
>   
>   	spin_lock_irqsave(shost->host_lock, flags);
>   
> -	if (rport->port_state != FC_PORTSTATE_ONLINE) {
> +	if ((rport->port_state != FC_PORTSTATE_ONLINE) &&
> +		(rport->port_state != FC_PORTSTATE_MARGINAL)) {
>   		spin_unlock_irqrestore(shost->host_lock, flags);
>   		return;
>   	}
> @@ -3100,7 +3201,8 @@ fc_timeout_deleted_rport(struct work_struct *work)
>   	 * target, validate it still is. If not, tear down the
>   	 * scsi_target on it.
>   	 */
> -	if ((rport->port_state == FC_PORTSTATE_ONLINE) &&
> +	if (((rport->port_state == FC_PORTSTATE_ONLINE) ||
> +		(rport->port_state == FC_PORTSTATE_MARGINAL)) &&
>   	    (rport->scsi_target_id != -1) &&
>   	    !(rport->roles & FC_PORT_ROLE_FCP_TARGET)) {
>   		dev_printk(KERN_ERR, &rport->dev,
> @@ -3243,7 +3345,8 @@ fc_scsi_scan_rport(struct work_struct *work)
>   	struct fc_internal *i = to_fc_internal(shost->transportt);
>   	unsigned long flags;
>   
> -	if ((rport->port_state == FC_PORTSTATE_ONLINE) &&
> +	if (((rport->port_state == FC_PORTSTATE_ONLINE) ||
> +		(rport->port_state == FC_PORTSTATE_ONLINE)) &&
>   	    (rport->roles & FC_PORT_ROLE_FCP_TARGET) &&
>   	    !(i->f->disable_target_scan)) {
>   		scsi_scan_target(&rport->dev, rport->channel,
> @@ -3747,7 +3850,8 @@ static blk_status_t fc_bsg_rport_prep(struct fc_rport *rport)
>   	    !(rport->flags & FC_RPORT_FAST_FAIL_TIMEDOUT))
>   		return BLK_STS_RESOURCE;
>   
> -	if (rport->port_state != FC_PORTSTATE_ONLINE)
> +	if ((rport->port_state != FC_PORTSTATE_ONLINE) &&
> +		(rport->port_state != FC_PORTSTATE_MARGINAL))
>   		return BLK_STS_IOERR;
>   
>   	return BLK_STS_OK;
> 
All of this should be moved to the previous patch; this should be just 
for the sysfs attribute.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

RE: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Muneendra Kumar M]

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

Hi Hannes,
Thanks for the input.

>> @@ -2198,7 +2298,7 @@ fc_attach_transport(struct fc_function_template
>> *ft)
>>   	SETUP_STARGET_ATTRIBUTE_RD(node_name);
>>   	SETUP_STARGET_ATTRIBUTE_RD(port_name);
>   	SETUP_STARGET_ATTRIBUTE_RD(port_id);
>> -
>> +	SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(port_state);
>>   	BUG_ON(count > FC_STARGET_NUM_ATTRS);
>>
>>   	i->starget_attrs[count] = NULL;

>Why did you move it to be a 'starget' attribute?
>I would have thought it should be an 'rport' attribute, seeing that it's
>intrinsic to the fc transport class.
[Muneendra] Correct me if my understanding is wrong.
You want this to be part of /sys/class/fc_remote_ports/
rport-X\:Y-Z/port_state instead of
/sys/class/fc_transport/targetX\:Y\:Z/port_state

Regards,
Muneendra.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

RE: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Muneendra Kumar M]

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

Hi Hannes,


>> -
>> +	SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(port_state);
>>   	BUG_ON(count > FC_STARGET_NUM_ATTRS);
>>
>>   	i->starget_attrs[count] = NULL;

>Why did you move it to be a 'starget' attribute?
>I would have thought it should be an 'rport' attribute, seeing that it's
>intrinsic to the fc transport class.
>>[Muneendra] Correct me if my understanding is wrong.
>>You want this to be part of /sys/class/fc_remote_ports/
>>rport-X\:Y-Z/port_state instead of
>>/sys/class/fc_transport/targetX\:Y\:Z/port_state

Under rport we already have an attribute of port_state which is currently
used to show the port_state.
/sys/class/fc_remote_ports/ rport-X\:Y-Z/port_state

We can add the store functionality to the same to set the port_state.
Is this  approach fine ?


Regards,
Muneendra.

Regards,
Muneendra.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Hannes Reinecke]

On 10/5/20 11:18 AM, Muneendra Kumar M wrote:
> Hi Hannes,
> 
> 
>>> -
>>> +	SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(port_state);
>>>    	BUG_ON(count > FC_STARGET_NUM_ATTRS);
>>>
>>>    	i->starget_attrs[count] = NULL;
> 
>> Why did you move it to be a 'starget' attribute?
>> I would have thought it should be an 'rport' attribute, seeing that it's
>> intrinsic to the fc transport class.
>>> [Muneendra] Correct me if my understanding is wrong.
>>> You want this to be part of /sys/class/fc_remote_ports/
>>> rport-X\:Y-Z/port_state instead of
>>> /sys/class/fc_transport/targetX\:Y\:Z/port_state
> 
> Under rport we already have an attribute of port_state which is currently
> used to show the port_state.
> /sys/class/fc_remote_ports/ rport-X\:Y-Z/port_state
> 
> We can add the store functionality to the same to set the port_state.
> Is this  approach fine ?
> 
That was the idea, yes.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

RE: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Muneendra Kumar M]

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

Hi Hannes,
Thanks for the input.
I will incorporate all your review comments and will send the next version
of the patch.

Regards,
Muneendra.

-----Original Message-----
From: Hannes Reinecke [mailto:hare@suse.de]
Sent: Monday, October 5, 2020 3:00 PM
To: Muneendra Kumar M <muneendra.kumar@broadcom.com>;
linux-scsi@vger.kernel.org
Cc: jsmart2021@gmail.com; emilne@redhat.com; mkumar@redhat.com
Subject: Re: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute
port_state

On 10/5/20 11:18 AM, Muneendra Kumar M wrote:
> Hi Hannes,
>
>
>>> -
>>> +	SETUP_PRIVATE_STARGET_ATTRIBUTE_RW(port_state);
>>>    	BUG_ON(count > FC_STARGET_NUM_ATTRS);
>>>
>>>    	i->starget_attrs[count] = NULL;
>
>> Why did you move it to be a 'starget' attribute?
>> I would have thought it should be an 'rport' attribute, seeing that
>> it's intrinsic to the fc transport class.
>>> [Muneendra] Correct me if my understanding is wrong.
>>> You want this to be part of /sys/class/fc_remote_ports/
>>> rport-X\:Y-Z/port_state instead of
>>> /sys/class/fc_transport/targetX\:Y\:Z/port_state
>
> Under rport we already have an attribute of port_state which is
> currently used to show the port_state.
> /sys/class/fc_remote_ports/ rport-X\:Y-Z/port_state
>
> We can add the store functionality to the same to set the port_state.
> Is this  approach fine ?
>
That was the idea, yes.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG
Nürnberg), Geschäftsführer: Felix Imendörffer

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Benjamin Block]

On Mon, Sep 28, 2020 at 10:20:56AM +0530, Muneendra wrote:
> Added a new sysfs attribute port_state under fc_transport/target*/
> 
> With this new interface the user can move the port_state from
> Marginal -> Online and Online->Marginal.
> 
> On Marginal :This interface will set SCMD_NORETRIES_ABORT bit in
> scmd->state for all the pending io's on the scsi device associated
> with target port.
> 
> On Online :This interface will clear SCMD_NORETRIES_ABORT bit in
> scmd->state for all the pending io's on the scsi device associated
> with target port.
> 
> Below is the interface provided to set the port state to Marginal
> and Online.
> 
> echo "Marginal" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
> echo "Online" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
> 
> Also made changes in fc_remote_port_delete,fc_user_scan_tgt,
> fc_timeout_deleted_rport functions  to handle the new rport state
> FC_PORTSTATE_MARGINAL.

Hey Muneendra, out of curiosity, what drives these changes to the
port_state in userspace, and based on what?

I imagine something uses the other bunch of sysfs attributes that were
introduced recently that get feed by FPIN notifications about congestion
and such, or? 

If so, is there any plans to integrated something along the lines in
multipathd/multipath-tools? Or maybe I missed that.

                                                            - Benjamin

> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> Changed from a noretries_abort attribute under fc_transport/target*/ to
> port_state for changing the port_state to a marginal state
> ---
>  drivers/scsi/scsi_transport_fc.c | 140 +++++++++++++++++++++++++++----
>  1 file changed, 122 insertions(+), 18 deletions(-)
> 


-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Hannes Reinecke]

On 10/2/20 6:26 PM, Benjamin Block wrote:
> On Mon, Sep 28, 2020 at 10:20:56AM +0530, Muneendra wrote:
>> Added a new sysfs attribute port_state under fc_transport/target*/
>>
>> With this new interface the user can move the port_state from
>> Marginal -> Online and Online->Marginal.
>>
>> On Marginal :This interface will set SCMD_NORETRIES_ABORT bit in
>> scmd->state for all the pending io's on the scsi device associated
>> with target port.
>>
>> On Online :This interface will clear SCMD_NORETRIES_ABORT bit in
>> scmd->state for all the pending io's on the scsi device associated
>> with target port.
>>
>> Below is the interface provided to set the port state to Marginal
>> and Online.
>>
>> echo "Marginal" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
>> echo "Online" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
>>
>> Also made changes in fc_remote_port_delete,fc_user_scan_tgt,
>> fc_timeout_deleted_rport functions  to handle the new rport state
>> FC_PORTSTATE_MARGINAL.
> 
> Hey Muneendra, out of curiosity, what drives these changes to the
> port_state in userspace, and based on what?
> 
> I imagine something uses the other bunch of sysfs attributes that were
> introduced recently that get feed by FPIN notifications about congestion
> and such, or?
> 
> If so, is there any plans to integrated something along the lines in
> multipathd/multipath-tools? Or maybe I missed that.
> 
My idea here was that that 'marginal' port state is initiated by FPIN 
notifications; ideally set from the driver itself, but initially most 
likely from userspace (eg multipathd).

Problem here is that the FPIN comes in various flavours (eg congestion 
or link degradation), each of which will result in either one or several 
FPIN notifications.
EG for link congestion it is expected to get messages at a certain 
frequency for as long as the situation occurs.
Meaning we would have to have some sort of mechanism for checking that 
frequency, and reset the state if the condition persists.

How exactly we're going to do this, whether by external daemon or 
integrated into multipathd, is currently subject for debate and testing.
I would prefer to have it integrated into multipathd, but it might well 
become too complex such that an external daemon might be the better option.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v2 7/8] scsi_transport_fc: Added a new sysfs attribute port_state

[Benjamin Block]

On Mon, Oct 05, 2020 at 08:49:27AM +0200, Hannes Reinecke wrote:
> On 10/2/20 6:26 PM, Benjamin Block wrote:
> > On Mon, Sep 28, 2020 at 10:20:56AM +0530, Muneendra wrote:
> > > Added a new sysfs attribute port_state under fc_transport/target*/
> > > 
> > > With this new interface the user can move the port_state from
> > > Marginal -> Online and Online->Marginal.
> > > 
> > > On Marginal :This interface will set SCMD_NORETRIES_ABORT bit in
> > > scmd->state for all the pending io's on the scsi device associated
> > > with target port.
> > > 
> > > On Online :This interface will clear SCMD_NORETRIES_ABORT bit in
> > > scmd->state for all the pending io's on the scsi device associated
> > > with target port.
> > > 
> > > Below is the interface provided to set the port state to Marginal
> > > and Online.
> > > 
> > > echo "Marginal" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
> > > echo "Online" >> /sys/class/fc_transport/targetX\:Y\:Z/port_state
> > > 
> > > Also made changes in fc_remote_port_delete,fc_user_scan_tgt,
> > > fc_timeout_deleted_rport functions  to handle the new rport state
> > > FC_PORTSTATE_MARGINAL.
> > 
> > Hey Muneendra, out of curiosity, what drives these changes to the
> > port_state in userspace, and based on what?
> > 
> > I imagine something uses the other bunch of sysfs attributes that were
> > introduced recently that get feed by FPIN notifications about congestion
> > and such, or?
> > 
> > If so, is there any plans to integrated something along the lines in
> > multipathd/multipath-tools? Or maybe I missed that.
> > 
>
> My idea here was that that 'marginal' port state is initiated by FPIN
> notifications; ideally set from the driver itself, but initially most likely
> from userspace (eg multipathd).
> 
> Problem here is that the FPIN comes in various flavours (eg congestion or
> link degradation), each of which will result in either one or several FPIN
> notifications.
> EG for link congestion it is expected to get messages at a certain frequency
> for as long as the situation occurs.
> Meaning we would have to have some sort of mechanism for checking that
> frequency, and reset the state if the condition persists.

Yeah, that makes sense.

> 
> How exactly we're going to do this, whether by external daemon or integrated
> into multipathd, is currently subject for debate and testing.
> I would prefer to have it integrated into multipathd, but it might well
> become too complex such that an external daemon might be the better option.
> 

Less "moving parts" would be great. A proper FC setup with multipath is
already a burden on many users, yet an other daemon to juggle with and
coordinate adds more to that. But I'm not doing the work rn so that's
not my choice :)

Thanks for the explanation Hannes.

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

[PATCH v2 8/8] lpfc: Added support to handle marginal state

[Muneendra]

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

Added additional check to set SCMD_NORETRIES_ABORT bit in scmd->state
if port state is marginal prior to initiating io to the port.

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

---
v2:
New patch
---
 drivers/scsi/lpfc/lpfc_scsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index 5e802c8b22a9..1198351d34a8 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -4526,6 +4526,12 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
 		cmnd->result = err;
 		goto out_fail_command;
 	}
+
+	/*
+	 * If port state is marginal
+	 * Set the SCMD_NORETRIES_ABORT bit in scmd->state
+	 */
+	fc_rport_chkmarginal_set_noretries(rport, cmnd);
 	ndlp = rdata->pnode;
 
 	if ((scsi_get_prot_op(cmnd) != SCSI_PROT_NORMAL) &&
-- 
2.26.2


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 8/8] lpfc: Added support to handle marginal state

[Hannes Reinecke]

On 9/28/20 6:50 AM, Muneendra wrote:
> Added additional check to set SCMD_NORETRIES_ABORT bit in scmd->state
> if port state is marginal prior to initiating io to the port.
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
> 
> ---
> v2:
> New patch
> ---
>   drivers/scsi/lpfc/lpfc_scsi.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index 5e802c8b22a9..1198351d34a8 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -4526,6 +4526,12 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
>   		cmnd->result = err;
>   		goto out_fail_command;
>   	}
> +
> +	/*
> +	 * If port state is marginal
> +	 * Set the SCMD_NORETRIES_ABORT bit in scmd->state
> +	 */
> +	fc_rport_chkmarginal_set_noretries(rport, cmnd);
>   	ndlp = rdata->pnode;
>   
>   	if ((scsi_get_prot_op(cmnd) != SCSI_PROT_NORMAL) &&
> 
This really should be moved into the transport class; fc_block_rport() 
would be an ideal place for it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

RE: [PATCH v2 8/8] lpfc: Added support to handle marginal state

[Muneendra Kumar M]

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

Hi Hannes,
Thanks for the input.

>> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
>> @@ -4526,6 +4526,12 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct
>> scsi_cmnd *cmnd)
>>   		cmnd->result = err;
>>   		goto out_fail_command;
>>   	}
>> +
>> +	/*
>> +	 * If port state is marginal
>> +	 * Set the SCMD_NORETRIES_ABORT bit in scmd->state
>> +	 */
>> +	fc_rport_chkmarginal_set_noretries(rport, cmnd);
>>   	ndlp = rdata->pnode;
>>
>>   	if ((scsi_get_prot_op(cmnd) != SCSI_PROT_NORMAL) &&
>>
>This really should be moved into the transport class; fc_block_rport()
>would be an ideal place for it.

[Muneendra]Correct me if I didn't understand correctly.
As fc_block_rport cannot take arg of scsi_cmd can we add it as part of
fc_block_scsi_eh ?

Regards,
Muneendra.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 8/8] lpfc: Added support to handle marginal state

[Hannes Reinecke]

On 9/30/20 1:33 PM, Muneendra Kumar M wrote:
> Hi Hannes,
> Thanks for the input.
> 
>>> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
>>> @@ -4526,6 +4526,12 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct
>>> scsi_cmnd *cmnd)
>>>    		cmnd->result = err;
>>>    		goto out_fail_command;
>>>    	}
>>> +
>>> +	/*
>>> +	 * If port state is marginal
>>> +	 * Set the SCMD_NORETRIES_ABORT bit in scmd->state
>>> +	 */
>>> +	fc_rport_chkmarginal_set_noretries(rport, cmnd);
>>>    	ndlp = rdata->pnode;
>>>
>>>    	if ((scsi_get_prot_op(cmnd) != SCSI_PROT_NORMAL) &&
>>>
>> This really should be moved into the transport class; fc_block_rport()
>> would be an ideal place for it.
> 
> [Muneendra]Correct me if I didn't understand correctly.
> As fc_block_rport cannot take arg of scsi_cmd can we add it as part of
> fc_block_scsi_eh ?
> 
Ah, right.
Actually I meant 'fc_remote_port_chkready()'.
That doesnt' take an scmd as argument, but I don't see why we can't 
modify it to have an additonal 'sdev' parameter...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

RE: [PATCH v2 8/8] lpfc: Added support to handle marginal state

[Muneendra Kumar M]

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

Hi Hannes,

>>> +     * If port state is marginal
>>> +     * Set the SCMD_NORETRIES_ABORT bit in scmd->state
>>> +     */
>>> +               fc_rport_chkmarginal_set_noretries(rport, cmnd);
>>>        ndlp = rdata->pnode;
>>>
>>>        if ((scsi_get_prot_op(cmnd) != SCSI_PROT_NORMAL) &&
>>>
>> This really should be moved into the transport class;
>> fc_block_rport() would be an ideal place for it.
>
>> [Muneendra]Correct me if I didn't understand correctly.
>> As fc_block_rport cannot take arg of scsi_cmd can we add it as part of
>> fc_block_scsi_eh ?
> >
>>Ah, right.
>>Actually I meant 'fc_remote_port_chkready()'.
>>That doesnt' take an scmd as argument, but I don't see why we can't modify
>>it to have an additonal 'sdev' parameter...

fc_remote_port_chkready function is even getting called in slave alloc
(lpfc_slave_alloc, qla2xxx_slave_alloc,...)  and few ELS initiative
functions
bnx2fc_initiate_els, qedf_initiate_els.. where scsi_cmnd cannot be passed.
In these cases we need to pass NULL for scsi_cmnd ?

Regards,
Muneendra.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4177 bytes --]

Re: [PATCH v2 0/8] scsi: Support to handle Intermittent errors

[Mike Christie]

On 9/27/20 11:50 PM, Muneendra wrote:
> This patch adds a support to prevent retries of all the pending/inflight
> io's after an abort succeeds on a particular device when transport
> connectivity to the device is encountering intermittent errors.
> 
> Intermittent connectivity is a condition that can be detected by transport
> fabric notifications. A service can monitor the ELS notifications and
> take action on all the outstanding io's of a scsi device at that instant.
> 

Is the service mentioned above a new daemon or is it integrated into
something like multipathd?

What does the part about monitoring ELS notifications mean? Is the
service just doing something like a ELS ECHO, or is it able to watch
the IO on the wire/card (like if you did tcpdump and watched iscsi/tcp
traffic) or is it something completely different?

Re: [PATCH v2 0/8] scsi: Support to handle Intermittent errors

[James Smart]

On 10/2/2020 10:01 AM, Mike Christie wrote:
> On 9/27/20 11:50 PM, Muneendra wrote:
>> This patch adds a support to prevent retries of all the pending/inflight
>> io's after an abort succeeds on a particular device when transport
>> connectivity to the device is encountering intermittent errors.
>>
>> Intermittent connectivity is a condition that can be detected by transport
>> fabric notifications. A service can monitor the ELS notifications and
>> take action on all the outstanding io's of a scsi device at that instant.
>>
> 
> Is the service mentioned above a new daemon or is it integrated into
> something like multipathd?
> 
> What does the part about monitoring ELS notifications mean? Is the
> service just doing something like a ELS ECHO, or is it able to watch
> the IO on the wire/card (like if you did tcpdump and watched iscsi/tcp
> traffic) or is it something completely different?
> 

For the last part.... the FC drivers, when receiving FC FPIN ELS's are 
calling a scsi transport routine with the FPIN payload.  The transport 
is pushing this as an "event" via netlink.  An app bound to the local 
address used by the scsi transport can receive the event and parse it.

This is a new daemon, specific to FC, which monitors for FPIN events, 
parses the related topology devices, then interacts with sysfs and 
possibly multipath based on what it's seeing from the fabric.

-- james