Re: [PATCH] btrfs: trace: Remove unnecessary fs_info parameter for btrfs__reserve_extent event class

[David Sterba]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: trace: Remove unnecessary fs_info parameter for btrfs__reserve_extent event class

[Qu Wenruo]

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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] btrfs: trace: Remove unnecessary fs_info parameter for btrfs__reserve_extent event class

[David Sterba]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: trace: Remove unnecessary fs_info parameter for btrfs__reserve_extent event class

[David Sterba]

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 
0020
[   46.893206] PGD 210b6c067 P4D 210b6c067 PUD 214917067 PMD 0
[   46.899102] Oops:  [#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:a953816dbba8 EFLAGS: 00010297
[   46.980435] RAX:  RBX: 98f02b6fc000 RCX: 
[   46.987692] RDX: 1000 RSI:  RDI: 98f02b6fd000
[   46.994951] RBP: 1000 R08: 0001 R09: 98f057a3
[   47.002208] R10:  R11:  R12: 0800
[   47.009468] R13: 0004 R14: 98f03a509f88 R15: 
[   47.016753] FS:  7fcb43cbf700() GS:98f06680() 
knlGS:
[   47.025062] CS:  0010 DS:  ES:  CR0: 80050033
[   47.030951] CR2: 0020 CR3: 0002163ec000 CR4: 06e0
[   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.65] RIP: 0033:0x7fcb437f8849
[   47.114866] RSP: 002b:7ffdb3760278 EFLAGS: 0246 ORIG_RAX: 
0139
[   47.122638] RAX: ffda RBX: 01080390 RCX: 7fcb437f8849
[   47.129920] RDX:  RSI: 0041983b RDI: 0005
[   47.137180] RBP: 0041983b R08:  R09: 01080590
[   47.144452] R10: 0005 R11: 0246 R12: 010801c0
[   47.151734] R13:  R14: 0004 R15: 
[   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: 
a953816dbba8
 [   47.185792] CR2: 0020
 [   47.189613] ---[ end trace 3db5ec59da242443 ]---
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: trace: Remove unnecessary fs_info parameter for btrfs__reserve_extent event class

[David Sterba]

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 
> 
> I very  much hate function which take parameters solely for the purpose
> of trace points!
> 
> Reviewed-by: Nikolay Borisov 

Added to misc-next, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: trace: Remove unnecessary fs_info parameter for btrfs__reserve_extent event class

[Nikolay Borisov]

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 

I very  much hate function which take parameters solely for the purpose
of trace points!

Reviewed-by: Nikolay Borisov 
> ---
>  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(_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(_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