tools/filetop: Add --recursive for directory filter#5400
tools/filetop: Add --recursive for directory filter#5400wswsmao wants to merge 1 commit intoiovisor:masterfrom
Conversation
wswsmao
requested review from
brendangregg,
chenhengqi,
ekyooo and
yonghong-song
as code owners
tools/filetop.py
Outdated
| if args.recursive: | ||
| print(f'Tracing directory recursively: {args.directory} (Inode: {directory_inode})') | ||
| directory_filter = ("({ struct dentry *pde = de; int found = 0; " | ||
| "for (int i = 0; i < 50; i++) { " |
There was a problem hiding this comment.
-
Please replace the magic number 50 with a macro or constant for clarity and maintainability.
-
Is there a specific reason why 50 is chosen as the max depth? Could you also include your thoughts on performance, BPF instruction count, and verifier safety?
There was a problem hiding this comment.
1、done
2、I switched the bound to 32 and made it a macro FILETOP_MAX_DENTRY_DEPTH. 32 aligns with existing path-walking limits in this repo (e.g., libbpf-tools uses MAX_PATH_DEPTH=32) and is well above typical directory depths we see in practice.
tools/filetop.py
Outdated
| bpf_text = bpf_text.replace('DIRECTORY_FILTER', 'file->f_path.dentry->d_parent->d_inode->i_ino != %d' % directory_inode) | ||
| if args.recursive: | ||
| print(f'Tracing directory recursively: {args.directory} (Inode: {directory_inode})') | ||
| directory_filter = ("({ struct dentry *pde = de; int found = 0; " |
There was a problem hiding this comment.
The directory_filter code for the recursive case is hard to read.
Could you please refactor it for better readability and maintainability?
tools/filetop.py
Outdated
|
|
||
| // Limit dentry parent walk depth to satisfy eBPF verifier loop | ||
| #ifndef FILETOP_MAX_DENTRY_DEPTH | ||
| #define FILETOP_MAX_DENTRY_DEPTH 32 |
There was a problem hiding this comment.
While FILETOP_MAX_DENTRY_DEPTH is below 32 in most cases, there is still a chance that it could exceed this limit in certain scenarios. If that happens, the tool's behavior might deviate from the user's expectations. Would it be worth notifying users about this limitation to avoid potential confusion?
There was a problem hiding this comment.
done, add max depth log in --help and Tracing directory recursively log
Signed-off-by: abushwang <[email protected]>
| { | ||
| struct dentry *pde = de; | ||
| int found = 0; | ||
| #pragma unroll |
There was a problem hiding this comment.
Please add a blank line after the variable declaration section.
| exit(1) | ||
| else: | ||
| if args.recursive: | ||
| print("Error: --recursive can only be used with -d/--directory option") |
| parser.add_argument("-d", "--directory", type=str, | ||
| help="trace this directory only") | ||
| parser.add_argument("-R", "--recursive", action="store_true", | ||
| help=f"when used with -d, also trace files in subdirectories (max depth {MAX_DENTRY_DEPTH_VAL})") |
There was a problem hiding this comment.
It would be better to make it clearer that this is only available when using -d. The current wording makes it seem like there could be cases where it's possible to use without -d.
For example, "This feature is only supported when used with the -d option, tracing files inside subdirectories."
| { | ||
| struct dentry *pde = de; | ||
| int found = 0; | ||
| #pragma unroll |
There was a problem hiding this comment.
Is #pragma unroll (and __always_inline) intended?
please refer to:
https://lwn.net/Articles/1017116/
2 participants
The commit 137bd5f introduced a directory filter. However, files under subdirectories were not being traced.
This PR adds a --recursive option which, while remaining backward-compatible, enables recursive filtering so that files in subdirectories are also matched.