Re: scsi: non atomic allocation in mempool_alloc in atomic context

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Alexei Starovoitov]

On Mon, Jan 5, 2015 at 11:42 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Jan 05, 2015 at 12:38:04PM -0700, Jens Axboe wrote:
>> That was true in earlier kernels as well, going back a few versions at
>> least, preempt was disabled on calling __blk_mq_run_hw_queue(). Just
>> checked, and 3.16 and later have that as the behaviour. The only change
>> in 3.19 some shuffling around to avoid double preempt_disable in some
>> cases, it's now using get_cpu() and friends.
>>
>> So we probably want do mark that as stable so we reach back to when
>> scsi-mq was added, unless the originally referenced patch getting rid of
>> the gfp_t mask didn't have the issue.
>
> Before that commit we always passed down GFP_ATOMIC, so we'll only
> need the patch for 3.19.

I'm seeing the same splats... what tree I can pull the fix from ?

Thanks!

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Christoph Hellwig]

On Wed, Jan 07, 2015 at 07:55:42PM -0800, Alexei Starovoitov wrote:
> I'm seeing the same splats... what tree I can pull the fix from ?

None so far.  I'll still need a review to apply it to the scsi-queue tree.

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Alexei Starovoitov]

On Thu, Jan 8, 2015 at 1:29 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Jan 07, 2015 at 07:55:42PM -0800, Alexei Starovoitov wrote:
>> I'm seeing the same splats... what tree I can pull the fix from ?
>
> None so far.  I'll still need a review to apply it to the scsi-queue tree.

applied it manually and warning is gone. It used to
spam a lot of them.

scsi: non atomic allocation in mempool_alloc in atomic context

[Sasha Levin]

Hi Christoph,

I'm seeing an issue which was bisected down to 3c356bde1 ("scsi: stop passing
a gfp_mask argument down the command setup path"):

[ 3395.328221] BUG: sleeping function called from invalid context at mm/mempool.c:206
[ 3395.329540] in_atomic(): 1, irqs_disabled(): 0, pid: 6399, name: trinity-c531
[ 3395.331104] no locks held by trinity-c531/6399.
[ 3395.331849] Preemption disabled blk_execute_rq_nowait (block/blk-exec.c:95)
[ 3395.333145]
[ 3395.333392] CPU: 5 PID: 6399 Comm: trinity-c531 Not tainted 3.19.0-rc1-next-20141226-sasha-00051-g2dd3d73 #1646
[ 3395.335266]  0000000000000000 0000000000000000 ffff880608948000 ffff880645a07548
[ 3395.337679]  ffffffff9137c79d 0000000000000000 ffff880608948000 ffff880645a07588
[ 3395.340220]  ffffffff814ad713 ffffffff9de20590 ffff880608948000 ffffffff926a166c
[ 3395.342643] Call Trace:
[ 3395.345099] dump_stack (lib/dump_stack.c:52)
[ 3395.346793] ___might_sleep (kernel/sched/core.c:7342)
[ 3395.348571] __might_sleep (kernel/sched/core.c:7308)
[ 3395.351944] mempool_alloc (mm/mempool.c:206 (discriminator 1))
[ 3395.355196] scsi_sg_alloc (drivers/scsi/scsi_lib.c:582)
[ 3395.356893] __sg_alloc_table (lib/scatterlist.c:282)
[ 3395.358844] ? sdev_disable_disk_events (drivers/scsi/scsi_lib.c:577)
[ 3395.360873] scsi_alloc_sgtable (drivers/scsi/scsi_lib.c:608)
[ 3395.362769] scsi_init_sgtable (drivers/scsi/scsi_lib.c:1087)
[ 3395.364583] ? lockdep_init_map (kernel/locking/lockdep.c:2986)
[ 3395.366354] scsi_init_io (drivers/scsi/scsi_lib.c:1122)
[ 3395.368092] ? do_init_timer (kernel/time/timer.c:669)
[ 3395.369837] scsi_setup_cmnd (drivers/scsi/scsi_lib.c:1220 drivers/scsi/scsi_lib.c:1268)
[ 3395.371743] scsi_queue_rq (drivers/scsi/scsi_lib.c:1875 drivers/scsi/scsi_lib.c:1980)
[ 3395.373471] __blk_mq_run_hw_queue (block/blk-mq.c:751)
[ 3395.375481] blk_mq_run_hw_queue (block/blk-mq.c:831)
[ 3395.377324] blk_mq_insert_request (block/blk-mq.h:92 block/blk-mq.c:974)
[ 3395.379377] ? blk_rq_map_user (block/blk-map.c:78 block/blk-map.c:142)
[ 3395.381307] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2559 kernel/locking/lockdep.c:2601)
[ 3395.383485] blk_execute_rq_nowait (block/blk-exec.c:95)
[ 3395.386995] sg_common_write.isra.2 (drivers/scsi/sg.c:823)
[ 3395.388712] ? might_fault (mm/memory.c:3730)
[ 3395.390624] sg_write (drivers/scsi/sg.c:686)
[ 3395.403014] do_loop_readv_writev (fs/read_write.c:722)
[ 3395.407429] do_readv_writev (fs/read_write.c:854)
[ 3395.415486] vfs_writev (fs/read_write.c:893)
[ 3395.417116] SyS_writev (fs/read_write.c:926 fs/read_write.c:917)
[ 3395.418851] ? trace_hardirqs_on_thunk (arch/x86/lib/thunk_64.S:33)
[ 3395.420922] system_call_fastpath (arch/x86/kernel/entry_64.S:423)


Thanks,
Sasha

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Douglas Gilbert]

On 14-12-31 01:14 PM, Sasha Levin wrote:
> Hi Christoph,
>
> I'm seeing an issue which was bisected down to 3c356bde1 ("scsi: stop passing
> a gfp_mask argument down the command setup path"):
>
> [ 3395.328221] BUG: sleeping function called from invalid context at mm/mempool.c:206
> [ 3395.329540] in_atomic(): 1, irqs_disabled(): 0, pid: 6399, name: trinity-c531
> [ 3395.331104] no locks held by trinity-c531/6399.
> [ 3395.331849] Preemption disabled blk_execute_rq_nowait (block/blk-exec.c:95)
> [ 3395.333145]
> [ 3395.333392] CPU: 5 PID: 6399 Comm: trinity-c531 Not tainted 3.19.0-rc1-next-20141226-sasha-00051-g2dd3d73 #1646
> [ 3395.335266]  0000000000000000 0000000000000000 ffff880608948000 ffff880645a07548
> [ 3395.337679]  ffffffff9137c79d 0000000000000000 ffff880608948000 ffff880645a07588
> [ 3395.340220]  ffffffff814ad713 ffffffff9de20590 ffff880608948000 ffffffff926a166c
> [ 3395.342643] Call Trace:
> [ 3395.345099] dump_stack (lib/dump_stack.c:52)
> [ 3395.346793] ___might_sleep (kernel/sched/core.c:7342)
> [ 3395.348571] __might_sleep (kernel/sched/core.c:7308)
> [ 3395.351944] mempool_alloc (mm/mempool.c:206 (discriminator 1))
> [ 3395.355196] scsi_sg_alloc (drivers/scsi/scsi_lib.c:582)
> [ 3395.356893] __sg_alloc_table (lib/scatterlist.c:282)
> [ 3395.358844] ? sdev_disable_disk_events (drivers/scsi/scsi_lib.c:577)
> [ 3395.360873] scsi_alloc_sgtable (drivers/scsi/scsi_lib.c:608)
> [ 3395.362769] scsi_init_sgtable (drivers/scsi/scsi_lib.c:1087)
> [ 3395.364583] ? lockdep_init_map (kernel/locking/lockdep.c:2986)
> [ 3395.366354] scsi_init_io (drivers/scsi/scsi_lib.c:1122)
> [ 3395.368092] ? do_init_timer (kernel/time/timer.c:669)
> [ 3395.369837] scsi_setup_cmnd (drivers/scsi/scsi_lib.c:1220 drivers/scsi/scsi_lib.c:1268)
> [ 3395.371743] scsi_queue_rq (drivers/scsi/scsi_lib.c:1875 drivers/scsi/scsi_lib.c:1980)
> [ 3395.373471] __blk_mq_run_hw_queue (block/blk-mq.c:751)
> [ 3395.375481] blk_mq_run_hw_queue (block/blk-mq.c:831)
> [ 3395.377324] blk_mq_insert_request (block/blk-mq.h:92 block/blk-mq.c:974)
> [ 3395.379377] ? blk_rq_map_user (block/blk-map.c:78 block/blk-map.c:142)
> [ 3395.381307] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2559 kernel/locking/lockdep.c:2601)
> [ 3395.383485] blk_execute_rq_nowait (block/blk-exec.c:95)
> [ 3395.386995] sg_common_write.isra.2 (drivers/scsi/sg.c:823)
> [ 3395.388712] ? might_fault (mm/memory.c:3730)
> [ 3395.390624] sg_write (drivers/scsi/sg.c:686)
> [ 3395.403014] do_loop_readv_writev (fs/read_write.c:722)
> [ 3395.407429] do_readv_writev (fs/read_write.c:854)
> [ 3395.415486] vfs_writev (fs/read_write.c:893)
> [ 3395.417116] SyS_writev (fs/read_write.c:926 fs/read_write.c:917)
> [ 3395.418851] ? trace_hardirqs_on_thunk (arch/x86/lib/thunk_64.S:33)
> [ 3395.420922] system_call_fastpath (arch/x86/kernel/entry_64.S:423)

Looks interesting: vfs injecting SCSI commands with iovec
through the sg driver's async interface.


The problem seems to be here in scsi_lib.c, mq=true:

static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents, bool mq)
{
         struct scatterlist *first_chunk = NULL;
         gfp_t gfp_mask = mq ? GFP_NOIO : GFP_ATOMIC;
         int ret;
...

What is the downside of setting gfp_mask to GFP_ATOMIC
in all cases?

Doug Gilbert

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Christoph Hellwig]

On Wed, Dec 31, 2014 at 01:14:19PM -0500, Sasha Levin wrote:
> Hi Christoph,
> 
> I'm seeing an issue which was bisected down to 3c356bde1 ("scsi: stop passing
> a gfp_mask argument down the command setup path"):

->queue_rq in blk-mq context is designed to be able to sleep and be called
from process context without any spinlocks held or irqs disabled, so we
really should fix the
caller instead.

That being said your trace seems odd to me:

> [ 3395.328221] BUG: sleeping function called from invalid context at mm/mempool.c:206
> [ 3395.329540] in_atomic(): 1, irqs_disabled(): 0, pid: 6399, name: trinity-c531
> [ 3395.331104] no locks held by trinity-c531/6399.
> [ 3395.331849] Preemption disabled blk_execute_rq_nowait (block/blk-exec.c:95)

blk_execute_rq_nowait only takes a lock for the non-blk-mq case.  In my
current kernel that's in line 79, but can you verify that for you
line 95 is the spin_lock_irq in the !q->mq_ops case?

> [ 3395.348571] __might_sleep (kernel/sched/core.c:7308)
> [ 3395.351944] mempool_alloc (mm/mempool.c:206 (discriminator 1))
> [ 3395.355196] scsi_sg_alloc (drivers/scsi/scsi_lib.c:582)
> [ 3395.356893] __sg_alloc_table (lib/scatterlist.c:282)
> [ 3395.358844] ? sdev_disable_disk_events (drivers/scsi/scsi_lib.c:577)
> [ 3395.360873] scsi_alloc_sgtable (drivers/scsi/scsi_lib.c:608)
> [ 3395.362769] scsi_init_sgtable (drivers/scsi/scsi_lib.c:1087)
> [ 3395.364583] ? lockdep_init_map (kernel/locking/lockdep.c:2986)
> [ 3395.366354] scsi_init_io (drivers/scsi/scsi_lib.c:1122)
> [ 3395.368092] ? do_init_timer (kernel/time/timer.c:669)
> [ 3395.369837] scsi_setup_cmnd (drivers/scsi/scsi_lib.c:1220 drivers/scsi/scsi_lib.c:1268)
> [ 3395.371743] scsi_queue_rq (drivers/scsi/scsi_lib.c:1875 drivers/scsi/scsi_lib.c:1980)
> [ 3395.373471] __blk_mq_run_hw_queue (block/blk-mq.c:751)
> [ 3395.375481] blk_mq_run_hw_queue (block/blk-mq.c:831)
> [ 3395.377324] blk_mq_insert_request (block/blk-mq.h:92 block/blk-mq.c:974)
> [ 3395.379377] ? blk_rq_map_user (block/blk-map.c:78 block/blk-map.c:142)
> [ 3395.381307] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2559 kernel/locking/lockdep.c:2601)
> [ 3395.383485] blk_execute_rq_nowait (block/blk-exec.c:95)

But this clearly is the blk-mq case.  How does your version of
blk_execute_rq_nowait look like?

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Sasha Levin]

On 01/05/2015 04:15 AM, Christoph Hellwig wrote:
> On Wed, Dec 31, 2014 at 01:14:19PM -0500, Sasha Levin wrote:
>> Hi Christoph,
>>
>> I'm seeing an issue which was bisected down to 3c356bde1 ("scsi: stop passing
>> a gfp_mask argument down the command setup path"):
> 
> ->queue_rq in blk-mq context is designed to be able to sleep and be called
> from process context without any spinlocks held or irqs disabled, so we
> really should fix the
> caller instead.
> 
> That being said your trace seems odd to me:
> 
>> [ 3395.328221] BUG: sleeping function called from invalid context at mm/mempool.c:206
>> [ 3395.329540] in_atomic(): 1, irqs_disabled(): 0, pid: 6399, name: trinity-c531
>> [ 3395.331104] no locks held by trinity-c531/6399.
>> [ 3395.331849] Preemption disabled blk_execute_rq_nowait (block/blk-exec.c:95)
> 
> blk_execute_rq_nowait only takes a lock for the non-blk-mq case.  In my
> current kernel that's in line 79, but can you verify that for you
> line 95 is the spin_lock_irq in the !q->mq_ops case?

That's line 79 for me as well. I'm not sure why addr2line said it's line 95 here.

>> [ 3395.348571] __might_sleep (kernel/sched/core.c:7308)
>> [ 3395.351944] mempool_alloc (mm/mempool.c:206 (discriminator 1))
>> [ 3395.355196] scsi_sg_alloc (drivers/scsi/scsi_lib.c:582)
>> [ 3395.356893] __sg_alloc_table (lib/scatterlist.c:282)
>> [ 3395.358844] ? sdev_disable_disk_events (drivers/scsi/scsi_lib.c:577)
>> [ 3395.360873] scsi_alloc_sgtable (drivers/scsi/scsi_lib.c:608)
>> [ 3395.362769] scsi_init_sgtable (drivers/scsi/scsi_lib.c:1087)
>> [ 3395.364583] ? lockdep_init_map (kernel/locking/lockdep.c:2986)
>> [ 3395.366354] scsi_init_io (drivers/scsi/scsi_lib.c:1122)
>> [ 3395.368092] ? do_init_timer (kernel/time/timer.c:669)
>> [ 3395.369837] scsi_setup_cmnd (drivers/scsi/scsi_lib.c:1220 drivers/scsi/scsi_lib.c:1268)
>> [ 3395.371743] scsi_queue_rq (drivers/scsi/scsi_lib.c:1875 drivers/scsi/scsi_lib.c:1980)
>> [ 3395.373471] __blk_mq_run_hw_queue (block/blk-mq.c:751)
>> [ 3395.375481] blk_mq_run_hw_queue (block/blk-mq.c:831)
>> [ 3395.377324] blk_mq_insert_request (block/blk-mq.h:92 block/blk-mq.c:974)
>> [ 3395.379377] ? blk_rq_map_user (block/blk-map.c:78 block/blk-map.c:142)
>> [ 3395.381307] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2559 kernel/locking/lockdep.c:2601)
>> [ 3395.383485] blk_execute_rq_nowait (block/blk-exec.c:95)
> 
> But this clearly is the blk-mq case.  How does your version of
> blk_execute_rq_nowait look like?

It's whatever -next had. I've looked at objdump and it looks like the compiler made
something "interesting" with it that might explain the odd line numbering for the
preemption off thing:

/home/sasha/linux-next/block/blk-exec.c:69
                blk_mq_insert_request(rq, at_head, true, false);
  b9:   31 f6                   xor    %esi,%esi
  bb:   45 85 ff                test   %r15d,%r15d
  be:   48 89 df                mov    %rbx,%rdi
  c1:   40 0f 95 c6             setne  %sil
  c5:   ba 01 00 00 00          mov    $0x1,%edx
  ca:   31 c9                   xor    %ecx,%ecx
  cc:   e8 00 00 00 00          callq  d1 <blk_execute_rq_nowait+0xd1>
                        cd: R_X86_64_PC32       blk_mq_insert_request-0x4
/home/sasha/linux-next/block/blk-exec.c:95
        __blk_run_queue(q);
        /* the queue is stopped so it won't be run */
        if (is_pm_resume)
                __blk_run_queue_uncond(q);
        spin_unlock_irq(q->queue_lock);
}
  d1:   48 83 c4 18             add    $0x18,%rsp
  d5:   5b                      pop    %rbx
  d6:   41 5c                   pop    %r12
  d8:   41 5d                   pop    %r13
  da:   41 5e                   pop    %r14
  dc:   41 5f                   pop    %r15
  de:   5d                      pop    %rbp
  df:   c3                      retq

Or with the whole stack trace really...


Thanks,
Sasha

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Jens Axboe]

On 01/05/2015 02:15 AM, Christoph Hellwig wrote:
> On Wed, Dec 31, 2014 at 01:14:19PM -0500, Sasha Levin wrote:
>> Hi Christoph,
>>
>> I'm seeing an issue which was bisected down to 3c356bde1 ("scsi: stop passing
>> a gfp_mask argument down the command setup path"):
> 
> ->queue_rq in blk-mq context is designed to be able to sleep and be called
> from process context without any spinlocks held or irqs disabled, so we
> really should fix the
> caller instead.

That's not quite true, the only guarantee is that it WILL execute on the
CPU (or CPUs) that are set in the mask. So unless it ends up offloading
the run to a specific workqueue, we'll disable preempt in the current
path before ->queue_rq() is called.

-- 
Jens Axboe

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Christoph Hellwig]

On Mon, Jan 05, 2015 at 12:00:58PM -0700, Jens Axboe wrote:
> That's not quite true, the only guarantee is that it WILL execute on the
> CPU (or CPUs) that are set in the mask. So unless it ends up offloading
> the run to a specific workqueue, we'll disable preempt in the current
> path before ->queue_rq() is called.

Oops.  Indeed, with those recent changes ->queue_rq can't safely block
for memory allocatios anymore.

The patch below should fix it:

---
From: Christoph Hellwig <hch@lst.de>
Subject: scsi: ->queue_rq can't sleep

Since Linux 3.19 blk-mq may disable preemption before calling into
->queue_rq, so we can't actually sleep anymore.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9ea95dd..6d5c0b8 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -591,7 +591,6 @@ static void scsi_free_sgtable(struct scsi_data_buffer *sdb, bool mq)
 static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents, bool mq)
 {
 	struct scatterlist *first_chunk = NULL;
-	gfp_t gfp_mask = mq ? GFP_NOIO : GFP_ATOMIC;
 	int ret;
 
 	BUG_ON(!nents);
@@ -606,7 +605,7 @@ static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents, bool mq)
 	}
 
 	ret = __sg_alloc_table(&sdb->table, nents, SCSI_MAX_SG_SEGMENTS,
-			       first_chunk, gfp_mask, scsi_sg_alloc);
+			       first_chunk, GFP_ATOMIC, scsi_sg_alloc);
 	if (unlikely(ret))
 		scsi_free_sgtable(sdb, mq);
 	return ret;
-- 
1.9.1

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Jens Axboe]

On 01/05/2015 12:32 PM, Christoph Hellwig wrote:
> On Mon, Jan 05, 2015 at 12:00:58PM -0700, Jens Axboe wrote:
>> That's not quite true, the only guarantee is that it WILL execute on the
>> CPU (or CPUs) that are set in the mask. So unless it ends up offloading
>> the run to a specific workqueue, we'll disable preempt in the current
>> path before ->queue_rq() is called.
> 
> Oops.  Indeed, with those recent changes ->queue_rq can't safely block
> for memory allocatios anymore.
> 
> The patch below should fix it:
> 
> ---
> From: Christoph Hellwig <hch@lst.de>
> Subject: scsi: ->queue_rq can't sleep
> 
> Since Linux 3.19 blk-mq may disable preemption before calling into
> ->queue_rq, so we can't actually sleep anymore.

That was true in earlier kernels as well, going back a few versions at
least, preempt was disabled on calling __blk_mq_run_hw_queue(). Just
checked, and 3.16 and later have that as the behaviour. The only change
in 3.19 some shuffling around to avoid double preempt_disable in some
cases, it's now using get_cpu() and friends.

So we probably want do mark that as stable so we reach back to when
scsi-mq was added, unless the originally referenced patch getting rid of
the gfp_t mask didn't have the issue.


-- 
Jens Axboe

Re: scsi: non atomic allocation in mempool_alloc in atomic context

[Christoph Hellwig]

On Mon, Jan 05, 2015 at 12:38:04PM -0700, Jens Axboe wrote:
> That was true in earlier kernels as well, going back a few versions at
> least, preempt was disabled on calling __blk_mq_run_hw_queue(). Just
> checked, and 3.16 and later have that as the behaviour. The only change
> in 3.19 some shuffling around to avoid double preempt_disable in some
> cases, it's now using get_cpu() and friends.
> 
> So we probably want do mark that as stable so we reach back to when
> scsi-mq was added, unless the originally referenced patch getting rid of
> the gfp_t mask didn't have the issue.

Before that commit we always passed down GFP_ATOMIC, so we'll only
need the patch for 3.19.