scsi: non atomic allocation in mempool_alloc in atomic context

DISCUSSION Sasha Levin 4 participants 8 messages Dec 31, 2014 – Jan 5, 2015 SCSI

Share & tools

Douglas Gilbert

2014-12-31 19:56 UTC #1 lore raw

Quoted 36 lines · 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)

Quoted 8 lines

[ 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

Christoph Hellwig

2015-01-05 09:15 UTC #2 lore raw

In reply to Sasha Levin

Quoted 4 lines · 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"):

->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:

Quoted 4 lines

[ 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?

Quoted 10 lines

[ 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)

Quoted 8 lines

[ 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?

Sasha Levin

author

2015-01-05 15:17 UTC #3 lore raw

Quoted 13 lines · Christoph Hellwig

Quoted 4 lines · 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"):
->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:

Quoted 8 lines

Quoted 4 lines
[ 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.

Quoted 19 lines

Quoted 10 lines

[ 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)

Quoted 8 lines

[ 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)

Quoted 2 lines

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

Jens Axboe

2015-01-05 19:00 UTC #4 lore raw

In reply to Christoph Hellwig

Quoted 10 lines · Christoph Hellwig

Quoted 4 lines · 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"):
->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.

Signature · 1 line

Jens Axboe

Christoph Hellwig

2015-01-05 19:32 UTC #5 lore raw

Quoted 4 lines · Jens Axboe

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 <[email protected]>
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 <[email protected]>

---
 drivers/scsi/scsi_lib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/scsi/scsi_lib.c +1 -2

index 9ea95dd..6d5c0b8 100644
@@ -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;

Signature · 1 line

1.9.1

Jens Axboe

2015-01-05 19:38 UTC #6 lore raw

Quoted 9 lines · Christoph Hellwig

Quoted 4 lines · Jens Axboe
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.

Quoted 8 lines

The patch below should fix it:
---
From: Christoph Hellwig <[email protected]>
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.

Signature · 1 line

Jens Axboe

Christoph Hellwig

2015-01-05 19:42 UTC #7 lore raw

Quoted 9 lines · Jens Axboe

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.