Mike Christie wrote:
Mike Christie wrote:
Dev, Vasu wrote:
-----Original Message-----
From: Mike Christie [mailto:[EMAIL PROTECTED]
So any idea why system locks up with your 14 patches?
Weird with the 14 patches it works almost perfect for me, but now it
does not work for you guys :)
It was not system lock-up instead my ssh session hanged after your these
14 patches with error test. I mostly work in ssh session and I thought
my system hanged without any additional log prints in my netconsole
window.
So I went ahead with power re-cycling system assuming system locked up.
This time I observed console was still alive for sometime after error
test
and I could do some command. However new ssh request failed with these
error persistently.
I found a lock up with the handle-vasu-comments.patch patch. Without
the patch we could call the resp function twice and bad things could
happen,
Without the handle-vasu-comments.patch patch, if fc_fcp.c sends an
abort, fc_seq_exch_abort sets the timer on the ep. If we get a abts
response at the same time fc_exch_timeout is starting to run,
fc_exch_abts_resp could get the lock first, do its work then drop the
ex_lock and call resp. Then fc_exch_timeout could grab the lock and call
the resp function (I guess fc_fcp_error could get FC_EX_TIMEOUT because
the abort code had set the timer on the ep and fc_exch_timeout was
returning FC_EX_TIMEOUT on the abort). If the first call freed the fsp
and ep, then when the second call to resp grabbed the scsi pkt lock who
knows what could happen.
With the handle-vasu-comments.patch I forgot to drop the ex_lock when I
exited fc_exch_timeout, so it will lock up the box if you are unlucky.
If you are lucky I think we can drubdge on for a while depending on what
else timesout and the processor setup and what else happens.
I rolled up the changes in the last two patches
handle-vasu-comments.patch
clear-complete-during-reset.patch
and fixed the issue above in handle-vasu-comments.patch in the attached
patch.
This patch should apply over the original 14 patches with some offsets.
I made the patch over some other patches, but the offsets should be ok.
Actually :)... forget that patch. Attached is a new one
fix-my-f-ups2.patch that fixes more possible bugs. I also stopped
abusing the ESB bits and defined internal state bits.
The bugs:
1. If fc_exch_rrq_resp is called because mgr reset is run, then we will
not drop the hold taken when the recov qual was found in the abts
response. The check for the qual in fc_exch_reset only handles if the
timer function has not sent the rrq yet. I added a ref to the aborted_ep
to the ep so we could make sure this gets cleaned up.
2. If fc_exch_rrq's call to fc_exch_seq_send failed, then it looks like
we want to retry the rrq, so we need to reset the ESB_ST_REC_QUAL bit.
3. If the timer function is sending the rrq, the ep for the rrq could
already be on the ex_list. If fc_exch_reset runs and sees it on the
list, then it would call fc_exch_done_locked (releasing the hold from
the timer functions alloc) on the ep while the timer function was still
accessing the ep. fc_exch_reset could then exit and we would release
fc_exch_mgr_reset's hold which would free the ep while the timer
function was still using it.
We really want to make sure the ep that we are sending the rrq for is
cleaned up first and that we use del_timer_sync to make sure the timer
fucntion is done accessing the rrq's ep. We cannot use del_timer_sync
because the lport lock is held in some cases. Do we need to have that
lock held?
I tried to close some of the races by setting FC_EX_RST_CLEANUP and then
checking for that in fc_exch_timeout and fc_exch_timer_set_locked so
would not at least start a new timer referencing bad memory.
It seems like we may need a more generic fix to this problem where the
fc_exch_mgr_reset can free a ep while someone is sending a sequence for
it. For fc_fcp.c we are safe because fc_fcp.c holds the scsi pkt lock
when we send IO and in the resp handler, but I am not sure about other
places. Are there more like with the rrq path?
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index a4d3901..6fd8030 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -60,6 +60,11 @@ struct fc_seq {
u32 rec_data; /* FC-4 value for REC */
};
+struct fc_exch;
+
+#define FC_EX_DONE (1 << 0) /* ep is completed */
+#define FC_EX_RST_CLEANUP (1 << 1) /* reset is forcing completion */
+
/*
* Exchange.
*
@@ -70,6 +75,7 @@ struct fc_seq {
*/
struct fc_exch {
struct fc_exch_mgr *em; /* exchange manager */
+ u32 state; /* internal driver state */
u16 xid; /* our exchange ID */
struct list_head ex_list; /* free or busy list linkage */
spinlock_t ex_lock; /* lock covering exchange state */
@@ -88,6 +94,7 @@ struct fc_exch {
u8 fh_type; /* frame type */
enum fc_class class; /* class of service */
struct fc_seq seq; /* single sequence */
+ struct fc_exch *aborted_ep; /* ref to ep rrq is cleaning up */
/*
* Handler for responses to this current exchange.
@@ -303,6 +310,9 @@ static void fc_exch_release(struct fc_exch *ep)
static inline void fc_exch_timer_set_locked(struct fc_exch *ep,
unsigned int timer_msec)
{
+ if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE))
+ return;
+
if (!mod_timer(&ep->ex_timer, jiffies + msecs_to_jiffies(timer_msec)))
fc_exch_hold(ep); /* hold for timer */
}
@@ -336,7 +346,8 @@ int fc_seq_exch_abort(const struct fc_seq *req_sp)
ep = fc_seq_exch(req_sp);
spin_lock_bh(&ep->ex_lock);
- if (ep->esb_stat & (ESB_ST_COMPLETE | ESB_ST_ABNORMAL)) {
+ if (ep->esb_stat & (ESB_ST_COMPLETE | ESB_ST_ABNORMAL) ||
+ ep->state & (FC_EX_DONE | FC_EX_RST_CLEANUP)) {
spin_unlock_bh(&ep->ex_lock);
return -ENXIO;
}
@@ -388,13 +399,17 @@ static void fc_exch_timeout(unsigned long ep_arg)
u32 e_stat;
spin_lock_bh(&ep->ex_lock);
+ if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE))
+ goto unlock;
+
e_stat = ep->esb_stat;
if (e_stat & ESB_ST_COMPLETE) {
- ep->esb_stat = e_stat & ~(ESB_ST_REC_QUAL | ESB_ST_COMPLETE);
+ ep->esb_stat = e_stat & ~ESB_ST_REC_QUAL;
spin_unlock_bh(&ep->ex_lock);
if (e_stat & ESB_ST_REC_QUAL)
fc_exch_rrq(ep);
- } else {
+ goto done;
+ } else if (!(e_stat & ESB_ST_ABNORMAL)) {
resp = ep->resp;
arg = ep->resp_arg;
/*
@@ -407,8 +422,11 @@ static void fc_exch_timeout(unsigned long ep_arg)
if (resp)
resp(sp, ERR_PTR(-FC_EX_TIMEOUT), arg);
fc_seq_exch_abort(sp);
+ goto done;
}
-
+unlock:
+ spin_unlock_bh(&ep->ex_lock);
+done:
/*
* This release matches the hold taken when the timer was set.
*/
@@ -566,11 +584,12 @@ static int fc_exch_done_locked(struct fc_exch *ep)
* complete, so it can be reused by the timer to send the rrq.
*/
ep->resp = NULL;
- if (ep->esb_stat & ESB_ST_COMPLETE)
+ if (ep->state & FC_EX_DONE)
return rc;
ep->esb_stat |= ESB_ST_COMPLETE;
if (!(ep->esb_stat & ESB_ST_REC_QUAL)) {
+ ep->state |= FC_EX_DONE;
if (del_timer(&ep->ex_timer))
atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
atomic_dec(&ep->ex_refcnt); /* drop hold from alloc */
@@ -1271,12 +1290,16 @@ static void fc_exch_abts_resp(struct fc_exch *ep,
struct fc_frame *fp)
struct fc_seq *sp;
u16 low;
u16 high;
- int rc = 1;
+ int rc = 1, has_rec = 0;
fh = fc_frame_header_get(fp);
if (fc_exch_debug)
FC_DBG("exch: BLS rctl %x - %s\n",
fh->fh_r_ctl, fc_exch_rctl_name(fh->fh_r_ctl));
+
+ if (del_timer_sync(&ep->ex_timer))
+ fc_exch_release(ep); /* release from pending timer hold */
+
spin_lock_bh(&ep->ex_lock);
switch (fh->fh_r_ctl) {
case FC_RCTL_BA_ACC:
@@ -1296,7 +1319,7 @@ static void fc_exch_abts_resp(struct fc_exch *ep, struct
fc_frame *fp)
ap->ba_seq_id == ep->seq_id) && low != high) {
ep->esb_stat |= ESB_ST_REC_QUAL;
fc_exch_hold(ep); /* hold for recovery qualifier */
- fc_exch_timer_set_locked(ep, ep->r_a_tov);
+ has_rec = 1;
}
break;
case FC_RCTL_BA_RJT:
@@ -1324,6 +1347,8 @@ static void fc_exch_abts_resp(struct fc_exch *ep, struct
fc_frame *fp)
if (resp)
resp(sp, fp, ex_resp_arg);
+ if (has_rec)
+ fc_exch_timer_set(ep, ep->r_a_tov);
fc_frame_free(fp);
}
@@ -1440,22 +1465,22 @@ static void fc_exch_reset(struct fc_exch *ep)
void *arg;
int rc = 1;
+ spin_lock_bh(&ep->ex_lock);
+ ep->state |= FC_EX_RST_CLEANUP;
/*
- * TODO: we should use del_timer_sync. If this is called for
- * shutdown, and the driver will unload after this exits, we
- * must not have any timers running. I do not think we can
- * call del_timer_sync here though, because it looks like it
- * is called with a port spin lock held.
+ * we really want to call del_timer_sync, but cannot due
+ * to the lport calling with the lport lock held (some resp
+ * functions can also grab the lport lock which could cause
+ * a deadlock).
*/
- spin_lock_bh(&ep->ex_lock);
+ if (del_timer(&ep->ex_timer))
+ atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
resp = ep->resp;
ep->resp = NULL;
if (ep->esb_stat & ESB_ST_REC_QUAL)
atomic_dec(&ep->ex_refcnt); /* drop hold for rec_qual */
ep->esb_stat &= ~ESB_ST_REC_QUAL;
arg = ep->resp_arg;
- if (del_timer(&ep->ex_timer))
- atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
sp = &ep->seq;
if (ep->fh_type != FC_TYPE_FCP)
@@ -1610,30 +1635,45 @@ reject:
*/
static void fc_exch_rrq_resp(struct fc_seq *sp, struct fc_frame *fp, void *arg)
{
- struct fc_exch *aborted_ep = arg;
+ struct fc_exch *ep = fc_seq_exch(sp);
+ struct fc_exch *aborted_ep;
+
unsigned int op;
if (IS_ERR(fp)) {
int err = PTR_ERR(fp);
+ if (err == -FC_EX_CLOSED)
+ goto cleanup;
FC_DBG("Cannot process RRQ, because of frame error %d\n", err);
return;
}
op = fc_frame_payload_op(fp);
+ fc_frame_free(fp);
+
switch (op) {
case ELS_LS_RJT:
FC_DBG("LS_RJT for RRQ");
- break;
+ /* fall through */
case ELS_LS_ACC:
- fc_exch_done(&aborted_ep->seq);
- fc_exch_release(aborted_ep); /* drop hold for rec qual */
- break;
+ goto cleanup;
default:
FC_DBG("unexpected response op %x for RRQ", op);
+ return;
}
- fc_frame_free(fp);
+cleanup:
+ spin_lock_bh(&ep->ex_lock);
+ aborted_ep = ep->aborted_ep;
+ ep->aborted_ep = NULL;
+ spin_unlock_bh(&ep->ex_lock);
+
+ if (aborted_ep) {
+ fc_exch_done(&aborted_ep->seq);
+ /* drop hold for rec qual */
+ fc_exch_release(aborted_ep);
+ }
}
/*
@@ -1646,6 +1686,8 @@ static void fc_exch_rrq(struct fc_exch *ep)
struct fc_lport *lp;
struct fc_els_rrq *rrq;
struct fc_frame *fp;
+ struct fc_seq *rrq_sp;
+ struct fc_exch *rrq_ep;
u32 did;
lp = ep->lp;
@@ -1664,11 +1706,21 @@ static void fc_exch_rrq(struct fc_exch *ep)
did = ep->did;
if (ep->esb_stat & ESB_ST_RESP)
did = ep->sid;
- if (!fc_exch_seq_send(lp, fp, fc_exch_rrq_resp, ep, lp->e_d_tov,
- lp->fid, did, FC_FC_SEQ_INIT | FC_FC_END_SEQ))
- fc_exch_timer_set(ep, ep->r_a_tov);
+ rrq_sp = fc_exch_seq_send(lp, fp, fc_exch_rrq_resp, ep, lp->e_d_tov,
+ lp->fid, did, FC_FC_SEQ_INIT | FC_FC_END_SEQ);
+ if (!rrq_sp) {
+ spin_lock_bh(&ep->ex_lock);
+ ep->esb_stat |= ESB_ST_REC_QUAL;
+ fc_exch_timer_set_locked(ep, ep->r_a_tov);
+ spin_unlock_bh(&ep->ex_lock);
+ return;
+ }
+
+ rrq_ep = fc_seq_exch(rrq_sp);
+ rrq_ep->aborted_ep = ep;
}
+
/*
* Handle incoming ELS RRQ - Reset Recovery Qualifier.
*/
diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index c519b43..2ca2946 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -1022,16 +1022,9 @@ static void fc_fcp_error(struct fc_fcp_pkt *fsp, struct
fc_frame *fp)
case -FC_EX_CLOSED:
fc_fcp_retry_cmd(fsp);
goto unlock;
- case -FC_EX_TIMEOUT:
- /*
- * exch layer decided to abort exchange -
- * will wait for response
- */
- fsp->state |= FC_SRB_ABORT_PENDING;
- goto unlock;
+ default:
+ FC_DBG("unknown error %ld\n", PTR_ERR(fp));
}
-
- FC_DBG("unknown error %ld\n", PTR_ERR(fp));
/*
* clear abort pending, because the lower layer
* decided to force completion.
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
