[Open-FCoE] deadlock in testing (Re: [RFC PATCH V3 11/13] fcoe: use RTNL mutex to serialize create/destroy and protect hostlist)

Chris Leech christopher.leech at intel.com
Thu Jul 9 22:28:18 UTC 2009


I ran a parallel create/destroy/remove test overnight and something
deadlocked.  Running with lockdep enabled gives a reproducible warning,
but I'm having trouble making sense of it.  I'm not sure I understand
what the "events" lock is here.

	Chris

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.31-rc2-cdl #112
-------------------------------------------------------
fcoeadm/4021 is trying to acquire lock:
 (events){+.+.+.}, at: [<ffffffff8105c744>] flush_work+0x3a/0xf0

but task is already holding lock:
 (rtnl_mutex){+.+.+.}, at: [<ffffffff813057e5>] rtnl_lock+0x12/0x14

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (rtnl_mutex){+.+.+.}:
       [<ffffffff8107039c>] __lock_acquire+0xa8c/0xc14
       [<ffffffff810705eb>] lock_acquire+0xc7/0xf3
       [<ffffffff813965e2>] __mutex_lock_common+0x48/0x39a
       [<ffffffff813969dd>] mutex_lock_nested+0x35/0x3a
       [<ffffffff813057e5>] rtnl_lock+0x12/0x14
       [<ffffffff813067af>] linkwatch_event+0x9/0x27
       [<ffffffff8105baee>] worker_thread+0x200/0x313
       [<ffffffff8105ffd4>] kthread+0x88/0x90
       [<ffffffff81012d2a>] child_rip+0xa/0x20
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #1 ((linkwatch_work).work){+.+.+.}:
       [<ffffffff8107039c>] __lock_acquire+0xa8c/0xc14
       [<ffffffff810705eb>] lock_acquire+0xc7/0xf3
       [<ffffffff8105bae5>] worker_thread+0x1f7/0x313
       [<ffffffff8105ffd4>] kthread+0x88/0x90
       [<ffffffff81012d2a>] child_rip+0xa/0x20
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #0 (events){+.+.+.}:
       [<ffffffff81070290>] __lock_acquire+0x980/0xc14
       [<ffffffff810705eb>] lock_acquire+0xc7/0xf3
       [<ffffffff8105c76f>] flush_work+0x65/0xf0
       [<ffffffffa0054c01>] fcoe_ctlr_destroy+0x25/0xa6 [libfcoe]
       [<ffffffffa011309f>] __fcoe_destroy+0xe7/0x203 [fcoe]
       [<ffffffffa01131ec>] fcoe_destroy+0x31/0x57 [fcoe]
       [<ffffffff8105e378>] param_attr_store+0x25/0x35
       [<ffffffff8105e3cd>] module_attr_store+0x21/0x25
       [<ffffffff81142511>] sysfs_write_file+0xe4/0x119
       [<ffffffff810efe6e>] vfs_write+0xab/0x105
       [<ffffffff810eff8c>] sys_write+0x47/0x6f
       [<ffffffff81011b42>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

other info that might help us debug this:

2 locks held by fcoeadm/4021:
 #0:  (&buffer->mutex){+.+.+.}, at: [<ffffffff81142465>] sysfs_write_file+0x38/0x119
 #1:  (rtnl_mutex){+.+.+.}, at: [<ffffffff813057e5>] rtnl_lock+0x12/0x14

stack backtrace:
Pid: 4021, comm: fcoeadm Not tainted 2.6.31-rc2-cdl #112
Call Trace:
 [<ffffffff8106f59a>] print_circular_bug_tail+0x71/0x7c
 [<ffffffff81070290>] __lock_acquire+0x980/0xc14
 [<ffffffff810705eb>] lock_acquire+0xc7/0xf3
 [<ffffffff8105c744>] ? flush_work+0x3a/0xf0
 [<ffffffff8105c76f>] flush_work+0x65/0xf0
 [<ffffffff8105c744>] ? flush_work+0x3a/0xf0
 [<ffffffff8106e9f5>] ? mark_lock+0x22/0x227
 [<ffffffff8106ec47>] ? mark_held_locks+0x4d/0x6b
 [<ffffffff8139640f>] ? __mutex_unlock_slowpath+0x128/0x13b
 [<ffffffff8106eeda>] ? trace_hardirqs_on_caller+0x110/0x134
 [<ffffffff8106ef0b>] ? trace_hardirqs_on+0xd/0xf
 [<ffffffffa0054c01>] fcoe_ctlr_destroy+0x25/0xa6 [libfcoe]
 [<ffffffffa011309f>] __fcoe_destroy+0xe7/0x203 [fcoe]
 [<ffffffffa01121a6>] ? fcoe_if_to_netdev+0x5c/0x63 [fcoe]
 [<ffffffffa01131ec>] fcoe_destroy+0x31/0x57 [fcoe]
 [<ffffffff8105e378>] param_attr_store+0x25/0x35
 [<ffffffff8105e3cd>] module_attr_store+0x21/0x25
 [<ffffffff81142511>] sysfs_write_file+0xe4/0x119
 [<ffffffff810efe6e>] vfs_write+0xab/0x105
 [<ffffffff810eff8c>] sys_write+0x47/0x6f
 [<ffffffff81011b42>] system_call_fastpath+0x16/0x1b


On Wed, Jul 08, 2009 at 06:04:41PM -0700, Joe Eykholt wrote:
> Chris Leech wrote:
> > Rather than rely on the hostlist_lock to be held while creating exchange
> > managers, serialize fcoe instance creation and destruction with a mutex.
> > This will allow the hostlist addition to be moved out of fcoe_if_create(),
> > which will simplify NPIV support.
> > 
> > RTNL mutex was used because it needs to be held in parts of both the create
> > and destroy path anyway, and because the netdevice notifier will be called
> > with RTNL held.  If a new mutex was added and fine-grained use of RTNL was
> > maintained, then destroying the interface when the netdevice was unregistered
> > would deadlock on RTNL unless it was deferred using a workqueue or similar.
> > 
> > Signed-off-by: Chris Leech <christopher.leech at intel.com>
> > ---
> > 
> >  drivers/scsi/fcoe/fcoe.c |   68 ++++++++++++++++++++++++++++------------------
> >  1 files changed, 41 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index 49a9d5b..a4b8991 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -49,9 +49,9 @@ MODULE_AUTHOR("Open-FCoE.org");
> >  MODULE_DESCRIPTION("FCoE");
> >  MODULE_LICENSE("GPL v2");
> >  
> > -/* fcoe host list */
> > +/* fcoe host list
> > + * must be accessed under rtnl_lock */
> >  LIST_HEAD(fcoe_hostlist);
> > -DEFINE_RWLOCK(fcoe_hostlist_lock);
> >  DEFINE_PER_CPU(struct fcoe_percpu_s, fcoe_percpu);
> >  
> >  /* Function Prototypes */
> > @@ -186,13 +186,11 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
> >  	 * or enter promiscuous mode if not capable of listening
> >  	 * for multiple unicast MACs.
> >  	 */
> > -	rtnl_lock();
> >  	memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
> >  	dev_unicast_add(netdev, flogi_maddr);
> >  	if (fip->spma)
> >  		dev_unicast_add(netdev, fip->ctl_src_addr);
> >  	dev_mc_add(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> > -	rtnl_unlock();
> >  
> >  	/*
> >  	 * setup the receive function from ethernet driver
> > @@ -259,7 +257,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
> >  	dev_remove_pack(&fcoe->fip_packet_type);
> >  
> >  	/* Delete secondary MAC addresses */
> > -	rtnl_lock();
> >  	memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
> >  	dev_unicast_delete(netdev, flogi_maddr);
> >  	if (!is_zero_ether_addr(fip->data_src_addr))
> > @@ -267,7 +264,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
> >  	if (fip->spma)
> >  		dev_unicast_delete(netdev, fip->ctl_src_addr);
> >  	dev_mc_delete(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> > -	rtnl_unlock();
> >  }
> >  
> >  /**
> > @@ -516,8 +512,6 @@ bool fcoe_oem_match(struct fc_frame *fp)
> >   * fcoe_em_config() - allocates em for this lport
> >   * @lp: the fcoe that em is to allocated for
> >   *
> > - * Called with write fcoe_hostlist_lock held.
> > - *
> >   * Returns : 0 on success
> >   */
> >  static inline int fcoe_em_config(struct fc_lport *lp)
> > @@ -739,11 +733,14 @@ static struct fc_lport *fcoe_if_create(struct fcoe_interface *fcoe,
> >  
> >  	/*
> >  	 * fcoe_em_alloc() and fcoe_hostlist_add() both
> > -	 * need to be atomic under fcoe_hostlist_lock
> > +	 * need to be atomic with respect to other changes to the hostlist
> >  	 * since fcoe_em_alloc() looks for an existing EM
> >  	 * instance on host list updated by fcoe_hostlist_add().
> > +	 *
> > +	 * This is OK because the entire create and destroy paths are called
> > +	 * with the RTNL mutex held.
> >  	 */
> > -	write_lock(&fcoe_hostlist_lock);
> > +
> >  	/* lport exch manager allocation */
> >  	rc = fcoe_em_config(lport);
> >  	if (rc) {
> > @@ -754,7 +751,6 @@ static struct fc_lport *fcoe_if_create(struct fcoe_interface *fcoe,
> >  
> >  	/* add to lports list */
> >  	fcoe_hostlist_add(lport);
> > -	write_unlock(&fcoe_hostlist_lock);
> >  
> >  	dev_hold(netdev);
> >  	fcoe_interface_get(fcoe);
> > @@ -795,6 +791,7 @@ static int __init fcoe_if_init(void)
> >  int __exit fcoe_if_exit(void)
> >  {
> >  	fc_release_transport(scsi_transport_fcoe_sw);
> > +	scsi_transport_fcoe_sw = NULL;
> >  	return 0;
> >  }
> >  
> > @@ -1524,14 +1521,14 @@ static int fcoe_device_notification(struct notifier_block *notifier,
> >  	u32 mfs;
> >  	int rc = NOTIFY_OK;
> >  
> > -	read_lock(&fcoe_hostlist_lock);
> > +	ASSERT_RTNL();
> > +
> >  	list_for_each_entry(fcoe, &fcoe_hostlist, list) {
> >  		if (fcoe->real_dev == real_dev) {
> >  			lp = fcoe->ctlr.lp;
> >  			break;
> >  		}
> >  	}
> > -	read_unlock(&fcoe_hostlist_lock);
> >  	if (lp == NULL) {
> >  		rc = NOTIFY_DONE;
> >  		goto out;
> > @@ -1674,6 +1671,18 @@ static int fcoe_destroy(const char *buffer, struct kernel_param *kp)
> >  	struct fc_lport *lport;
> >  	int rc;
> >  
> > +	rtnl_lock();
> > +
> > +	/*
> > +	 * Make sure the module has been initialized, and is not about to be
> > +	 * removed.  Module paramter sysfs files are writable before the
> > +	 * module_init function is called and after module_exit.
> > +	 */
> > +	if (!scsi_transport_fcoe_sw) {
> > +		rc = -ENODEV;
> > +		goto out_nodev;
> > +	}
> > +
> >  	netdev = fcoe_if_to_netdev(buffer);
> >  	if (!netdev) {
> >  		rc = -ENODEV;
> > @@ -1693,6 +1702,7 @@ static int fcoe_destroy(const char *buffer, struct kernel_param *kp)
> >  out_putdev:
> >  	dev_put(netdev);
> >  out_nodev:
> > +	rtnl_unlock();
> >  	return rc;
> >  }
> >  
> > @@ -1710,6 +1720,18 @@ static int fcoe_create(const char *buffer, struct kernel_param *kp)
> >  	struct fc_lport *lport;
> >  	struct net_device *netdev;
> >  
> > +	rtnl_lock();
> > +
> > +	/*
> > +	 * Make sure the module has been initialized, and is not about to be
> > +	 * removed.  Module paramter sysfs files are writable before the
> > +	 * module_init function is called and after module_exit.
> > +	 */
> > +	if (!scsi_transport_fcoe_sw) {
> > +		rc = -ENODEV;
> > +		goto out_nodev;
> > +	}
> > +
> >  	netdev = fcoe_if_to_netdev(buffer);
> >  	if (!netdev) {
> >  		rc = -ENODEV;
> > @@ -1746,14 +1768,15 @@ static int fcoe_create(const char *buffer, struct kernel_param *kp)
> >  	if (!fcoe_link_ok(lport))
> >  		fcoe_ctlr_link_up(&fcoe->ctlr);
> >  
> > -	dev_put(netdev);
> > -	return 0;
> > +	rc = 0;
> > +	goto out_putdev;
> >  
> >  out_free:
> >  	fcoe_interface_put(fcoe);
> >  out_putdev:
> >  	dev_put(netdev);
> >  out_nodev:
> > +	rtnl_unlock();
> >  	return rc;
> >  }
> >  
> > @@ -1872,8 +1895,6 @@ int fcoe_reset(struct Scsi_Host *shost)
> >   * fcoe_hostlist_lookup_port() - find the corresponding lport by a given device
> >   * @dev: this is currently ptr to net_device
> >   *
> > - * Called with fcoe_hostlist_lock held.
> > - *
> >   * Returns: NULL or the located fcoe_port
> >   */
> >  static struct fcoe_interface *
> > @@ -1898,10 +1919,7 @@ struct fc_lport *fcoe_hostlist_lookup(const struct net_device *netdev)
> >  {
> >  	struct fcoe_interface *fcoe;
> >  
> > -	read_lock(&fcoe_hostlist_lock);
> >  	fcoe = fcoe_hostlist_lookup_port(netdev);
> > -	read_unlock(&fcoe_hostlist_lock);
> > -
> >  	return (fcoe) ? fcoe->ctlr.lp : NULL;
> >  }
> >  
> > @@ -1909,8 +1927,6 @@ struct fc_lport *fcoe_hostlist_lookup(const struct net_device *netdev)
> >   * fcoe_hostlist_add() - Add a lport to lports list
> >   * @lp: ptr to the fc_lport to be added
> >   *
> > - * Called with write fcoe_hostlist_lock held.
> > - *
> >   * Returns: 0 for success
> >   */
> >  int fcoe_hostlist_add(const struct fc_lport *lport)
> > @@ -1937,11 +1953,9 @@ int fcoe_hostlist_remove(const struct fc_lport *lport)
> >  {
> >  	struct fcoe_interface *fcoe;
> >  
> > -	write_lock_bh(&fcoe_hostlist_lock);
> >  	fcoe = fcoe_hostlist_lookup_port(fcoe_netdev(lport));
> >  	BUG_ON(!fcoe);
> >  	list_del(&fcoe->list);
> > -	write_unlock_bh(&fcoe_hostlist_lock);
> >  
> >  	return 0;
> >  }
> > @@ -1957,9 +1971,6 @@ static int __init fcoe_init(void)
> >  	int rc = 0;
> >  	struct fcoe_percpu_s *p;
> >  
> > -	INIT_LIST_HEAD(&fcoe_hostlist);
> > -	rwlock_init(&fcoe_hostlist_lock);
> > -
> >  	for_each_possible_cpu(cpu) {
> >  		p = &per_cpu(fcoe_percpu, cpu);
> >  		skb_queue_head_init(&p->fcoe_rx_list);
> > @@ -2000,6 +2011,7 @@ static void __exit fcoe_exit(void)
> >  	struct fcoe_interface *fcoe, *tmp;
> >  
> >  	fcoe_dev_cleanup();
> > +	rtnl_lock();
> >  
> >  	/* releases the associated fcoe hosts */
> >  	list_for_each_entry_safe(fcoe, tmp, &fcoe_hostlist, list)
> > @@ -2012,5 +2024,7 @@ static void __exit fcoe_exit(void)
> >  
> >  	/* detach from scsi transport */
> >  	fcoe_if_exit();
> > +
> > +	rtnl_unlock();
> >  }
> >  module_exit(fcoe_exit);
> > 
> > _______________________________________________
> > devel mailing list
> > devel at open-fcoe.org
> > http://www.open-fcoe.org/mailman/listinfo/devel
> 
> This looks good to me.  Thanks for doing it.
> 
> 	Joe
> 



More information about the devel mailing list