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

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

[Qu Wenruo]

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

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 */
-- 
2.17.0

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 <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 */
> 

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

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

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.

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 888 bytes --]



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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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.