Re: [PATCH 1/7] aoe: improve handling of misbehaving network paths
On Dec 4, 2012, at 6:39 PM, Andrew Morton wrote:
> On Mon, 3 Dec 2012 20:40:55 -0500
> Ed Cashin 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
ecas...@coraid.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/7] aoe: improve handling of misbehaving network paths
On Dec 4, 2012, at 6:39 PM, Andrew Morton wrote:
On Mon, 3 Dec 2012 20:40:55 -0500
Ed Cashin ecas...@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
ecas...@coraid.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/7] aoe: improve handling of misbehaving network paths
On Mon, 3 Dec 2012 20:40:55 -0500
Ed Cashin 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();
> spin_lock_init();
> init_waitqueue_head();
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/7] aoe: improve handling of misbehaving network paths
On Mon, 3 Dec 2012 20:40:55 -0500
Ed Cashin ecas...@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);
...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/7] aoe: improve handling of misbehaving network paths
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
---
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(>head, >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 >= >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) {
[PATCH 1/7] aoe: improve handling of misbehaving network paths
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 ecas...@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++;
