* 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.