Vasu Dev wrote:
> From: Joe Eykholt <[email protected]>
>
> This patch has several improvements to the code in
> the fip timers.
>
> The current code allows an advertisement to be used
> even if it has been 3 times the FCF keep-alive
> advertisement period (FKA) since one was received from
> that FCF. The spec. calls for 2.5 times FKA.
>
> Fix this and make sure we detect missed keep-alives promptly.
>
> Signed-off-by: Joe Eykholt <[email protected]>
>
> --
> Use fka_period as periodic timeouts to age out fcf if
> keep alives are disabled due to fd_flags set and also
> stop updating keep alive values in that case.
>
> Update select fcf time only if fcf is not already selected or
> select time is not already determined from parse adv, and then
> have select time cleared only once after fcf is selected.
>
> Changed deadline check to time_after_eq() from time_after()
> since now next timeout will be on exact 2.5 times FKA followed
> by first advertisement.
>
> Signed-off-by: Vasu Dev <[email protected]>
> ---
>
> drivers/scsi/fcoe/libfcoe.c | 89
> ++++++++++++++++++++++++-------------------
> 1 files changed, 50 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c
> index 1d5b949..6ac1f0e 100644
> --- a/drivers/scsi/fcoe/libfcoe.c
> +++ b/drivers/scsi/fcoe/libfcoe.c
> @@ -557,38 +557,48 @@ EXPORT_SYMBOL(fcoe_ctlr_els_send);
> *
> * Called with lock held and preemption disabled.
> *
> - * An FCF is considered old if we have missed three advertisements.
> - * That is, there have been no valid advertisement from it for three
> - * times its keep-alive period including fuzz.
> + * An FCF is considered old if we have missed two advertisements.
> + * That is, there have been no valid advertisement from it for 2.5
> + * times its keep-alive period.
> *
> * In addition, determine the time when an FCF selection can occur.
> *
> * Also, increment the MissDiscAdvCount when no advertisement is received
> * for the corresponding FCF for 1.5 * FKA_ADV_PERIOD (FC-BB-5 LESB).
> + *
> + * Returns the time in jiffies for the next call.
> */
> -static void fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
> +static unsigned long fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
> {
> struct fcoe_fcf *fcf;
> struct fcoe_fcf *next;
> + unsigned long next_timer = jiffies + msecs_to_jiffies(FIP_DEF_FKA);
> + unsigned long deadline;
> unsigned long sel_time = 0;
> - unsigned long mda_time = 0;
> struct fcoe_dev_stats *stats;
>
> list_for_each_entry_safe(fcf, next, &fip->fcfs, list) {
> - mda_time = fcf->fka_period + (fcf->fka_period >> 1);
> - if ((fip->sel_fcf == fcf) &&
> - (time_after(jiffies, fcf->time + mda_time))) {
> - mod_timer(&fip->timer, jiffies + mda_time);
> - stats = per_cpu_ptr(fip->lp->dev_stats,
> - smp_processor_id());
> - stats->MissDiscAdvCount++;
> - printk(KERN_INFO "libfcoe: host%d: Missing Discovery "
> - "Advertisement for fab %16.16llx count %lld\n",
> - fip->lp->host->host_no, fcf->fabric_name,
> - stats->MissDiscAdvCount);
> + deadline = fcf->time + fcf->fka_period + fcf->fka_period / 2;
> + if (fip->sel_fcf == fcf) {
> + if (fcf->fd_flags &&
> + time_after(next_timer, jiffies + fcf->fka_period))
> + next_timer = jiffies + fcf->fka_period;
The fd_flags should be in the outer if, since that means we shouldn't
be looking for missing advertisements at all. Otherwise we'll set
next_timer to the deadline.
if (fip->sel_fcf == fcf && !fcf->fd_flags) {
if (time_after ...
> +
> + if (time_after(jiffies, deadline)) {
> + stats = per_cpu_ptr(fip->lp->dev_stats,
> + smp_processor_id());
> + stats->MissDiscAdvCount++;
> + printk(KERN_INFO "libfcoe: host%d: "
> + "Missing Discovery Advertisement "
> + "for fab %16.16llx count %lld\n",
> + fip->lp->host->host_no, fcf->fabric_name,
> + stats->MissDiscAdvCount);
> + } else if (time_after(next_timer, deadline))
> + next_timer = deadline;
> }
> - if (time_after(jiffies, fcf->time + fcf->fka_period * 3 +
> - msecs_to_jiffies(FIP_FCF_FUZZ * 3))) {
> +
> + deadline += fcf->fka_period;
> + if (time_after_eq(jiffies, deadline)) {
This is a pre-existing bug, but the above also should check
fcf->fd_flags around both the if and else so
if (!fcf->fd_flags) {
if (time_after_eq(...)) {
...
delete the fcf
continue; // so we don't check sel_time on this
one.
}
if (time_after(next_timer, deadline))
next_timer = deadline;
}
if (fcoe_ctlr_mtu_valid(fcf) && ...
This rearranges things a bit. I hope you see what I mean.
Let me know if you want to split that out into a separate
patch or want me to update this patch.
> if (fip->sel_fcf == fcf)
> fip->sel_fcf = NULL;
> list_del(&fcf->list);
> @@ -598,19 +608,20 @@ static void fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
> stats = per_cpu_ptr(fip->lp->dev_stats,
> smp_processor_id());
> stats->VLinkFailureCount++;
> - } else if (fcoe_ctlr_mtu_valid(fcf) &&
> - (!sel_time || time_before(sel_time, fcf->time))) {
> - sel_time = fcf->time;
> + } else {
> + if (time_after(next_timer, deadline))
> + next_timer = deadline;
> + if (fcoe_ctlr_mtu_valid(fcf) &&
> + (!sel_time || time_before(sel_time, fcf->time)))
> + sel_time = fcf->time;
> }
> }
> - if (sel_time) {
> + if (!(fip->sel_fcf || fip->sel_time)) {
Shouldn't this be:
if (sel_time && !fip->sel_fcf) {
sel_time non-zero indicates that one of the FCFs in the loop
> sel_time += msecs_to_jiffies(FCOE_CTLR_START_DELAY);
> fip->sel_time = sel_time;
> - if (time_before(sel_time, fip->timer.expires))
> - mod_timer(&fip->timer, sel_time);
> - } else {
> - fip->sel_time = 0;
> }
> +
> + return next_timer;
> }
>
> /**
> @@ -767,7 +778,7 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip,
> struct sk_buff *skb)
> * ignored after a usable solicited advertisement
> * has been received.
> */
> - if (fcf == fip->sel_fcf) {
> + if (fcf == fip->sel_fcf && !fcf->fd_flags) {
> fip->ctlr_ka_time -= fcf->fka_period;
> fip->ctlr_ka_time += new.fka_period;
> if (time_before(fip->ctlr_ka_time, fip->timer.expires))
> @@ -805,7 +816,7 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip,
> struct sk_buff *skb)
> * If this is the first validated FCF, note the time and
> * set a timer to trigger selection.
> */
> - if (mtu_valid && !fip->sel_time && fcoe_ctlr_fcf_usable(fcf)) {
> + if (mtu_valid && !fip->sel_fcf && fcoe_ctlr_fcf_usable(fcf)) {
> fip->sel_time = jiffies +
> msecs_to_jiffies(FCOE_CTLR_START_DELAY);
> if (!timer_pending(&fip->timer) ||
> @@ -1148,7 +1159,7 @@ static void fcoe_ctlr_timeout(unsigned long arg)
> struct fcoe_ctlr *fip = (struct fcoe_ctlr *)arg;
> struct fcoe_fcf *sel;
> struct fcoe_fcf *fcf;
> - unsigned long next_timer = jiffies + msecs_to_jiffies(FIP_VN_KA_PERIOD);
> + unsigned long next_timer;
>
> spin_lock_bh(&fip->lock);
> if (fip->state == FIP_ST_DISABLED) {
> @@ -1157,13 +1168,16 @@ static void fcoe_ctlr_timeout(unsigned long arg)
> }
>
> fcf = fip->sel_fcf;
> - fcoe_ctlr_age_fcfs(fip);
> + next_timer = fcoe_ctlr_age_fcfs(fip);
>
> sel = fip->sel_fcf;
> - if (!sel && fip->sel_time && time_after_eq(jiffies, fip->sel_time)) {
> - fcoe_ctlr_select(fip);
> - sel = fip->sel_fcf;
> - fip->sel_time = 0;
> + if (!sel && fip->sel_time) {
> + if (time_after_eq(jiffies, fip->sel_time)) {
> + fcoe_ctlr_select(fip);
> + sel = fip->sel_fcf;
> + fip->sel_time = 0;
> + } else if (time_after(next_timer, fip->sel_time))
> + next_timer = fip->sel_time;
> }
>
> if (sel != fcf) {
> @@ -1201,12 +1215,9 @@ static void fcoe_ctlr_timeout(unsigned long arg)
> }
> if (time_after(next_timer, fip->port_ka_time))
> next_timer = fip->port_ka_time;
> - mod_timer(&fip->timer, next_timer);
> - } else if (fip->sel_time) {
> - next_timer = fip->sel_time +
> - msecs_to_jiffies(FCOE_CTLR_START_DELAY);
> - mod_timer(&fip->timer, next_timer);
> }
> + if (!list_empty(&fip->fcfs))
> + mod_timer(&fip->timer, next_timer);
> if (fip->send_ctlr_ka || fip->send_port_ka)
> schedule_work(&fip->timer_work);
> spin_unlock_bh(&fip->lock);
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://www.open-fcoe.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
