| Message ID | 20180426062425.8574-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26.04.2018 09:24, Qu Wenruo wrote: > fs_info can be extracted from btrfs_block_group_cache, and all > btrfs_block_group_cache is created by btrfs_create_block_group_cache() > with fs_info initialized, no need to worry about NULL pointer > dereference. > > Signed-off-by: Qu Wenruo <wqu@suse.com> I very much hate function which take parameters solely for the purpose of trace points! Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/extent-tree.c | 8 ++++---- > include/trace/events/btrfs.h | 18 ++++++++---------- > 2 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 09d6e4544d6e..bf30be868f7a 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -7664,7 +7664,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info, > if (offset) { > /* we have a block, we're done */ > spin_unlock(&last_ptr->refill_lock); > - trace_btrfs_reserve_extent_cluster(fs_info, > + trace_btrfs_reserve_extent_cluster( > used_block_group, > search_start, num_bytes); > if (used_block_group != block_group) { > @@ -7737,7 +7737,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info, > if (offset) { > /* we found one, proceed */ > spin_unlock(&last_ptr->refill_lock); > - trace_btrfs_reserve_extent_cluster(fs_info, > + trace_btrfs_reserve_extent_cluster( > block_group, search_start, > num_bytes); > goto checks; > @@ -7837,8 +7837,8 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info, > ins->objectid = search_start; > ins->offset = num_bytes; > > - trace_btrfs_reserve_extent(fs_info, block_group, > - search_start, num_bytes); > + trace_btrfs_reserve_extent(block_group, search_start, > + num_bytes); > btrfs_release_block_group(block_group, delalloc); > break; > loop: > diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h > index d78d8ab4bc86..5af2479e9b16 100644 > --- a/include/trace/events/btrfs.h > +++ b/include/trace/events/btrfs.h > @@ -1146,11 +1146,10 @@ TRACE_EVENT(find_free_extent, > > DECLARE_EVENT_CLASS(btrfs__reserve_extent, > > - TP_PROTO(const struct btrfs_fs_info *fs_info, > - const struct btrfs_block_group_cache *block_group, u64 start, > + TP_PROTO(const struct btrfs_block_group_cache *block_group, u64 start, > u64 len), > > - TP_ARGS(fs_info, block_group, start, len), > + TP_ARGS(block_group, start, len), > > TP_STRUCT__entry_btrfs( > __field( u64, bg_objectid ) > @@ -1159,7 +1158,7 @@ DECLARE_EVENT_CLASS(btrfs__reserve_extent, > __field( u64, len ) > ), > > - TP_fast_assign_btrfs(fs_info, > + TP_fast_assign_btrfs(block_group->fs_info, > __entry->bg_objectid = block_group->key.objectid; > __entry->flags = block_group->flags; > __entry->start = start; > @@ -1177,20 +1176,18 @@ DECLARE_EVENT_CLASS(btrfs__reserve_extent, > > DEFINE_EVENT(btrfs__reserve_extent, btrfs_reserve_extent, > > - TP_PROTO(const struct btrfs_fs_info *fs_info, > - const struct btrfs_block_group_cache *block_group, u64 start, > + TP_PROTO(const struct btrfs_block_group_cache *block_group, u64 start, > u64 len), > > - TP_ARGS(fs_info, block_group, start, len) > + TP_ARGS(block_group, start, len) > ); > > DEFINE_EVENT(btrfs__reserve_extent, btrfs_reserve_extent_cluster, > > - TP_PROTO(const struct btrfs_fs_info *fs_info, > - const struct btrfs_block_group_cache *block_group, u64 start, > + TP_PROTO(const struct btrfs_block_group_cache *block_group, u64 start, > u64 len), > > - TP_ARGS(fs_info, block_group, start, len) > + TP_ARGS(block_group, start, len) > ); > > TRACE_EVENT(btrfs_find_cluster, > @@ -1807,6 +1804,7 @@ TRACE_EVENT(btrfs_inode_mod_outstanding_extents, > show_root_type(__entry->root_objectid), > __entry->ino, __entry->mod) > ); > + > #endif /* _TRACE_BTRFS_H */ > > /* This part must be outside protection */ > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 26, 2018 at 09:27:11AM +0300, Nikolay Borisov wrote: > > > On 26.04.2018 09:24, Qu Wenruo wrote: > > fs_info can be extracted from btrfs_block_group_cache, and all > > btrfs_block_group_cache is created by btrfs_create_block_group_cache() > > with fs_info initialized, no need to worry about NULL pointer > > dereference. > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > I very much hate function which take parameters solely for the purpose > of trace points! > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> Added to misc-next, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 26, 2018 at 02:24:25PM +0800, Qu Wenruo wrote: > fs_info can be extracted from btrfs_block_group_cache, and all > btrfs_block_group_cache is created by btrfs_create_block_group_cache() > with fs_info initialized, no need to worry about NULL pointer > dereference. Famous last words. [ 46.484499] Btrfs loaded, crc32c=crc32c-generic, debug=on, assert=on, ref-verify=on [ 46.484968] BTRFS: selftest: sectorsize: 4096 nodesize: 4096 [ 46.484972] BTRFS: selftest: Running btrfs free space cache tests [ 46.485096] BTRFS: selftest: Running extent only tests [ 46.485209] BTRFS: selftest: Running bitmap only tests [ 46.485311] BTRFS: selftest: Running bitmap and extent tests [ 46.485549] BTRFS: selftest: Running space stealing from bitmap to extent [ 46.486194] BTRFS: selftest: Free space cache tests finished [ 46.486197] BTRFS: selftest: Running extent buffer operation tests [ 46.486200] BTRFS: selftest: Running btrfs_split_item tests [ 46.486488] BTRFS: selftest: Running extent I/O tests [ 46.486491] BTRFS: selftest: Running find delalloc tests [ 46.865265] BTRFS: selftest: Running extent buffer bitmap tests [ 46.884948] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 [ 46.893206] PGD 210b6c067 P4D 210b6c067 PUD 214917067 PMD 0 [ 46.899102] Oops: 0000 [#1] PREEMPT SMP [ 46.903058] Modules linked in: btrfs(+) stp llc iscsi_ibft iscsi_boot_sysfs xor zstd_decompress zstd_compress xxhash i2c_algo_bit drm_kms_helper raid6_pq syscopyarea sysfillrect sysimgblt libcrc32c fb_sys_fop s ttm dm_mod drm tpm_infineon kvm_amd dax tpm_tis kvm tg3 tpm_tis_core tpm i2c_piix4 libphy k10temp mptctl acpi_cpufreq shpchp button pcspkr irqbypass ext4 mbcache jbd2 sr_mod cdrom ohci_pci ata_generic mptsas o hci_hcd ehci_pci scsi_transport_sas ehci_hcd mptscsih serio_raw mptbase pata_serverworks usbcore sata_svw sg scsi_dh_rdac scsi_dh_emc scsi_dh_alua [ 46.953728] CPU: 1 PID: 676 Comm: modprobe Not tainted 4.17.0-rc2-1.ge195904-vanilla+ #225 [ 46.962245] Hardware name: empty empty/S3993, BIOS PAQEX0-3 02/24/2008 [ 46.969082] RIP: 0010:memzero_extent_buffer+0xd3/0x1b0 [btrfs] [ 46.975082] RSP: 0018:ffffa953816dbba8 EFLAGS: 00010297 [ 46.980435] RAX: 0000000000000000 RBX: ffff98f02b6fc000 RCX: 0000000000000000 [ 46.987692] RDX: 0000000000001000 RSI: 0000000000000000 RDI: ffff98f02b6fd000 [ 46.994951] RBP: 0000000000001000 R08: 0000000000000001 R09: ffff98f057a30000 [ 47.002208] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000800 [ 47.009468] R13: 0000000000000004 R14: ffff98f03a509f88 R15: 0000000000000000 [ 47.016753] FS: 00007fcb43cbf700(0000) GS:ffff98f066800000(0000) knlGS:0000000000000000 [ 47.025062] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 47.030951] CR2: 0000000000000020 CR3: 00000002163ec000 CR4: 00000000000006e0 [ 47.038251] Call Trace: [ 47.040889] __test_eb_bitmaps+0x39/0x2b0 [btrfs] [ 47.045789] btrfs_test_extent_io+0xd9/0x140 [btrfs] [ 47.050932] btrfs_run_sanity_tests+0x91/0x140 [btrfs] [ 47.056250] ? trace_event_define_fields_btrfs__block_group+0xdb/0xdb [btrfs] [ 47.063561] init_btrfs_fs+0xca/0x129 [btrfs] [ 47.068066] do_one_initcall+0x5f/0x412 [ 47.072047] ? rcu_read_lock_sched_held+0x6b/0x80 [ 47.076894] ? kmem_cache_alloc_trace+0x27f/0x310 [ 47.081750] do_init_module+0x64/0x21a [ 47.085639] load_module+0x1d84/0x23a0 [ 47.089530] ? vfs_read+0x127/0x150 [ 47.093172] ? __do_sys_finit_module+0xb7/0xd0 [ 47.097743] __do_sys_finit_module+0xb7/0xd0 [ 47.102176] do_syscall_64+0x62/0x1c0 [ 47.105977] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 47.111165] RIP: 0033:0x7fcb437f8849 [ 47.114866] RSP: 002b:00007ffdb3760278 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 [ 47.122638] RAX: ffffffffffffffda RBX: 0000000001080390 RCX: 00007fcb437f8849 [ 47.129920] RDX: 0000000000000000 RSI: 000000000041983b RDI: 0000000000000005 [ 47.137180] RBP: 000000000041983b R08: 0000000000000000 R09: 0000000001080590 [ 47.144452] R10: 0000000000000005 R11: 0000000000000246 R12: 00000000010801c0 [ 47.151734] R13: 0000000000000000 R14: 0000000000040000 R15: 0000000000000000 [ 47.159017] Code: 40 f6 c5 02 c6 01 00 0f 85 95 00 00 00 49 83 c5 01 45 31 ff 49 29 ec 74 65 49 83 fd 0f 0f 87 bb 00 00 00 4b 8b b4 ee a0 01 00 00 <48> 8b 4e 20 48 8d 51 ff 83 e1 01 48 0f 44 d6 48 8b 12 83 e2 08 [ 47.178282] RIP: memzero_extent_buffer+0xd3/0x1b0 [btrfs] RSP: ffffa953816dbba8 [ 47.185792] CR2: 0000000000000020 [ 47.189613] ---[ end trace 3db5ec59da242443 ]--- -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 28, 2018 at 06:42:06PM +0200, David Sterba wrote: > On Thu, Apr 26, 2018 at 02:24:25PM +0800, Qu Wenruo wrote: > > fs_info can be extracted from btrfs_block_group_cache, and all > > btrfs_block_group_cache is created by btrfs_create_block_group_cache() > > with fs_info initialized, no need to worry about NULL pointer > > dereference. > > Famous last words. And it's a false alert, it's caused by another patch yet to be identified. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018年04月29日 01:45, David Sterba wrote: > On Sat, Apr 28, 2018 at 06:42:06PM +0200, David Sterba wrote: >> On Thu, Apr 26, 2018 at 02:24:25PM +0800, Qu Wenruo wrote: >>> fs_info can be extracted from btrfs_block_group_cache, and all >>> btrfs_block_group_cache is created by btrfs_create_block_group_cache() >>> with fs_info initialized, no need to worry about NULL pointer >>> dereference. >> >> Famous last words. > > And it's a false alert, it's caused by another patch yet to be > identified. Yep, I also spent serval minutes to find this out. As in that test there is nothing related to btrfs_reserve_extent() trace event. Thanks, Qu > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Sun, Apr 29, 2018 at 08:33:49AM +0800, Qu Wenruo wrote: > > > On 2018年04月29日 01:45, David Sterba wrote: > > On Sat, Apr 28, 2018 at 06:42:06PM +0200, David Sterba wrote: > >> On Thu, Apr 26, 2018 at 02:24:25PM +0800, Qu Wenruo wrote: > >>> fs_info can be extracted from btrfs_block_group_cache, and all > >>> btrfs_block_group_cache is created by btrfs_create_block_group_cache() > >>> with fs_info initialized, no need to worry about NULL pointer > >>> dereference. > >> > >> Famous last words. > > > > And it's a false alert, it's caused by another patch yet to be > > identified. > > Yep, I also spent serval minutes to find this out. > As in that test there is nothing related to btrfs_reserve_extent() trace > event. Ok, found it "btrfs: simplify counting number of eb pages", will be removed from misc-next. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 09d6e4544d6e..bf30be868f7a 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -7664,7 +7664,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info, if (offset) { /* we have a block, we're done */ spin_unlock(&last_ptr->refill_lock); - trace_btrfs_reserve_extent_cluster(fs_info, + trace_btrfs_reserve_extent_cluster( used_block_group, search_start, num_bytes); if (used_block_group != block_group) { @@ -7737,7 +7737,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info, if (offset) { /* we found one, proceed */ spin_unlock(&last_ptr->refill_lock); - trace_btrfs_reserve_extent_cluster(fs_info, + trace_btrfs_reserve_extent_cluster( block_group, search_start, num_bytes); goto checks; @@ -7837,8 +7837,8 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info, ins->objectid = search_start; ins->offset = num_bytes; - trace_btrfs_reserve_extent(fs_info, block_group, - search_start, num_bytes); + trace_btrfs_reserve_extent(block_group, search_start, + num_bytes); btrfs_release_block_group(block_group, delalloc); break; loop: diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h index d78d8ab4bc86..5af2479e9b16 100644 --- a/include/trace/events/btrfs.h +++ b/include/trace/events/btrfs.h @@ -1146,11 +1146,10 @@ TRACE_EVENT(find_free_extent, DECLARE_EVENT_CLASS(btrfs__reserve_extent, - TP_PROTO(const struct btrfs_fs_info *fs_info, - const struct btrfs_block_group_cache *block_group, u64 start, + TP_PROTO(const struct btrfs_block_group_cache *block_group, u64 start, u64 len), - TP_ARGS(fs_info, block_group, start, len), + TP_ARGS(block_group, start, len), TP_STRUCT__entry_btrfs( __field( u64, bg_objectid ) @@ -1159,7 +1158,7 @@ DECLARE_EVENT_CLASS(btrfs__reserve_extent, __field( u64, len ) ), - TP_fast_assign_btrfs(fs_info, + TP_fast_assign_btrfs(block_group->fs_info, __entry->bg_objectid = block_group->key.objectid; __entry->flags = block_group->flags; __entry->start = start; @@ -1177,20 +1176,18 @@ DECLARE_EVENT_CLASS(btrfs__reserve_extent, DEFINE_EVENT(btrfs__reserve_extent, btrfs_reserve_extent, - TP_PROTO(const struct btrfs_fs_info *fs_info, - const struct btrfs_block_group_cache *block_group, u64 start, + TP_PROTO(const struct btrfs_block_group_cache *block_group, u64 start, u64 len), - TP_ARGS(fs_info, block_group, start, len) + TP_ARGS(block_group, start, len) ); DEFINE_EVENT(btrfs__reserve_extent, btrfs_reserve_extent_cluster, - TP_PROTO(const struct btrfs_fs_info *fs_info, - const struct btrfs_block_group_cache *block_group, u64 start, + TP_PROTO(const struct btrfs_block_group_cache *block_group, u64 start, u64 len), - TP_ARGS(fs_info, block_group, start, len) + TP_ARGS(block_group, start, len) ); TRACE_EVENT(btrfs_find_cluster, @@ -1807,6 +1804,7 @@ TRACE_EVENT(btrfs_inode_mod_outstanding_extents, show_root_type(__entry->root_objectid), __entry->ino, __entry->mod) ); + #endif /* _TRACE_BTRFS_H */ /* This part must be outside protection */
fs_info can be extracted from btrfs_block_group_cache, and all btrfs_block_group_cache is created by btrfs_create_block_group_cache() with fs_info initialized, no need to worry about NULL pointer dereference. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent-tree.c | 8 ++++---- include/trace/events/btrfs.h | 18 ++++++++---------- 2 files changed, 12 insertions(+), 14 deletions(-)