* INFO: task syz-executor can't die for more than 143 seconds. (2)
@ 2019-10-23 7:56 syzbot
2019-10-24 10:08 ` Tetsuo Handa
0 siblings, 1 reply; 14+ messages in thread
From: syzbot @ 2019-10-23 7:56 UTC (permalink / raw)
To: axboe, linux-block, linux-kernel, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: c4b9850b Add linux-next specific files for 20191018
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=177b3ab0e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=c940ef12efcd1ec
dashboard link: https://syzkaller.appspot.com/bug?extid=b48daca8639150bc5e73
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1356b8ff600000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14f48687600000
Bisection is inconclusive: the first bad commit could be any of:
9211bfbf netfilter: add missing IS_ENABLED(CONFIG_BRIDGE_NETFILTER) checks
to header-file.
47e640af netfilter: add missing IS_ENABLED(CONFIG_NF_TABLES) check to
header-file.
a1b2f04e netfilter: add missing includes to a number of header-files.
0abc8bf4 netfilter: add missing IS_ENABLED(CONFIG_NF_CONNTRACK) checks to
some header-files.
bd96b4c7 netfilter: inline four headers files into another one.
43dd16ef netfilter: nf_tables: store data in offload context registers
78458e3e netfilter: add missing IS_ENABLED(CONFIG_NETFILTER) checks to some
header-files.
20a9379d netfilter: remove "#ifdef __KERNEL__" guards from some headers.
bd8699e9 netfilter: nft_bitwise: add offload support
2a475c40 kbuild: remove all netfilter headers from header-test blacklist.
7e59b3fe netfilter: remove unnecessary spaces
1b90af29 ipvs: Improve robustness to the ipvs sysctl
5785cf15 netfilter: nf_tables: add missing prototypes.
0a30ba50 netfilter: nf_nat_proto: make tables static
e84fb4b3 netfilter: conntrack: use shared sysctl constants
10533343 netfilter: connlabels: prefer static lock initialiser
8c0bb787 netfilter: synproxy: rename mss synproxy_options field
c162610c Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=143869e8e00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+b48daca8639150bc5e73@syzkaller.appspotmail.com
INFO: task syz-executor350:8656 can't die for more than 143 seconds.
syz-executor350 R running task 27256 8656 8654 0x00004006
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x94a/0x1e70 kernel/sched/core.c:4069
schedule+0xd9/0x260 kernel/sched/core.c:4136
io_schedule+0x1c/0x70 kernel/sched/core.c:5780
rq_qos_wait+0x301/0x3f0 block/blk-rq-qos.c:288
__wbt_wait block/blk-wbt.c:526 [inline]
wbt_wait+0x20b/0x370 block/blk-wbt.c:591
__rq_qos_throttle+0x56/0xa0 block/blk-rq-qos.c:72
rq_qos_throttle block/blk-rq-qos.h:182 [inline]
blk_mq_make_request+0x3d0/0x2280 block/blk-mq.c:1943
generic_make_request block/blk-core.c:1093 [inline]
generic_make_request+0x23c/0xb50 block/blk-core.c:1035
submit_bio+0x113/0x600 block/blk-core.c:1219
blk_next_bio+0x4a/0x60 block/blk-lib.c:19
__blkdev_issue_zero_pages+0x151/0x430 block/blk-lib.c:284
blkdev_issue_zeroout+0x434/0x4c0 block/blk-lib.c:378
blkdev_fallocate+0x2fc/0x410 fs/block_dev.c:2089
vfs_fallocate+0x4aa/0xa50 fs/open.c:309
ksys_fallocate+0x58/0xa0 fs/open.c:332
__do_sys_fallocate fs/open.c:340 [inline]
__se_sys_fallocate fs/open.c:338 [inline]
__x64_sys_fallocate+0x97/0xf0 fs/open.c:338
do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x441269
Code: e8 ac e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 eb 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffc6d0e55e8 EFLAGS: 00000246 ORIG_RAX: 000000000000011d
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000441269
RDX: 0000000000000000 RSI: 0000000000000011 RDI: 0000000000000003
RBP: 00000000006cb018 R08: 00000000004002c8 R09: 00000000004002c8
R10: 0000400000000200 R11: 0000000000000246 R12: 0000000000401fe0
R13: 0000000000402070 R14: 0000000000000000 R15: 0000000000000000
INFO: task syz-executor350:8661 can't die for more than 144 seconds.
syz-executor350 R running task 27256 8661 8660 0x00004006
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x94a/0x1e70 kernel/sched/core.c:4069
schedule+0xd9/0x260 kernel/sched/core.c:4136
io_schedule+0x1c/0x70 kernel/sched/core.c:5780
rq_qos_wait+0x301/0x3f0 block/blk-rq-qos.c:288
__wbt_wait block/blk-wbt.c:526 [inline]
wbt_wait+0x20b/0x370 block/blk-wbt.c:591
__rq_qos_throttle+0x56/0xa0 block/blk-rq-qos.c:72
rq_qos_throttle block/blk-rq-qos.h:182 [inline]
blk_mq_make_request+0x3d0/0x2280 block/blk-mq.c:1943
generic_make_request block/blk-core.c:1093 [inline]
generic_make_request+0x23c/0xb50 block/blk-core.c:1035
submit_bio+0x113/0x600 block/blk-core.c:1219
blk_next_bio+0x4a/0x60 block/blk-lib.c:19
__blkdev_issue_zero_pages+0x151/0x430 block/blk-lib.c:284
blkdev_issue_zeroout+0x434/0x4c0 block/blk-lib.c:378
blkdev_fallocate+0x2fc/0x410 fs/block_dev.c:2089
vfs_fallocate+0x4aa/0xa50 fs/open.c:309
ksys_fallocate+0x58/0xa0 fs/open.c:332
__do_sys_fallocate fs/open.c:340 [inline]
__se_sys_fallocate fs/open.c:338 [inline]
__x64_sys_fallocate+0x97/0xf0 fs/open.c:338
do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x441269
Code: e8 ac e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 eb 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffc6d0e55e8 EFLAGS: 00000246 ORIG_RAX: 000000000000011d
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000441269
RDX: 0000000000000000 RSI: 0000000000000011 RDI: 0000000000000003
RBP: 00000000006cb018 R08: 00000000004002c8 R09: 00000000004002c8
R10: 0000400000000200 R11: 0000000000000246 R12: 0000000000401fe0
R13: 0000000000402070 R14: 0000000000000000 R15: 0000000000000000
INFO: task syz-executor350:8662 can't die for more than 144 seconds.
syz-executor350 R running task 27256 8662 8658 0x00004006
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x94a/0x1e70 kernel/sched/core.c:4069
INFO: task syz-executor350:8663 can't die for more than 145 seconds.
syz-executor350 D27256 8663 8655 0x00004006
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x94a/0x1e70 kernel/sched/core.c:4069
schedule+0xd9/0x260 kernel/sched/core.c:4136
io_schedule+0x1c/0x70 kernel/sched/core.c:5780
rq_qos_wait+0x301/0x3f0 block/blk-rq-qos.c:288
__wbt_wait block/blk-wbt.c:526 [inline]
wbt_wait+0x20b/0x370 block/blk-wbt.c:591
__rq_qos_throttle+0x56/0xa0 block/blk-rq-qos.c:72
rq_qos_throttle block/blk-rq-qos.h:182 [inline]
blk_mq_make_request+0x3d0/0x2280 block/blk-mq.c:1943
generic_make_request block/blk-core.c:1093 [inline]
generic_make_request+0x23c/0xb50 block/blk-core.c:1035
submit_bio+0x113/0x600 block/blk-core.c:1219
blk_next_bio+0x4a/0x60 block/blk-lib.c:19
__blkdev_issue_zero_pages+0x151/0x430 block/blk-lib.c:284
blkdev_issue_zeroout+0x434/0x4c0 block/blk-lib.c:378
blkdev_fallocate+0x2fc/0x410 fs/block_dev.c:2089
vfs_fallocate+0x4aa/0xa50 fs/open.c:309
ksys_fallocate+0x58/0xa0 fs/open.c:332
__do_sys_fallocate fs/open.c:340 [inline]
__se_sys_fallocate fs/open.c:338 [inline]
__x64_sys_fallocate+0x97/0xf0 fs/open.c:338
do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x441269
Code: e8 ac e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 eb 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffc6d0e55e8 EFLAGS: 00000246 ORIG_RAX: 000000000000011d
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000441269
RDX: 0000000000000000 RSI: 0000000000000011 RDI: 0000000000000003
RBP: 00000000006cb018 R08: 00000000004002c8 R09: 00000000004002c8
R10: 0000400000000200 R11: 0000000000000246 R12: 0000000000401fe0
R13: 0000000000402070 R14: 0000000000000000 R15: 0000000000000000
INFO: task syz-executor350:8664 can't die for more than 146 seconds.
syz-executor350 R running task 27336 8664 8659 0x00004006
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x94a/0x1e70 kernel/sched/core.c:4069
schedule+0xd9/0x260 kernel/sched/core.c:4136
io_schedule+0xc/0x70 kernel/sched/core.c:5779
rq_qos_wait+0x301/0x3f0 block/blk-rq-qos.c:288
__wbt_wait block/blk-wbt.c:526 [inline]
wbt_wait+0x20b/0x370 block/blk-wbt.c:591
__rq_qos_throttle+0x56/0xa0 block/blk-rq-qos.c:72
rq_qos_throttle block/blk-rq-qos.h:182 [inline]
blk_mq_make_request+0x3d0/0x2280 block/blk-mq.c:1943
generic_make_request block/blk-core.c:1093 [inline]
generic_make_request+0x23c/0xb50 block/blk-core.c:1035
submit_bio+0x113/0x600 block/blk-core.c:1219
blk_next_bio+0x4a/0x60 block/blk-lib.c:19
__blkdev_issue_zero_pages+0x151/0x430 block/blk-lib.c:284
blkdev_issue_zeroout+0x434/0x4c0 block/blk-lib.c:378
blkdev_fallocate+0x2fc/0x410 fs/block_dev.c:2089
vfs_fallocate+0x4aa/0xa50 fs/open.c:309
ksys_fallocate+0x58/0xa0 fs/open.c:332
__do_sys_fallocate fs/open.c:340 [inline]
__se_sys_fallocate fs/open.c:338 [inline]
__x64_sys_fallocate+0x97/0xf0 fs/open.c:338
do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x441269
Code: e8 ac e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 eb 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffc6d0e55e8 EFLAGS: 00000246 ORIG_RAX: 000000000000011d
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000441269
RDX: 0000000000000000 RSI: 0000000000000011 RDI: 0000000000000003
RBP: 00000000006cb018 R08: 00000000004002c8 R09: 00000000004002c8
R10: 0000400000000200 R11: 0000000000000246 R12: 0000000000401fe0
R13: 0000000000402070 R14: 0000000000000000 R15: 0000000000000000
INFO: task syz-executor350:8665 can't die for more than 147 seconds.
syz-executor350 D27256 8665 8657 0x00004006
Call Trace:
context_switch kernel/sched/core.c:3384 [inline]
__schedule+0x94a/0x1e70 kernel/sched/core.c:4069
Showing all locks held in the system:
1 lock held by khungtaskd/1073:
#0: ffffffff88fab740 (rcu_read_lock){....}, at:
debug_show_all_locks+0x5f/0x279 kernel/locking/lockdep.c:5336
1 lock held by rsyslogd/8538:
#0: ffff888098390620 (&f->f_pos_lock){+.+.}, at: __fdget_pos+0xee/0x110
fs/file.c:801
2 locks held by getty/8628:
#0: ffff8880a1d05090 (&tty->ldisc_sem){++++}, at:
ldsem_down_read+0x33/0x40 drivers/tty/tty_ldsem.c:340
#1: ffffc90005f252e0 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x220/0x1bf0 drivers/tty/n_tty.c:2156
2 locks held by getty/8629:
#0: ffff8880a9790090 (&tty->ldisc_sem){++++}, at:
ldsem_down_read+0x33/0x40 drivers/tty/tty_ldsem.c:340
#1: ffffc90005f4b2e0 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x220/0x1bf0 drivers/tty/n_tty.c:2156
2 locks held by getty/8630:
#0: ffff8880a47ae090 (&tty->ldisc_sem){++++}, at:
ldsem_down_read+0x33/0x40 drivers/tty/tty_ldsem.c:340
#1: ffffc90005f3f2e0 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x220/0x1bf0 drivers/tty/n_tty.c:2156
2 locks held by getty/8631:
#0: ffff88809fc6a090 (&tty->ldisc_sem){++++}, at:
ldsem_down_read+0x33/0x40 drivers/tty/tty_ldsem.c:340
#1: ffffc90005f472e0 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x220/0x1bf0 drivers/tty/n_tty.c:2156
2 locks held by getty/8632:
#0: ffff8880946cf090 (&tty->ldisc_sem){++++}, at:
ldsem_down_read+0x33/0x40 drivers/tty/tty_ldsem.c:340
#1: ffffc90005f2d2e0 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x220/0x1bf0 drivers/tty/n_tty.c:2156
2 locks held by getty/8633:
#0: ffff888092b6e090 (&tty->ldisc_sem){++++}, at:
ldsem_down_read+0x33/0x40 drivers/tty/tty_ldsem.c:340
#1: ffffc90005f432e0 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x220/0x1bf0 drivers/tty/n_tty.c:2156
2 locks held by getty/8634:
#0: ffff88809a971090 (&tty->ldisc_sem){++++}, at:
ldsem_down_read+0x33/0x40 drivers/tty/tty_ldsem.c:340
#1: ffffc90005f192e0 (&ldata->atomic_read_lock){+.+.}, at:
n_tty_read+0x220/0x1bf0 drivers/tty/n_tty.c:2156
=============================================
NMI backtrace for cpu 1
CPU: 1 PID: 1073 Comm: khungtaskd Not tainted 5.4.0-rc3-next-20191018 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
nmi_cpu_backtrace.cold+0x70/0xb2 lib/nmi_backtrace.c:101
nmi_trigger_cpumask_backtrace+0x23b/0x28b lib/nmi_backtrace.c:62
arch_trigger_cpumask_backtrace+0x14/0x20 arch/x86/kernel/apic/hw_nmi.c:38
trigger_all_cpu_backtrace include/linux/nmi.h:146 [inline]
check_hung_uninterruptible_tasks kernel/hung_task.c:269 [inline]
watchdog+0xc8f/0x1350 kernel/hung_task.c:353
kthread+0x361/0x430 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Sending NMI from CPU 1 to CPUs 0:
NMI backtrace for cpu 0
CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.4.0-rc3-next-20191018 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:__sanitizer_cov_trace_pc+0x0/0x50 kernel/kcov.c:95
Code: 89 25 c4 94 3d 09 41 bc f4 ff ff ff e8 1d 8c e9 ff 48 c7 05 ae 94 3d
09 00 00 00 00 e9 77 e9 ff ff 90 90 90 90 90 90 90 90 90 <55> 48 89 e5 65
48 8b 04 25 80 fe 01 00 65 8b 15 74 b0 8e 7e 81 e2
RSP: 0018:ffff8880a989f9f0 EFLAGS: 00000092
RAX: 0000000000000286 RBX: ffff88809962f0c0 RCX: ffffffff81915655
RDX: 0000000000000100 RSI: ffff88809962f0c0 RDI: ffff88821acc9540
RBP: ffff8880a989fa28 R08: ffff8880a9886240 R09: ffff8880a9886ad8
R10: fffffbfff138fe50 R11: ffffffff89c7f287 R12: ffff88821acc9540
R13: ffff88809962f0c0 R14: 0000000000000286 R15: 0000000000000002
FS: 0000000000000000(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffff600400 CR3: 000000008fe26000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
kmem_cache_free+0x5b/0x320 mm/slab.c:3690
mempool_free_slab+0x1e/0x30 mm/mempool.c:520
mempool_free+0xeb/0x370 mm/mempool.c:502
bio_free+0x268/0x420 block/bio.c:255
bio_put+0xda/0x110 block/bio.c:549
__bio_chain_endio block/bio.c:307 [inline]
bio_endio+0x2c2/0xaf0 block/bio.c:1804
req_bio_endio block/blk-core.c:271 [inline]
blk_update_request+0x49e/0x10d0 block/blk-core.c:1491
blk_mq_end_request+0x5b/0x560 block/blk-mq.c:552
end_cmd drivers/block/null_blk_main.c:648 [inline]
end_cmd+0x111/0x310 drivers/block/null_blk_main.c:642
null_complete_rq+0x19/0x20 drivers/block/null_blk_main.c:675
blk_done_softirq+0x2fe/0x4d0 block/blk-softirq.c:37
__do_softirq+0x262/0x98c kernel/softirq.c:292
run_ksoftirqd kernel/softirq.c:603 [inline]
run_ksoftirqd+0x8e/0x110 kernel/softirq.c:595
smpboot_thread_fn+0x6a3/0xa40 kernel/smpboot.c:165
kthread+0x361/0x430 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: INFO: task syz-executor can't die for more than 143 seconds. (2) 2019-10-23 7:56 INFO: task syz-executor can't die for more than 143 seconds. (2) syzbot @ 2019-10-24 10:08 ` Tetsuo Handa 2019-10-28 8:51 ` Bob Liu 0 siblings, 1 reply; 14+ messages in thread From: Tetsuo Handa @ 2019-10-24 10:08 UTC (permalink / raw) To: axboe; +Cc: syzbot, linux-block, linux-kernel, syzkaller-bugs On 2019/10/23 16:56, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: c4b9850b Add linux-next specific files for 20191018 > git tree: linux-next > console output: https://syzkaller.appspot.com/x/log.txt?x=177b3ab0e00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=c940ef12efcd1ec > dashboard link: https://syzkaller.appspot.com/bug?extid=b48daca8639150bc5e73 > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1356b8ff600000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14f48687600000 The reproducer is trying to allocate 64TB of disk space on /dev/nullb0 using fallocate() but __blkdev_issue_zero_pages() cannot bail out upon SIGKILL (and therefore cannot terminate for minutes). Can we make it killable? I don't know what action is needed for undoing this loop... while (nr_sects != 0) { bio = blk_next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects), gfp_mask); bio->bi_iter.bi_sector = sector; bio_set_dev(bio, bdev); bio_set_op_attrs(bio, REQ_OP_WRITE, 0); while (nr_sects != 0) { sz = min((sector_t) PAGE_SIZE, nr_sects << 9); bi_size = bio_add_page(bio, ZERO_PAGE(0), sz, 0); nr_sects -= bi_size >> 9; sector += bi_size >> 9; if (bi_size < sz) break; } cond_resched(); } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: INFO: task syz-executor can't die for more than 143 seconds. (2) 2019-10-24 10:08 ` Tetsuo Handa @ 2019-10-28 8:51 ` Bob Liu 2019-11-08 11:41 ` [PATCH] block: Bail out iteration functions upon SIGKILL Tetsuo Handa 0 siblings, 1 reply; 14+ messages in thread From: Bob Liu @ 2019-10-28 8:51 UTC (permalink / raw) To: Tetsuo Handa, axboe; +Cc: syzbot, linux-block, linux-kernel, syzkaller-bugs On 10/24/19 6:08 PM, Tetsuo Handa wrote: > On 2019/10/23 16:56, syzbot wrote: >> Hello, >> >> syzbot found the following crash on: >> >> HEAD commit: c4b9850b Add linux-next specific files for 20191018 >> git tree: linux-next >> console output: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_log.txt-3Fx-3D177b3ab0e00000&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=1ktT0U2YS_I8Zz2o-MS1YcCAzWZ6hFGtyTgvVMGM7gI&m=wOlNeKk9puri9Fvxn8bGDrlWHd-4GPMeJ9rb2CVqXaE&s=PZfBliKlYjm16VnyPzu-3i0SgqlbByIB0iI8jVhcGuk&e= >> kernel config: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_.config-3Fx-3Dc940ef12efcd1ec&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=1ktT0U2YS_I8Zz2o-MS1YcCAzWZ6hFGtyTgvVMGM7gI&m=wOlNeKk9puri9Fvxn8bGDrlWHd-4GPMeJ9rb2CVqXaE&s=z8tV220wKFTQIJH1tSYLUl8ecAnll94C_mFVcHkuTlc&e= >> dashboard link: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3Db48daca8639150bc5e73&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=1ktT0U2YS_I8Zz2o-MS1YcCAzWZ6hFGtyTgvVMGM7gI&m=wOlNeKk9puri9Fvxn8bGDrlWHd-4GPMeJ9rb2CVqXaE&s=VZ2ZdnAqEd_AkXJujidN3EgwscGpUAdsZjuObKjXN-U&e= >> compiler: gcc (GCC) 9.0.0 20181231 (experimental) >> syz repro: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_repro.syz-3Fx-3D1356b8ff600000&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=1ktT0U2YS_I8Zz2o-MS1YcCAzWZ6hFGtyTgvVMGM7gI&m=wOlNeKk9puri9Fvxn8bGDrlWHd-4GPMeJ9rb2CVqXaE&s=Q_svUYj2OBYmIJXnResNzOWVUCyjRpxnpun2Cu15S9M&e= >> C reproducer: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_repro.c-3Fx-3D14f48687600000&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=1ktT0U2YS_I8Zz2o-MS1YcCAzWZ6hFGtyTgvVMGM7gI&m=wOlNeKk9puri9Fvxn8bGDrlWHd-4GPMeJ9rb2CVqXaE&s=FGZNxR7w-rU29MhJxJtno-c_wUXCJHgPC5V1YNp7h58&e= > > The reproducer is trying to allocate 64TB of disk space on /dev/nullb0 using fallocate() > but __blkdev_issue_zero_pages() cannot bail out upon SIGKILL (and therefore cannot > terminate for minutes). Can we make it killable? Yeah, I think we can consider add fatal_signal_pending(current) checking in the while() loop.. > I don't know what action is needed > for undoing this loop... > > while (nr_sects != 0) { > bio = blk_next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects), > gfp_mask); > bio->bi_iter.bi_sector = sector; > bio_set_dev(bio, bdev); > bio_set_op_attrs(bio, REQ_OP_WRITE, 0); > > while (nr_sects != 0) { > sz = min((sector_t) PAGE_SIZE, nr_sects << 9); > bi_size = bio_add_page(bio, ZERO_PAGE(0), sz, 0); > nr_sects -= bi_size >> 9; > sector += bi_size >> 9; > if (bi_size < sz) > break; > } > cond_resched(); > } > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-10-28 8:51 ` Bob Liu @ 2019-11-08 11:41 ` Tetsuo Handa 2019-11-08 18:13 ` Chaitanya Kulkarni 2019-11-12 4:05 ` Damien Le Moal 0 siblings, 2 replies; 14+ messages in thread From: Tetsuo Handa @ 2019-11-08 11:41 UTC (permalink / raw) To: Bob Liu, axboe; +Cc: linux-block syzbot found that a thread can stall for minutes inside fallocate() after that thread was killed by SIGKILL [1]. While trying to allocate 64TB of disk space using fallocate() is legal, delaying termination of killed thread for minutes is bad. Thus, allow iteration functions in block/blk-lib.c to be killable. [1] https://syzkaller.appspot.com/bug?id=9386d051e11e09973d5a4cf79af5e8cedf79386d Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reported-by: syzbot <syzbot+b48daca8639150bc5e73@syzkaller.appspotmail.com> --- block/blk-lib.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/block/blk-lib.c b/block/blk-lib.c index 5f2c429..6ca7cae 100644 --- a/block/blk-lib.c +++ b/block/blk-lib.c @@ -7,9 +7,22 @@ #include <linux/bio.h> #include <linux/blkdev.h> #include <linux/scatterlist.h> +#include <linux/sched/signal.h> #include "blk.h" +static int blk_should_abort(struct bio *bio) +{ + int ret; + + cond_resched(); + if (!fatal_signal_pending(current)) + return 0; + ret = submit_bio_wait(bio); + bio_put(bio); + return ret ? ret : -EINTR; +} + struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp) { struct bio *new = bio_alloc(gfp, nr_pages); @@ -55,6 +68,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, return -EINVAL; while (nr_sects) { + int ret; sector_t req_sects = min_t(sector_t, nr_sects, bio_allowed_max_sectors(q)); @@ -75,7 +89,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, * us to schedule out to avoid softlocking if preempt * is disabled. */ - cond_resched(); + ret = blk_should_abort(bio); + if (ret) { + *biop = NULL; + return ret; + } } *biop = bio; @@ -154,6 +172,8 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, max_write_same_sectors = bio_allowed_max_sectors(q); while (nr_sects) { + int ret; + bio = blk_next_bio(bio, 1, gfp_mask); bio->bi_iter.bi_sector = sector; bio_set_dev(bio, bdev); @@ -171,7 +191,11 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, bio->bi_iter.bi_size = nr_sects << 9; nr_sects = 0; } - cond_resched(); + ret = blk_should_abort(bio); + if (ret) { + *biop = NULL; + return ret; + } } *biop = bio; @@ -230,6 +254,8 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, return -EOPNOTSUPP; while (nr_sects) { + int ret; + bio = blk_next_bio(bio, 0, gfp_mask); bio->bi_iter.bi_sector = sector; bio_set_dev(bio, bdev); @@ -245,7 +271,11 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, bio->bi_iter.bi_size = nr_sects << 9; nr_sects = 0; } - cond_resched(); + ret = blk_should_abort(bio); + if (ret) { + *biop = NULL; + return ret; + } } *biop = bio; @@ -281,6 +311,8 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, return -EPERM; while (nr_sects != 0) { + int ret; + bio = blk_next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects), gfp_mask); bio->bi_iter.bi_sector = sector; @@ -295,7 +327,11 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, if (bi_size < sz) break; } - cond_resched(); + ret = blk_should_abort(bio); + if (ret) { + *biop = NULL; + return ret; + } } *biop = bio; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-08 11:41 ` [PATCH] block: Bail out iteration functions upon SIGKILL Tetsuo Handa @ 2019-11-08 18:13 ` Chaitanya Kulkarni 2019-11-08 22:18 ` Chaitanya Kulkarni 2019-11-12 4:05 ` Damien Le Moal 1 sibling, 1 reply; 14+ messages in thread From: Chaitanya Kulkarni @ 2019-11-08 18:13 UTC (permalink / raw) To: Tetsuo Handa, Bob Liu, axboe; +Cc: linux-block Thanks for the patch, this looks good to me, let me test this patch will send a review then. On 11/08/2019 03:54 AM, Tetsuo Handa wrote: > syzbot found that a thread can stall for minutes inside fallocate() > after that thread was killed by SIGKILL [1]. While trying to allocate > 64TB of disk space using fallocate() is legal, delaying termination of > killed thread for minutes is bad. Thus, allow iteration functions in > block/blk-lib.c to be killable. > > [1]https://syzkaller.appspot.com/bug?id=9386d051e11e09973d5a4cf79af5e8cedf79386d > > Signed-off-by: Tetsuo Handa<penguin-kernel@I-love.SAKURA.ne.jp> > Reported-by: syzbot<syzbot+b48daca8639150bc5e73@syzkaller.appspotmail.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-08 18:13 ` Chaitanya Kulkarni @ 2019-11-08 22:18 ` Chaitanya Kulkarni 0 siblings, 0 replies; 14+ messages in thread From: Chaitanya Kulkarni @ 2019-11-08 22:18 UTC (permalink / raw) To: Tetsuo Handa, Bob Liu, axboe; +Cc: linux-block Looks good. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> On 11/08/2019 10:13 AM, Chaitanya Kulkarni wrote: > Thanks for the patch, this looks good to me, let me test this patch > will send a review then. > > On 11/08/2019 03:54 AM, Tetsuo Handa wrote: >> syzbot found that a thread can stall for minutes inside fallocate() >> after that thread was killed by SIGKILL [1]. While trying to allocate >> 64TB of disk space using fallocate() is legal, delaying termination of >> killed thread for minutes is bad. Thus, allow iteration functions in >> block/blk-lib.c to be killable. >> >> [1]https://syzkaller.appspot.com/bug?id=9386d051e11e09973d5a4cf79af5e8cedf79386d >> >> Signed-off-by: Tetsuo Handa<penguin-kernel@I-love.SAKURA.ne.jp> >> Reported-by: syzbot<syzbot+b48daca8639150bc5e73@syzkaller.appspotmail.com> > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-08 11:41 ` [PATCH] block: Bail out iteration functions upon SIGKILL Tetsuo Handa 2019-11-08 18:13 ` Chaitanya Kulkarni @ 2019-11-12 4:05 ` Damien Le Moal 2019-11-12 14:47 ` Tetsuo Handa 1 sibling, 1 reply; 14+ messages in thread From: Damien Le Moal @ 2019-11-12 4:05 UTC (permalink / raw) To: Tetsuo Handa, Bob Liu, axboe; +Cc: linux-block On 2019/11/08 20:54, Tetsuo Handa wrote: > syzbot found that a thread can stall for minutes inside fallocate() > after that thread was killed by SIGKILL [1]. While trying to allocate > 64TB of disk space using fallocate() is legal, delaying termination of > killed thread for minutes is bad. Thus, allow iteration functions in > block/blk-lib.c to be killable. > > [1] https://syzkaller.appspot.com/bug?id=9386d051e11e09973d5a4cf79af5e8cedf79386d > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reported-by: syzbot <syzbot+b48daca8639150bc5e73@syzkaller.appspotmail.com> > --- > block/blk-lib.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 40 insertions(+), 4 deletions(-) > > diff --git a/block/blk-lib.c b/block/blk-lib.c > index 5f2c429..6ca7cae 100644 > --- a/block/blk-lib.c > +++ b/block/blk-lib.c > @@ -7,9 +7,22 @@ > #include <linux/bio.h> > #include <linux/blkdev.h> > #include <linux/scatterlist.h> > +#include <linux/sched/signal.h> > > #include "blk.h" > > +static int blk_should_abort(struct bio *bio) > +{ > + int ret; > + > + cond_resched(); > + if (!fatal_signal_pending(current)) > + return 0; > + ret = submit_bio_wait(bio); This will change the behavior of __blkdev_issue_discard() to a sync IO execution instead of the current async execution since submit_bio_wait() call is the responsibility of the caller (e.g. blkdev_issue_discard()). Have you checked if users of __blkdev_issue_discard() are OK with that ? f2fs, ext4, xfs, dm and nvme use this function. Looking at f2fs, this does not look like it is going to work as expected since the bio setup, including end_io callback, is done after this function is called and a regular submit_bio() execution is being used. > + bio_put(bio); > + return ret ? ret : -EINTR; > +} > + > struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp) > { > struct bio *new = bio_alloc(gfp, nr_pages); > @@ -55,6 +68,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > return -EINVAL; > > while (nr_sects) { > + int ret; Please add a white line after the declaration similarly to your change in __blkdev_issue_write_same() and __blkdev_issue_zero_pages(). > sector_t req_sects = min_t(sector_t, nr_sects, > bio_allowed_max_sectors(q)); > > @@ -75,7 +89,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > * us to schedule out to avoid softlocking if preempt > * is disabled. > */ > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > @@ -154,6 +172,8 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, > max_write_same_sectors = bio_allowed_max_sectors(q); > > while (nr_sects) { > + int ret; > + > bio = blk_next_bio(bio, 1, gfp_mask); > bio->bi_iter.bi_sector = sector; > bio_set_dev(bio, bdev); > @@ -171,7 +191,11 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, > bio->bi_iter.bi_size = nr_sects << 9; > nr_sects = 0; > } > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > @@ -230,6 +254,8 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, > return -EOPNOTSUPP; > > while (nr_sects) { > + int ret; > + > bio = blk_next_bio(bio, 0, gfp_mask); > bio->bi_iter.bi_sector = sector; > bio_set_dev(bio, bdev); > @@ -245,7 +271,11 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, > bio->bi_iter.bi_size = nr_sects << 9; > nr_sects = 0; > } > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > @@ -281,6 +311,8 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, > return -EPERM; > > while (nr_sects != 0) { > + int ret; > + > bio = blk_next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects), > gfp_mask); > bio->bi_iter.bi_sector = sector; > @@ -295,7 +327,11 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, > if (bi_size < sz) > break; > } > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-12 4:05 ` Damien Le Moal @ 2019-11-12 14:47 ` Tetsuo Handa 2019-11-13 1:54 ` Damien Le Moal 0 siblings, 1 reply; 14+ messages in thread From: Tetsuo Handa @ 2019-11-12 14:47 UTC (permalink / raw) To: Damien Le Moal, Bob Liu, axboe; +Cc: linux-block On 2019/11/12 13:05, Damien Le Moal wrote: > On 2019/11/08 20:54, Tetsuo Handa wrote: >> syzbot found that a thread can stall for minutes inside fallocate() >> after that thread was killed by SIGKILL [1]. While trying to allocate >> 64TB of disk space using fallocate() is legal, delaying termination of >> killed thread for minutes is bad. Thus, allow iteration functions in >> block/blk-lib.c to be killable. >> >> [1] https://syzkaller.appspot.com/bug?id=9386d051e11e09973d5a4cf79af5e8cedf79386d >> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Reported-by: syzbot <syzbot+b48daca8639150bc5e73@syzkaller.appspotmail.com> >> --- >> block/blk-lib.c | 44 ++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 40 insertions(+), 4 deletions(-) >> >> diff --git a/block/blk-lib.c b/block/blk-lib.c >> index 5f2c429..6ca7cae 100644 >> --- a/block/blk-lib.c >> +++ b/block/blk-lib.c >> @@ -7,9 +7,22 @@ >> #include <linux/bio.h> >> #include <linux/blkdev.h> >> #include <linux/scatterlist.h> >> +#include <linux/sched/signal.h> >> >> #include "blk.h" >> >> +static int blk_should_abort(struct bio *bio) >> +{ >> + int ret; >> + >> + cond_resched(); >> + if (!fatal_signal_pending(current)) >> + return 0; >> + ret = submit_bio_wait(bio); > > This will change the behavior of __blkdev_issue_discard() to a sync IO > execution instead of the current async execution since submit_bio_wait() > call is the responsibility of the caller (e.g. blkdev_issue_discard()). > Have you checked if users of __blkdev_issue_discard() are OK with that ? > f2fs, ext4, xfs, dm and nvme use this function. I'm not sure... > > Looking at f2fs, this does not look like it is going to work as expected > since the bio setup, including end_io callback, is done after this > function is called and a regular submit_bio() execution is being used. Then, just breaking the iteration like below? nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem? --- a/block/blk-lib.c +++ b/block/blk-lib.c @@ -7,6 +7,7 @@ #include <linux/bio.h> #include <linux/blkdev.h> #include <linux/scatterlist.h> +#include <linux/sched/signal.h> #include "blk.h" @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, struct bio *bio = *biop; unsigned int op; sector_t bs_mask; + int ret = 0; if (!q) return -ENXIO; @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, * is disabled. */ cond_resched(); + if (fatal_signal_pending(current)) { + ret = -EINTR; + break; + } } *biop = bio; - return 0; + return ret; } EXPORT_SYMBOL(__blkdev_issue_discard); @@ -136,6 +142,7 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, unsigned int max_write_same_sectors; struct bio *bio = *biop; sector_t bs_mask; + int ret = 0; if (!q) return -ENXIO; @@ -172,10 +179,14 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, nr_sects = 0; } cond_resched(); + if (fatal_signal_pending(current)) { + ret = -EINTR; + break; + } } *biop = bio; - return 0; + return ret; } /** @@ -216,6 +227,7 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, struct bio *bio = *biop; unsigned int max_write_zeroes_sectors; struct request_queue *q = bdev_get_queue(bdev); + int ret = 0; if (!q) return -ENXIO; @@ -246,10 +258,14 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, nr_sects = 0; } cond_resched(); + if (fatal_signal_pending(current)) { + ret = -EINTR; + break; + } } *biop = bio; - return 0; + return ret; } /* @@ -273,6 +289,7 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, struct bio *bio = *biop; int bi_size = 0; unsigned int sz; + int ret = 0; if (!q) return -ENXIO; @@ -296,10 +313,14 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, break; } cond_resched(); + if (fatal_signal_pending(current)) { + ret = -EINTR; + break; + } } *biop = bio; - return 0; + return ret; } /** > >> + bio_put(bio); >> + return ret ? ret : -EINTR; >> +} >> + >> struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp) >> { >> struct bio *new = bio_alloc(gfp, nr_pages); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-12 14:47 ` Tetsuo Handa @ 2019-11-13 1:54 ` Damien Le Moal 2019-11-13 6:55 ` Ming Lei 2019-11-15 10:05 ` Tetsuo Handa 0 siblings, 2 replies; 14+ messages in thread From: Damien Le Moal @ 2019-11-13 1:54 UTC (permalink / raw) To: Tetsuo Handa, Bob Liu, axboe; +Cc: linux-block On 2019/11/12 23:48, Tetsuo Handa wrote: [...] >>> +static int blk_should_abort(struct bio *bio) >>> +{ >>> + int ret; >>> + >>> + cond_resched(); >>> + if (!fatal_signal_pending(current)) >>> + return 0; >>> + ret = submit_bio_wait(bio); >> >> This will change the behavior of __blkdev_issue_discard() to a sync IO >> execution instead of the current async execution since submit_bio_wait() >> call is the responsibility of the caller (e.g. blkdev_issue_discard()). >> Have you checked if users of __blkdev_issue_discard() are OK with that ? >> f2fs, ext4, xfs, dm and nvme use this function. > > I'm not sure... > >> >> Looking at f2fs, this does not look like it is going to work as expected >> since the bio setup, including end_io callback, is done after this >> function is called and a regular submit_bio() execution is being used. > > Then, just breaking the iteration like below? > nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem? > > --- a/block/blk-lib.c > +++ b/block/blk-lib.c > @@ -7,6 +7,7 @@ > #include <linux/bio.h> > #include <linux/blkdev.h> > #include <linux/scatterlist.h> > +#include <linux/sched/signal.h> > > #include "blk.h" > > @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > struct bio *bio = *biop; > unsigned int op; > sector_t bs_mask; > + int ret = 0; > > if (!q) > return -ENXIO; > @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > * is disabled. > */ > cond_resched(); > + if (fatal_signal_pending(current)) { > + ret = -EINTR; > + break; > + } > } > > *biop = bio; > - return 0; > + return ret; This will leak a bio as blkdev_issue_discard() executes the bio only in the case "if (!ret && bio)". So that does not work as is, unless all callers of __blkdev_issue_discard() are also changed. Same problem for the other __blkdev_issue_xxx() functions. Looking more into this, if an error is returned here, no bio should be returned and we need to make sure that all started bios are also completed. So your helper blk_should_abort() did the right thing calling submit_bio_wait(). However, I Think it would be better to fail immediately the current loop bio instead of executing it and then reporting the -EINTR error, unconditionally, regardless of what the started bios completion status is. This could be done with the help of a function like this, very similar to submit_bio_wait(). void bio_chain_end_wait(struct bio *bio) { DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map); bio->bi_private = &done; bio->bi_end_io = submit_bio_wait_endio; bio->bi_opf |= REQ_SYNC; bio_endio(bio); wait_for_completion_io(&done); } And then your helper function becomes something like this: static int blk_should_abort(struct bio *bio) { int ret; cond_resched(); if (!fatal_signal_pending(current)) return 0; if (bio_flagged(bio, BIO_CHAIN)) bio_chain_end_wait(bio); bio_put(bio); return -EINTR; } Thoughts ? > } > EXPORT_SYMBOL(__blkdev_issue_discard); > > @@ -136,6 +142,7 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, > unsigned int max_write_same_sectors; > struct bio *bio = *biop; > sector_t bs_mask; > + int ret = 0; > > if (!q) > return -ENXIO; > @@ -172,10 +179,14 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, > nr_sects = 0; > } > cond_resched(); > + if (fatal_signal_pending(current)) { > + ret = -EINTR; > + break; > + } > } > > *biop = bio; > - return 0; > + return ret; > } > > /** > @@ -216,6 +227,7 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, > struct bio *bio = *biop; > unsigned int max_write_zeroes_sectors; > struct request_queue *q = bdev_get_queue(bdev); > + int ret = 0; > > if (!q) > return -ENXIO; > @@ -246,10 +258,14 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, > nr_sects = 0; > } > cond_resched(); > + if (fatal_signal_pending(current)) { > + ret = -EINTR; > + break; > + } > } > > *biop = bio; > - return 0; > + return ret; > } > > /* > @@ -273,6 +289,7 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, > struct bio *bio = *biop; > int bi_size = 0; > unsigned int sz; > + int ret = 0; > > if (!q) > return -ENXIO; > @@ -296,10 +313,14 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, > break; > } > cond_resched(); > + if (fatal_signal_pending(current)) { > + ret = -EINTR; > + break; > + } > } > > *biop = bio; > - return 0; > + return ret; > } > > /** > >> >>> + bio_put(bio); >>> + return ret ? ret : -EINTR; >>> +} >>> + >>> struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp) >>> { >>> struct bio *new = bio_alloc(gfp, nr_pages); > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-13 1:54 ` Damien Le Moal @ 2019-11-13 6:55 ` Ming Lei 2019-11-13 7:11 ` Damien Le Moal 2019-11-15 10:05 ` Tetsuo Handa 1 sibling, 1 reply; 14+ messages in thread From: Ming Lei @ 2019-11-13 6:55 UTC (permalink / raw) To: Damien Le Moal; +Cc: Tetsuo Handa, Bob Liu, axboe, linux-block On Wed, Nov 13, 2019 at 01:54:14AM +0000, Damien Le Moal wrote: > On 2019/11/12 23:48, Tetsuo Handa wrote: > [...] > >>> +static int blk_should_abort(struct bio *bio) > >>> +{ > >>> + int ret; > >>> + > >>> + cond_resched(); > >>> + if (!fatal_signal_pending(current)) > >>> + return 0; > >>> + ret = submit_bio_wait(bio); > >> > >> This will change the behavior of __blkdev_issue_discard() to a sync IO > >> execution instead of the current async execution since submit_bio_wait() > >> call is the responsibility of the caller (e.g. blkdev_issue_discard()). > >> Have you checked if users of __blkdev_issue_discard() are OK with that ? > >> f2fs, ext4, xfs, dm and nvme use this function. > > > > I'm not sure... > > > >> > >> Looking at f2fs, this does not look like it is going to work as expected > >> since the bio setup, including end_io callback, is done after this > >> function is called and a regular submit_bio() execution is being used. > > > > Then, just breaking the iteration like below? > > nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem? > > > > --- a/block/blk-lib.c > > +++ b/block/blk-lib.c > > @@ -7,6 +7,7 @@ > > #include <linux/bio.h> > > #include <linux/blkdev.h> > > #include <linux/scatterlist.h> > > +#include <linux/sched/signal.h> > > > > #include "blk.h" > > > > @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > > struct bio *bio = *biop; > > unsigned int op; > > sector_t bs_mask; > > + int ret = 0; > > > > if (!q) > > return -ENXIO; > > @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > > * is disabled. > > */ > > cond_resched(); > > + if (fatal_signal_pending(current)) { > > + ret = -EINTR; > > + break; > > + } > > } > > > > *biop = bio; > > - return 0; > > + return ret; > > This will leak a bio as blkdev_issue_discard() executes the bio only in > the case "if (!ret && bio)". So that does not work as is, unless all > callers of __blkdev_issue_discard() are also changed. Same problem for > the other __blkdev_issue_xxx() functions. > > Looking more into this, if an error is returned here, no bio should be > returned and we need to make sure that all started bios are also > completed. So your helper blk_should_abort() did the right thing calling > submit_bio_wait(). However, I Think it would be better to fail > immediately the current loop bio instead of executing it and then > reporting the -EINTR error, unconditionally, regardless of what the > started bios completion status is. > > This could be done with the help of a function like this, very similar > to submit_bio_wait(). > > void bio_chain_end_wait(struct bio *bio) > { > DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map); > > bio->bi_private = &done; > bio->bi_end_io = submit_bio_wait_endio; > bio->bi_opf |= REQ_SYNC; > bio_endio(bio); > wait_for_completion_io(&done); > } > > And then your helper function becomes something like this: > > static int blk_should_abort(struct bio *bio) > { > int ret; > > cond_resched(); > if (!fatal_signal_pending(current)) > return 0; > > if (bio_flagged(bio, BIO_CHAIN)) > bio_chain_end_wait(bio); > bio_put(bio); > > return -EINTR; > } > > Thoughts ? DISCARD request can be quite big, and any sync bio submission may cause serious performance regression. Not mention blkdev_issue_discard() may be called in non-block context. Thanks, Ming ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-13 6:55 ` Ming Lei @ 2019-11-13 7:11 ` Damien Le Moal 2019-11-13 7:49 ` Ming Lei 0 siblings, 1 reply; 14+ messages in thread From: Damien Le Moal @ 2019-11-13 7:11 UTC (permalink / raw) To: Ming Lei; +Cc: Tetsuo Handa, Bob Liu, axboe, linux-block On 2019/11/13 15:55, Ming Lei wrote: > On Wed, Nov 13, 2019 at 01:54:14AM +0000, Damien Le Moal wrote: >> On 2019/11/12 23:48, Tetsuo Handa wrote: >> [...] >>>>> +static int blk_should_abort(struct bio *bio) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + cond_resched(); >>>>> + if (!fatal_signal_pending(current)) >>>>> + return 0; >>>>> + ret = submit_bio_wait(bio); >>>> >>>> This will change the behavior of __blkdev_issue_discard() to a sync IO >>>> execution instead of the current async execution since submit_bio_wait() >>>> call is the responsibility of the caller (e.g. blkdev_issue_discard()). >>>> Have you checked if users of __blkdev_issue_discard() are OK with that ? >>>> f2fs, ext4, xfs, dm and nvme use this function. >>> >>> I'm not sure... >>> >>>> >>>> Looking at f2fs, this does not look like it is going to work as expected >>>> since the bio setup, including end_io callback, is done after this >>>> function is called and a regular submit_bio() execution is being used. >>> >>> Then, just breaking the iteration like below? >>> nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem? >>> >>> --- a/block/blk-lib.c >>> +++ b/block/blk-lib.c >>> @@ -7,6 +7,7 @@ >>> #include <linux/bio.h> >>> #include <linux/blkdev.h> >>> #include <linux/scatterlist.h> >>> +#include <linux/sched/signal.h> >>> >>> #include "blk.h" >>> >>> @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, >>> struct bio *bio = *biop; >>> unsigned int op; >>> sector_t bs_mask; >>> + int ret = 0; >>> >>> if (!q) >>> return -ENXIO; >>> @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, >>> * is disabled. >>> */ >>> cond_resched(); >>> + if (fatal_signal_pending(current)) { >>> + ret = -EINTR; >>> + break; >>> + } >>> } >>> >>> *biop = bio; >>> - return 0; >>> + return ret; >> >> This will leak a bio as blkdev_issue_discard() executes the bio only in >> the case "if (!ret && bio)". So that does not work as is, unless all >> callers of __blkdev_issue_discard() are also changed. Same problem for >> the other __blkdev_issue_xxx() functions. >> >> Looking more into this, if an error is returned here, no bio should be >> returned and we need to make sure that all started bios are also >> completed. So your helper blk_should_abort() did the right thing calling >> submit_bio_wait(). However, I Think it would be better to fail >> immediately the current loop bio instead of executing it and then >> reporting the -EINTR error, unconditionally, regardless of what the >> started bios completion status is. >> >> This could be done with the help of a function like this, very similar >> to submit_bio_wait(). >> >> void bio_chain_end_wait(struct bio *bio) >> { >> DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map); >> >> bio->bi_private = &done; >> bio->bi_end_io = submit_bio_wait_endio; >> bio->bi_opf |= REQ_SYNC; >> bio_endio(bio); >> wait_for_completion_io(&done); >> } >> >> And then your helper function becomes something like this: >> >> static int blk_should_abort(struct bio *bio) >> { >> int ret; >> >> cond_resched(); >> if (!fatal_signal_pending(current)) >> return 0; >> >> if (bio_flagged(bio, BIO_CHAIN)) >> bio_chain_end_wait(bio); >> bio_put(bio); >> >> return -EINTR; >> } >> >> Thoughts ? > > DISCARD request can be quite big, and any sync bio submission may cause > serious performance regression. Yes indeed. But if the bio issuing loop is interrupted with discard BIOs already issued, I do not think there is any other choice but to wait for their completion before returning. > Not mention blkdev_issue_discard() may be called in non-block context. This loop is calling cond_resched(), which checks might_sleep(). So certainly this function can block, no ? -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-13 7:11 ` Damien Le Moal @ 2019-11-13 7:49 ` Ming Lei 0 siblings, 0 replies; 14+ messages in thread From: Ming Lei @ 2019-11-13 7:49 UTC (permalink / raw) To: Damien Le Moal; +Cc: Tetsuo Handa, Bob Liu, axboe, linux-block On Wed, Nov 13, 2019 at 07:11:36AM +0000, Damien Le Moal wrote: > On 2019/11/13 15:55, Ming Lei wrote: > > On Wed, Nov 13, 2019 at 01:54:14AM +0000, Damien Le Moal wrote: > >> On 2019/11/12 23:48, Tetsuo Handa wrote: > >> [...] > >>>>> +static int blk_should_abort(struct bio *bio) > >>>>> +{ > >>>>> + int ret; > >>>>> + > >>>>> + cond_resched(); > >>>>> + if (!fatal_signal_pending(current)) > >>>>> + return 0; > >>>>> + ret = submit_bio_wait(bio); > >>>> > >>>> This will change the behavior of __blkdev_issue_discard() to a sync IO > >>>> execution instead of the current async execution since submit_bio_wait() > >>>> call is the responsibility of the caller (e.g. blkdev_issue_discard()). > >>>> Have you checked if users of __blkdev_issue_discard() are OK with that ? > >>>> f2fs, ext4, xfs, dm and nvme use this function. > >>> > >>> I'm not sure... > >>> > >>>> > >>>> Looking at f2fs, this does not look like it is going to work as expected > >>>> since the bio setup, including end_io callback, is done after this > >>>> function is called and a regular submit_bio() execution is being used. > >>> > >>> Then, just breaking the iteration like below? > >>> nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem? > >>> > >>> --- a/block/blk-lib.c > >>> +++ b/block/blk-lib.c > >>> @@ -7,6 +7,7 @@ > >>> #include <linux/bio.h> > >>> #include <linux/blkdev.h> > >>> #include <linux/scatterlist.h> > >>> +#include <linux/sched/signal.h> > >>> > >>> #include "blk.h" > >>> > >>> @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > >>> struct bio *bio = *biop; > >>> unsigned int op; > >>> sector_t bs_mask; > >>> + int ret = 0; > >>> > >>> if (!q) > >>> return -ENXIO; > >>> @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > >>> * is disabled. > >>> */ > >>> cond_resched(); > >>> + if (fatal_signal_pending(current)) { > >>> + ret = -EINTR; > >>> + break; > >>> + } > >>> } > >>> > >>> *biop = bio; > >>> - return 0; > >>> + return ret; > >> > >> This will leak a bio as blkdev_issue_discard() executes the bio only in > >> the case "if (!ret && bio)". So that does not work as is, unless all > >> callers of __blkdev_issue_discard() are also changed. Same problem for > >> the other __blkdev_issue_xxx() functions. > >> > >> Looking more into this, if an error is returned here, no bio should be > >> returned and we need to make sure that all started bios are also > >> completed. So your helper blk_should_abort() did the right thing calling > >> submit_bio_wait(). However, I Think it would be better to fail > >> immediately the current loop bio instead of executing it and then > >> reporting the -EINTR error, unconditionally, regardless of what the > >> started bios completion status is. > >> > >> This could be done with the help of a function like this, very similar > >> to submit_bio_wait(). > >> > >> void bio_chain_end_wait(struct bio *bio) > >> { > >> DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map); > >> > >> bio->bi_private = &done; > >> bio->bi_end_io = submit_bio_wait_endio; > >> bio->bi_opf |= REQ_SYNC; > >> bio_endio(bio); > >> wait_for_completion_io(&done); > >> } > >> > >> And then your helper function becomes something like this: > >> > >> static int blk_should_abort(struct bio *bio) > >> { > >> int ret; > >> > >> cond_resched(); > >> if (!fatal_signal_pending(current)) > >> return 0; > >> > >> if (bio_flagged(bio, BIO_CHAIN)) > >> bio_chain_end_wait(bio); > >> bio_put(bio); > >> > >> return -EINTR; > >> } > >> > >> Thoughts ? > > > > DISCARD request can be quite big, and any sync bio submission may cause > > serious performance regression. > > Yes indeed. But if the bio issuing loop is interrupted with discard BIOs > already issued, I do not think there is any other choice but to wait for > their completion before returning. Looks I miss the check on fatal_signal_pending(), then this approach seems fine. > > > Not mention blkdev_issue_discard() may be called in non-block context. > > This loop is calling cond_resched(), which checks might_sleep(). So > certainly this function can block, no ? Indeed, looks I misunderstood it. Thanks, Ming ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-13 1:54 ` Damien Le Moal 2019-11-13 6:55 ` Ming Lei @ 2019-11-15 10:05 ` Tetsuo Handa 2019-11-18 0:02 ` Damien Le Moal 1 sibling, 1 reply; 14+ messages in thread From: Tetsuo Handa @ 2019-11-15 10:05 UTC (permalink / raw) To: Damien Le Moal; +Cc: Bob Liu, axboe, linux-block On 2019/11/13 10:54, Damien Le Moal wrote: > On 2019/11/12 23:48, Tetsuo Handa wrote: > [...] >>>> +static int blk_should_abort(struct bio *bio) >>>> +{ >>>> + int ret; >>>> + >>>> + cond_resched(); >>>> + if (!fatal_signal_pending(current)) >>>> + return 0; >>>> + ret = submit_bio_wait(bio); >>> >>> This will change the behavior of __blkdev_issue_discard() to a sync IO >>> execution instead of the current async execution since submit_bio_wait() >>> call is the responsibility of the caller (e.g. blkdev_issue_discard()). >>> Have you checked if users of __blkdev_issue_discard() are OK with that ? >>> f2fs, ext4, xfs, dm and nvme use this function. >> >> I'm not sure... >> >>> >>> Looking at f2fs, this does not look like it is going to work as expected >>> since the bio setup, including end_io callback, is done after this >>> function is called and a regular submit_bio() execution is being used. >> >> Then, just breaking the iteration like below? >> nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem? >> >> --- a/block/blk-lib.c >> +++ b/block/blk-lib.c >> @@ -7,6 +7,7 @@ >> #include <linux/bio.h> >> #include <linux/blkdev.h> >> #include <linux/scatterlist.h> >> +#include <linux/sched/signal.h> >> >> #include "blk.h" >> >> @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, >> struct bio *bio = *biop; >> unsigned int op; >> sector_t bs_mask; >> + int ret = 0; >> >> if (!q) >> return -ENXIO; >> @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, >> * is disabled. >> */ >> cond_resched(); >> + if (fatal_signal_pending(current)) { >> + ret = -EINTR; >> + break; >> + } >> } >> >> *biop = bio; >> - return 0; >> + return ret; > > This will leak a bio as blkdev_issue_discard() executes the bio only in > the case "if (!ret && bio)". So that does not work as is, unless all > callers of __blkdev_issue_discard() are also changed. Same problem for > the other __blkdev_issue_xxx() functions. > > Looking more into this, if an error is returned here, no bio should be > returned and we need to make sure that all started bios are also > completed. So your helper blk_should_abort() did the right thing calling > submit_bio_wait(). However, I Think it would be better to fail > immediately the current loop bio instead of executing it and then > reporting the -EINTR error, unconditionally, regardless of what the > started bios completion status is. > > This could be done with the help of a function like this, very similar > to submit_bio_wait(). > > void bio_chain_end_wait(struct bio *bio) > { > DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map); > > bio->bi_private = &done; > bio->bi_end_io = submit_bio_wait_endio; > bio->bi_opf |= REQ_SYNC; > bio_endio(bio); > wait_for_completion_io(&done); > } > > And then your helper function becomes something like this: > > static int blk_should_abort(struct bio *bio) > { > int ret; > > cond_resched(); > if (!fatal_signal_pending(current)) > return 0; > > if (bio_flagged(bio, BIO_CHAIN)) > bio_chain_end_wait(bio); I don't know about block layer, but I feel this is bad because bio_put() will be called without submit_bio_wait() when bio_flagged() == false. Who calls submit_bio_wait() if bio_flagged() == false ? > bio_put(bio); > > return -EINTR; > } > > Thoughts ? > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] block: Bail out iteration functions upon SIGKILL. 2019-11-15 10:05 ` Tetsuo Handa @ 2019-11-18 0:02 ` Damien Le Moal 0 siblings, 0 replies; 14+ messages in thread From: Damien Le Moal @ 2019-11-18 0:02 UTC (permalink / raw) To: Tetsuo Handa; +Cc: Bob Liu, axboe, linux-block On 2019/11/15 19:05, Tetsuo Handa wrote: > On 2019/11/13 10:54, Damien Le Moal wrote: >> On 2019/11/12 23:48, Tetsuo Handa wrote: >> [...] >>>>> +static int blk_should_abort(struct bio *bio) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + cond_resched(); >>>>> + if (!fatal_signal_pending(current)) >>>>> + return 0; >>>>> + ret = submit_bio_wait(bio); >>>> >>>> This will change the behavior of __blkdev_issue_discard() to a sync IO >>>> execution instead of the current async execution since submit_bio_wait() >>>> call is the responsibility of the caller (e.g. blkdev_issue_discard()). >>>> Have you checked if users of __blkdev_issue_discard() are OK with that ? >>>> f2fs, ext4, xfs, dm and nvme use this function. >>> >>> I'm not sure... >>> >>>> >>>> Looking at f2fs, this does not look like it is going to work as expected >>>> since the bio setup, including end_io callback, is done after this >>>> function is called and a regular submit_bio() execution is being used. >>> >>> Then, just breaking the iteration like below? >>> nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem? >>> >>> --- a/block/blk-lib.c >>> +++ b/block/blk-lib.c >>> @@ -7,6 +7,7 @@ >>> #include <linux/bio.h> >>> #include <linux/blkdev.h> >>> #include <linux/scatterlist.h> >>> +#include <linux/sched/signal.h> >>> >>> #include "blk.h" >>> >>> @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, >>> struct bio *bio = *biop; >>> unsigned int op; >>> sector_t bs_mask; >>> + int ret = 0; >>> >>> if (!q) >>> return -ENXIO; >>> @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, >>> * is disabled. >>> */ >>> cond_resched(); >>> + if (fatal_signal_pending(current)) { >>> + ret = -EINTR; >>> + break; >>> + } >>> } >>> >>> *biop = bio; >>> - return 0; >>> + return ret; >> >> This will leak a bio as blkdev_issue_discard() executes the bio only in >> the case "if (!ret && bio)". So that does not work as is, unless all >> callers of __blkdev_issue_discard() are also changed. Same problem for >> the other __blkdev_issue_xxx() functions. >> >> Looking more into this, if an error is returned here, no bio should be >> returned and we need to make sure that all started bios are also >> completed. So your helper blk_should_abort() did the right thing calling >> submit_bio_wait(). However, I Think it would be better to fail >> immediately the current loop bio instead of executing it and then >> reporting the -EINTR error, unconditionally, regardless of what the >> started bios completion status is. >> >> This could be done with the help of a function like this, very similar >> to submit_bio_wait(). >> >> void bio_chain_end_wait(struct bio *bio) >> { >> DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map); >> >> bio->bi_private = &done; >> bio->bi_end_io = submit_bio_wait_endio; >> bio->bi_opf |= REQ_SYNC; >> bio_endio(bio); >> wait_for_completion_io(&done); >> } >> >> And then your helper function becomes something like this: >> >> static int blk_should_abort(struct bio *bio) >> { >> int ret; >> >> cond_resched(); >> if (!fatal_signal_pending(current)) >> return 0; >> >> if (bio_flagged(bio, BIO_CHAIN)) >> bio_chain_end_wait(bio); > > I don't know about block layer, but I feel this is bad because bio_put() > will be called without submit_bio_wait() when bio_flagged() == false. > Who calls submit_bio_wait() if bio_flagged() == false ? If the BIO is not flagged, then it is not chained and so does not need to be executed at all and can be dropped (freed) right away with bio_put(). No need (and in fact we do not want) to execute it at all. For other cases where bio is flagged, it means that it is chained and so that previous BIOs where already started by the submit_bio() call in bio_next(). In this case, the current BIO is still *not* executed at all and bio_endio() is called for it instead of submit_bio_wait(). But since bio_endio() is called after setting: bio->bi_end_io = submit_bio_wait_endio; the bio_endio() call has the same effect as the completion of the bio if it were executed: the previous chained BIO completion is waited for. > >> bio_put(bio); >> >> return -EINTR; >> } >> >> Thoughts ? >> > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-11-18 0:02 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-23 7:56 INFO: task syz-executor can't die for more than 143 seconds. (2) syzbot 2019-10-24 10:08 ` Tetsuo Handa 2019-10-28 8:51 ` Bob Liu 2019-11-08 11:41 ` [PATCH] block: Bail out iteration functions upon SIGKILL Tetsuo Handa 2019-11-08 18:13 ` Chaitanya Kulkarni 2019-11-08 22:18 ` Chaitanya Kulkarni 2019-11-12 4:05 ` Damien Le Moal 2019-11-12 14:47 ` Tetsuo Handa 2019-11-13 1:54 ` Damien Le Moal 2019-11-13 6:55 ` Ming Lei 2019-11-13 7:11 ` Damien Le Moal 2019-11-13 7:49 ` Ming Lei 2019-11-15 10:05 ` Tetsuo Handa 2019-11-18 0:02 ` Damien Le Moal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).