From 18cc4e02508e3f1fadf81f697837567431ee5a9c Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 11 Dec 2013 17:05:22 +0100 Subject: scsi_transport_srp: Block rport upon TL error even with fast_io_fail_tmo = off The current behavior of the SRP transport layer when a transport layer error is encountered is to block SCSI command processing only if fast_io_fail_tmo != off. The current behavior of the FC transport layer when a transport layer error is encountered is to block SCSI command processing no matter which value fast_io_fail_tmo has been set to. Make the behavior of the SRP transport layer consistent with that of the FC transport layer to avoid confusion. Signed-off-by: Bart Van Assche Signed-off-by: Roland Dreier --- drivers/scsi/scsi_transport_srp.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) (limited to 'drivers/scsi/scsi_transport_srp.c') diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c index 2700a5a09bd4..8b9cb22be963 100644 --- a/drivers/scsi/scsi_transport_srp.c +++ b/drivers/scsi/scsi_transport_srp.c @@ -67,7 +67,8 @@ static inline struct Scsi_Host *rport_to_shost(struct srp_rport *r) * * The combination of the timeout parameters must be such that SCSI commands * are finished in a reasonable time. Hence do not allow the fast I/O fail - * timeout to exceed SCSI_DEVICE_BLOCK_MAX_TIMEOUT. Furthermore, these + * timeout to exceed SCSI_DEVICE_BLOCK_MAX_TIMEOUT nor allow dev_loss_tmo to + * exceed that limit if failing I/O fast has been disabled. Furthermore, these * parameters must be such that multipath can detect failed paths timely. * Hence do not allow all three parameters to be disabled simultaneously. */ @@ -79,6 +80,9 @@ int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, int dev_loss_tmo) return -EINVAL; if (fast_io_fail_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT) return -EINVAL; + if (fast_io_fail_tmo < 0 && + dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT) + return -EINVAL; if (dev_loss_tmo >= LONG_MAX / HZ) return -EINVAL; if (fast_io_fail_tmo >= 0 && dev_loss_tmo >= 0 && @@ -463,20 +467,20 @@ static void __srp_start_tl_fail_timers(struct srp_rport *rport) queue_delayed_work(system_long_wq, &rport->reconnect_work, 1UL * delay * HZ); - if (fast_io_fail_tmo >= 0 && - srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0) { + if (srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0) { pr_debug("%s new state: %d\n", dev_name(&shost->shost_gendev), rport->state); scsi_target_block(&shost->shost_gendev); - queue_delayed_work(system_long_wq, - &rport->fast_io_fail_work, - 1UL * fast_io_fail_tmo * HZ); + if (fast_io_fail_tmo >= 0) + queue_delayed_work(system_long_wq, + &rport->fast_io_fail_work, + 1UL * fast_io_fail_tmo * HZ); + if (dev_loss_tmo >= 0) + queue_delayed_work(system_long_wq, + &rport->dev_loss_work, + 1UL * dev_loss_tmo * HZ); } - if (dev_loss_tmo >= 0) - queue_delayed_work(system_long_wq, - &rport->dev_loss_work, - 1UL * dev_loss_tmo * HZ); } else { pr_debug("%s has already been deleted\n", dev_name(&shost->shost_gendev)); @@ -578,9 +582,9 @@ int srp_reconnect_rport(struct srp_rport *rport) spin_unlock_irq(shost->host_lock); } else if (rport->state == SRP_RPORT_RUNNING) { /* - * srp_reconnect_rport() was invoked with fast_io_fail - * off. Mark the port as failed and start the TL failure - * timers if these had not yet been started. + * srp_reconnect_rport() has been invoked with fast_io_fail + * and dev_loss off. Mark the port as failed and start the TL + * failure timers if these had not yet been started. */ __rport_fail_io_fast(rport); scsi_target_unblock(&shost->shost_gendev, -- cgit v1.2.3-55-g7522 From 93079162bf0ed2934c7b0c3ee93ba894df8fb3cd Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 11 Dec 2013 17:06:14 +0100 Subject: scsi_transport_srp: Fix a race condition The rport timers must be stopped before the SRP initiator destroys the resources associated with the SCSI host. This is necessary because otherwise the callback functions invoked from the SRP transport layer could trigger a use-after-free. Stopping the rport timers before invoking scsi_remove_host() can trigger long delays in the SCSI error handler if a transport layer failure occurs while scsi_remove_host() is in progress. Hence move the code for stopping the rport timers from srp_rport_release() into a new function and invoke that function after scsi_remove_host() has finished. This patch fixes the following sporadic kernel crash: kernel BUG at include/asm-generic/dma-mapping-common.h:64! invalid opcode: 0000 [#1] SMP RIP: 0010:[] [] srp_unmap_data+0x121/0x130 [ib_srp] Call Trace: [] srp_free_req+0x3c/0x80 [ib_srp] [] srp_finish_req+0x48/0x70 [ib_srp] [] srp_terminate_io+0x4b/0x60 [ib_srp] [] __rport_fail_io_fast+0x75/0x80 [scsi_transport_srp] [] rport_fast_io_fail_timedout+0x88/0xc0 [scsi_transport_srp] [] worker_thread+0x170/0x2a0 [] kthread+0x96/0xa0 [] child_rip+0xa/0x20 Signed-off-by: Bart Van Assche Signed-off-by: Roland Dreier --- drivers/infiniband/ulp/srp/ib_srp.c | 1 + drivers/scsi/scsi_transport_srp.c | 83 +++++++++++++++++++------------------ include/scsi/scsi_transport_srp.h | 4 +- 3 files changed, 46 insertions(+), 42 deletions(-) (limited to 'drivers/scsi/scsi_transport_srp.c') diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index a88631918e85..529b6bcdca7a 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -660,6 +660,7 @@ static void srp_remove_target(struct srp_target_port *target) srp_rport_get(target->rport); srp_remove_host(target->scsi_host); scsi_remove_host(target->scsi_host); + srp_stop_rport_timers(target->rport); srp_disconnect_target(target); ib_destroy_cm_id(target->cm_id); srp_free_target_ib(target); diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c index 8b9cb22be963..a349d44c4c36 100644 --- a/drivers/scsi/scsi_transport_srp.c +++ b/drivers/scsi/scsi_transport_srp.c @@ -456,37 +456,29 @@ static void __srp_start_tl_fail_timers(struct srp_rport *rport) lockdep_assert_held(&rport->mutex); - if (!rport->deleted) { - delay = rport->reconnect_delay; - fast_io_fail_tmo = rport->fast_io_fail_tmo; - dev_loss_tmo = rport->dev_loss_tmo; - pr_debug("%s current state: %d\n", - dev_name(&shost->shost_gendev), rport->state); + delay = rport->reconnect_delay; + fast_io_fail_tmo = rport->fast_io_fail_tmo; + dev_loss_tmo = rport->dev_loss_tmo; + pr_debug("%s current state: %d\n", dev_name(&shost->shost_gendev), + rport->state); - if (delay > 0) + if (rport->state == SRP_RPORT_LOST) + return; + if (delay > 0) + queue_delayed_work(system_long_wq, &rport->reconnect_work, + 1UL * delay * HZ); + if (srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0) { + pr_debug("%s new state: %d\n", dev_name(&shost->shost_gendev), + rport->state); + scsi_target_block(&shost->shost_gendev); + if (fast_io_fail_tmo >= 0) queue_delayed_work(system_long_wq, - &rport->reconnect_work, - 1UL * delay * HZ); - if (srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0) { - pr_debug("%s new state: %d\n", - dev_name(&shost->shost_gendev), - rport->state); - scsi_target_block(&shost->shost_gendev); - if (fast_io_fail_tmo >= 0) - queue_delayed_work(system_long_wq, - &rport->fast_io_fail_work, - 1UL * fast_io_fail_tmo * HZ); - if (dev_loss_tmo >= 0) - queue_delayed_work(system_long_wq, - &rport->dev_loss_work, - 1UL * dev_loss_tmo * HZ); - } - } else { - pr_debug("%s has already been deleted\n", - dev_name(&shost->shost_gendev)); - srp_rport_set_state(rport, SRP_RPORT_FAIL_FAST); - scsi_target_unblock(&shost->shost_gendev, - SDEV_TRANSPORT_OFFLINE); + &rport->fast_io_fail_work, + 1UL * fast_io_fail_tmo * HZ); + if (dev_loss_tmo >= 0) + queue_delayed_work(system_long_wq, + &rport->dev_loss_work, + 1UL * dev_loss_tmo * HZ); } } @@ -560,7 +552,7 @@ int srp_reconnect_rport(struct srp_rport *rport) scsi_target_block(&shost->shost_gendev); while (scsi_request_fn_active(shost)) msleep(20); - res = i->f->reconnect(rport); + res = rport->state != SRP_RPORT_LOST ? i->f->reconnect(rport) : -ENODEV; pr_debug("%s (state %d): transport.reconnect() returned %d\n", dev_name(&shost->shost_gendev), rport->state, res); if (res == 0) { @@ -626,10 +618,6 @@ static void srp_rport_release(struct device *dev) { struct srp_rport *rport = dev_to_rport(dev); - cancel_delayed_work_sync(&rport->reconnect_work); - cancel_delayed_work_sync(&rport->fast_io_fail_work); - cancel_delayed_work_sync(&rport->dev_loss_work); - put_device(dev->parent); kfree(rport); } @@ -784,12 +772,6 @@ void srp_rport_del(struct srp_rport *rport) device_del(dev); transport_destroy_device(dev); - mutex_lock(&rport->mutex); - if (rport->state == SRP_RPORT_BLOCKED) - __rport_fail_io_fast(rport); - rport->deleted = true; - mutex_unlock(&rport->mutex); - put_device(dev); } EXPORT_SYMBOL_GPL(srp_rport_del); @@ -814,6 +796,27 @@ void srp_remove_host(struct Scsi_Host *shost) } EXPORT_SYMBOL_GPL(srp_remove_host); +/** + * srp_stop_rport_timers - stop the transport layer recovery timers + * + * Must be called after srp_remove_host() and scsi_remove_host(). The caller + * must hold a reference on the rport (rport->dev) and on the SCSI host + * (rport->dev.parent). + */ +void srp_stop_rport_timers(struct srp_rport *rport) +{ + mutex_lock(&rport->mutex); + if (rport->state == SRP_RPORT_BLOCKED) + __rport_fail_io_fast(rport); + srp_rport_set_state(rport, SRP_RPORT_LOST); + mutex_unlock(&rport->mutex); + + cancel_delayed_work_sync(&rport->reconnect_work); + cancel_delayed_work_sync(&rport->fast_io_fail_work); + cancel_delayed_work_sync(&rport->dev_loss_work); +} +EXPORT_SYMBOL_GPL(srp_stop_rport_timers); + static int srp_tsk_mgmt_response(struct Scsi_Host *shost, u64 nexus, u64 tm_id, int result) { diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h index 4ebf6913b7b2..f24e763fa430 100644 --- a/include/scsi/scsi_transport_srp.h +++ b/include/scsi/scsi_transport_srp.h @@ -19,7 +19,7 @@ struct srp_rport_identifiers { * @SRP_RPORT_BLOCKED: Transport layer not operational; fast I/O fail timer * is running and I/O has been blocked. * @SRP_RPORT_FAIL_FAST: Fast I/O fail timer has expired; fail I/O fast. - * @SRP_RPORT_LOST: Device loss timer has expired; port is being removed. + * @SRP_RPORT_LOST: Port is being removed. */ enum srp_rport_state { SRP_RPORT_RUNNING, @@ -48,7 +48,6 @@ struct srp_rport { struct mutex mutex; enum srp_rport_state state; - bool deleted; int reconnect_delay; int failed_reconnects; struct delayed_work reconnect_work; @@ -101,6 +100,7 @@ extern int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, extern int srp_reconnect_rport(struct srp_rport *rport); extern void srp_start_tl_fail_timers(struct srp_rport *rport); extern void srp_remove_host(struct Scsi_Host *); +extern void srp_stop_rport_timers(struct srp_rport *rport); /** * srp_chkready() - evaluate the transport layer state before I/O -- cgit v1.2.3-55-g7522 From 0c7f82189d2249153ee302fe4d02de15f165d220 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Mon, 13 Jan 2014 12:39:35 +0100 Subject: scsi_transport_srp: Fix kernel-doc warnings The following command has been used to verify that the kernel-doc tool no longer complains about undocumented fields: scripts/kernel-doc -html drivers/scsi/scsi_transport_srp.c \ include/scsi/scsi_transport_srp.h >srp-transport-doc.html Signed-off-by: Bart Van Assche Acked-by: Sebastian Riemer Acked-by: Randy Dunlap Signed-off-by: Roland Dreier --- drivers/scsi/scsi_transport_srp.c | 12 ++++++++++++ include/scsi/scsi_transport_srp.h | 32 ++++++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 4 deletions(-) (limited to 'drivers/scsi/scsi_transport_srp.c') diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c index a349d44c4c36..d47ffc8d3e43 100644 --- a/drivers/scsi/scsi_transport_srp.c +++ b/drivers/scsi/scsi_transport_srp.c @@ -64,6 +64,9 @@ static inline struct Scsi_Host *rport_to_shost(struct srp_rport *r) /** * srp_tmo_valid() - check timeout combination validity + * @reconnect_delay: Reconnect delay in seconds. + * @fast_io_fail_tmo: Fast I/O fail timeout in seconds. + * @dev_loss_tmo: Device loss timeout in seconds. * * The combination of the timeout parameters must be such that SCSI commands * are finished in a reasonable time. Hence do not allow the fast I/O fail @@ -372,6 +375,7 @@ invalid: /** * srp_reconnect_work() - reconnect and schedule a new attempt if necessary + * @work: Work structure used for scheduling this operation. */ static void srp_reconnect_work(struct work_struct *work) { @@ -412,6 +416,7 @@ static void __rport_fail_io_fast(struct srp_rport *rport) /** * rport_fast_io_fail_timedout() - fast I/O failure timeout handler + * @work: Work structure used for scheduling this operation. */ static void rport_fast_io_fail_timedout(struct work_struct *work) { @@ -430,6 +435,7 @@ static void rport_fast_io_fail_timedout(struct work_struct *work) /** * rport_dev_loss_timedout() - device loss timeout handler + * @work: Work structure used for scheduling this operation. */ static void rport_dev_loss_timedout(struct work_struct *work) { @@ -484,6 +490,7 @@ static void __srp_start_tl_fail_timers(struct srp_rport *rport) /** * srp_start_tl_fail_timers() - start the transport layer failure timers + * @rport: SRP target port. * * Start the transport layer fast I/O failure and device loss timers. Do not * modify a timer that was already started. @@ -498,6 +505,7 @@ EXPORT_SYMBOL(srp_start_tl_fail_timers); /** * scsi_request_fn_active() - number of kernel threads inside scsi_request_fn() + * @shost: SCSI host for which to count the number of scsi_request_fn() callers. */ static int scsi_request_fn_active(struct Scsi_Host *shost) { @@ -518,6 +526,7 @@ static int scsi_request_fn_active(struct Scsi_Host *shost) /** * srp_reconnect_rport() - reconnect to an SRP target port + * @rport: SRP target port. * * Blocks SCSI command queueing before invoking reconnect() such that * queuecommand() won't be invoked concurrently with reconnect() from outside @@ -595,6 +604,7 @@ EXPORT_SYMBOL(srp_reconnect_rport); /** * srp_timed_out() - SRP transport intercept of the SCSI timeout EH + * @scmd: SCSI command. * * If a timeout occurs while an rport is in the blocked state, ask the SCSI * EH to continue waiting (BLK_EH_RESET_TIMER). Otherwise let the SCSI core @@ -666,6 +676,7 @@ static int srp_host_match(struct attribute_container *cont, struct device *dev) /** * srp_rport_get() - increment rport reference count + * @rport: SRP target port. */ void srp_rport_get(struct srp_rport *rport) { @@ -675,6 +686,7 @@ EXPORT_SYMBOL(srp_rport_get); /** * srp_rport_put() - decrement rport reference count + * @rport: SRP target port. */ void srp_rport_put(struct srp_rport *rport) { diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h index f24e763fa430..b11da5c1331e 100644 --- a/include/scsi/scsi_transport_srp.h +++ b/include/scsi/scsi_transport_srp.h @@ -29,10 +29,26 @@ enum srp_rport_state { }; /** - * struct srp_rport - * @lld_data: LLD private data. - * @mutex: Protects against concurrent rport reconnect / fast_io_fail / - * dev_loss_tmo activity. + * struct srp_rport - SRP initiator or target port + * + * Fields that are relevant for SRP initiator and SRP target drivers: + * @dev: Device associated with this rport. + * @port_id: 16-byte port identifier. + * @roles: Role of this port - initiator or target. + * + * Fields that are only relevant for SRP initiator drivers: + * @lld_data: LLD private data. + * @mutex: Protects against concurrent rport reconnect / + * fast_io_fail / dev_loss_tmo activity. + * @state: rport state. + * @deleted: Whether or not srp_rport_del() has already been invoked. + * @reconnect_delay: Reconnect delay in seconds. + * @failed_reconnects: Number of failed reconnect attempts. + * @reconnect_work: Work structure used for scheduling reconnect attempts. + * @fast_io_fail_tmo: Fast I/O fail timeout in seconds. + * @dev_loss_tmo: Device loss timeout in seconds. + * @fast_io_fail_work: Work structure used for scheduling fast I/O fail work. + * @dev_loss_work: Work structure used for scheduling device loss work. */ struct srp_rport { /* for initiator and target drivers */ @@ -59,6 +75,8 @@ struct srp_rport { /** * struct srp_function_template + * + * Fields that are only relevant for SRP initiator drivers: * @has_rport_state: Whether or not to create the state, fast_io_fail_tmo and * dev_loss_tmo sysfs attribute for an rport. * @reset_timer_if_blocked: Whether or srp_timed_out() should reset the command @@ -70,6 +88,11 @@ struct srp_rport { * srp_reconnect_rport(). * @terminate_rport_io: Callback function for terminating all outstanding I/O * requests for an rport. + * @rport_delete: Callback function that deletes an rport. + * + * Fields that are only relevant for SRP target drivers: + * @tsk_mgmt_response: Callback function for sending a task management response. + * @it_nexus_response: Callback function for processing an IT nexus response. */ struct srp_function_template { /* for initiator drivers */ @@ -104,6 +127,7 @@ extern void srp_stop_rport_timers(struct srp_rport *rport); /** * srp_chkready() - evaluate the transport layer state before I/O + * @rport: SRP target port pointer. * * Returns a SCSI result code that can be returned by the LLD queuecommand() * implementation. The role of this function is similar to that of -- cgit v1.2.3-55-g7522