All of lore.kernel.org
 help / color / mirror / Atom feed
* raid5 async_xor: sleep in atomic
@ 2015-12-22 11:58 Stanislav Samsonov
  2015-12-23  2:34 ` NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Stanislav Samsonov @ 2015-12-22 11:58 UTC (permalink / raw)
  To: linux-raid

Hi,

Kernel 4.1.3 : there is some troubling kernel message that shows up
after enabling CONFIG_DEBUG_ATOMIC_SLEEP and testing DMA XOR
acceleration for raid5:

BUG: sleeping function called from invalid context at mm/mempool.c:320
in_atomic(): 1, irqs_disabled(): 0, pid: 1048, name: md127_raid5
INFO: lockdep is turned off.
CPU: 1 PID: 1048 Comm: md127_raid5 Not tainted 4.1.15.alpine.1-dirty #1
Hardware name: Annapurna Labs Alpine
[<c00169d8>] (unwind_backtrace) from [<c0012a78>] (show_stack+0x10/0x14)
[<c0012a78>] (show_stack) from [<c07462ec>] (dump_stack+0x80/0xb4)
[<c07462ec>] (dump_stack) from [<c00bf2f0>] (mempool_alloc+0x68/0x13c)
[<c00bf2f0>] (mempool_alloc) from [<c041c9b4>]
(dmaengine_get_unmap_data+0x24/0x4c)
[<c041c9b4>] (dmaengine_get_unmap_data) from [<c03a8084>]
(async_xor_val+0x60/0x3a0)
[<c03a8084>] (async_xor_val) from [<c058e4c0>] (raid_run_ops+0xb70/0x1248)
[<c058e4c0>] (raid_run_ops) from [<c05915d4>] (handle_stripe+0x1068/0x22a8)
[<c05915d4>] (handle_stripe) from [<c0592ae4>]
(handle_active_stripes+0x2d0/0x3dc)
[<c0592ae4>] (handle_active_stripes) from [<c059300c>] (raid5d+0x384/0x5b0)
[<c059300c>] (raid5d) from [<c059db6c>] (md_thread+0x114/0x138)
[<c059db6c>] (md_thread) from [<c0042d54>] (kthread+0xe4/0x104)
[<c0042d54>] (kthread) from [<c000f658>] (ret_from_fork+0x14/0x3c)

The reason is that async_xor_val() in crypto/async_tx/async_xor.c is
called in atomic context (preemption disabled) by raid_run_ops(). Then
it calls dmaengine_get_unmap_data() an then mempool_alloc() with
GFP_NOIO flag - this allocation type might sleep under some condition.

Checked latest kernel 4.3 and it has exactly same flow.

Any advice regarding this issue?

Thanks,
Slava Samsonov

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2015-12-22 11:58 raid5 async_xor: sleep in atomic Stanislav Samsonov
@ 2015-12-23  2:34 ` NeilBrown
  2015-12-23 17:35   ` Dan Williams
  0 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2015-12-23  2:34 UTC (permalink / raw)
  To: Stanislav Samsonov, linux-raid; +Cc: dan.j.williams

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Tue, Dec 22 2015, Stanislav Samsonov wrote:

> Hi,
>
> Kernel 4.1.3 : there is some troubling kernel message that shows up
> after enabling CONFIG_DEBUG_ATOMIC_SLEEP and testing DMA XOR
> acceleration for raid5:
>
> BUG: sleeping function called from invalid context at mm/mempool.c:320
> in_atomic(): 1, irqs_disabled(): 0, pid: 1048, name: md127_raid5
> INFO: lockdep is turned off.
> CPU: 1 PID: 1048 Comm: md127_raid5 Not tainted 4.1.15.alpine.1-dirty #1
> Hardware name: Annapurna Labs Alpine
> [<c00169d8>] (unwind_backtrace) from [<c0012a78>] (show_stack+0x10/0x14)
> [<c0012a78>] (show_stack) from [<c07462ec>] (dump_stack+0x80/0xb4)
> [<c07462ec>] (dump_stack) from [<c00bf2f0>] (mempool_alloc+0x68/0x13c)
> [<c00bf2f0>] (mempool_alloc) from [<c041c9b4>]
> (dmaengine_get_unmap_data+0x24/0x4c)
> [<c041c9b4>] (dmaengine_get_unmap_data) from [<c03a8084>]
> (async_xor_val+0x60/0x3a0)
> [<c03a8084>] (async_xor_val) from [<c058e4c0>] (raid_run_ops+0xb70/0x1248)
> [<c058e4c0>] (raid_run_ops) from [<c05915d4>] (handle_stripe+0x1068/0x22a8)
> [<c05915d4>] (handle_stripe) from [<c0592ae4>]
> (handle_active_stripes+0x2d0/0x3dc)
> [<c0592ae4>] (handle_active_stripes) from [<c059300c>] (raid5d+0x384/0x5b0)
> [<c059300c>] (raid5d) from [<c059db6c>] (md_thread+0x114/0x138)
> [<c059db6c>] (md_thread) from [<c0042d54>] (kthread+0xe4/0x104)
> [<c0042d54>] (kthread) from [<c000f658>] (ret_from_fork+0x14/0x3c)
>
> The reason is that async_xor_val() in crypto/async_tx/async_xor.c is
> called in atomic context (preemption disabled) by raid_run_ops(). Then
> it calls dmaengine_get_unmap_data() an then mempool_alloc() with
> GFP_NOIO flag - this allocation type might sleep under some condition.
>
> Checked latest kernel 4.3 and it has exactly same flow.
>
> Any advice regarding this issue?

Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
issue... or make it crash even worse :-)

Dan: do you have any wisdom here?  The xor is using the percpu data in
raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
Does the code handle failure to get_unmap_data() safely?  It looks like
it probably does.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2015-12-23  2:34 ` NeilBrown
@ 2015-12-23 17:35   ` Dan Williams
  2015-12-23 22:39     ` NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2015-12-23 17:35 UTC (permalink / raw)
  To: NeilBrown; +Cc: Stanislav Samsonov, linux-raid

On Tue, Dec 22, 2015 at 6:34 PM, NeilBrown <neilb@suse.com> wrote:
> On Tue, Dec 22 2015, Stanislav Samsonov wrote:
>
>> Hi,
>>
>> Kernel 4.1.3 : there is some troubling kernel message that shows up
>> after enabling CONFIG_DEBUG_ATOMIC_SLEEP and testing DMA XOR
>> acceleration for raid5:
>>
>> BUG: sleeping function called from invalid context at mm/mempool.c:320
>> in_atomic(): 1, irqs_disabled(): 0, pid: 1048, name: md127_raid5
>> INFO: lockdep is turned off.
>> CPU: 1 PID: 1048 Comm: md127_raid5 Not tainted 4.1.15.alpine.1-dirty #1
>> Hardware name: Annapurna Labs Alpine
>> [<c00169d8>] (unwind_backtrace) from [<c0012a78>] (show_stack+0x10/0x14)
>> [<c0012a78>] (show_stack) from [<c07462ec>] (dump_stack+0x80/0xb4)
>> [<c07462ec>] (dump_stack) from [<c00bf2f0>] (mempool_alloc+0x68/0x13c)
>> [<c00bf2f0>] (mempool_alloc) from [<c041c9b4>]
>> (dmaengine_get_unmap_data+0x24/0x4c)
>> [<c041c9b4>] (dmaengine_get_unmap_data) from [<c03a8084>]
>> (async_xor_val+0x60/0x3a0)
>> [<c03a8084>] (async_xor_val) from [<c058e4c0>] (raid_run_ops+0xb70/0x1248)
>> [<c058e4c0>] (raid_run_ops) from [<c05915d4>] (handle_stripe+0x1068/0x22a8)
>> [<c05915d4>] (handle_stripe) from [<c0592ae4>]
>> (handle_active_stripes+0x2d0/0x3dc)
>> [<c0592ae4>] (handle_active_stripes) from [<c059300c>] (raid5d+0x384/0x5b0)
>> [<c059300c>] (raid5d) from [<c059db6c>] (md_thread+0x114/0x138)
>> [<c059db6c>] (md_thread) from [<c0042d54>] (kthread+0xe4/0x104)
>> [<c0042d54>] (kthread) from [<c000f658>] (ret_from_fork+0x14/0x3c)
>>
>> The reason is that async_xor_val() in crypto/async_tx/async_xor.c is
>> called in atomic context (preemption disabled) by raid_run_ops(). Then
>> it calls dmaengine_get_unmap_data() an then mempool_alloc() with
>> GFP_NOIO flag - this allocation type might sleep under some condition.
>>
>> Checked latest kernel 4.3 and it has exactly same flow.
>>
>> Any advice regarding this issue?
>
> Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
> dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
> issue... or make it crash even worse :-)
>
> Dan: do you have any wisdom here?  The xor is using the percpu data in
> raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
> Does the code handle failure to get_unmap_data() safely?  It looks like
> it probably does.

Those GFP_NOIO should move to GFP_NOWAIT.  We don't want GFP_ATOMIC
allocations to consume emergency reserves for a performance
optimization.  Longer term async_tx needs to be merged into md
directly as we can allocate this unmap data statically per-stripe
rather than per request. This asyntc_tx re-write has been on the todo
list for years, but never seems to make it to the top.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2015-12-23 17:35   ` Dan Williams
@ 2015-12-23 22:39     ` NeilBrown
  2015-12-23 22:46       ` Dan Williams
  0 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2015-12-23 22:39 UTC (permalink / raw)
  To: Dan Williams; +Cc: Stanislav Samsonov, linux-raid

[-- Attachment #1: Type: text/plain, Size: 4947 bytes --]

On Thu, Dec 24 2015, Dan Williams wrote:
>> Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
>> dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
>> issue... or make it crash even worse :-)
>>
>> Dan: do you have any wisdom here?  The xor is using the percpu data in
>> raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
>> Does the code handle failure to get_unmap_data() safely?  It looks like
>> it probably does.
>
> Those GFP_NOIO should move to GFP_NOWAIT.  We don't want GFP_ATOMIC
> allocations to consume emergency reserves for a performance
> optimization.  Longer term async_tx needs to be merged into md
> directly as we can allocate this unmap data statically per-stripe
> rather than per request. This asyntc_tx re-write has been on the todo
> list for years, but never seems to make it to the top.

So the following maybe?
If I could get an acked-by from you Dan, and a Tested-by: from you
Slava, I'll submit upstream.

Thanks,
NeilBrown

From: NeilBrown <neilb@suse.com>
Date: Thu, 24 Dec 2015 09:35:18 +1100
Subject: [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO

These async_XX functions are called from md/raid5 in an atomic
section, between get_cpu() and put_cpu(), so they must not sleep.
So use GFP_NOWAIT rather than GFP_IO.

Dan Williams writes: Longer term async_tx needs to be merged into md
directly as we can allocate this unmap data statically per-stripe
rather than per request.

Reported-by: Stanislav Samsonov <slava@annapurnalabs.com>
Signed-off-by: NeilBrown <neilb@suse.com>

diff --git a/crypto/async_tx/async_memcpy.c b/crypto/async_tx/async_memcpy.c
index f8c0b8dbeb75..88bc8e6b2a54 100644
--- a/crypto/async_tx/async_memcpy.c
+++ b/crypto/async_tx/async_memcpy.c
@@ -53,7 +53,7 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,
 	struct dmaengine_unmap_data *unmap = NULL;
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOWAIT);
 
 	if (unmap && is_dma_copy_aligned(device, src_offset, dest_offset, len)) {
 		unsigned long dma_prep_flags = 0;
diff --git a/crypto/async_tx/async_pq.c b/crypto/async_tx/async_pq.c
index 5d355e0c2633..c0748bbd4c08 100644
--- a/crypto/async_tx/async_pq.c
+++ b/crypto/async_tx/async_pq.c
@@ -188,7 +188,7 @@ async_gen_syndrome(struct page **blocks, unsigned int offset, int disks,
 	BUG_ON(disks > 255 || !(P(blocks, disks) || Q(blocks, disks)));
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOWAIT);
 
 	/* XORing P/Q is only implemented in software */
 	if (unmap && !(submit->flags & ASYNC_TX_PQ_XOR_DST) &&
@@ -307,7 +307,7 @@ async_syndrome_val(struct page **blocks, unsigned int offset, int disks,
 	BUG_ON(disks < 4);
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOWAIT);
 
 	if (unmap && disks <= dma_maxpq(device, 0) &&
 	    is_dma_pq_aligned(device, offset, 0, len)) {
diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index 934a84981495..8fab6275ea1f 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -41,7 +41,7 @@ async_sum_product(struct page *dest, struct page **srcs, unsigned char *coef,
 	u8 *a, *b, *c;
 
 	if (dma)
-		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOWAIT);
 
 	if (unmap) {
 		struct device *dev = dma->dev;
@@ -105,7 +105,7 @@ async_mult(struct page *dest, struct page *src, u8 coef, size_t len,
 	u8 *d, *s;
 
 	if (dma)
-		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOWAIT);
 
 	if (unmap) {
 		dma_addr_t dma_dest[2];
diff --git a/crypto/async_tx/async_xor.c b/crypto/async_tx/async_xor.c
index e1bce26cd4f9..da75777f2b3f 100644
--- a/crypto/async_tx/async_xor.c
+++ b/crypto/async_tx/async_xor.c
@@ -182,7 +182,7 @@ async_xor(struct page *dest, struct page **src_list, unsigned int offset,
 	BUG_ON(src_cnt <= 1);
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, src_cnt+1, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, src_cnt+1, GFP_NOWAIT);
 
 	if (unmap && is_dma_xor_aligned(device, offset, 0, len)) {
 		struct dma_async_tx_descriptor *tx;
@@ -278,7 +278,7 @@ async_xor_val(struct page *dest, struct page **src_list, unsigned int offset,
 	BUG_ON(src_cnt <= 1);
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, src_cnt, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, src_cnt, GFP_NOWAIT);
 
 	if (unmap && src_cnt <= device->max_xor &&
 	    is_dma_xor_aligned(device, offset, 0, len)) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2015-12-23 22:39     ` NeilBrown
@ 2015-12-23 22:46       ` Dan Williams
  2015-12-28  8:43         ` Stanislav Samsonov
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2015-12-23 22:46 UTC (permalink / raw)
  To: NeilBrown; +Cc: Stanislav Samsonov, linux-raid

On Wed, Dec 23, 2015 at 2:39 PM, NeilBrown <neilb@suse.com> wrote:
> On Thu, Dec 24 2015, Dan Williams wrote:
>>> Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
>>> dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
>>> issue... or make it crash even worse :-)
>>>
>>> Dan: do you have any wisdom here?  The xor is using the percpu data in
>>> raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
>>> Does the code handle failure to get_unmap_data() safely?  It looks like
>>> it probably does.
>>
>> Those GFP_NOIO should move to GFP_NOWAIT.  We don't want GFP_ATOMIC
>> allocations to consume emergency reserves for a performance
>> optimization.  Longer term async_tx needs to be merged into md
>> directly as we can allocate this unmap data statically per-stripe
>> rather than per request. This asyntc_tx re-write has been on the todo
>> list for years, but never seems to make it to the top.
>
> So the following maybe?
> If I could get an acked-by from you Dan, and a Tested-by: from you
> Slava, I'll submit upstream.
>
> Thanks,
> NeilBrown
>
> From: NeilBrown <neilb@suse.com>
> Date: Thu, 24 Dec 2015 09:35:18 +1100
> Subject: [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO
>
> These async_XX functions are called from md/raid5 in an atomic
> section, between get_cpu() and put_cpu(), so they must not sleep.
> So use GFP_NOWAIT rather than GFP_IO.
>
> Dan Williams writes: Longer term async_tx needs to be merged into md
> directly as we can allocate this unmap data statically per-stripe
> rather than per request.
>
> Reported-by: Stanislav Samsonov <slava@annapurnalabs.com>
> Signed-off-by: NeilBrown <neilb@suse.com>

Acked-by: Dan Williams <dan.j.williams@intel.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2015-12-23 22:46       ` Dan Williams
@ 2015-12-28  8:43         ` Stanislav Samsonov
  2016-01-04  1:33           ` NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Stanislav Samsonov @ 2015-12-28  8:43 UTC (permalink / raw)
  To: Dan Williams; +Cc: NeilBrown, linux-raid

On 24 December 2015 at 00:46, Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Dec 23, 2015 at 2:39 PM, NeilBrown <neilb@suse.com> wrote:
> > On Thu, Dec 24 2015, Dan Williams wrote:
> >>> Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
> >>> dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
> >>> issue... or make it crash even worse :-)
> >>>
> >>> Dan: do you have any wisdom here?  The xor is using the percpu data in
> >>> raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
> >>> Does the code handle failure to get_unmap_data() safely?  It looks like
> >>> it probably does.
> >>
> >> Those GFP_NOIO should move to GFP_NOWAIT.  We don't want GFP_ATOMIC
> >> allocations to consume emergency reserves for a performance
> >> optimization.  Longer term async_tx needs to be merged into md
> >> directly as we can allocate this unmap data statically per-stripe
> >> rather than per request. This asyntc_tx re-write has been on the todo
> >> list for years, but never seems to make it to the top.
> >
> > So the following maybe?
> > If I could get an acked-by from you Dan, and a Tested-by: from you
> > Slava, I'll submit upstream.
> >
> > Thanks,
> > NeilBrown
> >
> > From: NeilBrown <neilb@suse.com>
> > Date: Thu, 24 Dec 2015 09:35:18 +1100
> > Subject: [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO
> >
> > These async_XX functions are called from md/raid5 in an atomic
> > section, between get_cpu() and put_cpu(), so they must not sleep.
> > So use GFP_NOWAIT rather than GFP_IO.
> >
> > Dan Williams writes: Longer term async_tx needs to be merged into md
> > directly as we can allocate this unmap data statically per-stripe
> > rather than per request.
> >
> > Reported-by: Stanislav Samsonov <slava@annapurnalabs.com>
> > Signed-off-by: NeilBrown <neilb@suse.com>
>
> Acked-by: Dan Williams <dan.j.williams@intel.com>

Tested-by: Slava Samsonov <slava@annapurnalabs.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2015-12-28  8:43         ` Stanislav Samsonov
@ 2016-01-04  1:33           ` NeilBrown
  2016-01-04 17:28             ` Dan Williams
  0 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2016-01-04  1:33 UTC (permalink / raw)
  To: Stanislav Samsonov, Dan Williams; +Cc: linux-raid

[-- Attachment #1: Type: text/plain, Size: 2376 bytes --]

On Mon, Dec 28 2015, Stanislav Samsonov wrote:

> On 24 December 2015 at 00:46, Dan Williams <dan.j.williams@intel.com> wrote:
>>
>> On Wed, Dec 23, 2015 at 2:39 PM, NeilBrown <neilb@suse.com> wrote:
>> > On Thu, Dec 24 2015, Dan Williams wrote:
>> >>> Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
>> >>> dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
>> >>> issue... or make it crash even worse :-)
>> >>>
>> >>> Dan: do you have any wisdom here?  The xor is using the percpu data in
>> >>> raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
>> >>> Does the code handle failure to get_unmap_data() safely?  It looks like
>> >>> it probably does.
>> >>
>> >> Those GFP_NOIO should move to GFP_NOWAIT.  We don't want GFP_ATOMIC
>> >> allocations to consume emergency reserves for a performance
>> >> optimization.  Longer term async_tx needs to be merged into md
>> >> directly as we can allocate this unmap data statically per-stripe
>> >> rather than per request. This asyntc_tx re-write has been on the todo
>> >> list for years, but never seems to make it to the top.
>> >
>> > So the following maybe?
>> > If I could get an acked-by from you Dan, and a Tested-by: from you
>> > Slava, I'll submit upstream.
>> >
>> > Thanks,
>> > NeilBrown
>> >
>> > From: NeilBrown <neilb@suse.com>
>> > Date: Thu, 24 Dec 2015 09:35:18 +1100
>> > Subject: [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO
>> >
>> > These async_XX functions are called from md/raid5 in an atomic
>> > section, between get_cpu() and put_cpu(), so they must not sleep.
>> > So use GFP_NOWAIT rather than GFP_IO.
>> >
>> > Dan Williams writes: Longer term async_tx needs to be merged into md
>> > directly as we can allocate this unmap data statically per-stripe
>> > rather than per request.
>> >
>> > Reported-by: Stanislav Samsonov <slava@annapurnalabs.com>
>> > Signed-off-by: NeilBrown <neilb@suse.com>
>>
>> Acked-by: Dan Williams <dan.j.williams@intel.com>
>
> Tested-by: Slava Samsonov <slava@annapurnalabs.com>

Thanks.

I guess this was problem was introduced by
Commit: 7476bd79fc01 ("async_pq: convert to dmaengine_unmap_data")
in 3.13.  Do we think it deserves to go to -stable?

(I just realised that this is really Dan's code more than mine,
 so why am I submitting it ??? But we are here now so it may as well go
 in through the md tree.)

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2016-01-04  1:33           ` NeilBrown
@ 2016-01-04 17:28             ` Dan Williams
  2016-01-06  9:08               ` Vinod Koul
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2016-01-04 17:28 UTC (permalink / raw)
  To: NeilBrown; +Cc: Stanislav Samsonov, linux-raid, dmaengine, Vinod Koul

On Sun, Jan 3, 2016 at 5:33 PM, NeilBrown <neilb@suse.com> wrote:
> On Mon, Dec 28 2015, Stanislav Samsonov wrote:
>
>> On 24 December 2015 at 00:46, Dan Williams <dan.j.williams@intel.com> wrote:
>>>
>>> On Wed, Dec 23, 2015 at 2:39 PM, NeilBrown <neilb@suse.com> wrote:
>>> > On Thu, Dec 24 2015, Dan Williams wrote:
>>> >>> Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
>>> >>> dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
>>> >>> issue... or make it crash even worse :-)
>>> >>>
>>> >>> Dan: do you have any wisdom here?  The xor is using the percpu data in
>>> >>> raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
>>> >>> Does the code handle failure to get_unmap_data() safely?  It looks like
>>> >>> it probably does.
>>> >>
>>> >> Those GFP_NOIO should move to GFP_NOWAIT.  We don't want GFP_ATOMIC
>>> >> allocations to consume emergency reserves for a performance
>>> >> optimization.  Longer term async_tx needs to be merged into md
>>> >> directly as we can allocate this unmap data statically per-stripe
>>> >> rather than per request. This asyntc_tx re-write has been on the todo
>>> >> list for years, but never seems to make it to the top.
>>> >
>>> > So the following maybe?
>>> > If I could get an acked-by from you Dan, and a Tested-by: from you
>>> > Slava, I'll submit upstream.
>>> >
>>> > Thanks,
>>> > NeilBrown
>>> >
>>> > From: NeilBrown <neilb@suse.com>
>>> > Date: Thu, 24 Dec 2015 09:35:18 +1100
>>> > Subject: [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO
>>> >
>>> > These async_XX functions are called from md/raid5 in an atomic
>>> > section, between get_cpu() and put_cpu(), so they must not sleep.
>>> > So use GFP_NOWAIT rather than GFP_IO.
>>> >
>>> > Dan Williams writes: Longer term async_tx needs to be merged into md
>>> > directly as we can allocate this unmap data statically per-stripe
>>> > rather than per request.
>>> >
>>> > Reported-by: Stanislav Samsonov <slava@annapurnalabs.com>
>>> > Signed-off-by: NeilBrown <neilb@suse.com>
>>>
>>> Acked-by: Dan Williams <dan.j.williams@intel.com>
>>
>> Tested-by: Slava Samsonov <slava@annapurnalabs.com>
>
> Thanks.
>
> I guess this was problem was introduced by
> Commit: 7476bd79fc01 ("async_pq: convert to dmaengine_unmap_data")
> in 3.13.

Yes.

> Do we think it deserves to go to -stable?

I think so, yes.

> (I just realised that this is really Dan's code more than mine,
>  so why am I submitting it ???

True!  I was grateful for your offer, but I should have taken over
coordination...

> But we are here now so it may as well go
>  in through the md tree.)

That or Vinod is maintaining drivers/dma/ these days (added Cc's).

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: raid5 async_xor: sleep in atomic
  2016-01-04 17:28             ` Dan Williams
@ 2016-01-06  9:08               ` Vinod Koul
  2016-01-07  0:02                 ` [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2016-01-06  9:08 UTC (permalink / raw)
  To: Dan Williams; +Cc: NeilBrown, Stanislav Samsonov, linux-raid, dmaengine

On Mon, Jan 04, 2016 at 09:28:52AM -0800, Dan Williams wrote:
> On Sun, Jan 3, 2016 at 5:33 PM, NeilBrown <neilb@suse.com> wrote:
> > On Mon, Dec 28 2015, Stanislav Samsonov wrote:
> >
> >> On 24 December 2015 at 00:46, Dan Williams <dan.j.williams@intel.com> wrote:
> >>>
> >>> On Wed, Dec 23, 2015 at 2:39 PM, NeilBrown <neilb@suse.com> wrote:
> >>> > On Thu, Dec 24 2015, Dan Williams wrote:
> >>> >>> Changing the GFP_NOIO to GFP_ATOMIC in all the calls to
> >>> >>> dmaengine_get_unmap_data() in crypto/async_tx/ would probably fix the
> >>> >>> issue... or make it crash even worse :-)
> >>> >>>
> >>> >>> Dan: do you have any wisdom here?  The xor is using the percpu data in
> >>> >>> raid5, so it cannot be sleep, but GFP_NOIO allows sleep.
> >>> >>> Does the code handle failure to get_unmap_data() safely?  It looks like
> >>> >>> it probably does.
> >>> >>
> >>> >> Those GFP_NOIO should move to GFP_NOWAIT.  We don't want GFP_ATOMIC
> >>> >> allocations to consume emergency reserves for a performance
> >>> >> optimization.  Longer term async_tx needs to be merged into md
> >>> >> directly as we can allocate this unmap data statically per-stripe
> >>> >> rather than per request. This asyntc_tx re-write has been on the todo
> >>> >> list for years, but never seems to make it to the top.
> >>> >
> >>> > So the following maybe?
> >>> > If I could get an acked-by from you Dan, and a Tested-by: from you
> >>> > Slava, I'll submit upstream.
> >>> >
> >>> > Thanks,
> >>> > NeilBrown
> >>> >
> >>> > From: NeilBrown <neilb@suse.com>
> >>> > Date: Thu, 24 Dec 2015 09:35:18 +1100
> >>> > Subject: [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO
> >>> >
> >>> > These async_XX functions are called from md/raid5 in an atomic
> >>> > section, between get_cpu() and put_cpu(), so they must not sleep.
> >>> > So use GFP_NOWAIT rather than GFP_IO.
> >>> >
> >>> > Dan Williams writes: Longer term async_tx needs to be merged into md
> >>> > directly as we can allocate this unmap data statically per-stripe
> >>> > rather than per request.
> >>> >
> >>> > Reported-by: Stanislav Samsonov <slava@annapurnalabs.com>
> >>> > Signed-off-by: NeilBrown <neilb@suse.com>
> >>>
> >>> Acked-by: Dan Williams <dan.j.williams@intel.com>
> >>
> >> Tested-by: Slava Samsonov <slava@annapurnalabs.com>
> >
> > Thanks.
> >
> > I guess this was problem was introduced by
> > Commit: 7476bd79fc01 ("async_pq: convert to dmaengine_unmap_data")
> > in 3.13.
> 
> Yes.
> 
> > Do we think it deserves to go to -stable?
> 
> I think so, yes.
> 
> > (I just realised that this is really Dan's code more than mine,
> >  so why am I submitting it ???
> 
> True!  I was grateful for your offer, but I should have taken over
> coordination...
> 
> > But we are here now so it may as well go
> >  in through the md tree.)
> 
> That or Vinod is maintaining drivers/dma/ these days (added Cc's).

I can queue it up, pls send me the patch with ACKs. Looks like this might be
4.4 material, I am finalizing that in next day or so :)

-- 
~Vinod

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO
  2016-01-06  9:08               ` Vinod Koul
@ 2016-01-07  0:02                 ` NeilBrown
  2016-01-07  5:39                   ` Vinod Koul
  0 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2016-01-07  0:02 UTC (permalink / raw)
  To: Vinod Koul, Dan Williams; +Cc: Stanislav Samsonov, linux-raid, dmaengine

[-- Attachment #1: Type: text/plain, Size: 4391 bytes --]


These async_XX functions are called from md/raid5 in an atomic
section, between get_cpu() and put_cpu(), so they must not sleep.
So use GFP_NOWAIT rather than GFP_IO.

Dan Williams writes: Longer term async_tx needs to be merged into md
directly as we can allocate this unmap data statically per-stripe
rather than per request.

Fixed: 7476bd79fc01 ("async_pq: convert to dmaengine_unmap_data")
Cc: stable@vger.kernel.org (v3.13+)
Reported-and-tested-by: Stanislav Samsonov <slava@annapurnalabs.com>
Acked-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---

Thanks for taking this Vinod.
It is currently in linux-next from my md tree, but I've just de-staged
it so the next linux-next won't have it from me.
NeilBrown

 crypto/async_tx/async_memcpy.c      | 2 +-
 crypto/async_tx/async_pq.c          | 4 ++--
 crypto/async_tx/async_raid6_recov.c | 4 ++--
 crypto/async_tx/async_xor.c         | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/crypto/async_tx/async_memcpy.c b/crypto/async_tx/async_memcpy.c
index f8c0b8dbeb75..88bc8e6b2a54 100644
--- a/crypto/async_tx/async_memcpy.c
+++ b/crypto/async_tx/async_memcpy.c
@@ -53,7 +53,7 @@ async_memcpy(struct page *dest, struct page *src, unsigned int dest_offset,
 	struct dmaengine_unmap_data *unmap = NULL;
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOWAIT);
 
 	if (unmap && is_dma_copy_aligned(device, src_offset, dest_offset, len)) {
 		unsigned long dma_prep_flags = 0;
diff --git a/crypto/async_tx/async_pq.c b/crypto/async_tx/async_pq.c
index 5d355e0c2633..c0748bbd4c08 100644
--- a/crypto/async_tx/async_pq.c
+++ b/crypto/async_tx/async_pq.c
@@ -188,7 +188,7 @@ async_gen_syndrome(struct page **blocks, unsigned int offset, int disks,
 	BUG_ON(disks > 255 || !(P(blocks, disks) || Q(blocks, disks)));
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOWAIT);
 
 	/* XORing P/Q is only implemented in software */
 	if (unmap && !(submit->flags & ASYNC_TX_PQ_XOR_DST) &&
@@ -307,7 +307,7 @@ async_syndrome_val(struct page **blocks, unsigned int offset, int disks,
 	BUG_ON(disks < 4);
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOWAIT);
 
 	if (unmap && disks <= dma_maxpq(device, 0) &&
 	    is_dma_pq_aligned(device, offset, 0, len)) {
diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index 934a84981495..8fab6275ea1f 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -41,7 +41,7 @@ async_sum_product(struct page *dest, struct page **srcs, unsigned char *coef,
 	u8 *a, *b, *c;
 
 	if (dma)
-		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOWAIT);
 
 	if (unmap) {
 		struct device *dev = dma->dev;
@@ -105,7 +105,7 @@ async_mult(struct page *dest, struct page *src, u8 coef, size_t len,
 	u8 *d, *s;
 
 	if (dma)
-		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(dma->dev, 3, GFP_NOWAIT);
 
 	if (unmap) {
 		dma_addr_t dma_dest[2];
diff --git a/crypto/async_tx/async_xor.c b/crypto/async_tx/async_xor.c
index e1bce26cd4f9..da75777f2b3f 100644
--- a/crypto/async_tx/async_xor.c
+++ b/crypto/async_tx/async_xor.c
@@ -182,7 +182,7 @@ async_xor(struct page *dest, struct page **src_list, unsigned int offset,
 	BUG_ON(src_cnt <= 1);
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, src_cnt+1, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, src_cnt+1, GFP_NOWAIT);
 
 	if (unmap && is_dma_xor_aligned(device, offset, 0, len)) {
 		struct dma_async_tx_descriptor *tx;
@@ -278,7 +278,7 @@ async_xor_val(struct page *dest, struct page **src_list, unsigned int offset,
 	BUG_ON(src_cnt <= 1);
 
 	if (device)
-		unmap = dmaengine_get_unmap_data(device->dev, src_cnt, GFP_NOIO);
+		unmap = dmaengine_get_unmap_data(device->dev, src_cnt, GFP_NOWAIT);
 
 	if (unmap && src_cnt <= device->max_xor &&
 	    is_dma_xor_aligned(device, offset, 0, len)) {
-- 
2.6.4


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO
  2016-01-07  0:02                 ` [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO NeilBrown
@ 2016-01-07  5:39                   ` Vinod Koul
  0 siblings, 0 replies; 11+ messages in thread
From: Vinod Koul @ 2016-01-07  5:39 UTC (permalink / raw)
  To: NeilBrown; +Cc: Dan Williams, Stanislav Samsonov, linux-raid, dmaengine

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On Thu, Jan 07, 2016 at 11:02:34AM +1100, NeilBrown wrote:
> 
> These async_XX functions are called from md/raid5 in an atomic
> section, between get_cpu() and put_cpu(), so they must not sleep.
> So use GFP_NOWAIT rather than GFP_IO.
> 
> Dan Williams writes: Longer term async_tx needs to be merged into md
> directly as we can allocate this unmap data statically per-stripe
> rather than per request.
> 
> Fixed: 7476bd79fc01 ("async_pq: convert to dmaengine_unmap_data")
> Cc: stable@vger.kernel.org (v3.13+)
> Reported-and-tested-by: Stanislav Samsonov <slava@annapurnalabs.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> 
> Thanks for taking this Vinod.
> It is currently in linux-next from my md tree, but I've just de-staged
> it so the next linux-next won't have it from me.

Okay this is not in dmaengine. But since we all agreed, I have picked and
will send to Linus later today. If anyone has any objections please speak up

Thanks
-- 
~Vinod

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-01-07  5:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-22 11:58 raid5 async_xor: sleep in atomic Stanislav Samsonov
2015-12-23  2:34 ` NeilBrown
2015-12-23 17:35   ` Dan Williams
2015-12-23 22:39     ` NeilBrown
2015-12-23 22:46       ` Dan Williams
2015-12-28  8:43         ` Stanislav Samsonov
2016-01-04  1:33           ` NeilBrown
2016-01-04 17:28             ` Dan Williams
2016-01-06  9:08               ` Vinod Koul
2016-01-07  0:02                 ` [PATCH] async_tx: use GFP_NOWAIT rather than GFP_IO NeilBrown
2016-01-07  5:39                   ` Vinod Koul

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.