[Open-FCoE] [PATCH] libfc, fcoe: fixed locking issues with lport->lp_mutex around lport->link_status

Joe Eykholt jeykholt at cisco.com
Thu Jan 15 18:30:45 UTC 2009


Vasu Dev wrote:
> The fcoe_xmit could call fc_pause in case the pending skb queue len is larger
> than FCOE_MAX_QUEUE_DEPTH, the fc_pause was trying to grab lport->lp_muex to
> change lport->link_status and that caused these issues :-
> 
> 1. The fcoe_xmit was getting called with bh disabled, thus casing
> "BUG: scheduling while atomic" when grabbing lport->lp_muex while bh disabled.
> 
> 2. fc_linkup and fc_linkdown function calls lport_enter function with
> lport->lp_mutex held and these enter function in turn calls fcoe_xmit to send
> lport related FC frame, for instance fc_linkup calling fc_lport_enter_flogi
> to send flogi req. In this call flow grabbing same lport->lp_mutex in
> fc_puase from fcoe_xmit would cause deadlock.
> 
> The lport->lp_mutex required to set FC_PAUSE in fcoe_xmit path but
> I don't see FC_PAUSE directly used anywhere beside just setting and clear this
> bit in lport->link_status. Shouldn't we report SCSI_MLQUEUE_DEVICE_BUSY or
> SCSI_MLQUEUE_HOST_BUSY for any new command while FC_PUASE set?

Absolutely.  It used to try to do that.  It might not have ever worked, though.
This is important for reliable non-drop behavior.  If we don't stop SCSI, it
could allocate a lot of frames which would stay in the fcoe queue.  Pause
could last a long time, in theory.  Good catch.

> I'm not sure that lock is really required for lport->link_status but for now
> changed this to atomic to avoid above listed two issues but I'll look into
> eliminating this atomic since this is used from fc_queuecommand. If FC_PAUSE
> is really required then I could add a separate field for this to eliminate
> locking need for least FC_PAUSE bit setting.

We should put it in a separate field now, which would eliminate the
need for locking or lock it under the lport lock.

See below for more comments, but I think the bottom line is that this patch
should be redone such that pause is a separate field and only used in the fcoe
module.  The link_status field shouldn't have pause in it.

	Regards,
	Joe


> Signed-off-by: Vasu Dev <vasu.dev at intel.com>
> ---
> 
>  drivers/scsi/fcoe/fcoe_sw.c   |    8 +++++---
>  drivers/scsi/fcoe/libfcoe.c   |    6 +++---
>  drivers/scsi/libfc/fc_fcp.c   |    5 +++--
>  drivers/scsi/libfc/fc_lport.c |   40 ++++++++++++++++++++++++----------------
>  include/scsi/libfc.h          |    2 +-
>  5 files changed, 36 insertions(+), 25 deletions(-)
> 
> 
> diff --git a/drivers/scsi/fcoe/fcoe_sw.c b/drivers/scsi/fcoe/fcoe_sw.c
> index dc4cd5e..ac10f0f 100644
> --- a/drivers/scsi/fcoe/fcoe_sw.c
> +++ b/drivers/scsi/fcoe/fcoe_sw.c
> @@ -116,7 +116,7 @@ static int fcoe_sw_lport_config(struct fc_lport *lp)
>  {
>  	int i = 0;
>  
> -	lp->link_status = 0;
> +	atomic_set(&lp->link_status, 0);
>  	lp->max_retry_count = 3;
>  	lp->e_d_tov = 2 * 1000;	/* FC-FS default */
>  	lp->r_a_tov = 2 * 2 * 1000;
> @@ -153,6 +153,7 @@ static int fcoe_sw_netdev_config(struct fc_lport *lp, struct net_device *netdev)
>  	u64 wwnn, wwpn;
>  	struct fcoe_softc *fc;
>  	u8 flogi_maddr[ETH_ALEN];
> +	int status;
>  
>  	/* Setup lport private data to point to fcoe softc */
>  	fc = lport_priv(lp);
> @@ -181,9 +182,10 @@ static int fcoe_sw_netdev_config(struct fc_lport *lp, struct net_device *netdev)
>  	if (fc_set_mfs(lp, mfs))
>  		return -EINVAL;
>  
> -	lp->link_status = ~FC_PAUSE & ~FC_LINK_UP;
> +	status =  ~FC_PAUSE & ~FC_LINK_UP;
>  	if (!fcoe_link_ok(lp))
> -		lp->link_status |= FC_LINK_UP;
> +		status |= FC_LINK_UP;
> +	atomic_set(&lp->link_status, status);
>  
>  	/* offload features support */
>  	if (fc->real_dev->features & NETIF_F_SG)
> diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c
> index e419f48..50cbf50 100644
> --- a/drivers/scsi/fcoe/libfcoe.c
> +++ b/drivers/scsi/fcoe/libfcoe.c
> @@ -873,7 +873,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
>  	struct net_device *real_dev = ptr;
>  	struct fcoe_softc *fc;
>  	struct fcoe_dev_stats *stats;
> -	u16 new_status;
> +	int new_status;
>  	u32 mfs;
>  	int rc = NOTIFY_OK;
>  
> @@ -890,7 +890,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
>  		goto out;
>  	}
>  
> -	new_status = lp->link_status;
> +	new_status = atomic_read(&lp->link_status);
>  	switch (event) {
>  	case NETDEV_DOWN:
>  	case NETDEV_GOING_DOWN:
> @@ -917,7 +917,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
>  	default:
>  		FC_DBG("unknown event %ld call", event);
>  	}
> -	if (lp->link_status != new_status) {
> +	if (atomic_read(&lp->link_status) != new_status) {
>  		if ((new_status & FC_LINK_UP) == FC_LINK_UP)
>  			fc_linkup(lp);
>  		else {
> diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
> index 404e63f..fd2d642 100644
> --- a/drivers/scsi/libfc/fc_fcp.c
> +++ b/drivers/scsi/libfc/fc_fcp.c
> @@ -1621,7 +1621,8 @@ out:
>  static inline int fc_fcp_lport_queue_ready(struct fc_lport *lp)
>  {
>  	/* lock ? */
> -	return (lp->state == LPORT_ST_READY) && (lp->link_status & FC_LINK_UP);
> +	return (lp->state == LPORT_ST_READY)
> +		&& (atomic_read(&lp->link_status) & FC_LINK_UP);
>  }
>  
>  /**
> @@ -1890,7 +1891,7 @@ int fc_eh_abort(struct scsi_cmnd *sc_cmd)
>  	lp = shost_priv(sc_cmd->device->host);
>  	if (lp->state != LPORT_ST_READY)
>  		return rc;
> -	else if (!(lp->link_status & FC_LINK_UP))
> +	else if (!(atomic_read(&lp->link_status) & FC_LINK_UP))
>  		return rc;
>  
>  	spin_lock_irqsave(lp->host->host_lock, flags);
> diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
> index 5db223c..51a5482 100644
> --- a/drivers/scsi/libfc/fc_lport.c
> +++ b/drivers/scsi/libfc/fc_lport.c
> @@ -250,7 +250,7 @@ void fc_get_host_port_state(struct Scsi_Host *shost)
>  {
>  	struct fc_lport *lp = shost_priv(shost);
>  
> -	if ((lp->link_status & FC_LINK_UP) == FC_LINK_UP)
> +	if ((atomic_read(&lp->link_status) & FC_LINK_UP) == FC_LINK_UP)
>  		fc_host_port_state(shost) = FC_PORTSTATE_ONLINE;
>  	else
>  		fc_host_port_state(shost) = FC_PORTSTATE_OFFLINE;
> @@ -573,17 +573,21 @@ EXPORT_SYMBOL(fc_fabric_login);
>   */
>  void fc_linkup(struct fc_lport *lport)
>  {
> +	int status;
> +
>  	FC_DEBUG_LPORT("Link is up for port (%6x)\n",
>  		       fc_host_port_id(lport->host));
>  
> -	mutex_lock(&lport->lp_mutex);
> -	if ((lport->link_status & FC_LINK_UP) != FC_LINK_UP) {
> -		lport->link_status |= FC_LINK_UP;
> +	status = atomic_read(&lport->link_status);
> +	if ((status & FC_LINK_UP) != FC_LINK_UP) {

I know this was there before, but to test the bit is 0, we should
just do:
	if (!(status & FC_LINK_UP)) {

> +		status |= FC_LINK_UP;
> +		atomic_set(&lport->link_status, status);
>  
> +		mutex_lock(&lport->lp_mutex);
>  		if (lport->state == LPORT_ST_RESET)
>  			fc_lport_enter_flogi(lport);
> +		mutex_unlock(&lport->lp_mutex);
>  	}
> -	mutex_unlock(&lport->lp_mutex);
>  }
>  EXPORT_SYMBOL(fc_linkup);
>  
> @@ -593,16 +597,20 @@ EXPORT_SYMBOL(fc_linkup);
>   */
>  void fc_linkdown(struct fc_lport *lport)
>  {
> -	mutex_lock(&lport->lp_mutex);
> +	int status;
> +
>  	FC_DEBUG_LPORT("Link is down for port (%6x)\n",
>  		       fc_host_port_id(lport->host));
>  
> -	if ((lport->link_status & FC_LINK_UP) == FC_LINK_UP) {
> -		lport->link_status &= ~(FC_LINK_UP);
> +	status = atomic_read(&lport->link_status);
> +	if ((status & FC_LINK_UP) == FC_LINK_UP) {
> +		status &= ~FC_LINK_UP;
> +		atomic_set(&lport->link_status, status);

This logic isn't atomic even though you're using atomic_read/set.
The whole idea of atomics is to make read-modify-write atomic
as a unit.  It doesn't work to merely do the read and write
as atomics.  You could use atomic_set_maxk or atomic_clear_mask,
except that they are x86-specific.

If we were going to keep the fields combined, since we're about
to grab the lp_mutex, we could just use that to protect
the state and that way you don't need the atomic ops.  They're expensive
on some architectures, and never as cheap as non-atomics.

> +		mutex_lock(&lport->lp_mutex);
>  		fc_lport_enter_reset(lport);
>  		lport->tt.fcp_cleanup(lport);
> +		mutex_unlock(&lport->lp_mutex);
>  	}
> -	mutex_unlock(&lport->lp_mutex);
>  }
>  EXPORT_SYMBOL(fc_linkdown);
>  
> @@ -612,9 +620,9 @@ EXPORT_SYMBOL(fc_linkdown);
>   */
>  void fc_pause(struct fc_lport *lport)
>  {
> -	mutex_lock(&lport->lp_mutex);
> -	lport->link_status |= FC_PAUSE;
> -	mutex_unlock(&lport->lp_mutex);
> +	int status;

Put a blank line between declarations and code.

> +	status = atomic_read(&lport->link_status) | FC_PAUSE;
> +	atomic_set(&lport->link_status, status);

Again, this isn't atomic.  A separate field would be better.
And, since the pause logic is FCoE-module-specific, it can be there only,
and not in libfc.  That unclutters the lport stuff a bit.

   }
>  EXPORT_SYMBOL(fc_pause);
>  
> @@ -624,9 +632,9 @@ EXPORT_SYMBOL(fc_pause);
>   */
>  void fc_unpause(struct fc_lport *lport)
>  {
> -	mutex_lock(&lport->lp_mutex);
> -	lport->link_status &= ~(FC_PAUSE);
> -	mutex_unlock(&lport->lp_mutex);
> +	int status;
> +	status = atomic_read(&lport->link_status) & ~FC_PAUSE;
> +	atomic_set(&lport->link_status, status);
>  }
>  EXPORT_SYMBOL(fc_unpause);
>  
> @@ -977,7 +985,7 @@ static void fc_lport_enter_reset(struct fc_lport *lport)
>  	fc_host_fabric_name(lport->host) = 0;
>  	fc_host_port_id(lport->host) = 0;
>  
> -	if ((lport->link_status & FC_LINK_UP) == FC_LINK_UP)
> +	if ((atomic_read(&lport->link_status) & FC_LINK_UP) == FC_LINK_UP)

See above comment ... to test for one, just do the AND, not the comparison.
I think the form was there because we used to test multiple flags together.

>  		fc_lport_enter_flogi(lport);
>  }
>  
> diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
> index 042f4ad..f7dca26 100644
> --- a/include/scsi/libfc.h
> +++ b/include/scsi/libfc.h
> @@ -603,7 +603,7 @@ struct fc_lport {
>  
>  	/* Operational Information */
>  	struct libfc_function_template tt;
> -	u16			link_status;
> +	atomic_t	link_status;
>  	enum fc_lport_state	state;
>  	unsigned long		boot_time;
>  
> 
> _______________________________________________
> devel mailing list
> devel at open-fcoe.org
> http://www.open-fcoe.org/mailman/listinfo/devel




More information about the devel mailing list