* bioset_exit poison from dm_destroy
@ 2022-05-29 0:17 ` Matthew Wilcox
0 siblings, 0 replies; 28+ messages in thread
From: Matthew Wilcox @ 2022-05-29 0:17 UTC (permalink / raw)
To: dm-devel, linux-block
Not quite sure whose bug this is. Current Linus head running xfstests
against ext4 (probably not ext4's fault?)
01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
01818 PKRU: 55555554
01818 Call Trace:
01818 <TASK>
01818 ? kfree+0x66/0x250
01818 bioset_exit+0x32/0x210
01818 cleanup_mapped_device+0x34/0xf0
01818 __dm_destroy+0x149/0x1f0
01818 ? table_clear+0xc0/0xc0
01818 dm_destroy+0xe/0x10
01818 dev_remove+0xd9/0x120
01818 ctl_ioctl+0x1cb/0x420
01818 dm_ctl_ioctl+0x9/0x10
01818 __x64_sys_ioctl+0x89/0xb0
01818 do_syscall_64+0x35/0x80
01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
01818 RIP: 0033:0x7ff56de3b397
01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
01818 </TASK>
01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [dm-devel] bioset_exit poison from dm_destroy
@ 2022-05-29 0:17 ` Matthew Wilcox
0 siblings, 0 replies; 28+ messages in thread
From: Matthew Wilcox @ 2022-05-29 0:17 UTC (permalink / raw)
To: dm-devel, linux-block
Not quite sure whose bug this is. Current Linus head running xfstests
against ext4 (probably not ext4's fault?)
01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
01818 EXT4-fs (dm-0): unmounting filesystem.
01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
01818 PKRU: 55555554
01818 Call Trace:
01818 <TASK>
01818 ? kfree+0x66/0x250
01818 bioset_exit+0x32/0x210
01818 cleanup_mapped_device+0x34/0xf0
01818 __dm_destroy+0x149/0x1f0
01818 ? table_clear+0xc0/0xc0
01818 dm_destroy+0xe/0x10
01818 dev_remove+0xd9/0x120
01818 ctl_ioctl+0x1cb/0x420
01818 dm_ctl_ioctl+0x9/0x10
01818 __x64_sys_ioctl+0x89/0xb0
01818 do_syscall_64+0x35/0x80
01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
01818 RIP: 0033:0x7ff56de3b397
01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
01818 </TASK>
01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-05-29 0:17 ` [dm-devel] " Matthew Wilcox
@ 2022-05-29 12:46 ` Jens Axboe
-1 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-05-29 12:46 UTC (permalink / raw)
To: Matthew Wilcox, dm-devel, linux-block
On 5/28/22 6:17 PM, Matthew Wilcox wrote:
> Not quite sure whose bug this is. Current Linus head running xfstests
> against ext4 (probably not ext4's fault?)
>
> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
> 01818 PKRU: 55555554
> 01818 Call Trace:
> 01818 <TASK>
> 01818 ? kfree+0x66/0x250
> 01818 bioset_exit+0x32/0x210
> 01818 cleanup_mapped_device+0x34/0xf0
> 01818 __dm_destroy+0x149/0x1f0
> 01818 ? table_clear+0xc0/0xc0
> 01818 dm_destroy+0xe/0x10
> 01818 dev_remove+0xd9/0x120
> 01818 ctl_ioctl+0x1cb/0x420
> 01818 dm_ctl_ioctl+0x9/0x10
> 01818 __x64_sys_ioctl+0x89/0xb0
> 01818 do_syscall_64+0x35/0x80
> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 01818 RIP: 0033:0x7ff56de3b397
> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
> 01818 </TASK>
> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
I suspect dm is calling bioset_exit() multiple times? Which it probably
should not.
The reset of bioset_exit() is resilient against this, so might be best
to include bio_alloc_cache_destroy() in that.
diff --git a/block/bio.c b/block/bio.c
index a3893d80dccc..be3937b84e68 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
bio_alloc_cache_prune(cache, -1U);
}
free_percpu(bs->cache);
+ bs->cache = NULL;
}
/**
--
Jens Axboe
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-05-29 12:46 ` Jens Axboe
0 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-05-29 12:46 UTC (permalink / raw)
To: Matthew Wilcox, dm-devel, linux-block
On 5/28/22 6:17 PM, Matthew Wilcox wrote:
> Not quite sure whose bug this is. Current Linus head running xfstests
> against ext4 (probably not ext4's fault?)
>
> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> 01818 EXT4-fs (dm-0): unmounting filesystem.
> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
> 01818 PKRU: 55555554
> 01818 Call Trace:
> 01818 <TASK>
> 01818 ? kfree+0x66/0x250
> 01818 bioset_exit+0x32/0x210
> 01818 cleanup_mapped_device+0x34/0xf0
> 01818 __dm_destroy+0x149/0x1f0
> 01818 ? table_clear+0xc0/0xc0
> 01818 dm_destroy+0xe/0x10
> 01818 dev_remove+0xd9/0x120
> 01818 ctl_ioctl+0x1cb/0x420
> 01818 dm_ctl_ioctl+0x9/0x10
> 01818 __x64_sys_ioctl+0x89/0xb0
> 01818 do_syscall_64+0x35/0x80
> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 01818 RIP: 0033:0x7ff56de3b397
> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
> 01818 </TASK>
> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
I suspect dm is calling bioset_exit() multiple times? Which it probably
should not.
The reset of bioset_exit() is resilient against this, so might be best
to include bio_alloc_cache_destroy() in that.
diff --git a/block/bio.c b/block/bio.c
index a3893d80dccc..be3937b84e68 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
bio_alloc_cache_prune(cache, -1U);
}
free_percpu(bs->cache);
+ bs->cache = NULL;
}
/**
--
Jens Axboe
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-05-29 12:46 ` [dm-devel] " Jens Axboe
@ 2022-05-31 18:58 ` Mike Snitzer
-1 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-05-31 18:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: Matthew Wilcox, dm-devel, linux-block, david
On Sun, May 29 2022 at 8:46P -0400,
Jens Axboe <axboe@kernel.dk> wrote:
> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
> > Not quite sure whose bug this is. Current Linus head running xfstests
> > against ext4 (probably not ext4's fault?)
> >
> > 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
> > 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
> > 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> > 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
> > 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
> > 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
> > 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
> > 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
> > 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
> > 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
> > 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
> > 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
> > 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
> > 01818 PKRU: 55555554
> > 01818 Call Trace:
> > 01818 <TASK>
> > 01818 ? kfree+0x66/0x250
> > 01818 bioset_exit+0x32/0x210
> > 01818 cleanup_mapped_device+0x34/0xf0
> > 01818 __dm_destroy+0x149/0x1f0
> > 01818 ? table_clear+0xc0/0xc0
> > 01818 dm_destroy+0xe/0x10
> > 01818 dev_remove+0xd9/0x120
> > 01818 ctl_ioctl+0x1cb/0x420
> > 01818 dm_ctl_ioctl+0x9/0x10
> > 01818 __x64_sys_ioctl+0x89/0xb0
> > 01818 do_syscall_64+0x35/0x80
> > 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
> > 01818 RIP: 0033:0x7ff56de3b397
> > 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
> > 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> > 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
> > 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
> > 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
> > 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
> > 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
> > 01818 </TASK>
> > 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
>
> I suspect dm is calling bioset_exit() multiple times? Which it probably
> should not.
>
> The reset of bioset_exit() is resilient against this, so might be best
> to include bio_alloc_cache_destroy() in that.
>
>
> diff --git a/block/bio.c b/block/bio.c
> index a3893d80dccc..be3937b84e68 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
> bio_alloc_cache_prune(cache, -1U);
> }
> free_percpu(bs->cache);
> + bs->cache = NULL;
> }
>
> /**
Yes, we need the above to fix the crash. Does it also make sense to
add this?
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 49eff01fb829..f410c78e9c0c 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -681,7 +681,7 @@ struct bio_set {
static inline bool bioset_initialized(struct bio_set *bs)
{
- return bs->bio_slab != NULL;
+ return (bs->bio_slab != NULL || bs->cache != NULL);
}
#if defined(CONFIG_BLK_DEV_INTEGRITY)
FYI, DM's unique use of bioset_init_from_src() is the primary reason
why this stale pointer is biting us.
I dug into this issue further and have queued this patch:
From: Mike Snitzer <snitzer@kernel.org>
Date: Tue, 31 May 2022 12:16:49 -0400
Subject: [PATCH] dm table: fix dm_table_supports_poll to return false if no data devices
It was reported that the "generic/250" test in xfstests (which uses
the dm-error target) demonstrates a regression where the kernel
crashes in bioset_exit().
Since commit cfc97abcbe0b ("dm: conditionally enable
BIOSET_PERCPU_CACHE for dm_io bioset") the bioset_init() for the dm_io
bioset will setup the bioset's per-cpu alloc cache if all devices have
QUEUE_FLAG_POLL set.
But there was an bug where a target that doesn't have any data devices
(and that doesn't even set the .iterate_devices dm target callback)
will incorrectly return true from dm_table_supports_poll().
Fix this by updating dm_table_supports_poll() to follow dm-table.c's
well-worn pattern for testing that _all_ targets in a DM table do in
fact have underlying devices that set QUEUE_FLAG_POLL.
NOTE: An additional block fix is still needed so that
bio_alloc_cache_destroy() clears the bioset's ->cache member.
Otherwise, a DM device's table reload that transitions the DM device's
bioset from using a per-cpu alloc cache to _not_ using one will result
in bioset_exit() crashing in bio_alloc_cache_destroy() because dm's
dm_io bioset ("io_bs") was left with a stale ->cache member.
Fixes: cfc97abcbe0b ("dm: conditionally enable BIOSET_PERCPU_CACHE for dm_io bioset")
Reported-by: Matthew Wilcox <willy@infradead.org>
Reported-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
drivers/md/dm-table.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index a37c7b763643..0e833a154b31 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1005,7 +1005,7 @@ bool dm_table_request_based(struct dm_table *t)
return __table_type_request_based(dm_table_get_type(t));
}
-static int dm_table_supports_poll(struct dm_table *t);
+static bool dm_table_supports_poll(struct dm_table *t);
static int dm_table_alloc_md_mempools(struct dm_table *t, struct mapped_device *md)
{
@@ -1027,7 +1027,7 @@ static int dm_table_alloc_md_mempools(struct dm_table *t, struct mapped_device *
per_io_data_size = max(per_io_data_size, ti->per_io_data_size);
min_pool_size = max(min_pool_size, ti->num_flush_bios);
}
- poll_supported = !!dm_table_supports_poll(t);
+ poll_supported = dm_table_supports_poll(t);
}
t->mempools = dm_alloc_md_mempools(md, type, per_io_data_size, min_pool_size,
@@ -1547,9 +1547,20 @@ static int count_device(struct dm_target *ti, struct dm_dev *dev,
return 0;
}
-static int dm_table_supports_poll(struct dm_table *t)
+static bool dm_table_supports_poll(struct dm_table *t)
{
- return !dm_table_any_dev_attr(t, device_not_poll_capable, NULL);
+ struct dm_target *ti;
+ unsigned i = 0;
+
+ while (i < dm_table_get_num_targets(t)) {
+ ti = dm_table_get_target(t, i++);
+
+ if (!ti->type->iterate_devices ||
+ ti->type->iterate_devices(ti, device_not_poll_capable, NULL))
+ return false;
+ }
+
+ return true;
}
/*
--
2.15.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-05-31 18:58 ` Mike Snitzer
0 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-05-31 18:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, dm-devel, david, Matthew Wilcox
On Sun, May 29 2022 at 8:46P -0400,
Jens Axboe <axboe@kernel.dk> wrote:
> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
> > Not quite sure whose bug this is. Current Linus head running xfstests
> > against ext4 (probably not ext4's fault?)
> >
> > 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> > 01818 EXT4-fs (dm-0): unmounting filesystem.
> > 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
> > 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
> > 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> > 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
> > 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
> > 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
> > 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
> > 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
> > 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
> > 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
> > 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
> > 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
> > 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
> > 01818 PKRU: 55555554
> > 01818 Call Trace:
> > 01818 <TASK>
> > 01818 ? kfree+0x66/0x250
> > 01818 bioset_exit+0x32/0x210
> > 01818 cleanup_mapped_device+0x34/0xf0
> > 01818 __dm_destroy+0x149/0x1f0
> > 01818 ? table_clear+0xc0/0xc0
> > 01818 dm_destroy+0xe/0x10
> > 01818 dev_remove+0xd9/0x120
> > 01818 ctl_ioctl+0x1cb/0x420
> > 01818 dm_ctl_ioctl+0x9/0x10
> > 01818 __x64_sys_ioctl+0x89/0xb0
> > 01818 do_syscall_64+0x35/0x80
> > 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
> > 01818 RIP: 0033:0x7ff56de3b397
> > 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
> > 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> > 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
> > 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
> > 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
> > 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
> > 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
> > 01818 </TASK>
> > 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
>
> I suspect dm is calling bioset_exit() multiple times? Which it probably
> should not.
>
> The reset of bioset_exit() is resilient against this, so might be best
> to include bio_alloc_cache_destroy() in that.
>
>
> diff --git a/block/bio.c b/block/bio.c
> index a3893d80dccc..be3937b84e68 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
> bio_alloc_cache_prune(cache, -1U);
> }
> free_percpu(bs->cache);
> + bs->cache = NULL;
> }
>
> /**
Yes, we need the above to fix the crash. Does it also make sense to
add this?
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 49eff01fb829..f410c78e9c0c 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -681,7 +681,7 @@ struct bio_set {
static inline bool bioset_initialized(struct bio_set *bs)
{
- return bs->bio_slab != NULL;
+ return (bs->bio_slab != NULL || bs->cache != NULL);
}
#if defined(CONFIG_BLK_DEV_INTEGRITY)
FYI, DM's unique use of bioset_init_from_src() is the primary reason
why this stale pointer is biting us.
I dug into this issue further and have queued this patch:
From: Mike Snitzer <snitzer@kernel.org>
Date: Tue, 31 May 2022 12:16:49 -0400
Subject: [PATCH] dm table: fix dm_table_supports_poll to return false if no data devices
It was reported that the "generic/250" test in xfstests (which uses
the dm-error target) demonstrates a regression where the kernel
crashes in bioset_exit().
Since commit cfc97abcbe0b ("dm: conditionally enable
BIOSET_PERCPU_CACHE for dm_io bioset") the bioset_init() for the dm_io
bioset will setup the bioset's per-cpu alloc cache if all devices have
QUEUE_FLAG_POLL set.
But there was an bug where a target that doesn't have any data devices
(and that doesn't even set the .iterate_devices dm target callback)
will incorrectly return true from dm_table_supports_poll().
Fix this by updating dm_table_supports_poll() to follow dm-table.c's
well-worn pattern for testing that _all_ targets in a DM table do in
fact have underlying devices that set QUEUE_FLAG_POLL.
NOTE: An additional block fix is still needed so that
bio_alloc_cache_destroy() clears the bioset's ->cache member.
Otherwise, a DM device's table reload that transitions the DM device's
bioset from using a per-cpu alloc cache to _not_ using one will result
in bioset_exit() crashing in bio_alloc_cache_destroy() because dm's
dm_io bioset ("io_bs") was left with a stale ->cache member.
Fixes: cfc97abcbe0b ("dm: conditionally enable BIOSET_PERCPU_CACHE for dm_io bioset")
Reported-by: Matthew Wilcox <willy@infradead.org>
Reported-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
drivers/md/dm-table.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index a37c7b763643..0e833a154b31 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1005,7 +1005,7 @@ bool dm_table_request_based(struct dm_table *t)
return __table_type_request_based(dm_table_get_type(t));
}
-static int dm_table_supports_poll(struct dm_table *t);
+static bool dm_table_supports_poll(struct dm_table *t);
static int dm_table_alloc_md_mempools(struct dm_table *t, struct mapped_device *md)
{
@@ -1027,7 +1027,7 @@ static int dm_table_alloc_md_mempools(struct dm_table *t, struct mapped_device *
per_io_data_size = max(per_io_data_size, ti->per_io_data_size);
min_pool_size = max(min_pool_size, ti->num_flush_bios);
}
- poll_supported = !!dm_table_supports_poll(t);
+ poll_supported = dm_table_supports_poll(t);
}
t->mempools = dm_alloc_md_mempools(md, type, per_io_data_size, min_pool_size,
@@ -1547,9 +1547,20 @@ static int count_device(struct dm_target *ti, struct dm_dev *dev,
return 0;
}
-static int dm_table_supports_poll(struct dm_table *t)
+static bool dm_table_supports_poll(struct dm_table *t)
{
- return !dm_table_any_dev_attr(t, device_not_poll_capable, NULL);
+ struct dm_target *ti;
+ unsigned i = 0;
+
+ while (i < dm_table_get_num_targets(t)) {
+ ti = dm_table_get_target(t, i++);
+
+ if (!ti->type->iterate_devices ||
+ ti->type->iterate_devices(ti, device_not_poll_capable, NULL))
+ return false;
+ }
+
+ return true;
}
/*
--
2.15.0
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-05-31 18:58 ` [dm-devel] " Mike Snitzer
@ 2022-05-31 19:00 ` Jens Axboe
-1 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-05-31 19:00 UTC (permalink / raw)
To: Mike Snitzer; +Cc: Matthew Wilcox, dm-devel, linux-block, david
On 5/31/22 12:58 PM, Mike Snitzer wrote:
> On Sun, May 29 2022 at 8:46P -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
>>> Not quite sure whose bug this is. Current Linus head running xfstests
>>> against ext4 (probably not ext4's fault?)
>>>
>>> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
>>> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
>>> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
>>> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
>>> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
>>> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
>>> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
>>> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
>>> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
>>> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
>>> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
>>> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
>>> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
>>> 01818 PKRU: 55555554
>>> 01818 Call Trace:
>>> 01818 <TASK>
>>> 01818 ? kfree+0x66/0x250
>>> 01818 bioset_exit+0x32/0x210
>>> 01818 cleanup_mapped_device+0x34/0xf0
>>> 01818 __dm_destroy+0x149/0x1f0
>>> 01818 ? table_clear+0xc0/0xc0
>>> 01818 dm_destroy+0xe/0x10
>>> 01818 dev_remove+0xd9/0x120
>>> 01818 ctl_ioctl+0x1cb/0x420
>>> 01818 dm_ctl_ioctl+0x9/0x10
>>> 01818 __x64_sys_ioctl+0x89/0xb0
>>> 01818 do_syscall_64+0x35/0x80
>>> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>> 01818 RIP: 0033:0x7ff56de3b397
>>> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
>>> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
>>> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
>>> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
>>> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
>>> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
>>> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
>>> 01818 </TASK>
>>> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
>>
>> I suspect dm is calling bioset_exit() multiple times? Which it probably
>> should not.
>>
>> The reset of bioset_exit() is resilient against this, so might be best
>> to include bio_alloc_cache_destroy() in that.
>>
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index a3893d80dccc..be3937b84e68 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
>> bio_alloc_cache_prune(cache, -1U);
>> }
>> free_percpu(bs->cache);
>> + bs->cache = NULL;
>> }
>>
>> /**
>
> Yes, we need the above to fix the crash. Does it also make sense to
> add this?
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 49eff01fb829..f410c78e9c0c 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -681,7 +681,7 @@ struct bio_set {
>
> static inline bool bioset_initialized(struct bio_set *bs)
> {
> - return bs->bio_slab != NULL;
> + return (bs->bio_slab != NULL || bs->cache != NULL);
> }
Should not be possible to have valid bs->cache without bs->bio_slab?
--
Jens Axboe
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-05-31 19:00 ` Jens Axboe
0 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-05-31 19:00 UTC (permalink / raw)
To: Mike Snitzer; +Cc: linux-block, dm-devel, david, Matthew Wilcox
On 5/31/22 12:58 PM, Mike Snitzer wrote:
> On Sun, May 29 2022 at 8:46P -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
>>> Not quite sure whose bug this is. Current Linus head running xfstests
>>> against ext4 (probably not ext4's fault?)
>>>
>>> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
>>> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
>>> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
>>> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
>>> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
>>> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
>>> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
>>> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
>>> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
>>> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
>>> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
>>> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
>>> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
>>> 01818 PKRU: 55555554
>>> 01818 Call Trace:
>>> 01818 <TASK>
>>> 01818 ? kfree+0x66/0x250
>>> 01818 bioset_exit+0x32/0x210
>>> 01818 cleanup_mapped_device+0x34/0xf0
>>> 01818 __dm_destroy+0x149/0x1f0
>>> 01818 ? table_clear+0xc0/0xc0
>>> 01818 dm_destroy+0xe/0x10
>>> 01818 dev_remove+0xd9/0x120
>>> 01818 ctl_ioctl+0x1cb/0x420
>>> 01818 dm_ctl_ioctl+0x9/0x10
>>> 01818 __x64_sys_ioctl+0x89/0xb0
>>> 01818 do_syscall_64+0x35/0x80
>>> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>> 01818 RIP: 0033:0x7ff56de3b397
>>> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
>>> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
>>> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
>>> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
>>> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
>>> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
>>> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
>>> 01818 </TASK>
>>> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
>>
>> I suspect dm is calling bioset_exit() multiple times? Which it probably
>> should not.
>>
>> The reset of bioset_exit() is resilient against this, so might be best
>> to include bio_alloc_cache_destroy() in that.
>>
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index a3893d80dccc..be3937b84e68 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
>> bio_alloc_cache_prune(cache, -1U);
>> }
>> free_percpu(bs->cache);
>> + bs->cache = NULL;
>> }
>>
>> /**
>
> Yes, we need the above to fix the crash. Does it also make sense to
> add this?
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 49eff01fb829..f410c78e9c0c 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -681,7 +681,7 @@ struct bio_set {
>
> static inline bool bioset_initialized(struct bio_set *bs)
> {
> - return bs->bio_slab != NULL;
> + return (bs->bio_slab != NULL || bs->cache != NULL);
> }
Should not be possible to have valid bs->cache without bs->bio_slab?
--
Jens Axboe
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-05-31 19:00 ` [dm-devel] " Jens Axboe
@ 2022-05-31 19:49 ` Mike Snitzer
-1 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-05-31 19:49 UTC (permalink / raw)
To: Jens Axboe; +Cc: Matthew Wilcox, dm-devel, linux-block, david
On Tue, May 31 2022 at 3:00P -0400,
Jens Axboe <axboe@kernel.dk> wrote:
> On 5/31/22 12:58 PM, Mike Snitzer wrote:
> > On Sun, May 29 2022 at 8:46P -0400,
> > Jens Axboe <axboe@kernel.dk> wrote:
> >
> >> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
> >>> Not quite sure whose bug this is. Current Linus head running xfstests
> >>> against ext4 (probably not ext4's fault?)
> >>>
> >>> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
> >>> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
> >>> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> >>> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
> >>> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
> >>> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
> >>> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
> >>> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
> >>> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
> >>> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
> >>> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
> >>> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
> >>> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
> >>> 01818 PKRU: 55555554
> >>> 01818 Call Trace:
> >>> 01818 <TASK>
> >>> 01818 ? kfree+0x66/0x250
> >>> 01818 bioset_exit+0x32/0x210
> >>> 01818 cleanup_mapped_device+0x34/0xf0
> >>> 01818 __dm_destroy+0x149/0x1f0
> >>> 01818 ? table_clear+0xc0/0xc0
> >>> 01818 dm_destroy+0xe/0x10
> >>> 01818 dev_remove+0xd9/0x120
> >>> 01818 ctl_ioctl+0x1cb/0x420
> >>> 01818 dm_ctl_ioctl+0x9/0x10
> >>> 01818 __x64_sys_ioctl+0x89/0xb0
> >>> 01818 do_syscall_64+0x35/0x80
> >>> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
> >>> 01818 RIP: 0033:0x7ff56de3b397
> >>> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
> >>> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> >>> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
> >>> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
> >>> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
> >>> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
> >>> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
> >>> 01818 </TASK>
> >>> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
> >>
> >> I suspect dm is calling bioset_exit() multiple times? Which it probably
> >> should not.
> >>
> >> The reset of bioset_exit() is resilient against this, so might be best
> >> to include bio_alloc_cache_destroy() in that.
> >>
> >>
> >> diff --git a/block/bio.c b/block/bio.c
> >> index a3893d80dccc..be3937b84e68 100644
> >> --- a/block/bio.c
> >> +++ b/block/bio.c
> >> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
> >> bio_alloc_cache_prune(cache, -1U);
> >> }
> >> free_percpu(bs->cache);
> >> + bs->cache = NULL;
> >> }
> >>
> >> /**
> >
> > Yes, we need the above to fix the crash. Does it also make sense to
> > add this?
> >
> > diff --git a/include/linux/bio.h b/include/linux/bio.h
> > index 49eff01fb829..f410c78e9c0c 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -681,7 +681,7 @@ struct bio_set {
> >
> > static inline bool bioset_initialized(struct bio_set *bs)
> > {
> > - return bs->bio_slab != NULL;
> > + return (bs->bio_slab != NULL || bs->cache != NULL);
> > }
>
> Should not be possible to have valid bs->cache without bs->bio_slab?
Not quite following your question, but I sprinkled this extra check in
purely because DM core uses bioset_initialized() to verify that the
bioset is _not_ yet initialized.
But it really doesn't make sense to consider the bioset _fully_
initialized if bs->bio_slab is NULL but bs->cache is NOT...
Anyway, really don't need this change but we do need your earlier
patch to reset bs->cache to NULL.
Thanks.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-05-31 19:49 ` Mike Snitzer
0 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-05-31 19:49 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, dm-devel, david, Matthew Wilcox
On Tue, May 31 2022 at 3:00P -0400,
Jens Axboe <axboe@kernel.dk> wrote:
> On 5/31/22 12:58 PM, Mike Snitzer wrote:
> > On Sun, May 29 2022 at 8:46P -0400,
> > Jens Axboe <axboe@kernel.dk> wrote:
> >
> >> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
> >>> Not quite sure whose bug this is. Current Linus head running xfstests
> >>> against ext4 (probably not ext4's fault?)
> >>>
> >>> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
> >>> 01818 EXT4-fs (dm-0): unmounting filesystem.
> >>> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
> >>> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
> >>> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> >>> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
> >>> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
> >>> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
> >>> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
> >>> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
> >>> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
> >>> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
> >>> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
> >>> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
> >>> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
> >>> 01818 PKRU: 55555554
> >>> 01818 Call Trace:
> >>> 01818 <TASK>
> >>> 01818 ? kfree+0x66/0x250
> >>> 01818 bioset_exit+0x32/0x210
> >>> 01818 cleanup_mapped_device+0x34/0xf0
> >>> 01818 __dm_destroy+0x149/0x1f0
> >>> 01818 ? table_clear+0xc0/0xc0
> >>> 01818 dm_destroy+0xe/0x10
> >>> 01818 dev_remove+0xd9/0x120
> >>> 01818 ctl_ioctl+0x1cb/0x420
> >>> 01818 dm_ctl_ioctl+0x9/0x10
> >>> 01818 __x64_sys_ioctl+0x89/0xb0
> >>> 01818 do_syscall_64+0x35/0x80
> >>> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
> >>> 01818 RIP: 0033:0x7ff56de3b397
> >>> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
> >>> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> >>> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
> >>> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
> >>> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
> >>> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
> >>> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
> >>> 01818 </TASK>
> >>> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
> >>
> >> I suspect dm is calling bioset_exit() multiple times? Which it probably
> >> should not.
> >>
> >> The reset of bioset_exit() is resilient against this, so might be best
> >> to include bio_alloc_cache_destroy() in that.
> >>
> >>
> >> diff --git a/block/bio.c b/block/bio.c
> >> index a3893d80dccc..be3937b84e68 100644
> >> --- a/block/bio.c
> >> +++ b/block/bio.c
> >> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
> >> bio_alloc_cache_prune(cache, -1U);
> >> }
> >> free_percpu(bs->cache);
> >> + bs->cache = NULL;
> >> }
> >>
> >> /**
> >
> > Yes, we need the above to fix the crash. Does it also make sense to
> > add this?
> >
> > diff --git a/include/linux/bio.h b/include/linux/bio.h
> > index 49eff01fb829..f410c78e9c0c 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -681,7 +681,7 @@ struct bio_set {
> >
> > static inline bool bioset_initialized(struct bio_set *bs)
> > {
> > - return bs->bio_slab != NULL;
> > + return (bs->bio_slab != NULL || bs->cache != NULL);
> > }
>
> Should not be possible to have valid bs->cache without bs->bio_slab?
Not quite following your question, but I sprinkled this extra check in
purely because DM core uses bioset_initialized() to verify that the
bioset is _not_ yet initialized.
But it really doesn't make sense to consider the bioset _fully_
initialized if bs->bio_slab is NULL but bs->cache is NOT...
Anyway, really don't need this change but we do need your earlier
patch to reset bs->cache to NULL.
Thanks.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-05-31 19:49 ` [dm-devel] " Mike Snitzer
@ 2022-06-01 3:27 ` Jens Axboe
-1 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-06-01 3:27 UTC (permalink / raw)
To: Mike Snitzer; +Cc: Matthew Wilcox, dm-devel, linux-block, david
On 5/31/22 1:49 PM, Mike Snitzer wrote:
> On Tue, May 31 2022 at 3:00P -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 5/31/22 12:58 PM, Mike Snitzer wrote:
>>> On Sun, May 29 2022 at 8:46P -0400,
>>> Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>>> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
>>>>> Not quite sure whose bug this is. Current Linus head running xfstests
>>>>> against ext4 (probably not ext4's fault?)
>>>>>
>>>>> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
>>>>> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
>>>>> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
>>>>> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
>>>>> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
>>>>> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
>>>>> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
>>>>> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
>>>>> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
>>>>> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
>>>>> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
>>>>> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
>>>>> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
>>>>> 01818 PKRU: 55555554
>>>>> 01818 Call Trace:
>>>>> 01818 <TASK>
>>>>> 01818 ? kfree+0x66/0x250
>>>>> 01818 bioset_exit+0x32/0x210
>>>>> 01818 cleanup_mapped_device+0x34/0xf0
>>>>> 01818 __dm_destroy+0x149/0x1f0
>>>>> 01818 ? table_clear+0xc0/0xc0
>>>>> 01818 dm_destroy+0xe/0x10
>>>>> 01818 dev_remove+0xd9/0x120
>>>>> 01818 ctl_ioctl+0x1cb/0x420
>>>>> 01818 dm_ctl_ioctl+0x9/0x10
>>>>> 01818 __x64_sys_ioctl+0x89/0xb0
>>>>> 01818 do_syscall_64+0x35/0x80
>>>>> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>>>> 01818 RIP: 0033:0x7ff56de3b397
>>>>> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
>>>>> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
>>>>> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
>>>>> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
>>>>> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
>>>>> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
>>>>> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
>>>>> 01818 </TASK>
>>>>> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
>>>>
>>>> I suspect dm is calling bioset_exit() multiple times? Which it probably
>>>> should not.
>>>>
>>>> The reset of bioset_exit() is resilient against this, so might be best
>>>> to include bio_alloc_cache_destroy() in that.
>>>>
>>>>
>>>> diff --git a/block/bio.c b/block/bio.c
>>>> index a3893d80dccc..be3937b84e68 100644
>>>> --- a/block/bio.c
>>>> +++ b/block/bio.c
>>>> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
>>>> bio_alloc_cache_prune(cache, -1U);
>>>> }
>>>> free_percpu(bs->cache);
>>>> + bs->cache = NULL;
>>>> }
>>>>
>>>> /**
>>>
>>> Yes, we need the above to fix the crash. Does it also make sense to
>>> add this?
>>>
>>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>>> index 49eff01fb829..f410c78e9c0c 100644
>>> --- a/include/linux/bio.h
>>> +++ b/include/linux/bio.h
>>> @@ -681,7 +681,7 @@ struct bio_set {
>>>
>>> static inline bool bioset_initialized(struct bio_set *bs)
>>> {
>>> - return bs->bio_slab != NULL;
>>> + return (bs->bio_slab != NULL || bs->cache != NULL);
>>> }
>>
>> Should not be possible to have valid bs->cache without bs->bio_slab?
>
> Not quite following your question, but I sprinkled this extra check in
> purely because DM core uses bioset_initialized() to verify that the
> bioset is _not_ yet initialized.
>
> But it really doesn't make sense to consider the bioset _fully_
> initialized if bs->bio_slab is NULL but bs->cache is NOT...
Honestly, I'd be way happier getting rid of bioset_initialized()!
> Anyway, really don't need this change but we do need your earlier
> patch to reset bs->cache to NULL.
Certainly, that patch will be going upstream in a day or two.
--
Jens Axboe
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-01 3:27 ` Jens Axboe
0 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-06-01 3:27 UTC (permalink / raw)
To: Mike Snitzer; +Cc: linux-block, dm-devel, david, Matthew Wilcox
On 5/31/22 1:49 PM, Mike Snitzer wrote:
> On Tue, May 31 2022 at 3:00P -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 5/31/22 12:58 PM, Mike Snitzer wrote:
>>> On Sun, May 29 2022 at 8:46P -0400,
>>> Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>>> On 5/28/22 6:17 PM, Matthew Wilcox wrote:
>>>>> Not quite sure whose bug this is. Current Linus head running xfstests
>>>>> against ext4 (probably not ext4's fault?)
>>>>>
>>>>> 01818 generic/250 run fstests generic/250 at 2022-05-28 23:48:09
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 Buffer I/O error on dev dm-0, logical block 3670000, async page read
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 EXT4-fs (dm-0): mounted filesystem with ordered data mode. Quota mode: none.
>>>>> 01818 EXT4-fs (dm-0): unmounting filesystem.
>>>>> 01818 general protection fault, probably for non-canonical address 0xdead000000000122: 0000 [#1] PREEMPT SMP NOPTI
>>>>> 01818 CPU: 0 PID: 1579117 Comm: dmsetup Kdump: loaded Not tainted 5.18.0-11049-g1dec3d7fd0c3-dirty #262
>>>>> 01818 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
>>>>> 01818 RIP: 0010:__cpuhp_state_remove_instance+0xf0/0x1b0
>>>>> 01818 Code: a0 f8 d7 81 42 3b 1c 28 7f d9 4c 89 e1 31 d2 89 de 89 7d dc e8 01 fd ff ff 8b 7d dc eb c5 49 8b 04 24 49 8b 54 24 08 48 85 c0 <48> 89 02 74 04 48 89 50 08 48 b8 00 01 00 00 00 00 ad de 48 c7 c7
>>>>> 01818 RSP: 0018:ffff888101fcfc60 EFLAGS: 00010286
>>>>> 01818 RAX: dead000000000100 RBX: 0000000000000017 RCX: 0000000000000000
>>>>> 01818 RDX: dead000000000122 RSI: ffff8881233b0ae8 RDI: ffffffff81e3b080
>>>>> 01818 RBP: ffff888101fcfc88 R08: 0000000000000008 R09: 0000000000000003
>>>>> 01818 R10: 0000000000000000 R11: 00000000002dc6c0 R12: ffff8881233b0ae8
>>>>> 01818 R13: 0000000000000000 R14: ffffffff81e38f58 R15: ffff88817b5a3c00
>>>>> 01818 FS: 00007ff56daec280(0000) GS:ffff888275800000(0000) knlGS:0000000000000000
>>>>> 01818 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> 01818 CR2: 00005591ad94f198 CR3: 000000017b5a0004 CR4: 0000000000770eb0
>>>>> 01818 PKRU: 55555554
>>>>> 01818 Call Trace:
>>>>> 01818 <TASK>
>>>>> 01818 ? kfree+0x66/0x250
>>>>> 01818 bioset_exit+0x32/0x210
>>>>> 01818 cleanup_mapped_device+0x34/0xf0
>>>>> 01818 __dm_destroy+0x149/0x1f0
>>>>> 01818 ? table_clear+0xc0/0xc0
>>>>> 01818 dm_destroy+0xe/0x10
>>>>> 01818 dev_remove+0xd9/0x120
>>>>> 01818 ctl_ioctl+0x1cb/0x420
>>>>> 01818 dm_ctl_ioctl+0x9/0x10
>>>>> 01818 __x64_sys_ioctl+0x89/0xb0
>>>>> 01818 do_syscall_64+0x35/0x80
>>>>> 01818 entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>>>> 01818 RIP: 0033:0x7ff56de3b397
>>>>> 01818 Code: 3c 1c e8 1c ff ff ff 85 c0 79 87 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a9 da 0d 00 f7 d8 64 89 01 48
>>>>> 01818 RSP: 002b:00007ffe55367ef8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
>>>>> 01818 RAX: ffffffffffffffda RBX: 00007ff56df31a8e RCX: 00007ff56de3b397
>>>>> 01818 RDX: 000055daad7cab30 RSI: 00000000c138fd04 RDI: 0000000000000003
>>>>> 01818 RBP: 00007ffe55367fb0 R08: 00007ff56df81558 R09: 00007ffe55367d60
>>>>> 01818 R10: 00007ff56df808a2 R11: 0000000000000206 R12: 00007ff56df808a2
>>>>> 01818 R13: 00007ff56df808a2 R14: 00007ff56df808a2 R15: 00007ff56df808a2
>>>>> 01818 </TASK>
>>>>> 01818 Modules linked in: crct10dif_generic crct10dif_common [last unloaded: crc_t10dif]
>>>>
>>>> I suspect dm is calling bioset_exit() multiple times? Which it probably
>>>> should not.
>>>>
>>>> The reset of bioset_exit() is resilient against this, so might be best
>>>> to include bio_alloc_cache_destroy() in that.
>>>>
>>>>
>>>> diff --git a/block/bio.c b/block/bio.c
>>>> index a3893d80dccc..be3937b84e68 100644
>>>> --- a/block/bio.c
>>>> +++ b/block/bio.c
>>>> @@ -722,6 +722,7 @@ static void bio_alloc_cache_destroy(struct bio_set *bs)
>>>> bio_alloc_cache_prune(cache, -1U);
>>>> }
>>>> free_percpu(bs->cache);
>>>> + bs->cache = NULL;
>>>> }
>>>>
>>>> /**
>>>
>>> Yes, we need the above to fix the crash. Does it also make sense to
>>> add this?
>>>
>>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>>> index 49eff01fb829..f410c78e9c0c 100644
>>> --- a/include/linux/bio.h
>>> +++ b/include/linux/bio.h
>>> @@ -681,7 +681,7 @@ struct bio_set {
>>>
>>> static inline bool bioset_initialized(struct bio_set *bs)
>>> {
>>> - return bs->bio_slab != NULL;
>>> + return (bs->bio_slab != NULL || bs->cache != NULL);
>>> }
>>
>> Should not be possible to have valid bs->cache without bs->bio_slab?
>
> Not quite following your question, but I sprinkled this extra check in
> purely because DM core uses bioset_initialized() to verify that the
> bioset is _not_ yet initialized.
>
> But it really doesn't make sense to consider the bioset _fully_
> initialized if bs->bio_slab is NULL but bs->cache is NOT...
Honestly, I'd be way happier getting rid of bioset_initialized()!
> Anyway, really don't need this change but we do need your earlier
> patch to reset bs->cache to NULL.
Certainly, that patch will be going upstream in a day or two.
--
Jens Axboe
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-05-31 18:58 ` [dm-devel] " Mike Snitzer
@ 2022-06-01 6:04 ` Christoph Hellwig
-1 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-01 6:04 UTC (permalink / raw)
To: Mike Snitzer; +Cc: Jens Axboe, Matthew Wilcox, dm-devel, linux-block, david
On Tue, May 31, 2022 at 02:58:00PM -0400, Mike Snitzer wrote:
> Yes, we need the above to fix the crash. Does it also make sense to
> add this?
Can we just stop treating bio_sets so sloppily and make the callers
handle their lifetime properly? No one should have to use
bioset_initialized (or double free bio_sets).
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-01 6:04 ` Christoph Hellwig
0 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-01 6:04 UTC (permalink / raw)
To: Mike Snitzer; +Cc: Jens Axboe, linux-block, dm-devel, david, Matthew Wilcox
On Tue, May 31, 2022 at 02:58:00PM -0400, Mike Snitzer wrote:
> Yes, we need the above to fix the crash. Does it also make sense to
> add this?
Can we just stop treating bio_sets so sloppily and make the callers
handle their lifetime properly? No one should have to use
bioset_initialized (or double free bio_sets).
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-06-01 6:04 ` [dm-devel] " Christoph Hellwig
@ 2022-06-01 6:08 ` Jens Axboe
-1 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-06-01 6:08 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: Matthew Wilcox, dm-devel, linux-block, david
On 6/1/22 12:04 AM, Christoph Hellwig wrote:
> On Tue, May 31, 2022 at 02:58:00PM -0400, Mike Snitzer wrote:
>> Yes, we need the above to fix the crash. Does it also make sense to
>> add this?
>
> Can we just stop treating bio_sets so sloppily and make the callers
> handle their lifetime properly? No one should have to use
> bioset_initialized (or double free bio_sets).
Yes, that was my point in the previous email - get rid of
bioset_initialized() and just fixup dm, which is the only user of it.
There really should be no need to have this kind of conditional checking
and initialization.
--
Jens Axboe
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-01 6:08 ` Jens Axboe
0 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-06-01 6:08 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: linux-block, dm-devel, david, Matthew Wilcox
On 6/1/22 12:04 AM, Christoph Hellwig wrote:
> On Tue, May 31, 2022 at 02:58:00PM -0400, Mike Snitzer wrote:
>> Yes, we need the above to fix the crash. Does it also make sense to
>> add this?
>
> Can we just stop treating bio_sets so sloppily and make the callers
> handle their lifetime properly? No one should have to use
> bioset_initialized (or double free bio_sets).
Yes, that was my point in the previous email - get rid of
bioset_initialized() and just fixup dm, which is the only user of it.
There really should be no need to have this kind of conditional checking
and initialization.
--
Jens Axboe
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-06-01 6:08 ` [dm-devel] " Jens Axboe
@ 2022-06-01 6:18 ` Christoph Hellwig
-1 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-01 6:18 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Mike Snitzer, Matthew Wilcox, dm-devel,
linux-block, david
On Wed, Jun 01, 2022 at 12:08:57AM -0600, Jens Axboe wrote:
> Yes, that was my point in the previous email - get rid of
> bioset_initialized() and just fixup dm, which is the only user of it.
> There really should be no need to have this kind of conditional checking
> and initialization.
dm and md, but yes, we should not really use this check anywhere.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-01 6:18 ` Christoph Hellwig
0 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-01 6:18 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Mike Snitzer, david, Matthew Wilcox,
Christoph Hellwig, dm-devel
On Wed, Jun 01, 2022 at 12:08:57AM -0600, Jens Axboe wrote:
> Yes, that was my point in the previous email - get rid of
> bioset_initialized() and just fixup dm, which is the only user of it.
> There really should be no need to have this kind of conditional checking
> and initialization.
dm and md, but yes, we should not really use this check anywhere.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
2022-06-01 6:04 ` [dm-devel] " Christoph Hellwig
@ 2022-06-01 14:13 ` Mike Snitzer
-1 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-06-01 14:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, linux-block, dm-devel, david, Matthew Wilcox
On Wed, Jun 01 2022 at 2:04P -0400,
Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, May 31, 2022 at 02:58:00PM -0400, Mike Snitzer wrote:
> > Yes, we need the above to fix the crash. Does it also make sense to
> > add this?
>
> Can we just stop treating bio_sets so sloppily and make the callers
> handle their lifetime properly? No one should have to use
> bioset_initialized (or double free bio_sets).
>
Please take the time to look at the code and save your judgement until
you do. That said, I'm not in love with the complexity of how DM
handles bioset initialization. But both you and Jens keep taking
shots at DM for doing things wrong without actually looking.
DM uses bioset_init_from_src(). Yet you've both assumed double frees
and such (while not entirely wrong your glossing over the detail that
there is intervening reinitialization of DM's biosets between the
bioset_exit()s)
And it really can just be that the block code had a bug where it
didn't clear bs->cache. Doesn't need to be cause for attacks.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
@ 2022-06-01 14:13 ` Mike Snitzer
0 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-06-01 14:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Matthew Wilcox, dm-devel, linux-block, david
On Wed, Jun 01 2022 at 2:04P -0400,
Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, May 31, 2022 at 02:58:00PM -0400, Mike Snitzer wrote:
> > Yes, we need the above to fix the crash. Does it also make sense to
> > add this?
>
> Can we just stop treating bio_sets so sloppily and make the callers
> handle their lifetime properly? No one should have to use
> bioset_initialized (or double free bio_sets).
>
Please take the time to look at the code and save your judgement until
you do. That said, I'm not in love with the complexity of how DM
handles bioset initialization. But both you and Jens keep taking
shots at DM for doing things wrong without actually looking.
DM uses bioset_init_from_src(). Yet you've both assumed double frees
and such (while not entirely wrong your glossing over the detail that
there is intervening reinitialization of DM's biosets between the
bioset_exit()s)
And it really can just be that the block code had a bug where it
didn't clear bs->cache. Doesn't need to be cause for attacks.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-06-01 14:13 ` Mike Snitzer
@ 2022-06-02 8:12 ` Christoph Hellwig
-1 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-02 8:12 UTC (permalink / raw)
To: Mike Snitzer
Cc: Christoph Hellwig, Jens Axboe, Matthew Wilcox, dm-devel,
linux-block, david
On Wed, Jun 01, 2022 at 10:13:34AM -0400, Mike Snitzer wrote:
> Please take the time to look at the code and save your judgement until
> you do. That said, I'm not in love with the complexity of how DM
> handles bioset initialization. But both you and Jens keep taking
> shots at DM for doing things wrong without actually looking.
I'm not taking shots. I'm just saying we should kill this API. In
the worse case the caller can keep track of if a bioset is initialized,
but in most cases we should be able to deduct it in a nicer way.
> DM uses bioset_init_from_src(). Yet you've both assumed double frees
> and such (while not entirely wrong your glossing over the detail that
> there is intervening reinitialization of DM's biosets between the
> bioset_exit()s)
And looking at the code, that use of bioset_init_from_src is completely
broken. It does not actually preallocated anything as intended by
dm (maybe that isn't actually needed) but just uses the biosets in
dm_md_mempools as an awkward way to carry parameters. And completely
loses bringing over the integrity allocations. And no, this is not
intended as a "cheap shot" against Jens who did that either..
This is what I think should fix this, and will allow us to remove
bioset_init_from_src which was a bad idea from the start:
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index d21648a923ea9..54c0473a51dde 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -33,6 +33,14 @@ struct dm_kobject_holder {
* access their members!
*/
+/*
+ * For mempools pre-allocation at the table loading time.
+ */
+struct dm_md_mempools {
+ struct bio_set bs;
+ struct bio_set io_bs;
+};
+
struct mapped_device {
struct mutex suspend_lock;
@@ -110,8 +118,7 @@ struct mapped_device {
/*
* io objects are allocated from here.
*/
- struct bio_set io_bs;
- struct bio_set bs;
+ struct dm_md_mempools *mempools;
/* kobject and completion */
struct dm_kobject_holder kobj_holder;
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 6087cdcaad46d..a83b98a8d2a99 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -319,7 +319,7 @@ static int setup_clone(struct request *clone, struct request *rq,
{
int r;
- r = blk_rq_prep_clone(clone, rq, &tio->md->bs, gfp_mask,
+ r = blk_rq_prep_clone(clone, rq, &tio->md->mempools->bs, gfp_mask,
dm_rq_bio_constructor, tio);
if (r)
return r;
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 0e833a154b31d..a8b016d6bf16e 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1044,11 +1044,6 @@ void dm_table_free_md_mempools(struct dm_table *t)
t->mempools = NULL;
}
-struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t)
-{
- return t->mempools;
-}
-
static int setup_indexes(struct dm_table *t)
{
int i;
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index dfb0a551bd880..8b21155d3c4f5 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -136,14 +136,6 @@ static int get_swap_bios(void)
return latch;
}
-/*
- * For mempools pre-allocation at the table loading time.
- */
-struct dm_md_mempools {
- struct bio_set bs;
- struct bio_set io_bs;
-};
-
struct table_device {
struct list_head list;
refcount_t count;
@@ -581,7 +573,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
struct dm_target_io *tio;
struct bio *clone;
- clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->io_bs);
+ clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->mempools->io_bs);
/* Set default bdev, but target must bio_set_dev() before issuing IO */
clone->bi_bdev = md->disk->part0;
@@ -628,7 +620,8 @@ static struct bio *alloc_tio(struct clone_info *ci, struct dm_target *ti,
} else {
struct mapped_device *md = ci->io->md;
- clone = bio_alloc_clone(NULL, ci->bio, gfp_mask, &md->bs);
+ clone = bio_alloc_clone(NULL, ci->bio, gfp_mask,
+ &md->mempools->bs);
if (!clone)
return NULL;
/* Set default bdev, but target must bio_set_dev() before issuing IO */
@@ -1876,8 +1869,7 @@ static void cleanup_mapped_device(struct mapped_device *md)
{
if (md->wq)
destroy_workqueue(md->wq);
- bioset_exit(&md->bs);
- bioset_exit(&md->io_bs);
+ dm_free_md_mempools(md->mempools);
if (md->dax_dev) {
dax_remove_host(md->disk);
@@ -2049,48 +2041,6 @@ static void free_dev(struct mapped_device *md)
kvfree(md);
}
-static int __bind_mempools(struct mapped_device *md, struct dm_table *t)
-{
- struct dm_md_mempools *p = dm_table_get_md_mempools(t);
- int ret = 0;
-
- if (dm_table_bio_based(t)) {
- /*
- * The md may already have mempools that need changing.
- * If so, reload bioset because front_pad may have changed
- * because a different table was loaded.
- */
- bioset_exit(&md->bs);
- bioset_exit(&md->io_bs);
-
- } else if (bioset_initialized(&md->bs)) {
- /*
- * There's no need to reload with request-based dm
- * because the size of front_pad doesn't change.
- * Note for future: If you are to reload bioset,
- * prep-ed requests in the queue may refer
- * to bio from the old bioset, so you must walk
- * through the queue to unprep.
- */
- goto out;
- }
-
- BUG_ON(!p ||
- bioset_initialized(&md->bs) ||
- bioset_initialized(&md->io_bs));
-
- ret = bioset_init_from_src(&md->bs, &p->bs);
- if (ret)
- goto out;
- ret = bioset_init_from_src(&md->io_bs, &p->io_bs);
- if (ret)
- bioset_exit(&md->bs);
-out:
- /* mempool bind completed, no longer need any mempools in the table */
- dm_table_free_md_mempools(t);
- return ret;
-}
-
/*
* Bind a table to the device.
*/
@@ -2144,12 +2094,28 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
* immutable singletons - used to optimize dm_mq_queue_rq.
*/
md->immutable_target = dm_table_get_immutable_target(t);
- }
- ret = __bind_mempools(md, t);
- if (ret) {
- old_map = ERR_PTR(ret);
- goto out;
+ /*
+ * There is no need to reload with request-based dm because the
+ * size of front_pad doesn't change.
+ *
+ * Note for future: If you are to reload bioset, prep-ed
+ * requests in the queue may refer to bio from the old bioset,
+ * so you must walk through the queue to unprep.
+ */
+ if (!md->mempools) {
+ md->mempools = t->mempools;
+ t->mempools = NULL;
+ }
+ } else {
+ /*
+ * The md may already have mempools that need changing.
+ * If so, reload bioset because front_pad may have changed
+ * because a different table was loaded.
+ */
+ dm_free_md_mempools(md->mempools);
+ md->mempools = t->mempools;
+ t->mempools = NULL;
}
ret = dm_table_set_restrictions(t, md->queue, limits);
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 3f89664fea010..86642234d4adb 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -72,7 +72,6 @@ struct dm_target *dm_table_get_wildcard_target(struct dm_table *t);
bool dm_table_bio_based(struct dm_table *t);
bool dm_table_request_based(struct dm_table *t);
void dm_table_free_md_mempools(struct dm_table *t);
-struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
void dm_lock_md_type(struct mapped_device *md);
void dm_unlock_md_type(struct mapped_device *md);
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-02 8:12 ` Christoph Hellwig
0 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-02 8:12 UTC (permalink / raw)
To: Mike Snitzer
Cc: Jens Axboe, linux-block, david, Matthew Wilcox,
Christoph Hellwig, dm-devel
On Wed, Jun 01, 2022 at 10:13:34AM -0400, Mike Snitzer wrote:
> Please take the time to look at the code and save your judgement until
> you do. That said, I'm not in love with the complexity of how DM
> handles bioset initialization. But both you and Jens keep taking
> shots at DM for doing things wrong without actually looking.
I'm not taking shots. I'm just saying we should kill this API. In
the worse case the caller can keep track of if a bioset is initialized,
but in most cases we should be able to deduct it in a nicer way.
> DM uses bioset_init_from_src(). Yet you've both assumed double frees
> and such (while not entirely wrong your glossing over the detail that
> there is intervening reinitialization of DM's biosets between the
> bioset_exit()s)
And looking at the code, that use of bioset_init_from_src is completely
broken. It does not actually preallocated anything as intended by
dm (maybe that isn't actually needed) but just uses the biosets in
dm_md_mempools as an awkward way to carry parameters. And completely
loses bringing over the integrity allocations. And no, this is not
intended as a "cheap shot" against Jens who did that either..
This is what I think should fix this, and will allow us to remove
bioset_init_from_src which was a bad idea from the start:
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index d21648a923ea9..54c0473a51dde 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -33,6 +33,14 @@ struct dm_kobject_holder {
* access their members!
*/
+/*
+ * For mempools pre-allocation at the table loading time.
+ */
+struct dm_md_mempools {
+ struct bio_set bs;
+ struct bio_set io_bs;
+};
+
struct mapped_device {
struct mutex suspend_lock;
@@ -110,8 +118,7 @@ struct mapped_device {
/*
* io objects are allocated from here.
*/
- struct bio_set io_bs;
- struct bio_set bs;
+ struct dm_md_mempools *mempools;
/* kobject and completion */
struct dm_kobject_holder kobj_holder;
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 6087cdcaad46d..a83b98a8d2a99 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -319,7 +319,7 @@ static int setup_clone(struct request *clone, struct request *rq,
{
int r;
- r = blk_rq_prep_clone(clone, rq, &tio->md->bs, gfp_mask,
+ r = blk_rq_prep_clone(clone, rq, &tio->md->mempools->bs, gfp_mask,
dm_rq_bio_constructor, tio);
if (r)
return r;
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 0e833a154b31d..a8b016d6bf16e 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1044,11 +1044,6 @@ void dm_table_free_md_mempools(struct dm_table *t)
t->mempools = NULL;
}
-struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t)
-{
- return t->mempools;
-}
-
static int setup_indexes(struct dm_table *t)
{
int i;
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index dfb0a551bd880..8b21155d3c4f5 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -136,14 +136,6 @@ static int get_swap_bios(void)
return latch;
}
-/*
- * For mempools pre-allocation at the table loading time.
- */
-struct dm_md_mempools {
- struct bio_set bs;
- struct bio_set io_bs;
-};
-
struct table_device {
struct list_head list;
refcount_t count;
@@ -581,7 +573,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
struct dm_target_io *tio;
struct bio *clone;
- clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->io_bs);
+ clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->mempools->io_bs);
/* Set default bdev, but target must bio_set_dev() before issuing IO */
clone->bi_bdev = md->disk->part0;
@@ -628,7 +620,8 @@ static struct bio *alloc_tio(struct clone_info *ci, struct dm_target *ti,
} else {
struct mapped_device *md = ci->io->md;
- clone = bio_alloc_clone(NULL, ci->bio, gfp_mask, &md->bs);
+ clone = bio_alloc_clone(NULL, ci->bio, gfp_mask,
+ &md->mempools->bs);
if (!clone)
return NULL;
/* Set default bdev, but target must bio_set_dev() before issuing IO */
@@ -1876,8 +1869,7 @@ static void cleanup_mapped_device(struct mapped_device *md)
{
if (md->wq)
destroy_workqueue(md->wq);
- bioset_exit(&md->bs);
- bioset_exit(&md->io_bs);
+ dm_free_md_mempools(md->mempools);
if (md->dax_dev) {
dax_remove_host(md->disk);
@@ -2049,48 +2041,6 @@ static void free_dev(struct mapped_device *md)
kvfree(md);
}
-static int __bind_mempools(struct mapped_device *md, struct dm_table *t)
-{
- struct dm_md_mempools *p = dm_table_get_md_mempools(t);
- int ret = 0;
-
- if (dm_table_bio_based(t)) {
- /*
- * The md may already have mempools that need changing.
- * If so, reload bioset because front_pad may have changed
- * because a different table was loaded.
- */
- bioset_exit(&md->bs);
- bioset_exit(&md->io_bs);
-
- } else if (bioset_initialized(&md->bs)) {
- /*
- * There's no need to reload with request-based dm
- * because the size of front_pad doesn't change.
- * Note for future: If you are to reload bioset,
- * prep-ed requests in the queue may refer
- * to bio from the old bioset, so you must walk
- * through the queue to unprep.
- */
- goto out;
- }
-
- BUG_ON(!p ||
- bioset_initialized(&md->bs) ||
- bioset_initialized(&md->io_bs));
-
- ret = bioset_init_from_src(&md->bs, &p->bs);
- if (ret)
- goto out;
- ret = bioset_init_from_src(&md->io_bs, &p->io_bs);
- if (ret)
- bioset_exit(&md->bs);
-out:
- /* mempool bind completed, no longer need any mempools in the table */
- dm_table_free_md_mempools(t);
- return ret;
-}
-
/*
* Bind a table to the device.
*/
@@ -2144,12 +2094,28 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
* immutable singletons - used to optimize dm_mq_queue_rq.
*/
md->immutable_target = dm_table_get_immutable_target(t);
- }
- ret = __bind_mempools(md, t);
- if (ret) {
- old_map = ERR_PTR(ret);
- goto out;
+ /*
+ * There is no need to reload with request-based dm because the
+ * size of front_pad doesn't change.
+ *
+ * Note for future: If you are to reload bioset, prep-ed
+ * requests in the queue may refer to bio from the old bioset,
+ * so you must walk through the queue to unprep.
+ */
+ if (!md->mempools) {
+ md->mempools = t->mempools;
+ t->mempools = NULL;
+ }
+ } else {
+ /*
+ * The md may already have mempools that need changing.
+ * If so, reload bioset because front_pad may have changed
+ * because a different table was loaded.
+ */
+ dm_free_md_mempools(md->mempools);
+ md->mempools = t->mempools;
+ t->mempools = NULL;
}
ret = dm_table_set_restrictions(t, md->queue, limits);
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 3f89664fea010..86642234d4adb 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -72,7 +72,6 @@ struct dm_target *dm_table_get_wildcard_target(struct dm_table *t);
bool dm_table_bio_based(struct dm_table *t);
bool dm_table_request_based(struct dm_table *t);
void dm_table_free_md_mempools(struct dm_table *t);
-struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
void dm_lock_md_type(struct mapped_device *md);
void dm_unlock_md_type(struct mapped_device *md);
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-06-02 8:12 ` [dm-devel] " Christoph Hellwig
@ 2022-06-02 8:18 ` Jens Axboe
-1 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-06-02 8:18 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: Matthew Wilcox, dm-devel, linux-block, david
On 6/2/22 2:12 AM, Christoph Hellwig wrote:
> On Wed, Jun 01, 2022 at 10:13:34AM -0400, Mike Snitzer wrote:
>> Please take the time to look at the code and save your judgement until
>> you do. That said, I'm not in love with the complexity of how DM
>> handles bioset initialization. But both you and Jens keep taking
>> shots at DM for doing things wrong without actually looking.
>
> I'm not taking shots. I'm just saying we should kill this API. In
> the worse case the caller can keep track of if a bioset is initialized,
> but in most cases we should be able to deduct it in a nicer way.
Yeah ditto, it's more an observation on needing something like
this_foo_was_initialized() is just a bad way to program it to begin
with. The caller should know this already, rather than us needing
functions and state in the struct to tell you about it.
>> DM uses bioset_init_from_src(). Yet you've both assumed double frees
>> and such (while not entirely wrong your glossing over the detail that
>> there is intervening reinitialization of DM's biosets between the
>> bioset_exit()s)
>
> And looking at the code, that use of bioset_init_from_src is completely
> broken. It does not actually preallocated anything as intended by
> dm (maybe that isn't actually needed) but just uses the biosets in
> dm_md_mempools as an awkward way to carry parameters. And completely
> loses bringing over the integrity allocations. And no, this is not
> intended as a "cheap shot" against Jens who did that either..
>
> This is what I think should fix this, and will allow us to remove
> bioset_init_from_src which was a bad idea from the start:
Based on a quick look, seems good to me.
--
Jens Axboe
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-02 8:18 ` Jens Axboe
0 siblings, 0 replies; 28+ messages in thread
From: Jens Axboe @ 2022-06-02 8:18 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: linux-block, dm-devel, david, Matthew Wilcox
On 6/2/22 2:12 AM, Christoph Hellwig wrote:
> On Wed, Jun 01, 2022 at 10:13:34AM -0400, Mike Snitzer wrote:
>> Please take the time to look at the code and save your judgement until
>> you do. That said, I'm not in love with the complexity of how DM
>> handles bioset initialization. But both you and Jens keep taking
>> shots at DM for doing things wrong without actually looking.
>
> I'm not taking shots. I'm just saying we should kill this API. In
> the worse case the caller can keep track of if a bioset is initialized,
> but in most cases we should be able to deduct it in a nicer way.
Yeah ditto, it's more an observation on needing something like
this_foo_was_initialized() is just a bad way to program it to begin
with. The caller should know this already, rather than us needing
functions and state in the struct to tell you about it.
>> DM uses bioset_init_from_src(). Yet you've both assumed double frees
>> and such (while not entirely wrong your glossing over the detail that
>> there is intervening reinitialization of DM's biosets between the
>> bioset_exit()s)
>
> And looking at the code, that use of bioset_init_from_src is completely
> broken. It does not actually preallocated anything as intended by
> dm (maybe that isn't actually needed) but just uses the biosets in
> dm_md_mempools as an awkward way to carry parameters. And completely
> loses bringing over the integrity allocations. And no, this is not
> intended as a "cheap shot" against Jens who did that either..
>
> This is what I think should fix this, and will allow us to remove
> bioset_init_from_src which was a bad idea from the start:
Based on a quick look, seems good to me.
--
Jens Axboe
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-06-02 8:12 ` [dm-devel] " Christoph Hellwig
@ 2022-06-03 13:09 ` Mike Snitzer
-1 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-06-03 13:09 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Matthew Wilcox, dm-devel, linux-block, david
On Thu, Jun 02 2022 at 4:12P -0400,
Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Jun 01, 2022 at 10:13:34AM -0400, Mike Snitzer wrote:
> > Please take the time to look at the code and save your judgement until
> > you do. That said, I'm not in love with the complexity of how DM
> > handles bioset initialization. But both you and Jens keep taking
> > shots at DM for doing things wrong without actually looking.
>
> I'm not taking shots. I'm just saying we should kill this API. In
> the worse case the caller can keep track of if a bioset is initialized,
> but in most cases we should be able to deduct it in a nicer way.
>
> > DM uses bioset_init_from_src(). Yet you've both assumed double frees
> > and such (while not entirely wrong your glossing over the detail that
> > there is intervening reinitialization of DM's biosets between the
> > bioset_exit()s)
>
> And looking at the code, that use of bioset_init_from_src is completely
> broken. It does not actually preallocated anything as intended by
> dm (maybe that isn't actually needed) but just uses the biosets in
> dm_md_mempools as an awkward way to carry parameters.
I think the point was to keep the biosets embedded in struct
mapped_device to avoid any possible cacheline bouncing due to the
bioset memory being disjoint.
But preserving that micro-optimization isn't something I've ever
quantified (at least not with focus).
> And completely loses bringing over the integrity allocations.
Good catch.
> This is what I think should fix this, and will allow us to remove
> bioset_init_from_src which was a bad idea from the start:
Looks fine, did you want to send an official patch with a proper
header and Signed-off-by?
Thanks,
Mike
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index d21648a923ea9..54c0473a51dde 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -33,6 +33,14 @@ struct dm_kobject_holder {
> * access their members!
> */
>
> +/*
> + * For mempools pre-allocation at the table loading time.
> + */
> +struct dm_md_mempools {
> + struct bio_set bs;
> + struct bio_set io_bs;
> +};
> +
> struct mapped_device {
> struct mutex suspend_lock;
>
> @@ -110,8 +118,7 @@ struct mapped_device {
> /*
> * io objects are allocated from here.
> */
> - struct bio_set io_bs;
> - struct bio_set bs;
> + struct dm_md_mempools *mempools;
>
> /* kobject and completion */
> struct dm_kobject_holder kobj_holder;
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 6087cdcaad46d..a83b98a8d2a99 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -319,7 +319,7 @@ static int setup_clone(struct request *clone, struct request *rq,
> {
> int r;
>
> - r = blk_rq_prep_clone(clone, rq, &tio->md->bs, gfp_mask,
> + r = blk_rq_prep_clone(clone, rq, &tio->md->mempools->bs, gfp_mask,
> dm_rq_bio_constructor, tio);
> if (r)
> return r;
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 0e833a154b31d..a8b016d6bf16e 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1044,11 +1044,6 @@ void dm_table_free_md_mempools(struct dm_table *t)
> t->mempools = NULL;
> }
>
> -struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t)
> -{
> - return t->mempools;
> -}
> -
> static int setup_indexes(struct dm_table *t)
> {
> int i;
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index dfb0a551bd880..8b21155d3c4f5 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -136,14 +136,6 @@ static int get_swap_bios(void)
> return latch;
> }
>
> -/*
> - * For mempools pre-allocation at the table loading time.
> - */
> -struct dm_md_mempools {
> - struct bio_set bs;
> - struct bio_set io_bs;
> -};
> -
> struct table_device {
> struct list_head list;
> refcount_t count;
> @@ -581,7 +573,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
> struct dm_target_io *tio;
> struct bio *clone;
>
> - clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->io_bs);
> + clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->mempools->io_bs);
> /* Set default bdev, but target must bio_set_dev() before issuing IO */
> clone->bi_bdev = md->disk->part0;
>
> @@ -628,7 +620,8 @@ static struct bio *alloc_tio(struct clone_info *ci, struct dm_target *ti,
> } else {
> struct mapped_device *md = ci->io->md;
>
> - clone = bio_alloc_clone(NULL, ci->bio, gfp_mask, &md->bs);
> + clone = bio_alloc_clone(NULL, ci->bio, gfp_mask,
> + &md->mempools->bs);
> if (!clone)
> return NULL;
> /* Set default bdev, but target must bio_set_dev() before issuing IO */
> @@ -1876,8 +1869,7 @@ static void cleanup_mapped_device(struct mapped_device *md)
> {
> if (md->wq)
> destroy_workqueue(md->wq);
> - bioset_exit(&md->bs);
> - bioset_exit(&md->io_bs);
> + dm_free_md_mempools(md->mempools);
>
> if (md->dax_dev) {
> dax_remove_host(md->disk);
> @@ -2049,48 +2041,6 @@ static void free_dev(struct mapped_device *md)
> kvfree(md);
> }
>
> -static int __bind_mempools(struct mapped_device *md, struct dm_table *t)
> -{
> - struct dm_md_mempools *p = dm_table_get_md_mempools(t);
> - int ret = 0;
> -
> - if (dm_table_bio_based(t)) {
> - /*
> - * The md may already have mempools that need changing.
> - * If so, reload bioset because front_pad may have changed
> - * because a different table was loaded.
> - */
> - bioset_exit(&md->bs);
> - bioset_exit(&md->io_bs);
> -
> - } else if (bioset_initialized(&md->bs)) {
> - /*
> - * There's no need to reload with request-based dm
> - * because the size of front_pad doesn't change.
> - * Note for future: If you are to reload bioset,
> - * prep-ed requests in the queue may refer
> - * to bio from the old bioset, so you must walk
> - * through the queue to unprep.
> - */
> - goto out;
> - }
> -
> - BUG_ON(!p ||
> - bioset_initialized(&md->bs) ||
> - bioset_initialized(&md->io_bs));
> -
> - ret = bioset_init_from_src(&md->bs, &p->bs);
> - if (ret)
> - goto out;
> - ret = bioset_init_from_src(&md->io_bs, &p->io_bs);
> - if (ret)
> - bioset_exit(&md->bs);
> -out:
> - /* mempool bind completed, no longer need any mempools in the table */
> - dm_table_free_md_mempools(t);
> - return ret;
> -}
> -
> /*
> * Bind a table to the device.
> */
> @@ -2144,12 +2094,28 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
> * immutable singletons - used to optimize dm_mq_queue_rq.
> */
> md->immutable_target = dm_table_get_immutable_target(t);
> - }
>
> - ret = __bind_mempools(md, t);
> - if (ret) {
> - old_map = ERR_PTR(ret);
> - goto out;
> + /*
> + * There is no need to reload with request-based dm because the
> + * size of front_pad doesn't change.
> + *
> + * Note for future: If you are to reload bioset, prep-ed
> + * requests in the queue may refer to bio from the old bioset,
> + * so you must walk through the queue to unprep.
> + */
> + if (!md->mempools) {
> + md->mempools = t->mempools;
> + t->mempools = NULL;
> + }
> + } else {
> + /*
> + * The md may already have mempools that need changing.
> + * If so, reload bioset because front_pad may have changed
> + * because a different table was loaded.
> + */
> + dm_free_md_mempools(md->mempools);
> + md->mempools = t->mempools;
> + t->mempools = NULL;
> }
>
> ret = dm_table_set_restrictions(t, md->queue, limits);
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 3f89664fea010..86642234d4adb 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -72,7 +72,6 @@ struct dm_target *dm_table_get_wildcard_target(struct dm_table *t);
> bool dm_table_bio_based(struct dm_table *t);
> bool dm_table_request_based(struct dm_table *t);
> void dm_table_free_md_mempools(struct dm_table *t);
> -struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
>
> void dm_lock_md_type(struct mapped_device *md);
> void dm_unlock_md_type(struct mapped_device *md);
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-03 13:09 ` Mike Snitzer
0 siblings, 0 replies; 28+ messages in thread
From: Mike Snitzer @ 2022-06-03 13:09 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, linux-block, dm-devel, david, Matthew Wilcox
On Thu, Jun 02 2022 at 4:12P -0400,
Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Jun 01, 2022 at 10:13:34AM -0400, Mike Snitzer wrote:
> > Please take the time to look at the code and save your judgement until
> > you do. That said, I'm not in love with the complexity of how DM
> > handles bioset initialization. But both you and Jens keep taking
> > shots at DM for doing things wrong without actually looking.
>
> I'm not taking shots. I'm just saying we should kill this API. In
> the worse case the caller can keep track of if a bioset is initialized,
> but in most cases we should be able to deduct it in a nicer way.
>
> > DM uses bioset_init_from_src(). Yet you've both assumed double frees
> > and such (while not entirely wrong your glossing over the detail that
> > there is intervening reinitialization of DM's biosets between the
> > bioset_exit()s)
>
> And looking at the code, that use of bioset_init_from_src is completely
> broken. It does not actually preallocated anything as intended by
> dm (maybe that isn't actually needed) but just uses the biosets in
> dm_md_mempools as an awkward way to carry parameters.
I think the point was to keep the biosets embedded in struct
mapped_device to avoid any possible cacheline bouncing due to the
bioset memory being disjoint.
But preserving that micro-optimization isn't something I've ever
quantified (at least not with focus).
> And completely loses bringing over the integrity allocations.
Good catch.
> This is what I think should fix this, and will allow us to remove
> bioset_init_from_src which was a bad idea from the start:
Looks fine, did you want to send an official patch with a proper
header and Signed-off-by?
Thanks,
Mike
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index d21648a923ea9..54c0473a51dde 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -33,6 +33,14 @@ struct dm_kobject_holder {
> * access their members!
> */
>
> +/*
> + * For mempools pre-allocation at the table loading time.
> + */
> +struct dm_md_mempools {
> + struct bio_set bs;
> + struct bio_set io_bs;
> +};
> +
> struct mapped_device {
> struct mutex suspend_lock;
>
> @@ -110,8 +118,7 @@ struct mapped_device {
> /*
> * io objects are allocated from here.
> */
> - struct bio_set io_bs;
> - struct bio_set bs;
> + struct dm_md_mempools *mempools;
>
> /* kobject and completion */
> struct dm_kobject_holder kobj_holder;
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 6087cdcaad46d..a83b98a8d2a99 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -319,7 +319,7 @@ static int setup_clone(struct request *clone, struct request *rq,
> {
> int r;
>
> - r = blk_rq_prep_clone(clone, rq, &tio->md->bs, gfp_mask,
> + r = blk_rq_prep_clone(clone, rq, &tio->md->mempools->bs, gfp_mask,
> dm_rq_bio_constructor, tio);
> if (r)
> return r;
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 0e833a154b31d..a8b016d6bf16e 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1044,11 +1044,6 @@ void dm_table_free_md_mempools(struct dm_table *t)
> t->mempools = NULL;
> }
>
> -struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t)
> -{
> - return t->mempools;
> -}
> -
> static int setup_indexes(struct dm_table *t)
> {
> int i;
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index dfb0a551bd880..8b21155d3c4f5 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -136,14 +136,6 @@ static int get_swap_bios(void)
> return latch;
> }
>
> -/*
> - * For mempools pre-allocation at the table loading time.
> - */
> -struct dm_md_mempools {
> - struct bio_set bs;
> - struct bio_set io_bs;
> -};
> -
> struct table_device {
> struct list_head list;
> refcount_t count;
> @@ -581,7 +573,7 @@ static struct dm_io *alloc_io(struct mapped_device *md, struct bio *bio)
> struct dm_target_io *tio;
> struct bio *clone;
>
> - clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->io_bs);
> + clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->mempools->io_bs);
> /* Set default bdev, but target must bio_set_dev() before issuing IO */
> clone->bi_bdev = md->disk->part0;
>
> @@ -628,7 +620,8 @@ static struct bio *alloc_tio(struct clone_info *ci, struct dm_target *ti,
> } else {
> struct mapped_device *md = ci->io->md;
>
> - clone = bio_alloc_clone(NULL, ci->bio, gfp_mask, &md->bs);
> + clone = bio_alloc_clone(NULL, ci->bio, gfp_mask,
> + &md->mempools->bs);
> if (!clone)
> return NULL;
> /* Set default bdev, but target must bio_set_dev() before issuing IO */
> @@ -1876,8 +1869,7 @@ static void cleanup_mapped_device(struct mapped_device *md)
> {
> if (md->wq)
> destroy_workqueue(md->wq);
> - bioset_exit(&md->bs);
> - bioset_exit(&md->io_bs);
> + dm_free_md_mempools(md->mempools);
>
> if (md->dax_dev) {
> dax_remove_host(md->disk);
> @@ -2049,48 +2041,6 @@ static void free_dev(struct mapped_device *md)
> kvfree(md);
> }
>
> -static int __bind_mempools(struct mapped_device *md, struct dm_table *t)
> -{
> - struct dm_md_mempools *p = dm_table_get_md_mempools(t);
> - int ret = 0;
> -
> - if (dm_table_bio_based(t)) {
> - /*
> - * The md may already have mempools that need changing.
> - * If so, reload bioset because front_pad may have changed
> - * because a different table was loaded.
> - */
> - bioset_exit(&md->bs);
> - bioset_exit(&md->io_bs);
> -
> - } else if (bioset_initialized(&md->bs)) {
> - /*
> - * There's no need to reload with request-based dm
> - * because the size of front_pad doesn't change.
> - * Note for future: If you are to reload bioset,
> - * prep-ed requests in the queue may refer
> - * to bio from the old bioset, so you must walk
> - * through the queue to unprep.
> - */
> - goto out;
> - }
> -
> - BUG_ON(!p ||
> - bioset_initialized(&md->bs) ||
> - bioset_initialized(&md->io_bs));
> -
> - ret = bioset_init_from_src(&md->bs, &p->bs);
> - if (ret)
> - goto out;
> - ret = bioset_init_from_src(&md->io_bs, &p->io_bs);
> - if (ret)
> - bioset_exit(&md->bs);
> -out:
> - /* mempool bind completed, no longer need any mempools in the table */
> - dm_table_free_md_mempools(t);
> - return ret;
> -}
> -
> /*
> * Bind a table to the device.
> */
> @@ -2144,12 +2094,28 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
> * immutable singletons - used to optimize dm_mq_queue_rq.
> */
> md->immutable_target = dm_table_get_immutable_target(t);
> - }
>
> - ret = __bind_mempools(md, t);
> - if (ret) {
> - old_map = ERR_PTR(ret);
> - goto out;
> + /*
> + * There is no need to reload with request-based dm because the
> + * size of front_pad doesn't change.
> + *
> + * Note for future: If you are to reload bioset, prep-ed
> + * requests in the queue may refer to bio from the old bioset,
> + * so you must walk through the queue to unprep.
> + */
> + if (!md->mempools) {
> + md->mempools = t->mempools;
> + t->mempools = NULL;
> + }
> + } else {
> + /*
> + * The md may already have mempools that need changing.
> + * If so, reload bioset because front_pad may have changed
> + * because a different table was loaded.
> + */
> + dm_free_md_mempools(md->mempools);
> + md->mempools = t->mempools;
> + t->mempools = NULL;
> }
>
> ret = dm_table_set_restrictions(t, md->queue, limits);
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 3f89664fea010..86642234d4adb 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -72,7 +72,6 @@ struct dm_target *dm_table_get_wildcard_target(struct dm_table *t);
> bool dm_table_bio_based(struct dm_table *t);
> bool dm_table_request_based(struct dm_table *t);
> void dm_table_free_md_mempools(struct dm_table *t);
> -struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
>
> void dm_lock_md_type(struct mapped_device *md);
> void dm_unlock_md_type(struct mapped_device *md);
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: bioset_exit poison from dm_destroy
2022-06-03 13:09 ` [dm-devel] " Mike Snitzer
@ 2022-06-03 13:19 ` Christoph Hellwig
-1 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-03 13:19 UTC (permalink / raw)
To: Mike Snitzer
Cc: Christoph Hellwig, Jens Axboe, Matthew Wilcox, dm-devel,
linux-block, david
On Fri, Jun 03, 2022 at 09:09:06AM -0400, Mike Snitzer wrote:
> I think the point was to keep the biosets embedded in struct
> mapped_device to avoid any possible cacheline bouncing due to the
> bioset memory being disjoint.
Probably.
> But preserving that micro-optimization isn't something I've ever
> quantified (at least not with focus).
But we can either avoid the pointer or do the preallocation. Doing both
doesn't really work. OTOH given that we've surived quite a while without
the preallocation actually working I wonder if it actually is needed.
> Looks fine, did you want to send an official patch with a proper
> header and Signed-off-by?
I plan to send a series with a few follow on after testing it a bit
more.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [dm-devel] bioset_exit poison from dm_destroy
@ 2022-06-03 13:19 ` Christoph Hellwig
0 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2022-06-03 13:19 UTC (permalink / raw)
To: Mike Snitzer
Cc: Jens Axboe, linux-block, david, Matthew Wilcox,
Christoph Hellwig, dm-devel
On Fri, Jun 03, 2022 at 09:09:06AM -0400, Mike Snitzer wrote:
> I think the point was to keep the biosets embedded in struct
> mapped_device to avoid any possible cacheline bouncing due to the
> bioset memory being disjoint.
Probably.
> But preserving that micro-optimization isn't something I've ever
> quantified (at least not with focus).
But we can either avoid the pointer or do the preallocation. Doing both
doesn't really work. OTOH given that we've surived quite a while without
the preallocation actually working I wonder if it actually is needed.
> Looks fine, did you want to send an official patch with a proper
> header and Signed-off-by?
I plan to send a series with a few follow on after testing it a bit
more.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-06-03 13:47 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-29 0:17 bioset_exit poison from dm_destroy Matthew Wilcox
2022-05-29 0:17 ` [dm-devel] " Matthew Wilcox
2022-05-29 12:46 ` Jens Axboe
2022-05-29 12:46 ` [dm-devel] " Jens Axboe
2022-05-31 18:58 ` Mike Snitzer
2022-05-31 18:58 ` [dm-devel] " Mike Snitzer
2022-05-31 19:00 ` Jens Axboe
2022-05-31 19:00 ` [dm-devel] " Jens Axboe
2022-05-31 19:49 ` Mike Snitzer
2022-05-31 19:49 ` [dm-devel] " Mike Snitzer
2022-06-01 3:27 ` Jens Axboe
2022-06-01 3:27 ` [dm-devel] " Jens Axboe
2022-06-01 6:04 ` Christoph Hellwig
2022-06-01 6:04 ` [dm-devel] " Christoph Hellwig
2022-06-01 6:08 ` Jens Axboe
2022-06-01 6:08 ` [dm-devel] " Jens Axboe
2022-06-01 6:18 ` Christoph Hellwig
2022-06-01 6:18 ` [dm-devel] " Christoph Hellwig
2022-06-01 14:13 ` Mike Snitzer
2022-06-01 14:13 ` Mike Snitzer
2022-06-02 8:12 ` Christoph Hellwig
2022-06-02 8:12 ` [dm-devel] " Christoph Hellwig
2022-06-02 8:18 ` Jens Axboe
2022-06-02 8:18 ` [dm-devel] " Jens Axboe
2022-06-03 13:09 ` Mike Snitzer
2022-06-03 13:09 ` [dm-devel] " Mike Snitzer
2022-06-03 13:19 ` Christoph Hellwig
2022-06-03 13:19 ` [dm-devel] " Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.