[PATCH 1/7] aoe: improve handling of misbehaving network paths

[PATCH 1/7] aoe: improve handling of misbehaving network paths

[Ed Cashin, Ed Cashin]

An AoE target can have multiple network ports used for AoE, and
in the aoe driver, those are tracked by the aoetgt struct.  These
changes allow the aoe driver to handle network paths, or aoetgts,
that are not working well, compared to the others.

Paths that do not get responses despite the retransmission of AoE
commands are marked as "tainted", and non-tainted paths are
preferred.

Meanwhile, the aoe driver attempts to "probe" the tainted path in
the background by issuing reads of LBA 0 that are padded out to
full (possibly jumbo-frame) size.  If the probes get responses,
then the path is "redeemed", and its taint is removed.

This mechanism has been shown to be helpful in transparently
handling and recovering from real-world network "brown outs" in
ways that the earlier "shoot the help-needing target in the head"
mechanism could not.

Signed-off-by: Ed Cashin <ecashin@coraid.com>
---
 drivers/block/aoe/aoe.h    |   11 ++-
 drivers/block/aoe/aoecmd.c |  377 ++++++++++++++++++++++++++++++--------------
 drivers/block/aoe/aoedev.c |    1 -
 3 files changed, 268 insertions(+), 121 deletions(-)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index bfd765c..b6d2b16 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -91,6 +91,9 @@ enum {
 	RTTDSCALE = 3,
 	RTTAVG_INIT = USEC_PER_SEC / 4 << RTTSCALE,
 	RTTDEV_INIT = RTTAVG_INIT / 4,
+
+	HARD_SCORN_SECS = 10,	/* try another remote port after this */
+	MAX_TAINT = 1000,	/* cap on aoetgt taint */
 };
 
 struct buf {
@@ -103,6 +106,10 @@ struct buf {
 	struct request *rq;
 };
 
+enum frame_flags {
+	FFL_PROBE = 1,
+};
+
 struct frame {
 	struct list_head head;
 	u32 tag;
@@ -118,6 +125,7 @@ struct frame {
 	struct bio_vec *bv;
 	ulong bcnt;
 	ulong bv_off;
+	char flags;
 };
 
 struct aoeif {
@@ -138,8 +146,10 @@ struct aoetgt {
 	ushort next_cwnd;	/* incr maxout after decrementing to zero */
 	ushort ssthresh;	/* slow start threshold */
 	ulong falloc;		/* number of allocated frames */
+	int taint;		/* how much we want to avoid this aoetgt */
 	int minbcnt;
 	int wpkts, rpkts;
+	char nout_probes;
 };
 
 struct aoedev {
@@ -174,7 +184,6 @@ struct aoedev {
 	struct list_head rexmitq; /* deferred retransmissions */
 	struct aoetgt *targets[NTARGETS];
 	struct aoetgt **tgt;	/* target in use when working */
-	struct aoetgt *htgt;	/* target needing rexmit assistance */
 	ulong ntargets;
 	ulong kicked;
 	char ident[512];
diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index 391dd8e..000f7fb 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -22,6 +22,7 @@
 #define MAXIOC (8192)	/* default meant to avoid most soft lockups */
 
 static void ktcomplete(struct frame *, struct sk_buff *);
+static int count_targets(struct aoedev *d, int *untainted);
 
 static struct buf *nextbuf(struct aoedev *);
 
@@ -43,6 +44,8 @@ static struct {
 	spinlock_t lock;
 } iocq;
 
+static struct page *empty_page;
+
 static struct sk_buff *
 new_skb(ulong len)
 {
@@ -179,8 +182,10 @@ aoe_freetframe(struct frame *f)
 
 	t = f->t;
 	f->buf = NULL;
+	f->lba = 0;
 	f->bv = NULL;
 	f->r_skb = NULL;
+	f->flags = 0;
 	list_add(&f->head, &t->ffree);
 }
 
@@ -234,20 +239,25 @@ newframe(struct aoedev *d)
 	struct frame *f;
 	struct aoetgt *t, **tt;
 	int totout = 0;
+	int use_tainted;
+	int has_untainted;
 
 	if (d->targets[0] == NULL) {	/* shouldn't happen, but I'm paranoid */
 		printk(KERN_ERR "aoe: NULL TARGETS!\n");
 		return NULL;
 	}
 	tt = d->tgt;	/* last used target */
-	for (;;) {
+	for (use_tainted = 0, has_untainted = 0;;) {
 		tt++;
 		if (tt >= &d->targets[NTARGETS] || !*tt)
 			tt = d->targets;
 		t = *tt;
-		totout += t->nout;
+		if (!t->taint) {
+			has_untainted = 1;
+			totout += t->nout;
+		}
 		if (t->nout < t->maxout
-		&& t != d->htgt
+		&& (use_tainted || !t->taint)
 		&& t->ifp->nd) {
 			f = newtframe(d, t);
 			if (f) {
@@ -256,8 +266,12 @@ newframe(struct aoedev *d)
 				return f;
 			}
 		}
-		if (tt == d->tgt)	/* we've looped and found nada */
-			break;
+		if (tt == d->tgt) {	/* we've looped and found nada */
+			if (!use_tainted && !has_untainted)
+				use_tainted = 1;
+			else
+				break;
+		}
 	}
 	if (totout == 0) {
 		d->kicked++;
@@ -294,21 +308,68 @@ fhash(struct frame *f)
 	list_add_tail(&f->head, &d->factive[n]);
 }
 
+static void
+ata_rw_frameinit(struct frame *f)
+{
+	struct aoetgt *t;
+	struct aoe_hdr *h;
+	struct aoe_atahdr *ah;
+	struct sk_buff *skb;
+	char writebit, extbit;
+
+	skb = f->skb;
+	h = (struct aoe_hdr *) skb_mac_header(skb);
+	ah = (struct aoe_atahdr *) (h + 1);
+	skb_put(skb, sizeof(*h) + sizeof(*ah));
+	memset(h, 0, skb->len);
+
+	writebit = 0x10;
+	extbit = 0x4;
+
+	t = f->t;
+	f->tag = aoehdr_atainit(t->d, t, h);
+	fhash(f);
+	t->nout++;
+	f->waited = 0;
+	f->waited_total = 0;
+	if (f->buf)
+		f->lba = f->buf->sector;
+
+	/* set up ata header */
+	ah->scnt = f->bcnt >> 9;
+	put_lba(ah, f->lba);
+	if (t->d->flags & DEVFL_EXT) {
+		ah->aflags |= AOEAFL_EXT;
+	} else {
+		extbit = 0;
+		ah->lba3 &= 0x0f;
+		ah->lba3 |= 0xe0;	/* LBA bit + obsolete 0xa0 */
+	}
+	if (f->buf && bio_data_dir(f->buf->bio) == WRITE) {
+		skb_fillup(skb, f->bv, f->bv_off, f->bcnt);
+		ah->aflags |= AOEAFL_WRITE;
+		skb->len += f->bcnt;
+		skb->data_len = f->bcnt;
+		skb->truesize += f->bcnt;
+		t->wpkts++;
+	} else {
+		t->rpkts++;
+		writebit = 0;
+	}
+
+	ah->cmdstat = ATA_CMD_PIO_READ | writebit | extbit;
+	skb->dev = t->ifp->nd;
+}
+
 static int
 aoecmd_ata_rw(struct aoedev *d)
 {
 	struct frame *f;
-	struct aoe_hdr *h;
-	struct aoe_atahdr *ah;
 	struct buf *buf;
 	struct aoetgt *t;
 	struct sk_buff *skb;
 	struct sk_buff_head queue;
 	ulong bcnt, fbcnt;
-	char writebit, extbit;
-
-	writebit = 0x10;
-	extbit = 0x4;
 
 	buf = nextbuf(d);
 	if (buf == NULL)
@@ -343,50 +404,15 @@ aoecmd_ata_rw(struct aoedev *d)
 	} while (fbcnt);
 
 	/* initialize the headers & frame */
-	skb = f->skb;
-	h = (struct aoe_hdr *) skb_mac_header(skb);
-	ah = (struct aoe_atahdr *) (h+1);
-	skb_put(skb, sizeof *h + sizeof *ah);
-	memset(h, 0, skb->len);
-	f->tag = aoehdr_atainit(d, t, h);
-	fhash(f);
-	t->nout++;
-	f->waited = 0;
-	f->waited_total = 0;
 	f->buf = buf;
 	f->bcnt = bcnt;
-	f->lba = buf->sector;
-
-	/* set up ata header */
-	ah->scnt = bcnt >> 9;
-	put_lba(ah, buf->sector);
-	if (d->flags & DEVFL_EXT) {
-		ah->aflags |= AOEAFL_EXT;
-	} else {
-		extbit = 0;
-		ah->lba3 &= 0x0f;
-		ah->lba3 |= 0xe0;	/* LBA bit + obsolete 0xa0 */
-	}
-	if (bio_data_dir(buf->bio) == WRITE) {
-		skb_fillup(skb, f->bv, f->bv_off, bcnt);
-		ah->aflags |= AOEAFL_WRITE;
-		skb->len += bcnt;
-		skb->data_len = bcnt;
-		skb->truesize += bcnt;
-		t->wpkts++;
-	} else {
-		t->rpkts++;
-		writebit = 0;
-	}
-
-	ah->cmdstat = ATA_CMD_PIO_READ | writebit | extbit;
+	ata_rw_frameinit(f);
 
 	/* mark all tracking fields and load out */
 	buf->nframesout += 1;
 	buf->sector += bcnt >> 9;
 
-	skb->dev = t->ifp->nd;
-	skb = skb_clone(skb, GFP_ATOMIC);
+	skb = skb_clone(f->skb, GFP_ATOMIC);
 	if (skb) {
 		do_gettimeofday(&f->sent);
 		f->sent_jiffs = (u32) jiffies;
@@ -462,11 +488,14 @@ resend(struct aoedev *d, struct frame *f)
 	h = (struct aoe_hdr *) skb_mac_header(skb);
 	ah = (struct aoe_atahdr *) (h+1);
 
-	snprintf(buf, sizeof buf,
-		"%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm nout=%d\n",
-		"retransmit", d->aoemajor, d->aoeminor, f->tag, jiffies, n,
-		h->src, h->dst, t->nout);
-	aoechr_error(buf);
+	if (!(f->flags & FFL_PROBE)) {
+		snprintf(buf, sizeof(buf),
+			"%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm nout=%d\n",
+			"retransmit", d->aoemajor, d->aoeminor,
+			f->tag, jiffies, n,
+			h->src, h->dst, t->nout);
+		aoechr_error(buf);
+	}
 
 	f->tag = n;
 	fhash(f);
@@ -558,18 +587,18 @@ ejectif(struct aoetgt *t, struct aoeif *ifp)
 }
 
 static struct frame *
-reassign_frame(struct list_head *pos)
+reassign_frame(struct frame *f)
 {
-	struct frame *f;
 	struct frame *nf;
 	struct sk_buff *skb;
 
-	f = list_entry(pos, struct frame, head);
 	nf = newframe(f->t->d);
 	if (!nf)
 		return NULL;
-
-	list_del(pos);
+	if (nf->t == f->t) {
+		aoe_freetframe(nf);
+		return NULL;
+	}
 
 	skb = nf->skb;
 	nf->skb = f->skb;
@@ -583,52 +612,67 @@ reassign_frame(struct list_head *pos)
 	nf->sent = f->sent;
 	nf->sent_jiffs = f->sent_jiffs;
 	f->skb = skb;
-	aoe_freetframe(f);
-	f->t->nout--;
-	nf->t->nout++;
 
 	return nf;
 }
 
-static int
-sthtith(struct aoedev *d)
+static void
+probe(struct aoetgt *t)
 {
-	struct frame *f, *nf;
-	struct list_head *nx, *pos, *head;
-	struct aoetgt *ht = d->htgt;
-	int i;
+	struct aoedev *d;
+	struct frame *f;
+	struct sk_buff *skb;
+	struct sk_buff_head queue;
+	size_t n, m;
+	int frag;
 
-	/* look through the active and pending retransmit frames */
-	for (i = 0; i < NFACTIVE; i++) {
-		head = &d->factive[i];
-		list_for_each_safe(pos, nx, head) {
-			f = list_entry(pos, struct frame, head);
-			if (f->t != ht)
-				continue;
-			nf = reassign_frame(pos);
-			if (!nf)
-				return 0;
-			resend(d, nf);
-		}
+	d = t->d;
+	f = newtframe(d, t);
+	if (!f) {
+		pr_err("%s %pm for e%ld.%d: %s\n",
+			"aoe: cannot probe remote address",
+			t->addr,
+			(long) d->aoemajor, d->aoeminor,
+			"no frame available");
+		return;
 	}
-	head = &d->rexmitq;
-	list_for_each_safe(pos, nx, head) {
-		f = list_entry(pos, struct frame, head);
-		if (f->t != ht)
-			continue;
-		nf = reassign_frame(pos);
-		if (!nf)
-			return 0;
-		resend(d, nf);
+	f->flags |= FFL_PROBE;
+	ifrotate(t);
+	f->bcnt = t->d->maxbcnt ? t->d->maxbcnt : DEFAULTBCNT;
+	ata_rw_frameinit(f);
+	skb = f->skb;
+	for (frag = 0, n = f->bcnt; n > 0; ++frag, n -= m) {
+		if (n < PAGE_SIZE)
+			m = n;
+		else
+			m = PAGE_SIZE;
+		skb_fill_page_desc(skb, frag, empty_page, 0, m);
 	}
-	/* We've cleaned up the outstanding so take away his
-	 * interfaces so he won't be used.  We should remove him from
-	 * the target array here, but cleaning up a target is
-	 * involved.  PUNT!
-	 */
-	memset(ht->ifs, 0, sizeof ht->ifs);
-	d->htgt = NULL;
-	return 1;
+	skb->len += f->bcnt;
+	skb->data_len = f->bcnt;
+	skb->truesize += f->bcnt;
+
+	skb = skb_clone(f->skb, GFP_ATOMIC);
+	if (skb) {
+		do_gettimeofday(&f->sent);
+		f->sent_jiffs = (u32) jiffies;
+		__skb_queue_head_init(&queue);
+		__skb_queue_tail(&queue, skb);
+		aoenet_xmit(&queue);
+	}
+}
+
+static long
+rto(struct aoedev *d)
+{
+	long t;
+
+	t = 2 * d->rttavg >> RTTSCALE;
+	t += 8 * d->rttdev >> RTTDSCALE;
+	if (t == 0)
+		t = 1;
+
+	return t;
 }
 
 static void
@@ -636,17 +680,53 @@ rexmit_deferred(struct aoedev *d)
 {
 	struct aoetgt *t;
 	struct frame *f;
+	struct frame *nf;
 	struct list_head *pos, *nx, *head;
 	int since;
+	int untainted;
+
+	count_targets(d, &untainted);
 
 	head = &d->rexmitq;
 	list_for_each_safe(pos, nx, head) {
 		f = list_entry(pos, struct frame, head);
 		t = f->t;
+		if (t->taint) {
+			if (!(f->flags & FFL_PROBE)) {
+				nf = reassign_frame(f);
+				if (nf) {
+					if (t->nout_probes == 0
+					&& untainted > 0) {
+						probe(t);
+						t->nout_probes++;
+					}
+					list_replace(&f->head, &nf->head);
+					pos = &nf->head;
+					aoe_freetframe(f);
+					f = nf;
+					t = f->t;
+				}
+			} else if (untainted < 1) {
+				/* don't probe w/o other untainted aoetgts */
+				goto stop_probe;
+			} else if (tsince_hr(f) < t->taint * rto(d)) {
+				/* reprobe slowly when taint is high */
+				continue;
+			}
+		} else if (f->flags & FFL_PROBE) {
+stop_probe:		/* don't probe untainted aoetgts */
+			list_del(pos);
+			aoe_freetframe(f);
+			/* leaving d->kicked, because this is routine */
+			f->t->d->flags |= DEVFL_KICKME;
+			continue;
+		}
 		if (t->nout >= t->maxout)
 			continue;
 		list_del(pos);
 		t->nout++;
+		if (f->flags & FFL_PROBE)
+			t->nout_probes++;
 		since = tsince_hr(f);
 		f->waited += since;
 		f->waited_total += since;
@@ -654,6 +734,36 @@ rexmit_deferred(struct aoedev *d)
 	}
 }
 
+/* An aoetgt accumulates demerits quickly, and successful
+ * probing redeems the aoetgt slowly.
+ */
+static void
+scorn(struct aoetgt *t)
+{
+	int n;
+
+	n = t->taint++;
+	t->taint += t->taint * 2;
+	if (n > t->taint)
+		t->taint = n;
+	if (t->taint > MAX_TAINT)
+		t->taint = MAX_TAINT;
+}
+
+static int
+count_targets(struct aoedev *d, int *untainted)
+{
+	int i, good;
+
+	for (i = good = 0; i < d->ntargets && d->targets[i]; ++i)
+		if (d->targets[i]->taint == 0)
+			good++;
+
+	if (untainted)
+		*untainted = good;
+	return i;
+}
+
 static void
 rexmit_timer(ulong vp)
 {
@@ -666,6 +776,7 @@ rexmit_timer(ulong vp)
 	register long timeout;
 	ulong flags, n;
 	int i;
+	int utgts;	/* number of aoetgt descriptors (not slots) */
 	int since;
 
 	d = (struct aoedev *) vp;
@@ -673,10 +784,9 @@ rexmit_timer(ulong vp)
 	spin_lock_irqsave(&d->lock, flags);
 
 	/* timeout based on observed timings and variations */
-	timeout = 2 * d->rttavg >> RTTSCALE;
-	timeout += 8 * d->rttdev >> RTTDSCALE;
-	if (timeout == 0)
-		timeout = 1;
+	timeout = rto(d);
+
+	utgts = count_targets(d, NULL);
 
 	if (d->flags & DEVFL_TKILL) {
 		spin_unlock_irqrestore(&d->lock, flags);
@@ -702,7 +812,7 @@ rexmit_timer(ulong vp)
 		since = tsince_hr(f);
 		n = f->waited_total + since;
 		n /= USEC_PER_SEC;
-		if (n > aoe_deadsecs) {
+		if (n > aoe_deadsecs && !(f->flags & FFL_PROBE)) {
 			/* Waited too long.  Device failure.
 			 * Hang all frames on first hash bucket for downdev
 			 * to clean up.
@@ -713,19 +823,26 @@ rexmit_timer(ulong vp)
 		}
 
 		t = f->t;
-		if (n > aoe_deadsecs/2)
-			d->htgt = t; /* see if another target can help */
+		n = f->waited + since;
+		n /= USEC_PER_SEC;
+		if (aoe_deadsecs && utgts > 0
+		&& (n > aoe_deadsecs / utgts || n > HARD_SCORN_SECS))
+			scorn(t); /* avoid this target */
 
 		if (t->maxout != 1) {
 			t->ssthresh = t->maxout / 2;
 			t->maxout = 1;
 		}
 
-		ifp = getif(t, f->skb->dev);
-		if (ifp && ++ifp->lost > (t->nframes << 1)
-		&& (ifp != t->ifs || t->ifs[1].nd)) {
-			ejectif(t, ifp);
-			ifp = NULL;
+		if (f->flags & FFL_PROBE) {
+			t->nout_probes--;
+		} else {
+			ifp = getif(t, f->skb->dev);
+			if (ifp && ++ifp->lost > (t->nframes << 1)
+			&& (ifp != t->ifs || t->ifs[1].nd)) {
+				ejectif(t, ifp);
+				ifp = NULL;
+			}
 		}
 		list_move_tail(pos, &d->rexmitq);
 		t->nout--;
@@ -733,7 +850,7 @@ rexmit_timer(ulong vp)
 	rexmit_deferred(d);
 
 out:
-	if ((d->flags & DEVFL_KICKME || d->htgt) && d->blkq) {
+	if ((d->flags & DEVFL_KICKME) && d->blkq) {
 		d->flags &= ~DEVFL_KICKME;
 		d->blkq->request_fn(d->blkq);
 	}
@@ -854,8 +971,6 @@ nextbuf(struct aoedev *d)
 void
 aoecmd_work(struct aoedev *d)
 {
-	if (d->htgt && !sthtith(d))
-		return;
 	rexmit_deferred(d);
 	while (aoecmd_ata_rw(d))
 		;
@@ -1065,19 +1180,22 @@ ktiocomplete(struct frame *f)
 	struct aoeif *ifp;
 	struct aoedev *d;
 	long n;
+	int untainted;
 
 	if (f == NULL)
 		return;
 
 	t = f->t;
 	d = t->d;
+	skb = f->r_skb;
+	buf = f->buf;
+	if (f->flags & FFL_PROBE)
+		goto out;
+	if (!skb)		/* just fail the buf. */
+		goto noskb;
 
 	hout = (struct aoe_hdr *) skb_mac_header(f->skb);
 	ahout = (struct aoe_atahdr *) (hout+1);
-	buf = f->buf;
-	skb = f->r_skb;
-	if (skb == NULL)
-		goto noskb;	/* just fail the buf. */
 
 	hin = (struct aoe_hdr *) skb->data;
 	skb_pull(skb, sizeof(*hin));
@@ -1089,7 +1207,7 @@ ktiocomplete(struct frame *f)
 			d->aoemajor, d->aoeminor);
 noskb:		if (buf)
 			clear_bit(BIO_UPTODATE, &buf->bio->bi_flags);
-		goto badrsp;
+		goto out;
 	}
 
 	n = ahout->scnt << 9;
@@ -1109,8 +1227,6 @@ noskb:		if (buf)
 		ifp = getif(t, skb->dev);
 		if (ifp)
 			ifp->lost = 0;
-		if (d->htgt == t) /* I'll help myself, thank you. */
-			d->htgt = NULL;
 		spin_unlock_irq(&d->lock);
 		break;
 	case ATA_CMD_ID_ATA:
@@ -1131,8 +1247,17 @@ noskb:		if (buf)
 			be16_to_cpu(get_unaligned(&hin->major)),
 			hin->minor);
 	}
-badrsp:
+out:
 	spin_lock_irq(&d->lock);
+	if (t->taint > 0
+	&& --t->taint > 0
+	&& t->nout_probes == 0) {
+		count_targets(d, &untainted);
+		if (untainted > 0) {
+			probe(t);
+			t->nout_probes++;
+		}
+	}
 
 	aoe_freetframe(f);
 
@@ -1261,6 +1386,8 @@ aoecmd_ata_rsp(struct sk_buff *skb)
 	if (f) {
 		calc_rttavg(d, f->t, tsince_hr(f));
 		f->t->nout--;
+		if (f->flags & FFL_PROBE)
+			f->t->nout_probes--;
 	} else {
 		f = getframe_deferred(d, n);
 		if (f) {
@@ -1379,6 +1506,7 @@ addtgt(struct aoedev *d, char *addr, ulong nframes)
 	memcpy(t->addr, addr, sizeof t->addr);
 	t->ifp = t->ifs;
 	aoecmd_wreset(t);
+	t->maxout = t->nframes / 2;
 	INIT_LIST_HEAD(&t->ffree);
 	return *tt = t;
 }
@@ -1584,6 +1712,14 @@ aoe_flush_iocq(void)
 int __init
 aoecmd_init(void)
 {
+	void *p;
+
+	/* get_zeroed_page returns page with ref count 1 */
+	p = (void *) get_zeroed_page(GFP_KERNEL | __GFP_REPEAT);
+	if (!p)
+		return -ENOMEM;
+	empty_page = virt_to_page(p);
+
 	INIT_LIST_HEAD(&iocq.head);
 	spin_lock_init(&iocq.lock);
 	init_waitqueue_head(&ktiowq);
@@ -1599,4 +1735,7 @@ aoecmd_exit(void)
 {
 	aoe_ktstop(&kts);
 	aoe_flush_iocq();
+
+	free_page((unsigned long) page_address(empty_page));
+	empty_page = NULL;
 }
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index aaaea66..f0c0c74 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -223,7 +223,6 @@ aoedev_downdev(struct aoedev *d)
 
 	/* clean out the in-process request (if any) */
 	aoe_failip(d);
-	d->htgt = NULL;
 
 	/* fast fail all pending I/O */
 	if (d->blkq) {
-- 
1.7.1

Re: [PATCH 1/7] aoe: improve handling of misbehaving network paths

[Andrew Morton]

On Mon, 3 Dec 2012 20:40:55 -0500
Ed Cashin <ecashin@coraid.com> wrote:

> An AoE target can have multiple network ports used for AoE, and
> in the aoe driver, those are tracked by the aoetgt struct.  These
> changes allow the aoe driver to handle network paths, or aoetgts,
> that are not working well, compared to the others.
> 
> Paths that do not get responses despite the retransmission of AoE
> commands are marked as "tainted", and non-tainted paths are
> preferred.
> 
> Meanwhile, the aoe driver attempts to "probe" the tainted path in
> the background by issuing reads of LBA 0 that are padded out to
> full (possibly jumbo-frame) size.  If the probes get responses,
> then the path is "redeemed", and its taint is removed.
> 
> This mechanism has been shown to be helpful in transparently
> handling and recovering from real-world network "brown outs" in
> ways that the earlier "shoot the help-needing target in the head"
> mechanism could not.
> 
>
> ...
>
> +static void
> +ata_rw_frameinit(struct frame *f)
> +{
> +	struct aoetgt *t;
> +	struct aoe_hdr *h;
> +	struct aoe_atahdr *ah;
> +	struct sk_buff *skb;
> +	char writebit, extbit;
> +
> +	skb = f->skb;
> +	h = (struct aoe_hdr *) skb_mac_header(skb);
> +	ah = (struct aoe_atahdr *) (h + 1);

Well.  It would be neater to have a

struct whatever {
	struct aoe_hdr hdr;
	struct aoe_atahdr atahdr;
};

> +	skb_put(skb, sizeof(*h) + sizeof(*ah));
> +	memset(h, 0, skb->len);
> +
> +	writebit = 0x10;
> +	extbit = 0x4;
> +
>
> ...
>
> @@ -462,11 +488,14 @@ resend(struct aoedev *d, struct frame *f)
>  	h = (struct aoe_hdr *) skb_mac_header(skb);
>  	ah = (struct aoe_atahdr *) (h+1);
>  
> -	snprintf(buf, sizeof buf,
> -		"%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm nout=%d\n",
> -		"retransmit", d->aoemajor, d->aoeminor, f->tag, jiffies, n,
> -		h->src, h->dst, t->nout);
> -	aoechr_error(buf);
> +	if (!(f->flags & FFL_PROBE)) {
> +		snprintf(buf, sizeof(buf),
> +			"%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm nout=%d\n",
> +			"retransmit", d->aoemajor, d->aoeminor,
> +			f->tag, jiffies, n,
> +			h->src, h->dst, t->nout);
> +		aoechr_error(buf);

Could use kasprintf() here.  That avoids the fixed-size local buffer
and avoids the GFP_ATOMIC allocation and copy in aoechr_error().

> +	}
>  
>  	f->tag = n;
>  	fhash(f);
>
> ...
>
>  aoecmd_init(void)
>  {
> +	void *p;
> +
> +	/* get_zeroed_page returns page with ref count 1 */
> +	p = (void *) get_zeroed_page(GFP_KERNEL | __GFP_REPEAT);
> +	if (!p)
> +		return -ENOMEM;
> +	empty_page = virt_to_page(p);

Could use alloc_pages() and remove `p' and the virt_to_page().

Why is __GFP_REPEAT used?  I don't think this __init function is more
important than all the other ones in the kernel?


>  	INIT_LIST_HEAD(&iocq.head);
>  	spin_lock_init(&iocq.lock);
>  	init_waitqueue_head(&ktiowq);
>
> ...
>

Re: [PATCH 1/7] aoe: improve handling of misbehaving network paths

[Ed Cashin]

On Dec 4, 2012, at 6:39 PM, Andrew Morton wrote:

> On Mon, 3 Dec 2012 20:40:55 -0500
> Ed Cashin <ecashin@coraid.com> wrote:
...
>> +static void
>> +ata_rw_frameinit(struct frame *f)
>> +{
>> +	struct aoetgt *t;
>> +	struct aoe_hdr *h;
>> +	struct aoe_atahdr *ah;
>> +	struct sk_buff *skb;
>> +	char writebit, extbit;
>> +
>> +	skb = f->skb;
>> +	h = (struct aoe_hdr *) skb_mac_header(skb);
>> +	ah = (struct aoe_atahdr *) (h + 1);
> 
> Well.  It would be neater to have a
> 
> struct whatever {
> 	struct aoe_hdr hdr;
> 	struct aoe_atahdr atahdr;
> };

This code just moved from aoecmd_ata_rw into a function that can be 
used by both aoecmd_ata_rw() and the new probe() function, so this
"h+1" way of getting at the ATA header is not new.  I'll make a note of 
your recommendation for future new code and for cleanups.

...
>> @@ -462,11 +488,14 @@ resend(struct aoedev *d, struct frame *f)
>> 	h = (struct aoe_hdr *) skb_mac_header(skb);
>> 	ah = (struct aoe_atahdr *) (h+1);
>> 
>> -	snprintf(buf, sizeof buf,
>> -		"%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm nout=%d\n",
>> -		"retransmit", d->aoemajor, d->aoeminor, f->tag, jiffies, n,
>> -		h->src, h->dst, t->nout);
>> -	aoechr_error(buf);
>> +	if (!(f->flags & FFL_PROBE)) {
>> +		snprintf(buf, sizeof(buf),
>> +			"%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm nout=%d\n",
>> +			"retransmit", d->aoemajor, d->aoeminor,
>> +			f->tag, jiffies, n,
>> +			h->src, h->dst, t->nout);
>> +		aoechr_error(buf);
> 
> Could use kasprintf() here.  That avoids the fixed-size local buffer
> and avoids the GFP_ATOMIC allocation and copy in aoechr_error().

This patch doesn't change the way aoechr_error() is used.  But 
thinking about the suggestion, I see that avoiding the GFP_ATOMIC 
allocation isn't practical here, because resend() is called after 
spin_lock_irq().  Let me know if you still think it would be worth 
cleaning up.  It could go on the todo list.

...
>> aoecmd_init(void)
>> {
>> +	void *p;
>> +
>> +	/* get_zeroed_page returns page with ref count 1 */
>> +	p = (void *) get_zeroed_page(GFP_KERNEL | __GFP_REPEAT);
>> +	if (!p)
>> +		return -ENOMEM;
>> +	empty_page = virt_to_page(p);
> 
> Could use alloc_pages() and remove `p' and the virt_to_page().
> 
> Why is __GFP_REPEAT used?  I don't think this __init function is more
> important than all the other ones in the kernel?

I think you are not implying that it's hurting the rest of the kernel for
__GFP_REPEAT to be used here but that you mean the aoe driver's
initialization should behave like the other initialization code in the 
kernel, and nobody else is using __GFP_REPEAT, so aoe should 
follow suit.  Please let me know if I'm misinterpreting your feedback.

Anyway, it's fine with me if you want to remove __GFP_REPEAT, or
I can resubmit this patch without it if you prefer.

-- 
  Ed Cashin
  ecashin@coraid.com

[PATCH 2/7] aoe: avoid races between device destruction and discovery

[Ed Cashin, Ed Cashin]

This change avoids a race that could result in a NULL pointer
derference following a WARNing from kobject_add_internal, "don't
try to register things with the same name in the same directory."

The problem was found with a test that forgets and discovers an
aoe device in a loop:

  while test ! -r /tmp/stop; do
	aoe-flush -a
	aoe-discover
  done

The race was between aoedev_flush taking aoedevs out of the
devlist, allowing a new discovery of the same AoE target to take
place before the driver gets around to calling
sysfs_remove_group.  Fixing that one revealed another race
between do_open and add_disk, and this patch avoids that, too.

The fix required some care, because for flushing (forgetting) an
aoedev, some of the steps must be performed under lock and some
must be able to sleep.  Also, for discovering a new aoedev, some
steps might sleep.

The check for a bad aoedev pointer remains from a time when about
half of this patch was done, and it was possible for the
bdev->bd_disk->private_data to become corrupted.  The check
should be removed eventually, but it is not expected to add
significant overhead, occurring in the aoeblk_open routine.

Signed-off-by: Ed Cashin <ecashin@coraid.com>
---
 drivers/block/aoe/aoe.h    |    7 ++-
 drivers/block/aoe/aoeblk.c |   36 +++++++++-
 drivers/block/aoe/aoedev.c |  166 ++++++++++++++++++++++++++++----------------
 3 files changed, 146 insertions(+), 63 deletions(-)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index b6d2b16..d50e945 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -74,8 +74,11 @@ enum {
 	DEVFL_TKILL = (1<<1),	/* flag for timer to know when to kill self */
 	DEVFL_EXT = (1<<2),	/* device accepts lba48 commands */
 	DEVFL_GDALLOC = (1<<3),	/* need to alloc gendisk */
-	DEVFL_KICKME = (1<<4),	/* slow polling network card catch */
-	DEVFL_NEWSIZE = (1<<5),	/* need to update dev size in block layer */
+	DEVFL_GD_NOW = (1<<4),	/* allocating gendisk */
+	DEVFL_KICKME = (1<<5),	/* slow polling network card catch */
+	DEVFL_NEWSIZE = (1<<6),	/* need to update dev size in block layer */
+	DEVFL_FREEING = (1<<7),	/* set when device is being cleaned up */
+	DEVFL_FREED = (1<<8),	/* device has been cleaned up */
 };
 
 enum {
diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
index 57ac72c..6b5b787 100644
--- a/drivers/block/aoe/aoeblk.c
+++ b/drivers/block/aoe/aoeblk.c
@@ -147,9 +147,18 @@ aoeblk_open(struct block_device *bdev, fmode_t mode)
 	struct aoedev *d = bdev->bd_disk->private_data;
 	ulong flags;
 
+	if (!virt_addr_valid(d)) {
+		pr_crit("aoe: invalid device pointer in %s\n",
+			__func__);
+		WARN_ON(1);
+		return -ENODEV;
+	}
+	if (!(d->flags & DEVFL_UP) || d->flags & DEVFL_TKILL)
+		return -ENODEV;
+
 	mutex_lock(&aoeblk_mutex);
 	spin_lock_irqsave(&d->lock, flags);
-	if (d->flags & DEVFL_UP) {
+	if (d->flags & DEVFL_UP && !(d->flags & DEVFL_TKILL)) {
 		d->nopen++;
 		spin_unlock_irqrestore(&d->lock, flags);
 		mutex_unlock(&aoeblk_mutex);
@@ -259,6 +268,18 @@ aoeblk_gdalloc(void *vp)
 	struct request_queue *q;
 	enum { KB = 1024, MB = KB * KB, READ_AHEAD = 2 * MB, };
 	ulong flags;
+	int late = 0;
+
+	spin_lock_irqsave(&d->lock, flags);
+	if (d->flags & DEVFL_GDALLOC
+	&& !(d->flags & DEVFL_TKILL)
+	&& !(d->flags & DEVFL_GD_NOW))
+		d->flags |= DEVFL_GD_NOW;
+	else
+		late = 1;
+	spin_unlock_irqrestore(&d->lock, flags);
+	if (late)
+		return;
 
 	gd = alloc_disk(AOE_PARTITIONS);
 	if (gd == NULL) {
@@ -282,6 +303,11 @@ aoeblk_gdalloc(void *vp)
 	}
 
 	spin_lock_irqsave(&d->lock, flags);
+	WARN_ON(!(d->flags & DEVFL_GD_NOW));
+	WARN_ON(!(d->flags & DEVFL_GDALLOC));
+	WARN_ON(d->flags & DEVFL_TKILL);
+	WARN_ON(d->gd);
+	WARN_ON(d->flags & DEVFL_UP);
 	blk_queue_max_hw_sectors(q, BLK_DEF_MAX_SECTORS);
 	q->backing_dev_info.name = "aoe";
 	q->backing_dev_info.ra_pages = READ_AHEAD / PAGE_CACHE_SIZE;
@@ -306,6 +332,11 @@ aoeblk_gdalloc(void *vp)
 
 	add_disk(gd);
 	aoedisk_add_sysfs(d);
+
+	spin_lock_irqsave(&d->lock, flags);
+	WARN_ON(!(d->flags & DEVFL_GD_NOW));
+	d->flags &= ~DEVFL_GD_NOW;
+	spin_unlock_irqrestore(&d->lock, flags);
 	return;
 
 err_mempool:
@@ -314,7 +345,8 @@ err_disk:
 	put_disk(gd);
 err:
 	spin_lock_irqsave(&d->lock, flags);
-	d->flags &= ~DEVFL_GDALLOC;
+	d->flags &= ~DEVFL_GD_NOW;
+	schedule_work(&d->work);
 	spin_unlock_irqrestore(&d->lock, flags);
 }
 
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index f0c0c74..3776715 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -15,7 +15,6 @@
 #include "aoe.h"
 
 static void dummy_timer(ulong);
-static void aoedev_freedev(struct aoedev *);
 static void freetgt(struct aoedev *d, struct aoetgt *t);
 static void skbpoolfree(struct aoedev *d);
 
@@ -236,29 +235,6 @@ aoedev_downdev(struct aoedev *d)
 		set_capacity(d->gd, 0);
 }
 
-static void
-aoedev_freedev(struct aoedev *d)
-{
-	struct aoetgt **t, **e;
-
-	cancel_work_sync(&d->work);
-	if (d->gd) {
-		aoedisk_rm_sysfs(d);
-		del_gendisk(d->gd);
-		put_disk(d->gd);
-		blk_cleanup_queue(d->blkq);
-	}
-	t = d->targets;
-	e = t + NTARGETS;
-	for (; t < e && *t; t++)
-		freetgt(d, *t);
-	if (d->bufpool)
-		mempool_destroy(d->bufpool);
-	skbpoolfree(d);
-	minor_free(d->sysminor);
-	kfree(d);
-}
-
 /* return whether the user asked for this particular
  * device to be flushed
  */
@@ -283,17 +259,62 @@ user_req(char *s, size_t slen, struct aoedev *d)
 	return !strncmp(s, p, lim);
 }
 
-int
-aoedev_flush(const char __user *str, size_t cnt)
+static void
+freedev(struct aoedev *d)
+{
+	struct aoetgt **t, **e;
+	int freeing = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&d->lock, flags);
+	if (d->flags & DEVFL_TKILL
+	&& !(d->flags & DEVFL_FREEING)) {
+		d->flags |= DEVFL_FREEING;
+		freeing = 1;
+	}
+	spin_unlock_irqrestore(&d->lock, flags);
+	if (!freeing)
+		return;
+
+	del_timer_sync(&d->timer);
+	if (d->gd) {
+		aoedisk_rm_sysfs(d);
+		del_gendisk(d->gd);
+		put_disk(d->gd);
+		blk_cleanup_queue(d->blkq);
+	}
+	t = d->targets;
+	e = t + NTARGETS;
+	for (; t < e && *t; t++)
+		freetgt(d, *t);
+	if (d->bufpool)
+		mempool_destroy(d->bufpool);
+	skbpoolfree(d);
+	minor_free(d->sysminor);
+
+	spin_lock_irqsave(&d->lock, flags);
+	d->flags |= DEVFL_FREED;
+	spin_unlock_irqrestore(&d->lock, flags);
+}
+
+enum flush_parms {
+	NOT_EXITING = 0,
+	EXITING = 1,
+};
+
+static int
+flush(const char __user *str, size_t cnt, int exiting)
 {
 	ulong flags;
 	struct aoedev *d, **dd;
-	struct aoedev *rmd = NULL;
 	char buf[16];
 	int all = 0;
 	int specified = 0;	/* flush a specific device */
+	unsigned int skipflags;
+
+	skipflags = DEVFL_GDALLOC | DEVFL_NEWSIZE | DEVFL_TKILL;
 
-	if (cnt >= 3) {
+	if (!exiting && cnt >= 3) {
 		if (cnt > sizeof buf)
 			cnt = sizeof buf;
 		if (copy_from_user(buf, str, cnt))
@@ -303,39 +324,71 @@ aoedev_flush(const char __user *str, size_t cnt)
 			specified = 1;
 	}
 
+	flush_scheduled_work();
+	/* pass one: without sleeping, do aoedev_downdev */
 	spin_lock_irqsave(&devlist_lock, flags);
-	dd = &devlist;
-	while ((d = *dd)) {
+	for (d = devlist; d; d = d->next) {
 		spin_lock(&d->lock);
-		if (specified) {
+		if (exiting) {
+			/* unconditionally take each device down */
+		} else if (specified) {
 			if (!user_req(buf, cnt, d))
-				goto skip;
+				goto cont;
 		} else if ((!all && (d->flags & DEVFL_UP))
-		|| (d->flags & (DEVFL_GDALLOC|DEVFL_NEWSIZE))
+		|| d->flags & skipflags
 		|| d->nopen
 		|| d->ref)
-			goto skip;
+			goto cont;
 
-		*dd = d->next;
 		aoedev_downdev(d);
 		d->flags |= DEVFL_TKILL;
+cont:
 		spin_unlock(&d->lock);
-		d->next = rmd;
-		rmd = d;
-		continue;
-skip:
-		spin_unlock(&d->lock);
-		dd = &d->next;
 	}
 	spin_unlock_irqrestore(&devlist_lock, flags);
-	while ((d = rmd)) {
-		rmd = d->next;
-		del_timer_sync(&d->timer);
-		aoedev_freedev(d);	/* must be able to sleep */
+
+	/* pass two: call freedev, which might sleep,
+	 * for aoedevs marked with DEVFL_TKILL
+	 */
+restart:
+	spin_lock_irqsave(&devlist_lock, flags);
+	for (d = devlist; d; d = d->next) {
+		spin_lock(&d->lock);
+		if (d->flags & DEVFL_TKILL
+		&& !(d->flags & DEVFL_FREEING)) {
+			spin_unlock(&d->lock);
+			spin_unlock_irqrestore(&devlist_lock, flags);
+			freedev(d);
+			goto restart;
+		}
+		spin_unlock(&d->lock);
 	}
+
+	/* pass three: remove aoedevs marked with DEVFL_FREED */
+	for (dd = &devlist, d = *dd; d; d = *dd) {
+		struct aoedev *doomed = NULL;
+
+		spin_lock(&d->lock);
+		if (d->flags & DEVFL_FREED) {
+			*dd = d->next;
+			doomed = d;
+		} else {
+			dd = &d->next;
+		}
+		spin_unlock(&d->lock);
+		kfree(doomed);
+	}
+	spin_unlock_irqrestore(&devlist_lock, flags);
+
 	return 0;
 }
 
+int
+aoedev_flush(const char __user *str, size_t cnt)
+{
+	return flush(str, cnt, NOT_EXITING);
+}
+
 /* This has been confirmed to occur once with Tms=3*1000 due to the
  * driver changing link and not processing its transmit ring.  The
  * problem is hard enough to solve by returning an error that I'm
@@ -388,7 +441,14 @@ aoedev_by_aoeaddr(ulong maj, int min, int do_alloc)
 
 	for (d=devlist; d; d=d->next)
 		if (d->aoemajor == maj && d->aoeminor == min) {
+			spin_lock(&d->lock);
+			if (d->flags & DEVFL_TKILL) {
+				spin_unlock(&d->lock);
+				d = NULL;
+				goto out;
+			}
 			d->ref++;
+			spin_unlock(&d->lock);
 			break;
 		}
 	if (d || !do_alloc || minor_get(&sysminor, maj, min) < 0)
@@ -448,21 +508,9 @@ freetgt(struct aoedev *d, struct aoetgt *t)
 void
 aoedev_exit(void)
 {
-	struct aoedev *d;
-	ulong flags;
-
+	flush_scheduled_work();
 	aoe_flush_iocq();
-	while ((d = devlist)) {
-		devlist = d->next;
-
-		spin_lock_irqsave(&d->lock, flags);
-		aoedev_downdev(d);
-		d->flags |= DEVFL_TKILL;
-		spin_unlock_irqrestore(&d->lock, flags);
-
-		del_timer_sync(&d->timer);
-		aoedev_freedev(d);
-	}
+	flush(NULL, 0, EXITING);
 }
 
 int __init
-- 
1.7.1

Re: [PATCH 2/7] aoe: avoid races between device destruction and discovery

[Andrew Morton]

On Mon, 3 Dec 2012 20:42:56 -0500
Ed Cashin <ecashin@coraid.com> wrote:

> This change avoids a race that could result in a NULL pointer
> derference following a WARNing from kobject_add_internal, "don't
> try to register things with the same name in the same directory."
> 
> The problem was found with a test that forgets and discovers an
> aoe device in a loop:
> 
>   while test ! -r /tmp/stop; do
> 	aoe-flush -a
> 	aoe-discover
>   done
> 
> The race was between aoedev_flush taking aoedevs out of the
> devlist, allowing a new discovery of the same AoE target to take
> place before the driver gets around to calling
> sysfs_remove_group.  Fixing that one revealed another race
> between do_open and add_disk, and this patch avoids that, too.
> 
> The fix required some care, because for flushing (forgetting) an
> aoedev, some of the steps must be performed under lock and some
> must be able to sleep.  Also, for discovering a new aoedev, some
> steps might sleep.
> 
> The check for a bad aoedev pointer remains from a time when about
> half of this patch was done, and it was possible for the
> bdev->bd_disk->private_data to become corrupted.  The check
> should be removed eventually, but it is not expected to add
> significant overhead, occurring in the aoeblk_open routine.
> 
>
> ...
>
> --- a/drivers/block/aoe/aoeblk.c
> +++ b/drivers/block/aoe/aoeblk.c
> @@ -147,9 +147,18 @@ aoeblk_open(struct block_device *bdev, fmode_t mode)
>  	struct aoedev *d = bdev->bd_disk->private_data;
>  	ulong flags;
>  
> +	if (!virt_addr_valid(d)) {
> +		pr_crit("aoe: invalid device pointer in %s\n",
> +			__func__);
> +		WARN_ON(1);
> +		return -ENODEV;
> +	}

Can this ever happen?

> +	if (!(d->flags & DEVFL_UP) || d->flags & DEVFL_TKILL)
> +		return -ENODEV;
> +
>  	mutex_lock(&aoeblk_mutex);
>  	spin_lock_irqsave(&d->lock, flags);
> -	if (d->flags & DEVFL_UP) {
> +	if (d->flags & DEVFL_UP && !(d->flags & DEVFL_TKILL)) {
>  		d->nopen++;
>  		spin_unlock_irqrestore(&d->lock, flags);
>  		mutex_unlock(&aoeblk_mutex);
> @@ -259,6 +268,18 @@ aoeblk_gdalloc(void *vp)
>  	struct request_queue *q;
>  	enum { KB = 1024, MB = KB * KB, READ_AHEAD = 2 * MB, };
>  	ulong flags;
> +	int late = 0;
> +
> +	spin_lock_irqsave(&d->lock, flags);
> +	if (d->flags & DEVFL_GDALLOC
> +	&& !(d->flags & DEVFL_TKILL)
> +	&& !(d->flags & DEVFL_GD_NOW))

That's pretty sickly-looking code layout.

We often do

	if ((d->flags & (DEVFL_GDALLOC|DEVFL_TKILL|DEVFL_GD_NOW)) ==
		DEVFL_GDALLOC)

in these cases.

> +		d->flags |= DEVFL_GD_NOW;
> +	else
> +		late = 1;
> +	spin_unlock_irqrestore(&d->lock, flags);
> +	if (late)
> +		return;
>  
>  	gd = alloc_disk(AOE_PARTITIONS);
>  	if (gd == NULL) {
>
> ...
>

Re: [PATCH 2/7] aoe: avoid races between device destruction and discovery

[Ed Cashin]

On Dec 4, 2012, at 6:45 PM, Andrew Morton wrote:

> On Mon, 3 Dec 2012 20:42:56 -0500
> Ed Cashin <ecashin@coraid.com> wrote:
> 
>> This change avoids a race that could result in a NULL pointer
>> derference following a WARNing from kobject_add_internal, "don't
>> try to register things with the same name in the same directory."
...
>> The check for a bad aoedev pointer remains from a time when about
>> half of this patch was done, and it was possible for the
>> bdev->bd_disk->private_data to become corrupted.  The check
>> should be removed eventually, but it is not expected to add
>> significant overhead, occurring in the aoeblk_open routine.
...
>> --- a/drivers/block/aoe/aoeblk.c
>> +++ b/drivers/block/aoe/aoeblk.c
>> @@ -147,9 +147,18 @@ aoeblk_open(struct block_device *bdev, fmode_t mode)
>> 	struct aoedev *d = bdev->bd_disk->private_data;
>> 	ulong flags;
>> 
>> +	if (!virt_addr_valid(d)) {
>> +		pr_crit("aoe: invalid device pointer in %s\n",
>> +			__func__);
>> +		WARN_ON(1);
>> +		return -ENODEV;
>> +	}
> 
> Can this ever happen?

This is the check mentioned in the last paragraph of the changelog
message.  I don't think it can happen now.  Folks have been using
it like this and nobody has seen the invalid device pointer message.

I'll go ahead and remove the check and resubmit the patch.

...
>> @@ -259,6 +268,18 @@ aoeblk_gdalloc(void *vp)
>> 	struct request_queue *q;
>> 	enum { KB = 1024, MB = KB * KB, READ_AHEAD = 2 * MB, };
>> 	ulong flags;
>> +	int late = 0;
>> +
>> +	spin_lock_irqsave(&d->lock, flags);
>> +	if (d->flags & DEVFL_GDALLOC
>> +	&& !(d->flags & DEVFL_TKILL)
>> +	&& !(d->flags & DEVFL_GD_NOW))
> 
> That's pretty sickly-looking code layout.
> 
> We often do
> 
> 	if ((d->flags & (DEVFL_GDALLOC|DEVFL_TKILL|DEVFL_GD_NOW)) ==
> 		DEVFL_GDALLOC)
> 
> in these cases.

OK.  When I'm resubmitting these patches, I'm planning to submit
the series of 7 patches and include info in the cover letter about
what has changed in the resubmission.

-- 
  Ed Cashin
  ecashin@coraid.com

[PATCH 3/7] aoe: use dynamic number of remote ports for AoE storage target

[Ed Cashin, Ed Cashin]

Many AoE targets have four or fewer network ports, but some
existing storage devices have many, and the AoE protocol sets no
limit.

This patch allows the use of more than eight remote MAC addresses
per AoE target, while reducing the amount of memory used by the
aoe driver in cases where there are many AoE targets with fewer
than eight MAC addresses each.

Signed-off-by: Ed Cashin <ecashin@coraid.com>
---
 drivers/block/aoe/aoe.h    |    6 ++--
 drivers/block/aoe/aoeblk.c |    2 +-
 drivers/block/aoe/aoecmd.c |   50 ++++++++++++++++++++++++++++++-------------
 drivers/block/aoe/aoedev.c |   12 ++++++++-
 4 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index d50e945..0f478dc 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -84,7 +84,7 @@ enum {
 enum {
 	DEFAULTBCNT = 2 * 512,	/* 2 sectors */
 	MIN_BUFS = 16,
-	NTARGETS = 8,
+	NTARGETS = 4,
 	NAOEIFS = 8,
 	NSKBPOOLMAX = 256,
 	NFACTIVE = 61,
@@ -185,9 +185,9 @@ struct aoedev {
 	ulong maxbcnt;
 	struct list_head factive[NFACTIVE];	/* hash of active frames */
 	struct list_head rexmitq; /* deferred retransmissions */
-	struct aoetgt *targets[NTARGETS];
+	struct aoetgt **targets;
+	ulong ntargets;		/* number of allocated aoetgt pointers */
 	struct aoetgt **tgt;	/* target in use when working */
-	ulong ntargets;
 	ulong kicked;
 	char ident[512];
 };
diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
index 6b5b787..a129f8c 100644
--- a/drivers/block/aoe/aoeblk.c
+++ b/drivers/block/aoe/aoeblk.c
@@ -67,7 +67,7 @@ static ssize_t aoedisk_show_netif(struct device *dev,
 	nd = nds;
 	ne = nd + ARRAY_SIZE(nds);
 	t = d->targets;
-	te = t + NTARGETS;
+	te = t + d->ntargets;
 	for (; t < te && *t; t++) {
 		ifp = (*t)->ifs;
 		e = ifp + NAOEIFS;
diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index 000f7fb..da360f9 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -242,14 +242,14 @@ newframe(struct aoedev *d)
 	int use_tainted;
 	int has_untainted;
 
-	if (d->targets[0] == NULL) {	/* shouldn't happen, but I'm paranoid */
+	if (!d->targets || !d->targets[0]) {
 		printk(KERN_ERR "aoe: NULL TARGETS!\n");
 		return NULL;
 	}
 	tt = d->tgt;	/* last used target */
 	for (use_tainted = 0, has_untainted = 0;;) {
 		tt++;
-		if (tt >= &d->targets[NTARGETS] || !*tt)
+		if (tt >= &d->targets[d->ntargets] || !*tt)
 			tt = d->targets;
 		t = *tt;
 		if (!t->taint) {
@@ -1104,7 +1104,7 @@ gettgt(struct aoedev *d, char *addr)
 	struct aoetgt **t, **e;
 
 	t = d->targets;
-	e = t + NTARGETS;
+	e = t + d->ntargets;
 	for (; t < e && *t; t++)
 		if (memcmp((*t)->addr, addr, sizeof((*t)->addr)) == 0)
 			return *t;
@@ -1479,28 +1479,44 @@ aoecmd_ata_id(struct aoedev *d)
 	return skb;
 }
 
+static struct aoetgt **
+grow_targets(struct aoedev *d)
+{
+	ulong oldn, newn;
+	struct aoetgt **tt;
+
+	oldn = d->ntargets;
+	newn = oldn * 2;
+	tt = kcalloc(newn, sizeof(*d->targets), GFP_ATOMIC);
+	if (!tt)
+		return NULL;
+	memmove(tt, d->targets, sizeof(*d->targets) * oldn);
+	d->tgt = tt + (d->tgt - d->targets);
+	kfree(d->targets);
+	d->targets = tt;
+	d->ntargets = newn;
+
+	return &d->targets[oldn];
+}
+
 static struct aoetgt *
 addtgt(struct aoedev *d, char *addr, ulong nframes)
 {
 	struct aoetgt *t, **tt, **te;
 
 	tt = d->targets;
-	te = tt + NTARGETS;
+	te = tt + d->ntargets;
 	for (; tt < te && *tt; tt++)
 		;
 
 	if (tt == te) {
-		printk(KERN_INFO
-			"aoe: device addtgt failure; too many targets\n");
-		return NULL;
+		tt = grow_targets(d);
+		if (!tt)
+			goto nomem;
 	}
 	t = kzalloc(sizeof(*t), GFP_ATOMIC);
-	if (!t) {
-		printk(KERN_INFO "aoe: cannot allocate memory to add target\n");
-		return NULL;
-	}
-
-	d->ntargets++;
+	if (!t)
+		goto nomem;
 	t->nframes = nframes;
 	t->d = d;
 	memcpy(t->addr, addr, sizeof t->addr);
@@ -1509,6 +1525,10 @@ addtgt(struct aoedev *d, char *addr, ulong nframes)
 	t->maxout = t->nframes / 2;
 	INIT_LIST_HEAD(&t->ffree);
 	return *tt = t;
+
+ nomem:
+	pr_info("aoe: cannot allocate memory to add target\n");
+	return NULL;
 }
 
 static void
@@ -1518,7 +1538,7 @@ setdbcnt(struct aoedev *d)
 	int bcnt = 0;
 
 	t = d->targets;
-	e = t + NTARGETS;
+	e = t + d->ntargets;
 	for (; t < e && *t; t++)
 		if (bcnt == 0 || bcnt > (*t)->minbcnt)
 			bcnt = (*t)->minbcnt;
@@ -1662,7 +1682,7 @@ aoecmd_cleanslate(struct aoedev *d)
 	d->maxbcnt = 0;
 
 	t = d->targets;
-	te = t + NTARGETS;
+	te = t + d->ntargets;
 	for (; t < te && *t; t++)
 		aoecmd_wreset(*t);
 }
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index 3776715..e66ccbf 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -214,7 +214,7 @@ aoedev_downdev(struct aoedev *d)
 
 	/* reset window dressings */
 	tt = d->targets;
-	te = tt + NTARGETS;
+	te = tt + d->ntargets;
 	for (; tt < te && (t = *tt); tt++) {
 		aoecmd_wreset(t);
 		t->nout = 0;
@@ -284,7 +284,7 @@ freedev(struct aoedev *d)
 		blk_cleanup_queue(d->blkq);
 	}
 	t = d->targets;
-	e = t + NTARGETS;
+	e = t + d->ntargets;
 	for (; t < e && *t; t++)
 		freetgt(d, *t);
 	if (d->bufpool)
@@ -376,6 +376,8 @@ restart:
 			dd = &d->next;
 		}
 		spin_unlock(&d->lock);
+		if (doomed)
+			kfree(doomed->targets);
 		kfree(doomed);
 	}
 	spin_unlock_irqrestore(&devlist_lock, flags);
@@ -456,6 +458,12 @@ aoedev_by_aoeaddr(ulong maj, int min, int do_alloc)
 	d = kcalloc(1, sizeof *d, GFP_ATOMIC);
 	if (!d)
 		goto out;
+	d->targets = kcalloc(NTARGETS, sizeof(*d->targets), GFP_ATOMIC);
+	if (!d->targets) {
+		kfree(d);
+		goto out;
+	}
+	d->ntargets = NTARGETS;
 	INIT_WORK(&d->work, aoecmd_sleepwork);
 	spin_lock_init(&d->lock);
 	skb_queue_head_init(&d->skbpool);
-- 
1.7.1

[PATCH 4/7] aoe: allow user to disable target failure timeout

[Ed Cashin, Ed Cashin]

With this change, the aoe driver treats the value zero as special
for the aoe_deadsecs module parameter.  Normally, this value
specifies the number of seconds during which the driver will
continue to attempt retransmits to an unresponsive AoE target.
After aoe_deadsecs has elapsed, the aoe driver marks the aoe
device as "down" and fails all I/O.

The new meaning of an aoe_deadsecs of zero is for the driver to
retransmit commands indefinitely.

Signed-off-by: Ed Cashin <ecashin@coraid.com>
---
 Documentation/aoe/aoe.txt  |    4 +++-
 drivers/block/aoe/aoecmd.c |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/aoe/aoe.txt b/Documentation/aoe/aoe.txt
index bfc9cb1..c71487d 100644
--- a/Documentation/aoe/aoe.txt
+++ b/Documentation/aoe/aoe.txt
@@ -125,7 +125,9 @@ DRIVER OPTIONS
   The aoe_deadsecs module parameter determines the maximum number of
   seconds that the driver will wait for an AoE device to provide a
   response to an AoE command.  After aoe_deadsecs seconds have
-  elapsed, the AoE device will be marked as "down".
+  elapsed, the AoE device will be marked as "down".  A value of zero
+  is supported for testing purposes and makes the aoe driver keep
+  trying AoE commands forever.
 
   The aoe_maxout module parameter has a default of 128.  This is the
   maximum number of unresponded packets that will be sent to an AoE
diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index da360f9..abf4ad2 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -812,7 +812,9 @@ rexmit_timer(ulong vp)
 		since = tsince_hr(f);
 		n = f->waited_total + since;
 		n /= USEC_PER_SEC;
-		if (n > aoe_deadsecs && !(f->flags & FFL_PROBE)) {
+		if (aoe_deadsecs
+		&& n > aoe_deadsecs
+		&& !(f->flags & FFL_PROBE)) {
 			/* Waited too long.  Device failure.
 			 * Hang all frames on first hash bucket for downdev
 			 * to clean up.
-- 
1.7.1

[PATCH 5/7] aoe: allow comma separator in aoe_iflist value

[Ed Cashin, Ed Cashin]

By default, the aoe driver uses any ethernet interface for AoE,
but the aoe_iflist module parameter provides a convenient way to
limit AoE traffic to a specific list of local network interfaces.

This change allows a list to be specified using the comma
character as a separator.  For example,

  modprobe aoe aoe_iflist=eth2,eth3

Before, it was inconvenient to get the quoting right in shell
scripts when setting aoe_iflist to have more than one network
interface.

Signed-off-by: Ed Cashin <ecashin@coraid.com>
---
 drivers/block/aoe/aoe.h    |    2 +-
 drivers/block/aoe/aoenet.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index 0f478dc..f8d6c7e 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -11,7 +11,7 @@
 #define AOE_PARTITIONS (16)
 #endif
 
-#define WHITESPACE " \t\v\f\n"
+#define WHITESPACE " \t\v\f\n,"
 
 enum {
 	AOECMD_ATA,
diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
index 2e47404..71d3ea8 100644
--- a/drivers/block/aoe/aoenet.c
+++ b/drivers/block/aoe/aoenet.c
@@ -31,7 +31,7 @@ enum {
 
 static char aoe_iflist[IFLISTSZ];
 module_param_string(aoe_iflist, aoe_iflist, IFLISTSZ, 0600);
-MODULE_PARM_DESC(aoe_iflist, "aoe_iflist=\"dev1 [dev2 ...]\"");
+MODULE_PARM_DESC(aoe_iflist, "aoe_iflist=dev1[,dev2...]");
 
 static wait_queue_head_t txwq;
 static struct ktstate kts;
-- 
1.7.1

[PATCH 6/7] aoe: identify source of runt AoE packets

[Ed Cashin, Ed Cashin]

This change only affects experimental AoE storage networks.

It modifies the console message about runt packets detected so
that the AoE major and minor addresses of the AoE target that
generated the runt are mentioned.

Signed-off-by: Ed Cashin <ecashin@coraid.com>
---
 drivers/block/aoe/aoecmd.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index abf4ad2..25ef5c0 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -1217,8 +1217,10 @@ noskb:		if (buf)
 	case ATA_CMD_PIO_READ:
 	case ATA_CMD_PIO_READ_EXT:
 		if (skb->len < n) {
-			pr_err("aoe: runt data size in read.  skb->len=%d need=%ld\n",
-				skb->len, n);
+			pr_err("%s e%ld.%d.  skb->len=%d need=%ld\n",
+				"aoe: runt data size in read from",
+				(long) d->aoemajor, d->aoeminor,
+			       skb->len, n);
 			clear_bit(BIO_UPTODATE, &buf->bio->bi_flags);
 			break;
 		}
@@ -1233,7 +1235,9 @@ noskb:		if (buf)
 		break;
 	case ATA_CMD_ID_ATA:
 		if (skb->len < 512) {
-			pr_info("aoe: runt data size in ataid.  skb->len=%d\n",
+			pr_info("%s e%ld.%d.  skb->len=%d need=512\n",
+				"aoe: runt data size in ataid from",
+				(long) d->aoemajor, d->aoeminor,
 				skb->len);
 			break;
 		}
-- 
1.7.1

[PATCH 7/7] aoe: update internal version number to 81

[Ed Cashin, Ed Cashin]

This version number is printed to the console on module
initialization and is available in sysfs, which is where the
userland aoe-version tool looks for it.

Signed-off-by: Ed Cashin <ecashin@coraid.com>
---
 drivers/block/aoe/aoe.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index f8d6c7e..1756494 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -1,6 +1,5 @@
-
 /* Copyright (c) 2012 Coraid, Inc.  See COPYING for GPL terms. */
-#define VERSION "64+"
+#define VERSION "81"
 #define AOE_MAJOR 152
 #define DEVICE_NAME "aoe"
 
-- 
1.7.1