Re: [PATCH v2 0/3] btrfs: trace: Add trace events for extent_io_tree

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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



On 2019/3/8 上午12:32, David Sterba wrote:
> On Fri, Mar 01, 2019 at 10:47:57AM +0800, Qu Wenruo wrote:
>> - Allow NULL fs_info for TP_fast_assign_fsid()
>>   There is extent bits operation in selftest which is too deep to pass
>>   fs_info. And since it's in selftest, it shouldn't trigger trace
>>   events.
>>   But to be safe, we still need to check fs_indo in
>>   TP_fast_assign_fsid(), for NULL fs_info, just keep fsid filled with
>>   zero.
> 
> Ok, better be safe here. I'd still like to remove the conditional, as
> the tests typically access only a single filesystem we could export the
> fs_info globally, avoiding the need to pass it around.

In fact, it's purely to be safe here.

Since the selftest is only executed at module load time, it's pretty
hard to enable trace event at that small time windows.
And if btrfs is complied into kernel, it's definitely impossible.

> 
>> There is one point which need extra attention:
>> 1) Those trace events are pretty heavy
>>    The following workload would generate over 400 trace events.
> 
> I'm not sure if 400 is considered a lot, I'd say 400.000 would be a lot
> for the steps below.

Considering other btrfs events are hardly to hit 40 for that small
workload, among btrfs specific events, it looks a little heavy.

> 
>> 	mkfs.btrfs -f $dev
>> 	start_trace
>> 	mount $dev $mnt -o enospc_debug
>> 	sync
>> 	touch $mnt/file1
>> 	touch $mnt/file2
>> 	touch $mnt/file3
>> 	xfs_io -f -c "pwrite 0 16k" $mnt/file4
>> 	umount $mnt
>> 	end_trace
>>    It's not recommended to use them in real world environment.
>>
>> Changelog:
>> v2:
>> - Introduce fs_info to distinguish different btrfs filesystems
>> - Code style change to make trace code more elegant
>> - Minor IO_TREE_* naming change.
>> - Use btrfs_ino() to replace raw inode number.
>> - Change extent_io_tree::owner declaration to avoid affecting spinlock.
> 
> v2 looks good to me, thanks. I'll add it to the 5.2 queue.
> 
Thanks for merging and update it for the renaming.

Thanks,
Qu


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