* [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer
@ 2016-03-14 18:14 Satish Kharat
2016-03-14 18:14 ` [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset Satish Kharat
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Satish Kharat @ 2016-03-14 18:14 UTC (permalink / raw)
To: satishkh, linux-scsi
If an I/O times out and an abort issued by host, if the abort is
successful we need to set scsi status as DID_ABORT. Or else the
mid-layer error handler which looks for this error code, will
offline the device. Also if the original I/O is not found in fnic
firmware, we will consider the abort as successful.
Fnic driver version changed from 1.6.0.17a to 1.6.0.19,
version 1.6.0.18 has been skipped
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/scsi/fnic/fnic.h | 2 +-
drivers/scsi/fnic/fnic_scsi.c | 32 +++++++++++++++++++++++++++-----
2 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index ce129e5..52a53f8 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -39,7 +39,7 @@
#define DRV_NAME "fnic"
#define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
-#define DRV_VERSION "1.6.0.17a"
+#define DRV_VERSION "1.6.0.19"
#define PFX DRV_NAME ": "
#define DFX DRV_NAME "%d: "
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 266b909..b9732b1 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -1092,6 +1092,11 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
atomic64_inc(
&term_stats->terminate_fw_timeouts);
break;
+ case FCPIO_ITMF_REJECTED:
+ FNIC_SCSI_DBG(KERN_INFO, fnic->lport->host,
+ "abort reject recd. id %d\n",
+ (int)(id & FNIC_TAG_MASK));
+ break;
case FCPIO_IO_NOT_FOUND:
if (CMD_FLAGS(sc) & FNIC_IO_ABTS_ISSUED)
atomic64_inc(&abts_stats->abort_io_not_found);
@@ -1112,9 +1117,14 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
spin_unlock_irqrestore(io_lock, flags);
return;
}
+
CMD_ABTS_STATUS(sc) = hdr_status;
CMD_FLAGS(sc) |= FNIC_IO_ABT_TERM_DONE;
+ /* If the status is IO not found consider it as success */
+ if (hdr_status == FCPIO_IO_NOT_FOUND)
+ CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
+
atomic64_dec(&fnic_stats->io_stats.active_ios);
if (atomic64_read(&fnic->io_cmpl_skip))
atomic64_dec(&fnic->io_cmpl_skip);
@@ -1927,21 +1937,33 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
+ start_time = io_req->start_time;
/*
* firmware completed the abort, check the status,
- * free the io_req irrespective of failure or success
+ * free the io_req if successful. If abort fails,
+ * Device reset will clean the I/O.
*/
- if (CMD_ABTS_STATUS(sc) != FCPIO_SUCCESS)
+ if (CMD_ABTS_STATUS(sc) == FCPIO_SUCCESS) {
+ CMD_SP(sc) = NULL;
+ }
+ else
+ {
ret = FAILED;
-
- CMD_SP(sc) = NULL;
+ spin_unlock_irqrestore(io_lock, flags);
+ goto fnic_abort_cmd_end;
+ }
spin_unlock_irqrestore(io_lock, flags);
- start_time = io_req->start_time;
fnic_release_ioreq_buf(fnic, io_req, sc);
mempool_free(io_req, fnic->io_req_pool);
+ if (sc->scsi_done) {
+ /* Call SCSI completion function to complete the IO */
+ sc->result = (DID_ABORT << 16);
+ sc->scsi_done(sc);
+ }
+
fnic_abort_cmd_end:
FNIC_TRACE(fnic_abort_cmd, sc->device->host->host_no,
sc->request->tag, sc,
--
2.4.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset
2016-03-14 18:14 [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Satish Kharat
@ 2016-03-14 18:14 ` Satish Kharat
2016-03-14 20:14 ` Ewan Milne
2016-03-15 9:53 ` Christoph Hellwig
2016-03-14 18:14 ` [PATCH 3/5] Using rport->dd_data to check rport online instead of rport_lookup Satish Kharat
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Satish Kharat @ 2016-03-14 18:14 UTC (permalink / raw)
To: satishkh, linux-scsi
In case of LUN reset, the device reset command is issued with one of
the I/Os that has timed out on that LUN. The change is to also return
this I/O with error status set to DID_RESET.
Fnic driver version changed from 1.6.0.19 to 1.6.0.20
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/scsi/fnic/fnic.h | 2 +-
drivers/scsi/fnic/fnic_scsi.c | 35 +++++++++++++++++++++++++++--------
2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 52a53f8..1023eae 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -39,7 +39,7 @@
#define DRV_NAME "fnic"
#define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
-#define DRV_VERSION "1.6.0.19"
+#define DRV_VERSION "1.6.0.20"
#define PFX DRV_NAME ": "
#define DFX DRV_NAME "%d: "
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index b9732b1..a3e0f69 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -2041,7 +2041,9 @@ lr_io_req_end:
* successfully aborted, 1 otherwise
*/
static int fnic_clean_pending_aborts(struct fnic *fnic,
- struct scsi_cmnd *lr_sc)
+ struct scsi_cmnd *lr_sc,
+ bool new_sc)
+
{
int tag, abt_tag;
struct fnic_io_req *io_req;
@@ -2059,10 +2061,10 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
spin_lock_irqsave(io_lock, flags);
sc = scsi_host_find_tag(fnic->lport->host, tag);
/*
- * ignore this lun reset cmd or cmds that do not belong to
- * this lun
+ * ignore this lun reset cmd if issued using new SC
+ * or cmds that do not belong to this lun
*/
- if (!sc || sc == lr_sc || sc->device != lun_dev) {
+ if (!sc || ((sc == lr_sc) && new_sc) || sc->device != lun_dev) {
spin_unlock_irqrestore(io_lock, flags);
continue;
}
@@ -2168,11 +2170,27 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
goto clean_pending_aborts_end;
}
CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
- CMD_SP(sc) = NULL;
+
+ /* original sc used for lr is handled by dev reset code */
+ if (sc != lr_sc)
+ CMD_SP(sc) = NULL;
spin_unlock_irqrestore(io_lock, flags);
- fnic_release_ioreq_buf(fnic, io_req, sc);
- mempool_free(io_req, fnic->io_req_pool);
+ /* original sc used for lr is handled by dev reset code */
+ if (sc != lr_sc) {
+ fnic_release_ioreq_buf(fnic, io_req, sc);
+ mempool_free(io_req, fnic->io_req_pool);
+ }
+
+ /*
+ * Any IO is returned during reset, it needs to call scsi_done
+ * to return the scsi_cmnd to upper layer.
+ */
+ if (sc->scsi_done) {
+ /* Set result to let upper SCSI layer retry */
+ sc->result = DID_RESET << 16;
+ sc->scsi_done(sc);
+ }
}
schedule_timeout(msecs_to_jiffies(2 * fnic->config.ed_tov));
@@ -2266,6 +2284,7 @@ int fnic_device_reset(struct scsi_cmnd *sc)
int tag = 0;
DECLARE_COMPLETION_ONSTACK(tm_done);
int tag_gen_flag = 0; /*to track tags allocated by fnic driver*/
+ bool new_sc = 0;
/* Wait for rport to unblock */
fc_block_scsi_eh(sc);
@@ -2452,7 +2471,7 @@ int fnic_device_reset(struct scsi_cmnd *sc)
* the lun reset cmd. If all cmds get cleaned, the lun reset
* succeeds
*/
- if (fnic_clean_pending_aborts(fnic, sc)) {
+ if (fnic_clean_pending_aborts(fnic, sc, new_sc)) {
spin_lock_irqsave(io_lock, flags);
io_req = (struct fnic_io_req *)CMD_SP(sc);
FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
--
2.4.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/5] Using rport->dd_data to check rport online instead of rport_lookup.
2016-03-14 18:14 [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Satish Kharat
2016-03-14 18:14 ` [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset Satish Kharat
@ 2016-03-14 18:14 ` Satish Kharat
2016-03-14 20:15 ` Ewan Milne
2016-03-14 18:14 ` [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue Satish Kharat
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Satish Kharat @ 2016-03-14 18:14 UTC (permalink / raw)
To: satishkh, linux-scsi
When issuing I/O we check if rport is online through libfc
rport_lookup() function which needs to be protected by mutex lock
that cannot acquired in I/O context. The change is to use midlayer
remote port’s dd_data which is preserved until its devloss timeout
and no protection is required.
Fnic driver version changed from 1.6.0.20 to 1.6.0.21
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/scsi/fnic/fnic.h | 2 +-
drivers/scsi/fnic/fnic_scsi.c | 20 +++++++++++---------
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 1023eae..9ddc920 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -39,7 +39,7 @@
#define DRV_NAME "fnic"
#define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
-#define DRV_VERSION "1.6.0.20"
+#define DRV_VERSION "1.6.0.21"
#define PFX DRV_NAME ": "
#define DFX DRV_NAME "%d: "
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index a3e0f69..8492143 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -439,7 +439,6 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
int sg_count = 0;
unsigned long flags = 0;
unsigned long ptr;
- struct fc_rport_priv *rdata;
spinlock_t *io_lock = NULL;
int io_lock_acquired = 0;
@@ -455,14 +454,17 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
return 0;
}
- rdata = lp->tt.rport_lookup(lp, rport->port_id);
- if (!rdata || (rdata->rp_state == RPORT_ST_DELETE)) {
- FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
- "returning IO as rport is removed\n");
- atomic64_inc(&fnic_stats->misc_stats.rport_not_ready);
- sc->result = DID_NO_CONNECT;
- done(sc);
- return 0;
+ if (rport) {
+ struct fc_rport_libfc_priv *rp = rport->dd_data;
+
+ if (!rp || rp->rp_state != RPORT_ST_READY) {
+ FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+ "returning DID_NO_CONNECT for IO as rport is removed\n");
+ atomic64_inc(&fnic_stats->misc_stats.rport_not_ready);
+ sc->result = DID_NO_CONNECT;
+ done(sc);
+ return 0;
+ }
}
if (lp->state != LPORT_ST_READY || !(lp->link_up))
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue
2016-03-14 18:14 [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Satish Kharat
2016-03-14 18:14 ` [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset Satish Kharat
2016-03-14 18:14 ` [PATCH 3/5] Using rport->dd_data to check rport online instead of rport_lookup Satish Kharat
@ 2016-03-14 18:14 ` Satish Kharat
2016-03-14 20:16 ` Ewan Milne
` (2 more replies)
2016-03-14 18:14 ` [PATCH 5/5] Leftshift returned scsi_cmnd error code 16 bits Satish Kharat
2016-03-14 19:59 ` [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Ewan Milne
4 siblings, 3 replies; 15+ messages in thread
From: Satish Kharat @ 2016-03-14 18:14 UTC (permalink / raw)
To: satishkh, linux-scsi
Fnic does not support mutiqueue. Setting the scsi_host_template in
fnic_host_template to indicate that.
Changing fnic_version from 1.6.0.21 to 1.6.0.22
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/scsi/fnic/fnic.h | 2 +-
drivers/scsi/fnic/fnic_main.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 9ddc920..35264c7 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -39,7 +39,7 @@
#define DRV_NAME "fnic"
#define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
-#define DRV_VERSION "1.6.0.21"
+#define DRV_VERSION "1.6.0.22"
#define PFX DRV_NAME ": "
#define DFX DRV_NAME "%d: "
diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
index 58ce902..fa8a121 100644
--- a/drivers/scsi/fnic/fnic_main.c
+++ b/drivers/scsi/fnic/fnic_main.c
@@ -119,6 +119,7 @@ static struct scsi_host_template fnic_host_template = {
.max_sectors = 0xffff,
.shost_attrs = fnic_attrs,
.track_queue_depth = 1,
+ .disable_blk_mq = 1,
};
static void
--
2.4.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/5] Leftshift returned scsi_cmnd error code 16 bits
2016-03-14 18:14 [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Satish Kharat
` (2 preceding siblings ...)
2016-03-14 18:14 ` [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue Satish Kharat
@ 2016-03-14 18:14 ` Satish Kharat
2016-03-14 20:17 ` Ewan Milne
2016-03-14 19:59 ` [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Ewan Milne
4 siblings, 1 reply; 15+ messages in thread
From: Satish Kharat @ 2016-03-14 18:14 UTC (permalink / raw)
To: satishkh, linux-scsi
The the scsi_cmnd error code is expected to be in the left 16 bits
of the result field. Change is to correct this.
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/scsi/fnic/fnic.h | 2 +-
drivers/scsi/fnic/fnic_scsi.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 35264c7..e4e22f1 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -39,7 +39,7 @@
#define DRV_NAME "fnic"
#define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
-#define DRV_VERSION "1.6.0.22"
+#define DRV_VERSION "1.6.0.24"
#define PFX DRV_NAME ": "
#define DFX DRV_NAME "%d: "
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 8492143..92a4ca3 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -461,7 +461,7 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
"returning DID_NO_CONNECT for IO as rport is removed\n");
atomic64_inc(&fnic_stats->misc_stats.rport_not_ready);
- sc->result = DID_NO_CONNECT;
+ sc->result = DID_NO_CONNECT<<16;
done(sc);
return 0;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer
2016-03-14 18:14 [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Satish Kharat
` (3 preceding siblings ...)
2016-03-14 18:14 ` [PATCH 5/5] Leftshift returned scsi_cmnd error code 16 bits Satish Kharat
@ 2016-03-14 19:59 ` Ewan Milne
2016-03-16 22:38 ` Satish Kharat (satishkh)
4 siblings, 1 reply; 15+ messages in thread
From: Ewan Milne @ 2016-03-14 19:59 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
On Mon, 2016-03-14 at 11:14 -0700, Satish Kharat wrote:
> If an I/O times out and an abort issued by host, if the abort is
> successful we need to set scsi status as DID_ABORT. Or else the
> mid-layer error handler which looks for this error code, will
> offline the device. Also if the original I/O is not found in fnic
> firmware, we will consider the abort as successful.
> Fnic driver version changed from 1.6.0.17a to 1.6.0.19,
> version 1.6.0.18 has been skipped
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_scsi.c | 32 +++++++++++++++++++++++++++-----
> 2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index ce129e5..52a53f8 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.17a"
> +#define DRV_VERSION "1.6.0.19"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index 266b909..b9732b1 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -1092,6 +1092,11 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> atomic64_inc(
> &term_stats->terminate_fw_timeouts);
> break;
> + case FCPIO_ITMF_REJECTED:
> + FNIC_SCSI_DBG(KERN_INFO, fnic->lport->host,
> + "abort reject recd. id %d\n",
> + (int)(id & FNIC_TAG_MASK));
> + break;
> case FCPIO_IO_NOT_FOUND:
> if (CMD_FLAGS(sc) & FNIC_IO_ABTS_ISSUED)
> atomic64_inc(&abts_stats->abort_io_not_found);
> @@ -1112,9 +1117,14 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> spin_unlock_irqrestore(io_lock, flags);
> return;
> }
> +
> CMD_ABTS_STATUS(sc) = hdr_status;
> CMD_FLAGS(sc) |= FNIC_IO_ABT_TERM_DONE;
>
> + /* If the status is IO not found consider it as success */
> + if (hdr_status == FCPIO_IO_NOT_FOUND)
> + CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
> +
Minor nitpick: I would have done:
if (hdr_status == FCP_IO_NOT_FOUND)
CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
else
CMD_ABTS_STATUS(sc) = hdr_status;
rather than overwriting the assignment made several lines above, it is
more explicit and less prone to errors if the code is later changed.
> atomic64_dec(&fnic_stats->io_stats.active_ios);
> if (atomic64_read(&fnic->io_cmpl_skip))
> atomic64_dec(&fnic->io_cmpl_skip);
> @@ -1927,21 +1937,33 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
>
> CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
>
> + start_time = io_req->start_time;
> /*
> * firmware completed the abort, check the status,
> - * free the io_req irrespective of failure or success
> + * free the io_req if successful. If abort fails,
> + * Device reset will clean the I/O.
> */
> - if (CMD_ABTS_STATUS(sc) != FCPIO_SUCCESS)
> + if (CMD_ABTS_STATUS(sc) == FCPIO_SUCCESS) {
> + CMD_SP(sc) = NULL;
> + }
> + else
> + {
> ret = FAILED;
> -
> - CMD_SP(sc) = NULL;
> + spin_unlock_irqrestore(io_lock, flags);
> + goto fnic_abort_cmd_end;
> + }
>
> spin_unlock_irqrestore(io_lock, flags);
>
> - start_time = io_req->start_time;
What was the point of moving the assignment of start_time?
Other places in the code still set it just before the call to
fnic_release_ioreq_buf().
> fnic_release_ioreq_buf(fnic, io_req, sc);
> mempool_free(io_req, fnic->io_req_pool);
>
> + if (sc->scsi_done) {
> + /* Call SCSI completion function to complete the IO */
> + sc->result = (DID_ABORT << 16);
> + sc->scsi_done(sc);
> + }
> +
> fnic_abort_cmd_end:
> FNIC_TRACE(fnic_abort_cmd, sc->device->host->host_no,
> sc->request->tag, sc,
Despite the above comments, I guess this looks OK.
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset
2016-03-14 18:14 ` [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset Satish Kharat
@ 2016-03-14 20:14 ` Ewan Milne
2016-03-15 9:53 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Ewan Milne @ 2016-03-14 20:14 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
On Mon, 2016-03-14 at 11:14 -0700, Satish Kharat wrote:
> In case of LUN reset, the device reset command is issued with one of
> the I/Os that has timed out on that LUN. The change is to also return
> this I/O with error status set to DID_RESET.
> Fnic driver version changed from 1.6.0.19 to 1.6.0.20
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_scsi.c | 35 +++++++++++++++++++++++++++--------
> 2 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 52a53f8..1023eae 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.19"
> +#define DRV_VERSION "1.6.0.20"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index b9732b1..a3e0f69 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -2041,7 +2041,9 @@ lr_io_req_end:
> * successfully aborted, 1 otherwise
> */
> static int fnic_clean_pending_aborts(struct fnic *fnic,
> - struct scsi_cmnd *lr_sc)
> + struct scsi_cmnd *lr_sc,
> + bool new_sc)
> +
I don't see that this or any of the subsequent patches do anything with
this argument except pass 0 to it. So why add it?
> {
> int tag, abt_tag;
> struct fnic_io_req *io_req;
> @@ -2059,10 +2061,10 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
> spin_lock_irqsave(io_lock, flags);
> sc = scsi_host_find_tag(fnic->lport->host, tag);
> /*
> - * ignore this lun reset cmd or cmds that do not belong to
> - * this lun
> + * ignore this lun reset cmd if issued using new SC
> + * or cmds that do not belong to this lun
> */
> - if (!sc || sc == lr_sc || sc->device != lun_dev) {
> + if (!sc || ((sc == lr_sc) && new_sc) || sc->device != lun_dev) {
> spin_unlock_irqrestore(io_lock, flags);
> continue;
> }
> @@ -2168,11 +2170,27 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
> goto clean_pending_aborts_end;
> }
> CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
> - CMD_SP(sc) = NULL;
> +
> + /* original sc used for lr is handled by dev reset code */
> + if (sc != lr_sc)
> + CMD_SP(sc) = NULL;
> spin_unlock_irqrestore(io_lock, flags);
>
> - fnic_release_ioreq_buf(fnic, io_req, sc);
> - mempool_free(io_req, fnic->io_req_pool);
> + /* original sc used for lr is handled by dev reset code */
> + if (sc != lr_sc) {
> + fnic_release_ioreq_buf(fnic, io_req, sc);
> + mempool_free(io_req, fnic->io_req_pool);
> + }
> +
> + /*
> + * Any IO is returned during reset, it needs to call scsi_done
> + * to return the scsi_cmnd to upper layer.
> + */
> + if (sc->scsi_done) {
> + /* Set result to let upper SCSI layer retry */
> + sc->result = DID_RESET << 16;
> + sc->scsi_done(sc);
> + }
> }
>
> schedule_timeout(msecs_to_jiffies(2 * fnic->config.ed_tov));
> @@ -2266,6 +2284,7 @@ int fnic_device_reset(struct scsi_cmnd *sc)
> int tag = 0;
> DECLARE_COMPLETION_ONSTACK(tm_done);
> int tag_gen_flag = 0; /*to track tags allocated by fnic driver*/
> + bool new_sc = 0;
>
> /* Wait for rport to unblock */
> fc_block_scsi_eh(sc);
> @@ -2452,7 +2471,7 @@ int fnic_device_reset(struct scsi_cmnd *sc)
> * the lun reset cmd. If all cmds get cleaned, the lun reset
> * succeeds
> */
> - if (fnic_clean_pending_aborts(fnic, sc)) {
> + if (fnic_clean_pending_aborts(fnic, sc, new_sc)) {
> spin_lock_irqsave(io_lock, flags);
> io_req = (struct fnic_io_req *)CMD_SP(sc);
> FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] Using rport->dd_data to check rport online instead of rport_lookup.
2016-03-14 18:14 ` [PATCH 3/5] Using rport->dd_data to check rport online instead of rport_lookup Satish Kharat
@ 2016-03-14 20:15 ` Ewan Milne
0 siblings, 0 replies; 15+ messages in thread
From: Ewan Milne @ 2016-03-14 20:15 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
On Mon, 2016-03-14 at 11:14 -0700, Satish Kharat wrote:
> When issuing I/O we check if rport is online through libfc
> rport_lookup() function which needs to be protected by mutex lock
> that cannot acquired in I/O context. The change is to use midlayer
> remote port’s dd_data which is preserved until its devloss timeout
> and no protection is required.
>
> Fnic driver version changed from 1.6.0.20 to 1.6.0.21
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_scsi.c | 20 +++++++++++---------
> 2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 1023eae..9ddc920 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.20"
> +#define DRV_VERSION "1.6.0.21"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index a3e0f69..8492143 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -439,7 +439,6 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
> int sg_count = 0;
> unsigned long flags = 0;
> unsigned long ptr;
> - struct fc_rport_priv *rdata;
> spinlock_t *io_lock = NULL;
> int io_lock_acquired = 0;
>
> @@ -455,14 +454,17 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
> return 0;
> }
>
> - rdata = lp->tt.rport_lookup(lp, rport->port_id);
> - if (!rdata || (rdata->rp_state == RPORT_ST_DELETE)) {
> - FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
> - "returning IO as rport is removed\n");
> - atomic64_inc(&fnic_stats->misc_stats.rport_not_ready);
> - sc->result = DID_NO_CONNECT;
You fixed this in patch 5/5 to << 16, might as well just do it right
in the first patch here and eliminate that one.
> - done(sc);
> - return 0;
> + if (rport) {
> + struct fc_rport_libfc_priv *rp = rport->dd_data;
> +
> + if (!rp || rp->rp_state != RPORT_ST_READY) {
> + FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
> + "returning DID_NO_CONNECT for IO as rport is removed\n");
> + atomic64_inc(&fnic_stats->misc_stats.rport_not_ready);
> + sc->result = DID_NO_CONNECT;
> + done(sc);
> + return 0;
> + }
> }
>
> if (lp->state != LPORT_ST_READY || !(lp->link_up))
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue
2016-03-14 18:14 ` [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue Satish Kharat
@ 2016-03-14 20:16 ` Ewan Milne
2016-03-15 9:46 ` Johannes Thumshirn
2016-03-15 9:51 ` Christoph Hellwig
2 siblings, 0 replies; 15+ messages in thread
From: Ewan Milne @ 2016-03-14 20:16 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
On Mon, 2016-03-14 at 11:14 -0700, Satish Kharat wrote:
> Fnic does not support mutiqueue. Setting the scsi_host_template in
> fnic_host_template to indicate that.
>
> Changing fnic_version from 1.6.0.21 to 1.6.0.22
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_main.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 9ddc920..35264c7 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.21"
> +#define DRV_VERSION "1.6.0.22"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
> index 58ce902..fa8a121 100644
> --- a/drivers/scsi/fnic/fnic_main.c
> +++ b/drivers/scsi/fnic/fnic_main.c
> @@ -119,6 +119,7 @@ static struct scsi_host_template fnic_host_template = {
> .max_sectors = 0xffff,
> .shost_attrs = fnic_attrs,
> .track_queue_depth = 1,
> + .disable_blk_mq = 1,
> };
>
> static void
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] Leftshift returned scsi_cmnd error code 16 bits
2016-03-14 18:14 ` [PATCH 5/5] Leftshift returned scsi_cmnd error code 16 bits Satish Kharat
@ 2016-03-14 20:17 ` Ewan Milne
0 siblings, 0 replies; 15+ messages in thread
From: Ewan Milne @ 2016-03-14 20:17 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
On Mon, 2016-03-14 at 11:14 -0700, Satish Kharat wrote:
> The the scsi_cmnd error code is expected to be in the left 16 bits
> of the result field. Change is to correct this.
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_scsi.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 35264c7..e4e22f1 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.22"
> +#define DRV_VERSION "1.6.0.24"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index 8492143..92a4ca3 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -461,7 +461,7 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
> FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
> "returning DID_NO_CONNECT for IO as rport is removed\n");
> atomic64_inc(&fnic_stats->misc_stats.rport_not_ready);
> - sc->result = DID_NO_CONNECT;
> + sc->result = DID_NO_CONNECT<<16;
> done(sc);
> return 0;
> }
Should just fix patch 3/5 instead.
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue
2016-03-14 18:14 ` [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue Satish Kharat
2016-03-14 20:16 ` Ewan Milne
@ 2016-03-15 9:46 ` Johannes Thumshirn
2016-03-15 9:51 ` Christoph Hellwig
2 siblings, 0 replies; 15+ messages in thread
From: Johannes Thumshirn @ 2016-03-15 9:46 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
On Mon, Mar 14, 2016 at 11:14:21AM -0700, Satish Kharat wrote:
> Fnic does not support mutiqueue. Setting the scsi_host_template in
> fnic_host_template to indicate that.
>
> Changing fnic_version from 1.6.0.21 to 1.6.0.22
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_main.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 9ddc920..35264c7 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.21"
> +#define DRV_VERSION "1.6.0.22"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
> index 58ce902..fa8a121 100644
> --- a/drivers/scsi/fnic/fnic_main.c
> +++ b/drivers/scsi/fnic/fnic_main.c
> @@ -119,6 +119,7 @@ static struct scsi_host_template fnic_host_template = {
> .max_sectors = 0xffff,
> .shost_attrs = fnic_attrs,
> .track_queue_depth = 1,
> + .disable_blk_mq = 1,
> };
>
> static void
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Can you please remove the check for shost_use_blk_mq() in fnic_scsi.c as well,
as it's a NOP?
Thanks,
Johannes
--
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue
2016-03-14 18:14 ` [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue Satish Kharat
2016-03-14 20:16 ` Ewan Milne
2016-03-15 9:46 ` Johannes Thumshirn
@ 2016-03-15 9:51 ` Christoph Hellwig
2 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2016-03-15 9:51 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
Sorry, but this patch is bullshit. Why doesn't it support using blk-mq?
And why didn't I see a single attempt from you to fix it first?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset
2016-03-14 18:14 ` [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset Satish Kharat
2016-03-14 20:14 ` Ewan Milne
@ 2016-03-15 9:53 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2016-03-15 9:53 UTC (permalink / raw)
To: Satish Kharat; +Cc: linux-scsi
The right fix is to get rid of fnic_clean_pending_aborts entirely.
It's not the low level drivers business to clean up pending commands.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer
2016-03-14 19:59 ` [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Ewan Milne
@ 2016-03-16 22:38 ` Satish Kharat (satishkh)
2016-03-17 17:51 ` Ewan D. Milne
0 siblings, 1 reply; 15+ messages in thread
From: Satish Kharat (satishkh) @ 2016-03-16 22:38 UTC (permalink / raw)
To: Ewan Milne; +Cc: linux-scsi
Thanks for the review comments. I incorporated the comments, below is the updated patch:
If an I/O times out and an abort issued by host, if the abort is
successful we need to set scsi status as DID_ABORT. Or else the
mid-layer error handler which looks for this error code, will
offline the device. Also if the original I/O is not found in fnic
firmware, we will consider the abort as successful.
The start_time assignment is moved because of the new goto.
Fnic driver version changed from 1.6.0.17a to 1.6.0.19,
version 1.6.0.18 has been skipped
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/scsi/fnic/fnic.h | 2 +-
drivers/scsi/fnic/fnic_scsi.c | 35 +++++++++++++++++++++++++++++------
2 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index ce129e5..52a53f8 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -39,7 +39,7 @@
#define DRV_NAME "fnic"
#define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
-#define DRV_VERSION "1.6.0.17a"
+#define DRV_VERSION "1.6.0.19"
#define PFX DRV_NAME ": "
#define DFX DRV_NAME "%d: "
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 266b909..b732fa3 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -1092,6 +1092,11 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
atomic64_inc(
&term_stats->terminate_fw_timeouts);
break;
+ case FCPIO_ITMF_REJECTED:
+ FNIC_SCSI_DBG(KERN_INFO, fnic->lport->host,
+ "abort reject recd. id %d\n",
+ (int)(id & FNIC_TAG_MASK));
+ break;
case FCPIO_IO_NOT_FOUND:
if (CMD_FLAGS(sc) & FNIC_IO_ABTS_ISSUED)
atomic64_inc(&abts_stats->abort_io_not_found);
@@ -1112,9 +1117,15 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
spin_unlock_irqrestore(io_lock, flags);
return;
}
- CMD_ABTS_STATUS(sc) = hdr_status;
+
CMD_FLAGS(sc) |= FNIC_IO_ABT_TERM_DONE;
+ /* If the status is IO not found consider it as success */
+ if (hdr_status == FCPIO_IO_NOT_FOUND)
+ CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
+ else
+ CMD_ABTS_STATUS(sc) = hdr_status;
+
atomic64_dec(&fnic_stats->io_stats.active_ios);
if (atomic64_read(&fnic->io_cmpl_skip))
atomic64_dec(&fnic->io_cmpl_skip);
@@ -1927,21 +1938,33 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
+ start_time = io_req->start_time;
/*
* firmware completed the abort, check the status,
- * free the io_req irrespective of failure or success
+ * free the io_req if successful. If abort fails,
+ * Device reset will clean the I/O.
*/
- if (CMD_ABTS_STATUS(sc) != FCPIO_SUCCESS)
+ if (CMD_ABTS_STATUS(sc) == FCPIO_SUCCESS) {
+ CMD_SP(sc) = NULL;
+ }
+ else
+ {
ret = FAILED;
-
- CMD_SP(sc) = NULL;
+ spin_unlock_irqrestore(io_lock, flags);
+ goto fnic_abort_cmd_end;
+ }
spin_unlock_irqrestore(io_lock, flags);
- start_time = io_req->start_time;
fnic_release_ioreq_buf(fnic, io_req, sc);
mempool_free(io_req, fnic->io_req_pool);
+ if (sc->scsi_done) {
+ /* Call SCSI completion function to complete the IO */
+ sc->result = (DID_ABORT << 16);
+ sc->scsi_done(sc);
+ }
+
fnic_abort_cmd_end:
FNIC_TRACE(fnic_abort_cmd, sc->device->host->host_no,
sc->request->tag, sc,
--
2.4.3
________________________________________
From: Ewan Milne <emilne@redhat.com>
Sent: Monday, March 14, 2016 12:59 PM
To: Satish Kharat (satishkh)
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer
On Mon, 2016-03-14 at 11:14 -0700, Satish Kharat wrote:
> If an I/O times out and an abort issued by host, if the abort is
> successful we need to set scsi status as DID_ABORT. Or else the
> mid-layer error handler which looks for this error code, will
> offline the device. Also if the original I/O is not found in fnic
> firmware, we will consider the abort as successful.
> Fnic driver version changed from 1.6.0.17a to 1.6.0.19,
> version 1.6.0.18 has been skipped
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_scsi.c | 32 +++++++++++++++++++++++++++-----
> 2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index ce129e5..52a53f8 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.17a"
> +#define DRV_VERSION "1.6.0.19"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index 266b909..b9732b1 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -1092,6 +1092,11 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> atomic64_inc(
> &term_stats->terminate_fw_timeouts);
> break;
> + case FCPIO_ITMF_REJECTED:
> + FNIC_SCSI_DBG(KERN_INFO, fnic->lport->host,
> + "abort reject recd. id %d\n",
> + (int)(id & FNIC_TAG_MASK));
> + break;
> case FCPIO_IO_NOT_FOUND:
> if (CMD_FLAGS(sc) & FNIC_IO_ABTS_ISSUED)
> atomic64_inc(&abts_stats->abort_io_not_found);
> @@ -1112,9 +1117,14 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> spin_unlock_irqrestore(io_lock, flags);
> return;
> }
> +
> CMD_ABTS_STATUS(sc) = hdr_status;
> CMD_FLAGS(sc) |= FNIC_IO_ABT_TERM_DONE;
>
> + /* If the status is IO not found consider it as success */
> + if (hdr_status == FCPIO_IO_NOT_FOUND)
> + CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
> +
Minor nitpick: I would have done:
if (hdr_status == FCP_IO_NOT_FOUND)
CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
else
CMD_ABTS_STATUS(sc) = hdr_status;
rather than overwriting the assignment made several lines above, it is
more explicit and less prone to errors if the code is later changed.
> atomic64_dec(&fnic_stats->io_stats.active_ios);
> if (atomic64_read(&fnic->io_cmpl_skip))
> atomic64_dec(&fnic->io_cmpl_skip);
> @@ -1927,21 +1937,33 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
>
> CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
>
> + start_time = io_req->start_time;
> /*
> * firmware completed the abort, check the status,
> - * free the io_req irrespective of failure or success
> + * free the io_req if successful. If abort fails,
> + * Device reset will clean the I/O.
> */
> - if (CMD_ABTS_STATUS(sc) != FCPIO_SUCCESS)
> + if (CMD_ABTS_STATUS(sc) == FCPIO_SUCCESS) {
> + CMD_SP(sc) = NULL;
> + }
> + else
> + {
> ret = FAILED;
> -
> - CMD_SP(sc) = NULL;
> + spin_unlock_irqrestore(io_lock, flags);
> + goto fnic_abort_cmd_end;
> + }
>
> spin_unlock_irqrestore(io_lock, flags);
>
> - start_time = io_req->start_time;
What was the point of moving the assignment of start_time?
Other places in the code still set it just before the call to
fnic_release_ioreq_buf().
> fnic_release_ioreq_buf(fnic, io_req, sc);
> mempool_free(io_req, fnic->io_req_pool);
>
> + if (sc->scsi_done) {
> + /* Call SCSI completion function to complete the IO */
> + sc->result = (DID_ABORT << 16);
> + sc->scsi_done(sc);
> + }
> +
> fnic_abort_cmd_end:
> FNIC_TRACE(fnic_abort_cmd, sc->device->host->host_no,
> sc->request->tag, sc,
Despite the above comments, I guess this looks OK.
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer
2016-03-16 22:38 ` Satish Kharat (satishkh)
@ 2016-03-17 17:51 ` Ewan D. Milne
0 siblings, 0 replies; 15+ messages in thread
From: Ewan D. Milne @ 2016-03-17 17:51 UTC (permalink / raw)
To: Satish Kharat (satishkh); +Cc: linux-scsi
On Wed, 2016-03-16 at 22:38 +0000, Satish Kharat (satishkh) wrote:
> Thanks for the review comments. I incorporated the comments, below is the updated patch:
>
>
> If an I/O times out and an abort issued by host, if the abort is
> successful we need to set scsi status as DID_ABORT. Or else the
> mid-layer error handler which looks for this error code, will
> offline the device. Also if the original I/O is not found in fnic
> firmware, we will consider the abort as successful.
> The start_time assignment is moved because of the new goto.
> Fnic driver version changed from 1.6.0.17a to 1.6.0.19,
> version 1.6.0.18 has been skipped
>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> ---
> drivers/scsi/fnic/fnic.h | 2 +-
> drivers/scsi/fnic/fnic_scsi.c | 35 +++++++++++++++++++++++++++++------
> 2 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index ce129e5..52a53f8 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -39,7 +39,7 @@
>
> #define DRV_NAME "fnic"
> #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> -#define DRV_VERSION "1.6.0.17a"
> +#define DRV_VERSION "1.6.0.19"
> #define PFX DRV_NAME ": "
> #define DFX DRV_NAME "%d: "
>
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index 266b909..b732fa3 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -1092,6 +1092,11 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> atomic64_inc(
> &term_stats->terminate_fw_timeouts);
> break;
> + case FCPIO_ITMF_REJECTED:
> + FNIC_SCSI_DBG(KERN_INFO, fnic->lport->host,
> + "abort reject recd. id %d\n",
> + (int)(id & FNIC_TAG_MASK));
> + break;
> case FCPIO_IO_NOT_FOUND:
> if (CMD_FLAGS(sc) & FNIC_IO_ABTS_ISSUED)
> atomic64_inc(&abts_stats->abort_io_not_found);
> @@ -1112,9 +1117,15 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> spin_unlock_irqrestore(io_lock, flags);
> return;
> }
> - CMD_ABTS_STATUS(sc) = hdr_status;
> +
> CMD_FLAGS(sc) |= FNIC_IO_ABT_TERM_DONE;
>
> + /* If the status is IO not found consider it as success */
> + if (hdr_status == FCPIO_IO_NOT_FOUND)
> + CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
> + else
> + CMD_ABTS_STATUS(sc) = hdr_status;
> +
> atomic64_dec(&fnic_stats->io_stats.active_ios);
> if (atomic64_read(&fnic->io_cmpl_skip))
> atomic64_dec(&fnic->io_cmpl_skip);
> @@ -1927,21 +1938,33 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
>
> CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
>
> + start_time = io_req->start_time;
> /*
> * firmware completed the abort, check the status,
> - * free the io_req irrespective of failure or success
> + * free the io_req if successful. If abort fails,
> + * Device reset will clean the I/O.
> */
> - if (CMD_ABTS_STATUS(sc) != FCPIO_SUCCESS)
> + if (CMD_ABTS_STATUS(sc) == FCPIO_SUCCESS) {
> + CMD_SP(sc) = NULL;
> + }
> + else
> + {
> ret = FAILED;
> -
> - CMD_SP(sc) = NULL;
> + spin_unlock_irqrestore(io_lock, flags);
> + goto fnic_abort_cmd_end;
> + }
>
> spin_unlock_irqrestore(io_lock, flags);
>
> - start_time = io_req->start_time;
> fnic_release_ioreq_buf(fnic, io_req, sc);
> mempool_free(io_req, fnic->io_req_pool);
>
> + if (sc->scsi_done) {
> + /* Call SCSI completion function to complete the IO */
> + sc->result = (DID_ABORT << 16);
> + sc->scsi_done(sc);
> + }
> +
> fnic_abort_cmd_end:
> FNIC_TRACE(fnic_abort_cmd, sc->device->host->host_no,
> sc->request->tag, sc,
> --
> 2.4.3
Reviewed-by: Ewan D. Milne <emilne@redhat.com>
>
> ________________________________________
> From: Ewan Milne <emilne@redhat.com>
> Sent: Monday, March 14, 2016 12:59 PM
> To: Satish Kharat (satishkh)
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer
>
> On Mon, 2016-03-14 at 11:14 -0700, Satish Kharat wrote:
> > If an I/O times out and an abort issued by host, if the abort is
> > successful we need to set scsi status as DID_ABORT. Or else the
> > mid-layer error handler which looks for this error code, will
> > offline the device. Also if the original I/O is not found in fnic
> > firmware, we will consider the abort as successful.
> > Fnic driver version changed from 1.6.0.17a to 1.6.0.19,
> > version 1.6.0.18 has been skipped
> >
> > Signed-off-by: Satish Kharat <satishkh@cisco.com>
> > ---
> > drivers/scsi/fnic/fnic.h | 2 +-
> > drivers/scsi/fnic/fnic_scsi.c | 32 +++++++++++++++++++++++++++-----
> > 2 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> > index ce129e5..52a53f8 100644
> > --- a/drivers/scsi/fnic/fnic.h
> > +++ b/drivers/scsi/fnic/fnic.h
> > @@ -39,7 +39,7 @@
> >
> > #define DRV_NAME "fnic"
> > #define DRV_DESCRIPTION "Cisco FCoE HBA Driver"
> > -#define DRV_VERSION "1.6.0.17a"
> > +#define DRV_VERSION "1.6.0.19"
> > #define PFX DRV_NAME ": "
> > #define DFX DRV_NAME "%d: "
> >
> > diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> > index 266b909..b9732b1 100644
> > --- a/drivers/scsi/fnic/fnic_scsi.c
> > +++ b/drivers/scsi/fnic/fnic_scsi.c
> > @@ -1092,6 +1092,11 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> > atomic64_inc(
> > &term_stats->terminate_fw_timeouts);
> > break;
> > + case FCPIO_ITMF_REJECTED:
> > + FNIC_SCSI_DBG(KERN_INFO, fnic->lport->host,
> > + "abort reject recd. id %d\n",
> > + (int)(id & FNIC_TAG_MASK));
> > + break;
> > case FCPIO_IO_NOT_FOUND:
> > if (CMD_FLAGS(sc) & FNIC_IO_ABTS_ISSUED)
> > atomic64_inc(&abts_stats->abort_io_not_found);
> > @@ -1112,9 +1117,14 @@ static void fnic_fcpio_itmf_cmpl_handler(struct fnic *fnic,
> > spin_unlock_irqrestore(io_lock, flags);
> > return;
> > }
> > +
> > CMD_ABTS_STATUS(sc) = hdr_status;
> > CMD_FLAGS(sc) |= FNIC_IO_ABT_TERM_DONE;
> >
> > + /* If the status is IO not found consider it as success */
> > + if (hdr_status == FCPIO_IO_NOT_FOUND)
> > + CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
> > +
>
> Minor nitpick: I would have done:
>
> if (hdr_status == FCP_IO_NOT_FOUND)
> CMD_ABTS_STATUS(sc) = FCPIO_SUCCESS;
> else
> CMD_ABTS_STATUS(sc) = hdr_status;
>
> rather than overwriting the assignment made several lines above, it is
> more explicit and less prone to errors if the code is later changed.
>
> > atomic64_dec(&fnic_stats->io_stats.active_ios);
> > if (atomic64_read(&fnic->io_cmpl_skip))
> > atomic64_dec(&fnic->io_cmpl_skip);
> > @@ -1927,21 +1937,33 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
> >
> > CMD_STATE(sc) = FNIC_IOREQ_ABTS_COMPLETE;
> >
> > + start_time = io_req->start_time;
> > /*
> > * firmware completed the abort, check the status,
> > - * free the io_req irrespective of failure or success
> > + * free the io_req if successful. If abort fails,
> > + * Device reset will clean the I/O.
> > */
> > - if (CMD_ABTS_STATUS(sc) != FCPIO_SUCCESS)
> > + if (CMD_ABTS_STATUS(sc) == FCPIO_SUCCESS) {
> > + CMD_SP(sc) = NULL;
> > + }
> > + else
> > + {
> > ret = FAILED;
> > -
> > - CMD_SP(sc) = NULL;
> > + spin_unlock_irqrestore(io_lock, flags);
> > + goto fnic_abort_cmd_end;
> > + }
> >
> > spin_unlock_irqrestore(io_lock, flags);
> >
> > - start_time = io_req->start_time;
>
> What was the point of moving the assignment of start_time?
> Other places in the code still set it just before the call to
> fnic_release_ioreq_buf().
>
> > fnic_release_ioreq_buf(fnic, io_req, sc);
> > mempool_free(io_req, fnic->io_req_pool);
> >
> > + if (sc->scsi_done) {
> > + /* Call SCSI completion function to complete the IO */
> > + sc->result = (DID_ABORT << 16);
> > + sc->scsi_done(sc);
> > + }
> > +
> > fnic_abort_cmd_end:
> > FNIC_TRACE(fnic_abort_cmd, sc->device->host->host_no,
> > sc->request->tag, sc,
>
> Despite the above comments, I guess this looks OK.
>
> Reviewed-by: Ewan D. Milne <emilne@redhat.com>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-03-17 17:51 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-14 18:14 [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Satish Kharat
2016-03-14 18:14 ` [PATCH 2/5] Cleanup the I/O that has timed out and is used to issue LUN reset Satish Kharat
2016-03-14 20:14 ` Ewan Milne
2016-03-15 9:53 ` Christoph Hellwig
2016-03-14 18:14 ` [PATCH 3/5] Using rport->dd_data to check rport online instead of rport_lookup Satish Kharat
2016-03-14 20:15 ` Ewan Milne
2016-03-14 18:14 ` [PATCH 4/5] Setting scsi host template to indicate that fnic does not support multiqueue Satish Kharat
2016-03-14 20:16 ` Ewan Milne
2016-03-15 9:46 ` Johannes Thumshirn
2016-03-15 9:51 ` Christoph Hellwig
2016-03-14 18:14 ` [PATCH 5/5] Leftshift returned scsi_cmnd error code 16 bits Satish Kharat
2016-03-14 20:17 ` Ewan Milne
2016-03-14 19:59 ` [PATCH 1/5] Fix to cleanup aborted IO to avoid device being offlined by mid-layer Ewan Milne
2016-03-16 22:38 ` Satish Kharat (satishkh)
2016-03-17 17:51 ` Ewan D. Milne
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.