zfcp fixes for 2.6.32-rc4
Subsystems (3)
cover zfcp fixes for 2.6.32-rc4
James,
here is the (hopefully) last round of zfcp fixes for the 2.6.32 cycle. The largest change is a fix for zfcp to handle storage devices that return a wrong wwpn in the plogi payload. The patches apply on the current linux-2.6 git tree (-rc4 + )
--
Christof Schmitt
PATCH 1/5 zfcp: fix kfree handling in zfcp_init_device_setup
From: Heiko Carstens <[email protected]>
The pointer that is allocated with kmalloc() is passed to strsep() which modifies it. Later on the modified pointer value will be passed to kfree. Save the original pointer and pass that one to kfree instead.
---
drivers/s390/scsi/zfcp_aux.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/s390/scsi/zfcp_aux.c +6 -5
@@ -128,12 +128,13 @@ out_ccwdev:static void __init zfcp_init_device_setup (char *devstr) {char *token; -char *str; +char *str, *str_saved;char busid[ZFCP_BUS_ID_SIZE ];u64 wwpn, lun;/* duplicate devstr and keep the original for sysfs presentation*/ - str =kmalloc (strlen (devstr) +1 ,GFP_KERNEL ); + str_saved =kmalloc (strlen (devstr) +1 ,GFP_KERNEL ); + str = str_saved;if (!str)return ;
@@ -152,12 +153,12 @@ static void __init zfcp_init_device_setuif (!token ||strict_strtoull (token,0 , (unsigned long long *) &lun)) goto err_out; -kfree (str); +kfree (str_saved);zfcp_init_device_configure (busid, wwpn, lun);return ; - err_out: -kfree (str); +err_out: +kfree (str_saved);pr_err ("%s is not a valid SCSI device\n" , devstr); }
PATCH 2/5 zfcp: Handle WWPN mismatch in PLOGI payload
In reply to Christof SchmittFrom: Christof Schmitt <[email protected]>
For ports, zfcp gets the DID from the FC nameserver and tries to open the port. If the open succeeds, zfcp compares the WWPN from the nameserver with the WWPN in the PLOGI payload. In case of a mismatch, zfcp assumes that the DID of the port just changed and we opened the wrong port. This means that zfcp has to forget the DID, lookup the DID again and retry.
This error case had a problem that zfcp forgets the DID, but never looks up a new one, stalling the ERP in this case. Fix this by triggering the DID lookup and properly exit from the ERP. The DID lookup will trigger a new ERP action.
Also ensure when trying to open the port again with the new DID, first close the open port, even in the NOESC case.
---
drivers/s390/scsi/zfcp_erp.c | 22 ++++++++++------------ drivers/s390/scsi/zfcp_ext.h | 1 + drivers/s390/scsi/zfcp_fc.c | 11 +++++++++++ 3 files changed, 22 insertions(+), 12 deletions(-)
drivers/s390/scsi/zfcp_erp.c +10 -12
@@ -858,10 +858,7 @@ static int zfcp_erp_port_strategy_open_cif (fc_host_port_type (adapter->scsi_host) ==FC_PORTTYPE_PTP )return zfcp_erp_open_ptp_port (act);if (!port->d_id) { -zfcp_port_get (port); -if (!queue_work (adapter->work_queue, - &port->gid_pn_work)) -zfcp_port_put (port); +zfcp_fc_trigger_did_lookup (port);return ZFCP_ERP_EXIT ; }return zfcp_erp_port_strategy_open_port (act);
@@ -869,12 +866,11 @@ static int zfcp_erp_port_strategy_open_ccase ZFCP_ERP_STEP_PORT_OPENING :/* D_ID might have changed during open */ if (p_status &ZFCP_STATUS_COMMON_OPEN ) { -if (port->d_id) -return ZFCP_ERP_SUCCEEDED ; -else { - act->step =ZFCP_ERP_STEP_PORT_CLOSING ; -return ZFCP_ERP_CONTINUES ; +if (!port->d_id) { +zfcp_fc_trigger_did_lookup (port); +return ZFCP_ERP_EXIT ; } +return ZFCP_ERP_SUCCEEDED ; }if (port->d_id && !(p_status &ZFCP_STATUS_COMMON_NOESC )) { port->d_id =0 ;
@@ -889,19 +885,21 @@ static int zfcp_erp_port_strategy_open_cstatic int zfcp_erp_port_strategy (struct zfcp_erp_action *erp_action) {struct zfcp_port *port = erp_action->port; +int p_status =atomic_read (&port->status); -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_NOESC ) +if ((p_status &ZFCP_STATUS_COMMON_NOESC ) && + !(p_status &ZFCP_STATUS_COMMON_OPEN )) goto close_init_done;switch (erp_action->step) {case ZFCP_ERP_STEP_UNINITIALIZED :zfcp_erp_port_strategy_clearstati (port); -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_OPEN ) +if (p_status &ZFCP_STATUS_COMMON_OPEN )return zfcp_erp_port_strategy_close (erp_action);break ;case ZFCP_ERP_STEP_PORT_CLOSING : -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_OPEN ) +if (p_status &ZFCP_STATUS_COMMON_OPEN )return ZFCP_ERP_FAILED ;break ; }
drivers/s390/scsi/zfcp_ext.h +1 -0
@@ -96,6 +96,7 @@ extern int zfcp_fc_scan_ports(struct zfcextern void _zfcp_fc_scan_ports_later (struct work_struct *);extern void zfcp_fc_incoming_els (struct zfcp_fsf_req *);extern void zfcp_fc_port_did_lookup (struct work_struct *); +extern void zfcp_fc_trigger_did_lookup (struct zfcp_port *);extern void zfcp_fc_plogi_evaluate (struct zfcp_port *,struct fsf_plogi *);extern void zfcp_fc_test_link (struct zfcp_port *);extern void zfcp_fc_link_test_work (struct work_struct *);
drivers/s390/scsi/zfcp_fc.c +11 -0
@@ -361,6 +361,17 @@ out: }/** +* zfcp_fc_trigger_did_ookup - trigger the d_id lookup using a GPN_FT request +* @port: The zfcp_port to lookup the d_id for. +*/ +void zfcp_fc_trigger_did_lookup (struct zfcp_port *port) +{ +zfcp_port_get (port); +if (!queue_work (port->adapter->work_queue, &port->gid_pn_work)) +zfcp_port_put (port); +} + +/** * zfcp_fc_plogi_evaluate - evaluate PLOGI playload * @port: zfcp_port structure * @plogi: plogi payload
Re: [patch 2/5] zfcp: Handle WWPN mismatch in PLOGI payload
Quoted 8 lines · Christof Schmitt
From: Christof Schmitt <[email protected]>
For ports, zfcp gets the DID from the FC nameserver and tries to open the port. If the open succeeds, zfcp compares the WWPN from the nameserver with the WWPN in the PLOGI payload. In case of a mismatch, zfcp assumes that the DID of the port just changed and we opened the wrong port. This means that zfcp has to forget the DID, lookup the DID again and retry.
Does this happen very often? Would you get an RSCN if a target's WWPN changed? Just wondering how this happens and whether libfc needs similar defenses. Is it due to a broken target?
See below for a minor typo.
Quoted 89 lines
This error case had a problem that zfcp forgets the DID, but never looks up a new one, stalling the ERP in this case. Fix this by triggering the DID lookup and properly exit from the ERP. The DID lookup will trigger a new ERP action.
Also ensure when trying to open the port again with the new DID, first close the open port, even in the NOESC case.
Reviewed-by Swen Schillig <[email protected]>Signed-off-by Christof Schmitt <[email protected]>---
drivers/s390/scsi/zfcp_erp.c | 22 ++++++++++------------ drivers/s390/scsi/zfcp_ext.h | 1 + drivers/s390/scsi/zfcp_fc.c | 11 +++++++++++ 3 files changed, 22 insertions(+), 12 deletions(-)diff -urpN
linux-2.6/drivers/s390/scsi/zfcp_erp.clinux-2.6-patched/drivers/s390/scsi/zfcp_erp.cdrivers/s390/scsi/zfcp_erp.c +10
-12@@ -858,10 +858,7 @@ static int zfcp_erp_port_strategy_open_cif (fc_host_port_type (adapter->scsi_host) ==FC_PORTTYPE_PTP )return zfcp_erp_open_ptp_port (act);if (!port->d_id) { -zfcp_port_get (port); -if (!queue_work (adapter->work_queue, - &port->gid_pn_work)) -zfcp_port_put (port); +zfcp_fc_trigger_did_lookup (port);return ZFCP_ERP_EXIT ; }return zfcp_erp_port_strategy_open_port (act);@@ -869,12 +866,11 @@ static int zfcp_erp_port_strategy_open_ccase ZFCP_ERP_STEP_PORT_OPENING :/* D_ID might have changed during open */ if (p_status &ZFCP_STATUS_COMMON_OPEN ) { -if (port->d_id) -return ZFCP_ERP_SUCCEEDED ; -else { - act->step =ZFCP_ERP_STEP_PORT_CLOSING ; -return ZFCP_ERP_CONTINUES ; +if (!port->d_id) { +zfcp_fc_trigger_did_lookup (port); +return ZFCP_ERP_EXIT ; } +return ZFCP_ERP_SUCCEEDED ; }if (port->d_id && !(p_status &ZFCP_STATUS_COMMON_NOESC )) { port->d_id =0 ;@@ -889,19 +885,21 @@ static int zfcp_erp_port_strategy_open_cstatic int zfcp_erp_port_strategy (struct zfcp_erp_action *erp_action) {struct zfcp_port *port = erp_action->port; +int p_status =atomic_read (&port->status); -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_NOESC ) +if ((p_status &ZFCP_STATUS_COMMON_NOESC ) && + !(p_status &ZFCP_STATUS_COMMON_OPEN )) goto close_init_done;switch (erp_action->step) {case ZFCP_ERP_STEP_UNINITIALIZED :zfcp_erp_port_strategy_clearstati (port); -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_OPEN ) +if (p_status &ZFCP_STATUS_COMMON_OPEN )return zfcp_erp_port_strategy_close (erp_action);break ;case ZFCP_ERP_STEP_PORT_CLOSING : -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_OPEN ) +if (p_status &ZFCP_STATUS_COMMON_OPEN )return ZFCP_ERP_FAILED ;break ; }diff -urpN
linux-2.6/drivers/s390/scsi/zfcp_ext.hlinux-2.6-patched/drivers/s390/scsi/zfcp_ext.hdrivers/s390/scsi/zfcp_ext.h +1
-0@@ -96,6 +96,7 @@ extern int zfcp_fc_scan_ports(struct zfcextern void _zfcp_fc_scan_ports_later (struct work_struct *);extern void zfcp_fc_incoming_els (struct zfcp_fsf_req *);extern void zfcp_fc_port_did_lookup (struct work_struct *); +extern void zfcp_fc_trigger_did_lookup (struct zfcp_port *);extern void zfcp_fc_plogi_evaluate (struct zfcp_port *,struct fsf_plogi *);extern void zfcp_fc_test_link (struct zfcp_port *);extern void zfcp_fc_link_test_work (struct work_struct *);diff -urpN
linux-2.6/drivers/s390/scsi/zfcp_fc.clinux-2.6-patched/drivers/s390/scsi/zfcp_fc.c
drivers/s390/scsi/zfcp_fc.c out: · line 364
Quoted 4 lines
drivers/s390/scsi/zfcp_fc.c +1
-0@@ -361,6 +361,17 @@ out: }/** +* zfcp_fc_trigger_did_ookup - trigger the d_id lookup using a GPN_FT request
s/ook/look/
I just happened to notice that. I didn't review the rest.
Quoted 5 lines
drivers/s390/scsi/zfcp_fc.c +5
-0+* @port: The zfcp_port to lookup the d_id for. +*/ +void zfcp_fc_trigger_did_lookup (struct zfcp_port *port) +{ +zfcp_port_get (port);
drivers/s390/scsi/zfcp_fc.c out:
Quoted 10 lines
drivers/s390/scsi/zfcp_fc.c +5
-1+if (!queue_work (port->adapter->work_queue, &port->gid_pn_work)) +zfcp_port_put (port); +} + +/** * zfcp_fc_plogi_evaluate - evaluate PLOGI playload * @port: zfcp_port structure * @plogi: plogi payload --
Cheers,
Joe
Re: [patch 2/5] zfcp: Handle WWPN mismatch in PLOGI payload
Quoted 12 lines · Joe Eykholt
Quoted 9 lines · Christof Schmitt
From: Christof Schmitt <[email protected]>
For ports, zfcp gets the DID from the FC nameserver and tries to open the port. If the open succeeds, zfcp compares the WWPN from the nameserver with the WWPN in the PLOGI payload. In case of a mismatch, zfcp assumes that the DID of the port just changed and we opened the wrong port. This means that zfcp has to forget the DID, lookup the DID again and retry.
Quoted 2 lines
Does this happen very often? Would you get an RSCN if a target's WWPN changed?
You should - as it is supposed to re-login with the fabric, thus the fabric login will update the nameserver, and nameserver will generate the RSCN. But, there's no guarantee as to how quickly you will see the RSCN. If you were on FC-AL, this happens all the time and never w/ an RSCN.
Quoted 3 lines
Just wondering how this happens and whether libfc needs similar defenses. Is it due to a broken target?
Libfc needs a similar defense. You may not need to compare against the "nameserver" values, but you had better compare against same N_Port_ID, different WWPN. or vice-versa. Its common to have RSCN events delayed, allowing discovery to occur in between events that may be outstanding (e.g. tgt plugs in - RSCN-1 event, tgt moves to another port (user error -kicks out and replugs) or an array failover such that WWPN moves to new switch port/N_Port_ID - RSCN-2 event; and your ADISC/PLOGI based on RSCN-1 arrives and is responded to sooner than the nameserver propagates its updates and you receive RSCN-2)
-- james s
Re: [patch 2/5] zfcp: Handle WWPN mismatch in PLOGI payload
In reply to Joe EykholtQuoted 10 lines · Joe Eykholt
Quoted 6 lines
For ports, zfcp gets the DID from the FC nameserver and tries to open the port. If the open succeeds, zfcp compares the WWPN from the nameserver with the WWPN in the PLOGI payload. In case of a mismatch, zfcp assumes that the DID of the port just changed and we opened the wrong port. This means that zfcp has to forget the DID, lookup the DID again and retry.
Does this happen very often? Would you get an RSCN if a target's WWPN changed? Just wondering how this happens and whether libfc needs similar defenses. Is it due to a broken target?
zfcp first gets all DIDs from the nameserver and then triggers to login to all ports. A change could happen at any time. With the RSCNs being asynchronous to the login, we have to check if the port is really the one we wanted to login to. James S. pointed out some more details.
In this particular case i am seeing in out test environment, the storage target always returns a wrong WWPN. I would recommend that any check being done also considers targets that return wrong data to avoid endless loops and such.
Quoted 1 line
See below for a minor typo.
[...]
Quoted 11 lines
drivers/s390/scsi/zfcp_fc.c out: · line 363
Quoted 7 lines
diff -urpN
linux-2.6/drivers/s390/scsi/zfcp_fc.clinux-2.6-patched/drivers/s390/scsi/zfcp_fc.cdrivers/s390/scsi/zfcp_fc.c +1
-0@@ -361,6 +361,17 @@ out: }/** +* zfcp_fc_trigger_did_ookup - trigger the d_id lookup using a GPN_FT request s/ook/look/
I just happened to notice that. I didn't review the rest.
Thanks for catching this. And looking at the line, it is supposed to read GID_PN and not GPN_FT.
I will resend the patch with the updated comment.
--
Christof
PATCH 2/5 zfcp: Handle WWPN mismatch in PLOGI payload
In reply to Christof SchmittFrom: Christof Schmitt <[email protected]>
For ports, zfcp gets the DID from the FC nameserver and tries to open the port. If the open succeeds, zfcp compares the WWPN from the nameserver with the WWPN in the PLOGI payload. In case of a mismatch, zfcp assumes that the DID of the port just changed and we opened the wrong port. This means that zfcp has to forget the DID, lookup the DID again and retry.
This error case had a problem that zfcp forgets the DID, but never looks up a new one, stalling the ERP in this case. Fix this by triggering the DID lookup and properly exit from the ERP. The DID lookup will trigger a new ERP action.
Also ensure when trying to open the port again with the new DID, first close the open port, even in the NOESC case.
---
drivers/s390/scsi/zfcp_erp.c | 22 ++++++++++------------ drivers/s390/scsi/zfcp_ext.h | 1 + drivers/s390/scsi/zfcp_fc.c | 11 +++++++++++ 3 files changed, 22 insertions(+), 12 deletions(-)
drivers/s390/scsi/zfcp_erp.c +10 -12
@@ -858,10 +858,7 @@ static int zfcp_erp_port_strategy_open_cif (fc_host_port_type (adapter->scsi_host) ==FC_PORTTYPE_PTP )return zfcp_erp_open_ptp_port (act);if (!port->d_id) { -zfcp_port_get (port); -if (!queue_work (adapter->work_queue, - &port->gid_pn_work)) -zfcp_port_put (port); +zfcp_fc_trigger_did_lookup (port);return ZFCP_ERP_EXIT ; }return zfcp_erp_port_strategy_open_port (act);
@@ -869,12 +866,11 @@ static int zfcp_erp_port_strategy_open_ccase ZFCP_ERP_STEP_PORT_OPENING :/* D_ID might have changed during open */ if (p_status &ZFCP_STATUS_COMMON_OPEN ) { -if (port->d_id) -return ZFCP_ERP_SUCCEEDED ; -else { - act->step =ZFCP_ERP_STEP_PORT_CLOSING ; -return ZFCP_ERP_CONTINUES ; +if (!port->d_id) { +zfcp_fc_trigger_did_lookup (port); +return ZFCP_ERP_EXIT ; } +return ZFCP_ERP_SUCCEEDED ; }if (port->d_id && !(p_status &ZFCP_STATUS_COMMON_NOESC )) { port->d_id =0 ;
@@ -889,19 +885,21 @@ static int zfcp_erp_port_strategy_open_cstatic int zfcp_erp_port_strategy (struct zfcp_erp_action *erp_action) {struct zfcp_port *port = erp_action->port; +int p_status =atomic_read (&port->status); -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_NOESC ) +if ((p_status &ZFCP_STATUS_COMMON_NOESC ) && + !(p_status &ZFCP_STATUS_COMMON_OPEN )) goto close_init_done;switch (erp_action->step) {case ZFCP_ERP_STEP_UNINITIALIZED :zfcp_erp_port_strategy_clearstati (port); -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_OPEN ) +if (p_status &ZFCP_STATUS_COMMON_OPEN )return zfcp_erp_port_strategy_close (erp_action);break ;case ZFCP_ERP_STEP_PORT_CLOSING : -if (atomic_read (&port->status) &ZFCP_STATUS_COMMON_OPEN ) +if (p_status &ZFCP_STATUS_COMMON_OPEN )return ZFCP_ERP_FAILED ;break ; }
drivers/s390/scsi/zfcp_ext.h +1 -0
@@ -96,6 +96,7 @@ extern int zfcp_fc_scan_ports(struct zfcextern void _zfcp_fc_scan_ports_later (struct work_struct *);extern void zfcp_fc_incoming_els (struct zfcp_fsf_req *);extern void zfcp_fc_port_did_lookup (struct work_struct *); +extern void zfcp_fc_trigger_did_lookup (struct zfcp_port *);extern void zfcp_fc_plogi_evaluate (struct zfcp_port *,struct fsf_plogi *);extern void zfcp_fc_test_link (struct zfcp_port *);extern void zfcp_fc_link_test_work (struct work_struct *);
drivers/s390/scsi/zfcp_fc.c +11 -0
@@ -361,6 +361,17 @@ out: }/** +* zfcp_fc_trigger_did_lookup - trigger the d_id lookup using a GID_PN request +* @port: The zfcp_port to lookup the d_id for. +*/ +void zfcp_fc_trigger_did_lookup (struct zfcp_port *port) +{ +zfcp_port_get (port); +if (!queue_work (port->adapter->work_queue, &port->gid_pn_work)) +zfcp_port_put (port); +} + +/** * zfcp_fc_plogi_evaluate - evaluate PLOGI playload * @port: zfcp_port structure * @plogi: plogi payload
PATCH 3/5 zfcp: Warn about storage devices with broken PLOGI data
In reply to Christof SchmittFrom: Christof Schmitt <[email protected]>
After opening a remote port zfcp checks if the WWPN returned in the PLOGI maches the WWPN of the port that should have been opened. On a mismatch zfcp assumes that the DID just changed, queries the FC nameserver and tries again. If the situation persists the erp will give up.
With this strategy, if the remote port always returns the wrong PLOGI data, the remote port will not be opened. Introduce a warning, so that the system administrator knows why the remote port is not being opened and to have a pointer to investigate the problem on the storage system.
---
drivers/s390/scsi/zfcp_fsf.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/s390/scsi/zfcp_fsf.c +9 -2
@@ -1475,9 +1475,16 @@ static void zfcp_fsf_open_port_handler(s plogi = (struct fsf_plogi *) req->qtcb->bottom.support.els;if (req->qtcb->bottom.support.els1_length >=FSF_PLOGI_MIN_LEN ) { -if (plogi->serv_param.wwpn != port->wwpn) +if (plogi->serv_param.wwpn != port->wwpn) { port->d_id =0 ; -else { +dev_warn (&port->adapter->ccw_device->dev, +"A port opened with WWPN 0x%016Lx " +"returned data that identifies it as " +"WWPN 0x%016Lx\n" , + (unsigned long long ) port->wwpn, + (unsigned long long ) + plogi->serv_param.wwpn); + }else { port->wwnn = plogi->serv_param.wwnn;zfcp_fc_plogi_evaluate (port, plogi); }
PATCH 4/5 zfcp: Fix timer initialization for ct and els requests
In reply to Christof SchmittFrom: Christof Schmitt <[email protected]>
Add HZ since the start_timer function expects jiffies, not seconds.
---
drivers/s390/scsi/zfcp_fsf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/s390/scsi/zfcp_fsf.c +1 -1
@@ -1079,7 +1079,7 @@ static int zfcp_fsf_setup_ct_els(struct/* common settings for ct/gs and els requests */ req->qtcb->bottom.support.service_class =FSF_CLASS_3 ; req->qtcb->bottom.support.timeout =2 *R_A_TOV ; -zfcp_fsf_start_timer (req,2 *R_A_TOV +10 ); +zfcp_fsf_start_timer (req, (2 *R_A_TOV +10 ) *HZ );return 0 ; }
PATCH 5/5 zfcp: Flush SCSI registration work when adding unit
In reply to Christof SchmittFrom: Christof Schmitt <[email protected]>
When configuring a LUN for use in zfcp, flush the SCSI work to ensure the SCSI device has been created before returning. This means that a configuration procedure can run these commands in a script and the SCSI device is available immediately after the unit_add:
echo 1 > /sys/bus/ccw/drivers/zfcp/0.0.181d/online
echo 0x401040C300000000 > \
/sys/bus/ccw/drivers/zfcp/0.0.181d/0x500507630313c562/unit_addlsscsi
---
drivers/s390/scsi/zfcp_sysfs.c | 1 + 1 file changed, 1 insertion(+)
drivers/s390/scsi/zfcp_sysfs.c +1 -0
@@ -224,6 +224,7 @@ static ssize_t zfcp_sysfs_unit_add_storezfcp_erp_unit_reopen (unit,0 ,"syuas_1" ,NULL );zfcp_erp_wait (unit->port->adapter); +flush_work (&unit->scsi_work);zfcp_unit_put (unit); out:mutex_unlock (&zfcp_data.config_mutex);