* [PATCH] raid5: panic() on dma_wait_for_async_tx() error
@ 2012-11-08 10:06 Bartlomiej Zolnierkiewicz
2012-11-08 11:15 ` Alan Cox
0 siblings, 1 reply; 8+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2012-11-08 10:06 UTC (permalink / raw)
To: linux-kernel, linux-raid
Cc: Neil Brown, Vinod Koul, Dan Williams, Tomasz Figa, Kyungmin Park
From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: [PATCH] raid5: panic() on dma_wait_for_async_tx() error
There is not much we can do on dma_wait_for_async_tx() error
so just panic() for now.
Cc: Neil Brown <neilb@suse.de>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: Dan Williams <djbw@fb.com>
Cc: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/md/raid5.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: b/drivers/md/raid5.c
===================================================================
--- a/drivers/md/raid5.c 2012-11-07 16:25:19.480876012 +0100
+++ b/drivers/md/raid5.c 2012-11-07 16:27:46.244875992 +0100
@@ -3223,7 +3223,9 @@ static void handle_stripe_expansion(stru
/* done submitting copies, wait for them to complete */
if (tx) {
async_tx_ack(tx);
- dma_wait_for_async_tx(tx);
+ if (dma_wait_for_async_tx(tx) != DMA_SUCCESS)
+ panic("%s: DMA error waiting for transaction\n",
+ __func__);
}
}
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] raid5: panic() on dma_wait_for_async_tx() error 2012-11-08 10:06 [PATCH] raid5: panic() on dma_wait_for_async_tx() error Bartlomiej Zolnierkiewicz @ 2012-11-08 11:15 ` Alan Cox 2012-11-08 11:20 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 8+ messages in thread From: Alan Cox @ 2012-11-08 11:15 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: linux-kernel, linux-raid, Neil Brown, Vinod Koul, Dan Williams, Tomasz Figa, Kyungmin Park On Thu, 08 Nov 2012 11:06:29 +0100 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Subject: [PATCH] raid5: panic() on dma_wait_for_async_tx() error > > There is not much we can do on dma_wait_for_async_tx() error > so just panic() for now. > > Cc: Neil Brown <neilb@suse.de> > Cc: Vinod Koul <vinod.koul@intel.com> > Cc: Dan Williams <djbw@fb.com> > Cc: Tomasz Figa <t.figa@samsung.com> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/md/raid5.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: b/drivers/md/raid5.c > =================================================================== > --- a/drivers/md/raid5.c 2012-11-07 16:25:19.480876012 +0100 > +++ b/drivers/md/raid5.c 2012-11-07 16:27:46.244875992 +0100 > @@ -3223,7 +3223,9 @@ static void handle_stripe_expansion(stru > /* done submitting copies, wait for them to complete */ > if (tx) { > async_tx_ack(tx); > - dma_wait_for_async_tx(tx); > + if (dma_wait_for_async_tx(tx) != DMA_SUCCESS) > + panic("%s: DMA error waiting for transaction\n", > + __func__); Thats a really horrible place to panic. Alan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] raid5: panic() on dma_wait_for_async_tx() error 2012-11-08 11:15 ` Alan Cox @ 2012-11-08 11:20 ` Bartlomiej Zolnierkiewicz 2012-11-19 1:06 ` NeilBrown 0 siblings, 1 reply; 8+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2012-11-08 11:20 UTC (permalink / raw) To: Alan Cox Cc: linux-kernel, linux-raid, Neil Brown, Vinod Koul, Dan Williams, Tomasz Figa, Kyungmin Park On Thursday 08 November 2012 12:15:26 Alan Cox wrote: > On Thu, 08 Nov 2012 11:06:29 +0100 > Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > > > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Subject: [PATCH] raid5: panic() on dma_wait_for_async_tx() error > > > > There is not much we can do on dma_wait_for_async_tx() error > > so just panic() for now. > > > > Cc: Neil Brown <neilb@suse.de> > > Cc: Vinod Koul <vinod.koul@intel.com> > > Cc: Dan Williams <djbw@fb.com> > > Cc: Tomasz Figa <t.figa@samsung.com> > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > --- > > drivers/md/raid5.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > Index: b/drivers/md/raid5.c > > =================================================================== > > --- a/drivers/md/raid5.c 2012-11-07 16:25:19.480876012 +0100 > > +++ b/drivers/md/raid5.c 2012-11-07 16:27:46.244875992 +0100 > > @@ -3223,7 +3223,9 @@ static void handle_stripe_expansion(stru > > /* done submitting copies, wait for them to complete */ > > if (tx) { > > async_tx_ack(tx); > > - dma_wait_for_async_tx(tx); > > + if (dma_wait_for_async_tx(tx) != DMA_SUCCESS) > > + panic("%s: DMA error waiting for transaction\n", > > + __func__); > > Thats a really horrible place to panic. Still it seems better thing to do than silently ignoring errors and trying to continue operations with inconsistent data. Unfortunately higher-layers don't support error conditions and fixing them seems to be non-trivial task. Best regards, -- Bartlomiej Zolnierkiewicz Samsung Poland R&D Center ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] raid5: panic() on dma_wait_for_async_tx() error 2012-11-08 11:20 ` Bartlomiej Zolnierkiewicz @ 2012-11-19 1:06 ` NeilBrown 2012-11-19 5:22 ` Dan Williams 0 siblings, 1 reply; 8+ messages in thread From: NeilBrown @ 2012-11-19 1:06 UTC (permalink / raw) To: Dan Williams Cc: Bartlomiej Zolnierkiewicz, Alan Cox, linux-kernel, linux-raid, Vinod Koul, Tomasz Figa, Kyungmin Park [-- Attachment #1: Type: text/plain, Size: 2407 bytes --] Hi Dan, could you comment on this please? Would it make sense to arrange for errors to propagate up? Or should we arrange to do a software-fallback in the dma engine is a problem? What sort of things can cause error here anyway? Thanks, NeilBrown On Thu, 08 Nov 2012 12:20:55 +0100 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > On Thursday 08 November 2012 12:15:26 Alan Cox wrote: > > On Thu, 08 Nov 2012 11:06:29 +0100 > > Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > > > > > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > > Subject: [PATCH] raid5: panic() on dma_wait_for_async_tx() error > > > > > > There is not much we can do on dma_wait_for_async_tx() error > > > so just panic() for now. > > > > > > Cc: Neil Brown <neilb@suse.de> > > > Cc: Vinod Koul <vinod.koul@intel.com> > > > Cc: Dan Williams <djbw@fb.com> > > > Cc: Tomasz Figa <t.figa@samsung.com> > > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > > --- > > > drivers/md/raid5.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > Index: b/drivers/md/raid5.c > > > =================================================================== > > > --- a/drivers/md/raid5.c 2012-11-07 16:25:19.480876012 +0100 > > > +++ b/drivers/md/raid5.c 2012-11-07 16:27:46.244875992 +0100 > > > @@ -3223,7 +3223,9 @@ static void handle_stripe_expansion(stru > > > /* done submitting copies, wait for them to complete */ > > > if (tx) { > > > async_tx_ack(tx); > > > - dma_wait_for_async_tx(tx); > > > + if (dma_wait_for_async_tx(tx) != DMA_SUCCESS) > > > + panic("%s: DMA error waiting for transaction\n", > > > + __func__); > > > > Thats a really horrible place to panic. > > Still it seems better thing to do than silently ignoring errors > and trying to continue operations with inconsistent data. > > Unfortunately higher-layers don't support error conditions and > fixing them seems to be non-trivial task. > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung Poland R&D Center > -- > To unsubscribe from this list: send the line "unsubscribe linux-raid" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] raid5: panic() on dma_wait_for_async_tx() error 2012-11-19 1:06 ` NeilBrown @ 2012-11-19 5:22 ` Dan Williams 2012-11-19 22:18 ` NeilBrown 0 siblings, 1 reply; 8+ messages in thread From: Dan Williams @ 2012-11-19 5:22 UTC (permalink / raw) To: NeilBrown Cc: Bartlomiej Zolnierkiewicz, Alan Cox, linux-kernel, linux-raid, Vinod Koul, Tomasz Figa, Kyungmin Park On 11/18/12 5:06 PM, "NeilBrown" <neilb@suse.de> wrote: > >Hi Dan, > could you comment on this please? Would it make sense to arrange for >errors > to propagate up? Or should we arrange to do a software-fallback in the >dma > engine is a problem? What sort of things can cause error here anyway? Propagating up is missing reliable "dma abort" operation. In these cases the engine failed to complete due to hardware hang / driver bug, or has hit a memory error (uncorrectable even with software fallback). This originally should have been using async_tx_quiesce() which also does the panic. The engines that I have worked with have either lacked support for aborting, or were otherwise unable to recover from a hardware hang. However, for engines that do support error recovery they should be able to hide the failure from the upper layers. -- Dan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] raid5: panic() on dma_wait_for_async_tx() error 2012-11-19 5:22 ` Dan Williams @ 2012-11-19 22:18 ` NeilBrown 2012-11-20 2:23 ` Dan Williams 0 siblings, 1 reply; 8+ messages in thread From: NeilBrown @ 2012-11-19 22:18 UTC (permalink / raw) To: Dan Williams Cc: Bartlomiej Zolnierkiewicz, Alan Cox, linux-kernel, linux-raid, Vinod Koul, Tomasz Figa, Kyungmin Park [-- Attachment #1: Type: text/plain, Size: 2402 bytes --] On Mon, 19 Nov 2012 05:22:25 +0000 Dan Williams <djbw@fb.com> wrote: > > > On 11/18/12 5:06 PM, "NeilBrown" <neilb@suse.de> wrote: > > > > >Hi Dan, > > could you comment on this please? Would it make sense to arrange for > >errors > > to propagate up? Or should we arrange to do a software-fallback in the > >dma > > engine is a problem? What sort of things can cause error here anyway? > > Propagating up is missing reliable "dma abort" operation. > > In these cases the engine failed to complete due to hardware hang / driver > bug, or has hit a memory error (uncorrectable even with software > fallback). This originally should have been using async_tx_quiesce() > which also does the panic. > > The engines that I have worked with have either lacked support for > aborting, or were otherwise unable to recover from a hardware hang. > However, for engines that do support error recovery they should be able to > hide the failure from the upper layers. > So maybe I could: diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index ac09fa4..ffbf0ca 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -3268,7 +3268,7 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh) /* done submitting copies, wait for them to complete */ if (tx) { async_tx_ack(tx); - dma_wait_for_async_tx(tx); + async_tx_quiesce(&tx); } } and then the panic would be somebody else's problem? I note that handle_stripe_expansion has: async_tx_ack(tx); dma_wait_for_async_tx(tx); while async_tx_quiesce() has: if (dma_wait_for_async_tx(*tx) == DMA_ERROR) panic("DMA_ERROR waiting for transaction\n"); async_tx_ack(*tx); i.e. the same two functions called in the reverse order. Is the order important? Is handle_stripe_expansion wrong? Should the patch I apply actually be: diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index ac09fa4..e51d903 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -3266,10 +3266,7 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh) } /* done submitting copies, wait for them to complete */ - if (tx) { - async_tx_ack(tx); - dma_wait_for_async_tx(tx); - } + async_tx_quiesce(&tx); } /* because async_tx_quiesce() does the NULL test too??? Thanks, NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] raid5: panic() on dma_wait_for_async_tx() error 2012-11-19 22:18 ` NeilBrown @ 2012-11-20 2:23 ` Dan Williams 2012-11-20 3:13 ` NeilBrown 0 siblings, 1 reply; 8+ messages in thread From: Dan Williams @ 2012-11-20 2:23 UTC (permalink / raw) To: NeilBrown Cc: Bartlomiej Zolnierkiewicz, Alan Cox, linux-kernel, linux-raid, Vinod Koul, Tomasz Figa, Kyungmin Park On Tue, 2012-11-20 at 09:18 +1100, NeilBrown wrote: > On Mon, 19 Nov 2012 05:22:25 +0000 Dan Williams <djbw@fb.com> wrote: > > > > > > > On 11/18/12 5:06 PM, "NeilBrown" <neilb@suse.de> wrote: > > > > > > > >Hi Dan, > > > could you comment on this please? Would it make sense to arrange for > > >errors > > > to propagate up? Or should we arrange to do a software-fallback in the > > >dma > > > engine is a problem? What sort of things can cause error here anyway? > > > > Propagating up is missing reliable "dma abort" operation. > > > > In these cases the engine failed to complete due to hardware hang / driver > > bug, or has hit a memory error (uncorrectable even with software > > fallback). This originally should have been using async_tx_quiesce() > > which also does the panic. > > > > The engines that I have worked with have either lacked support for > > aborting, or were otherwise unable to recover from a hardware hang. > > However, for engines that do support error recovery they should be able to > > hide the failure from the upper layers. > > > > So maybe I could: > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index ac09fa4..ffbf0ca 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -3268,7 +3268,7 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh) > /* done submitting copies, wait for them to complete */ > if (tx) { > async_tx_ack(tx); > - dma_wait_for_async_tx(tx); > + async_tx_quiesce(&tx); > } > } > > > > and then the panic would be somebody else's problem? > > I note that handle_stripe_expansion has: > > async_tx_ack(tx); > dma_wait_for_async_tx(tx); > > while async_tx_quiesce() has: > > if (dma_wait_for_async_tx(*tx) == DMA_ERROR) > panic("DMA_ERROR waiting for transaction\n"); > async_tx_ack(*tx); > > > i.e. the same two functions called in the reverse order. Is the order > important? Is handle_stripe_expansion wrong? Should the patch I apply > actually be: > > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index ac09fa4..e51d903 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -3266,10 +3266,7 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh) > > } > /* done submitting copies, wait for them to complete */ > - if (tx) { > - async_tx_ack(tx); > - dma_wait_for_async_tx(tx); > - } > + async_tx_quiesce(&tx); > } > Yes, this one, handles it like the other cases of needing to do a synchronous wait and does not care if tx is NULL. -- Dan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] raid5: panic() on dma_wait_for_async_tx() error 2012-11-20 2:23 ` Dan Williams @ 2012-11-20 3:13 ` NeilBrown 0 siblings, 0 replies; 8+ messages in thread From: NeilBrown @ 2012-11-20 3:13 UTC (permalink / raw) To: Dan Williams Cc: Bartlomiej Zolnierkiewicz, Alan Cox, linux-kernel, linux-raid, Vinod Koul, Tomasz Figa, Kyungmin Park [-- Attachment #1: Type: text/plain, Size: 4138 bytes --] On Mon, 19 Nov 2012 18:23:57 -0800 Dan Williams <djbw@fb.com> wrote: > On Tue, 2012-11-20 at 09:18 +1100, NeilBrown wrote: > > On Mon, 19 Nov 2012 05:22:25 +0000 Dan Williams <djbw@fb.com> wrote: > > > > > > > > > > > On 11/18/12 5:06 PM, "NeilBrown" <neilb@suse.de> wrote: > > > > > > > > > > >Hi Dan, > > > > could you comment on this please? Would it make sense to arrange for > > > >errors > > > > to propagate up? Or should we arrange to do a software-fallback in the > > > >dma > > > > engine is a problem? What sort of things can cause error here anyway? > > > > > > Propagating up is missing reliable "dma abort" operation. > > > > > > In these cases the engine failed to complete due to hardware hang / driver > > > bug, or has hit a memory error (uncorrectable even with software > > > fallback). This originally should have been using async_tx_quiesce() > > > which also does the panic. > > > > > > The engines that I have worked with have either lacked support for > > > aborting, or were otherwise unable to recover from a hardware hang. > > > However, for engines that do support error recovery they should be able to > > > hide the failure from the upper layers. > > > > > > > So maybe I could: > > > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > > index ac09fa4..ffbf0ca 100644 > > --- a/drivers/md/raid5.c > > +++ b/drivers/md/raid5.c > > @@ -3268,7 +3268,7 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh) > > /* done submitting copies, wait for them to complete */ > > if (tx) { > > async_tx_ack(tx); > > - dma_wait_for_async_tx(tx); > > + async_tx_quiesce(&tx); > > } > > } > > > > > > > > and then the panic would be somebody else's problem? > > > > I note that handle_stripe_expansion has: > > > > async_tx_ack(tx); > > dma_wait_for_async_tx(tx); > > > > while async_tx_quiesce() has: > > > > if (dma_wait_for_async_tx(*tx) == DMA_ERROR) > > panic("DMA_ERROR waiting for transaction\n"); > > async_tx_ack(*tx); > > > > > > i.e. the same two functions called in the reverse order. Is the order > > important? Is handle_stripe_expansion wrong? Should the patch I apply > > actually be: > > > > > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > > index ac09fa4..e51d903 100644 > > --- a/drivers/md/raid5.c > > +++ b/drivers/md/raid5.c > > @@ -3266,10 +3266,7 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh) > > > > } > > /* done submitting copies, wait for them to complete */ > > - if (tx) { > > - async_tx_ack(tx); > > - dma_wait_for_async_tx(tx); > > - } > > + async_tx_quiesce(&tx); > > } > > > > Yes, this one, handles it like the other cases of needing to do a > synchronous wait and does not care if tx is NULL. Thanks. Following is now in my for-next branch. NeilBrown From e25a8de38d6584ffd042dbef3a5a8eb518b8813b Mon Sep 17 00:00:00 2001 From: NeilBrown <neilb@suse.de> Date: Tue, 20 Nov 2012 14:11:15 +1100 Subject: [PATCH] md/raid5: use async_tx_quiesce() instead of open-coding it. handle_stripe_expansion contains: if (tx) { async_tx_ack(tx); dma_wait_for_async_tx(tx); } which is very similar to the body of async_tx_quiesce(), except that the later handles an error from dma_wait_for_async_tx() (admittedly by panicing, but that decision belongs in the dma code, not the md code). So just us async_tx_quiesce(). Acked-by: Dan Williams <djbw@fb.com> Reported-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Signed-off-by: NeilBrown <neilb@suse.de> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index ac09fa4..e51d903 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -3266,10 +3266,7 @@ static void handle_stripe_expansion(struct r5conf *conf, struct stripe_head *sh) } /* done submitting copies, wait for them to complete */ - if (tx) { - async_tx_ack(tx); - dma_wait_for_async_tx(tx); - } + async_tx_quiesce(&tx); } /* [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-11-20 3:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-08 10:06 [PATCH] raid5: panic() on dma_wait_for_async_tx() error Bartlomiej Zolnierkiewicz 2012-11-08 11:15 ` Alan Cox 2012-11-08 11:20 ` Bartlomiej Zolnierkiewicz 2012-11-19 1:06 ` NeilBrown 2012-11-19 5:22 ` Dan Williams 2012-11-19 22:18 ` NeilBrown 2012-11-20 2:23 ` Dan Williams 2012-11-20 3:13 ` NeilBrown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.