[Open-FCoE] [PATCH] libfc: rport retry on LS_RJT from certain ELS

Joe Eykholt jre at nuovasystems.com
Thu Jan 15 23:42:14 UTC 2009


Chris Leech wrote:
> This allows any rport ELS to retry on LS_RJT.
> 
> The rport error handling would only retry on resource allocation failures
> and exchange timeouts.  I have a target that will occasionally reject PLOGI
> when we do a quick LOGO/PLOGI.  When a critical ELS was rejected, libfc would
> fail silently leaving the rport in a dead state.

Good fix.

I have seen this situation here, even with 2 second timeouts.  We'd get
multiple rejects so maybe we're not retrying enough times.

> The retry delay now applies to any retry other than an exchange timeout,
> instead of just resource allocation failures.
> 
> By using a retry flag we open up the possibility of ELS response handlers
> actually checking the additional information in the LS_RJT frame and making
> more intelligent choices about when to retry.
> 
> Signed-off-by: Chris Leech <christopher.leech at intel.com>

The patch looks good.  It might be better to have two error routines, one
which retries and one which doesn't.  The one that retries can then call
the one that doesn't, in the case that retries are exhausted.  That generates
a little less code, since the callers now don't have to pass a parameter.
It isn't performance critical.

But, your way is fine, too.

The error FC_EX_CLOSED, which is caused by the fc_exch_mgr_reset() when a link
goes down (and should be when a remote port is deleted), should not cause a retry,
though (it didn't before).  That's the only other error besides FC_EX_TIMEOUT
for now.

	Joe

> ---
> 
>  drivers/scsi/libfc/fc_rport.c |   90 ++++++++++++++++++++---------------------
>  1 files changed, 43 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index e780d8c..c281b4b 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -80,7 +80,8 @@ static void fc_rport_recv_prlo_req(struct fc_rport *,
>  static void fc_rport_recv_logo_req(struct fc_rport *,
>  				   struct fc_seq *, struct fc_frame *);
>  static void fc_rport_timeout(struct work_struct *);
> -static void fc_rport_error(struct fc_rport *, struct fc_frame *);
> +static void fc_rport_error(struct fc_rport *, struct fc_frame *, int retry);
> +#define FC_RPORT_ERR_RETRY 1
>  static void fc_rport_work(struct work_struct *);
>  
>  static const char *fc_rport_state_names[] = {
> @@ -408,50 +409,45 @@ static void fc_rport_timeout(struct work_struct *work)
>   * fc_rport_error - Handler for any errors
>   * @rport: The fc_rport object
>   * @fp: The frame pointer
> + * @retry: Should the operation be retried or not, obeying the retry limit
>   *
> - * If the error was caused by a resource allocation failure
> - * then wait for half a second and retry, otherwise retry
> - * immediately.
> + * If the error was an exchange timeout retry immediately,
> + * otherwise wait for half a second
>   *
>   * Locking Note: The rport lock is expected to be held before
>   * calling this routine
>   */
> -static void fc_rport_error(struct fc_rport *rport, struct fc_frame *fp)
> +static void fc_rport_error(struct fc_rport *rport, struct fc_frame *fp,
> +			   int retry)
>  {
>  	struct fc_rport_libfc_priv *rdata = rport->dd_data;
> -	unsigned long delay = 0;
> +	unsigned long delay = msecs_to_jiffies(500);
>  
>  	FC_DEBUG_RPORT("Error %ld in state %s, retries %d\n",
>  		       PTR_ERR(fp), fc_rport_state(rport), rdata->retries);
>  
> -	if (!fp || PTR_ERR(fp) == -FC_EX_TIMEOUT) {
> -		/*
> -		 * Memory allocation failure, or the exchange timed out.
> -		 *  Retry after delay
> -		 */
> -		if (rdata->retries < rdata->local_port->max_retry_count) {
> -			rdata->retries++;
> -			if (!fp)
> -				delay = msecs_to_jiffies(500);
> -			get_device(&rport->dev);
> -			schedule_delayed_work(&rdata->retry_work, delay);
> -		} else {
> -			switch (rdata->rp_state) {
> -			case RPORT_ST_PLOGI:
> -			case RPORT_ST_PRLI:
> -			case RPORT_ST_LOGO:
> -				rdata->event = RPORT_EV_FAILED;
> -				queue_work(rport_event_queue,
> -					   &rdata->event_work);
> -				break;
> -			case RPORT_ST_RTV:
> -				fc_rport_enter_ready(rport);
> -				break;
> -			case RPORT_ST_NONE:
> -			case RPORT_ST_READY:
> -			case RPORT_ST_INIT:
> -				break;
> -			}
> +	if (retry && rdata->retries < rdata->local_port->max_retry_count) {
> +		rdata->retries++;
> +		if (PTR_ERR(fp) == -FC_EX_TIMEOUT)
> +			delay = 0;
> +		get_device(&rport->dev);
> +		schedule_delayed_work(&rdata->retry_work, delay);
> +	} else {
> +		switch (rdata->rp_state) {
> +		case RPORT_ST_PLOGI:
> +		case RPORT_ST_PRLI:
> +		case RPORT_ST_LOGO:
> +			rdata->event = RPORT_EV_FAILED;
> +			queue_work(rport_event_queue,
> +				   &rdata->event_work);
> +			break;
> +		case RPORT_ST_RTV:
> +			fc_rport_enter_ready(rport);
> +			break;
> +		case RPORT_ST_NONE:
> +		case RPORT_ST_READY:
> +		case RPORT_ST_INIT:
> +			break;
>  		}
>  	}
>  }
> @@ -490,7 +486,7 @@ static void fc_rport_plogi_resp(struct fc_seq *sp, struct fc_frame *fp,
>  	}
>  
>  	if (IS_ERR(fp)) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  		goto err;
>  	}
>  
> @@ -522,7 +518,7 @@ static void fc_rport_plogi_resp(struct fc_seq *sp, struct fc_frame *fp,
>  		else
>  			fc_rport_enter_prli(rport);
>  	} else
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  
>  out:
>  	fc_frame_free(fp);
> @@ -552,14 +548,14 @@ static void fc_rport_enter_plogi(struct fc_rport *rport)
>  	rport->maxframe_size = FC_MIN_MAX_PAYLOAD;
>  	fp = fc_frame_alloc(lport, sizeof(struct fc_els_flogi));
>  	if (!fp) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  		return;
>  	}
>  	rdata->e_d_tov = lport->e_d_tov;
>  
>  	if (!lport->tt.elsct_send(lport, rport, fp, ELS_PLOGI,
>  				  fc_rport_plogi_resp, rport, lport->e_d_tov))
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  	else
>  		get_device(&rport->dev);
>  }
> @@ -599,7 +595,7 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
>  	}
>  
>  	if (IS_ERR(fp)) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  		goto err;
>  	}
>  
> @@ -657,7 +653,7 @@ static void fc_rport_logo_resp(struct fc_seq *sp, struct fc_frame *fp,
>  		       rport->port_id);
>  
>  	if (IS_ERR(fp)) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  		goto err;
>  	}
>  
> @@ -707,13 +703,13 @@ static void fc_rport_enter_prli(struct fc_rport *rport)
>  
>  	fp = fc_frame_alloc(lport, sizeof(*pp));
>  	if (!fp) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  		return;
>  	}
>  
>  	if (!lport->tt.elsct_send(lport, rport, fp, ELS_PRLI,
>  				  fc_rport_prli_resp, rport, lport->e_d_tov))
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  	else
>  		get_device(&rport->dev);
>  }
> @@ -749,7 +745,7 @@ static void fc_rport_rtv_resp(struct fc_seq *sp, struct fc_frame *fp,
>  	}
>  
>  	if (IS_ERR(fp)) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, 0);
>  		goto err;
>  	}
>  
> @@ -804,13 +800,13 @@ static void fc_rport_enter_rtv(struct fc_rport *rport)
>  
>  	fp = fc_frame_alloc(lport, sizeof(struct fc_els_rtv));
>  	if (!fp) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  		return;
>  	}
>  
>  	if (!lport->tt.elsct_send(lport, rport, fp, ELS_RTV,
>  				     fc_rport_rtv_resp, rport, lport->e_d_tov))
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  	else
>  		get_device(&rport->dev);
>  }
> @@ -835,13 +831,13 @@ static void fc_rport_enter_logo(struct fc_rport *rport)
>  
>  	fp = fc_frame_alloc(lport, sizeof(struct fc_els_logo));
>  	if (!fp) {
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  		return;
>  	}
>  
>  	if (!lport->tt.elsct_send(lport, rport, fp, ELS_LOGO,
>  				  fc_rport_logo_resp, rport, lport->e_d_tov))
> -		fc_rport_error(rport, fp);
> +		fc_rport_error(rport, fp, FC_RPORT_ERR_RETRY);
>  	else
>  		get_device(&rport->dev);
>  }
> 
> _______________________________________________
> devel mailing list
> devel at open-fcoe.org
> http://www.open-fcoe.org/mailman/listinfo/devel




More information about the devel mailing list