Skip to content

add a new tool in libbpf-tools named ext4File - #5440

Open
niebowen666 wants to merge 1 commit into
iovisor:masterfrom
niebowen666:ext4File
Open

add a new tool in libbpf-tools named ext4File#5440
niebowen666 wants to merge 1 commit into
iovisor:masterfrom
niebowen666:ext4File

Conversation

@niebowen666

@niebowen666 niebowen666 commented Dec 18, 2025

Copy link
Copy Markdown

ext4file introduction

Overview

ext4file is used to monitor the I/O patterns (buffer or direct) of each file in the target ext4 filesystem, as well as the hint used by each file.

Why ext4file

ext4file is a tool used to track file-level buffer or direct I/O. Currently, in the repository, there exist block-layer tools to monitor I/O patterns of whole disk (such as biopattern, biolatency, etc.) and VFS-layer tools to trace file lifecycle and I/O behavior of files throughout the entire VFS (such as filelife and vfsstat, etc.).

Below is a comparative summary:

Tool Name Layer Main Function Tracks Filename? Differences
biopattern Block Measures the proportion of random vs. sequential I/O on a storage device ❌ No Can not achieve file-layer tracing(The other bio tools all have this problem)
fsdist VFS Tracks latency distribution of operations like read, write, open, and sync ❌ No Focus on latency, not I/O pattern
fsslower VFS Traces slow file operations (e.g., long-latency reads/writes), focuses on I/O size ✅ Yes Trace the I/O size and latency, not I/O pattern
filelife VFS Monitors file lifecycle events (creation and deletion) ✅ Yes Only focus on file creation and deletion
filetop VFS Shows real-time I/O activity of active files (displays only top entries to avoid verbosity) ✅ Yes There exists no distinction between buffer and direct I/O
ext4file ext4 Filesystem Tracks buffer vs. direct I/O patterns per file, enables fine-grained file-level monitoring using inode ✅ Yes

How to use

Run ext4file before executing your test. You can refer to ./ext4file -h to get the usage of the tool

Show I/O pattern for every file in ext4 filesystem.

Usage: ./ext4file [-h] [-d DIR] [-o FILE] [interval] [count]

Options:
  -h, --help                   Print this help message
  -d DIR, --dir=DIR            Trace the ext4 filesystem mounted on the specified directory
  -o FILE, --output=FILE       Write output to a file (optional; default: stdout)
  interval                     Time interval (in seconds) between reports (default: unlimited)
  count                        Number of reports to generate (default: unlimited)

Examples:
  ./ext4file -d /mnt/ext4                      # Trace I/O patterns of files on the ext4 filesystem mounted at /mnt/ext4
  ./ext4file -d /mnt/ext4 1 10                 # Generate 10 reports, one per second
  ./ext4file -d /mnt/ext4 -o output 1 10       # Generate 10 reports at 1-second intervals, saving output to ./output

The output could be:

root@server:/home/nbw/OpenSource/biohint/libbpf-tools# ./ext4file -d /mnt/ext4File/
EXT4 Filesystem Info: blocks_count=3750232064 blocks_per_group=32768 bg_cnt=114448
Tracing Ext4 read/write... Hit Ctrl-C to end.
2026-01-14 13:58:21
file_name            inode      pa_inode   hint   buffer_read     direct_read     buffer_write    direct_write    delete
test3                83361794   83361793   0      0               0               0               0               False
test2                34         2          2      8               0               1               0               False
dir1                 83361793   2          0      0               0               0               0               False
dir2                 440467457  2          0      0               0               0               0               True
test3                33         2          3      8               0               1               0               False
test1                33         2          5      8               0               1               0               True
test3                440467458  440467457  0      0               0               0               0               True

Below is the detailed explanation of each field in the ext4file output. This tool traces per-file I/O patterns (buffered vs. direct) on ext4 filesystems, providing fine-grained visibility into application behavior.

Field Description
file_name The name of the file (without full path). Note: multiple files may share the same name.
inode The inode number of the file. Inode is the unique identifier for a file within the filesystem, even across renames or hard links. This enables accurate tracking of I/O for specific files.
pa_inode The inode number of the file’s parent directory.
hint The FDP (Flexible Data Placement) hint value associated with the file. FDP is a new NVMe feature that enables the host to guide data placement on the SSD.
buffer_read Number of buffered read operations performed on the file. Buffered I/O goes through the kernel page cache.
direct_read Number of direct read operations performed on the file. Direct I/O bypasses the page cache.
direct_write Number of direct write operations performed on the file. Like direct read, it skips the page cache and writes data directly from user space to storage.
buffer_write Number of buffered write operations performed on the file. Data is first written to the page cache and later flushed to disk asynchronously by the kernel.
delete Indicates whether the file has been unlinked (deleted). If True, the file was removed from the directory but may still be accessible if held open by a process. I/O on such files can indicate resource leaks or long-running file handles.

Target Audience

This tool is intended for ext4 filesystem developers and performance engineers who need to analyze I/O behavior at the file level.

@Bojun-Seo

Bojun-Seo commented Jan 6, 2026

Copy link
Copy Markdown
Contributor

Here are my quick notes:

  • Docs: Need more explanation
  • Naming: ext4File -> ext4file
  • Patch splitting: Please split patches functionally or logically

Thanks

@niebowen666

Copy link
Copy Markdown
Author

Here are my quick notes:

  • Docs: Need more explanation
  • Naming: ext4File -> ext4file
  • Patch splitting: Please split patches functionally or logically

Thanks

Thanks for your reply.
But I wonder what kind of docs should I offer and which directory should I submit these docs to.
Besides, another two PR has been submitted: #5439 and #5429.
Could you take a look if you have time. Thanks a lot!

@Bojun-Seo

Copy link
Copy Markdown
Contributor

When I said docs, I actually meant the commit message.
I want you to provide the purpose, necessity, value, and usage instructions in the commit message.

@niebowen666
niebowen666 force-pushed the ext4File branch 2 times, most recently from 7cc4f5e to a5912d8 Compare January 15, 2026 11:43
@niebowen666

Copy link
Copy Markdown
Author

When I said docs, I actually meant the commit message. I want you to provide the purpose, necessity, value, and usage instructions in the commit message.

Hi Bojun,
I have fix my code and update the commit message.

  • Detailed explanation has been commit
  • The name have been changed to ext4file
  • I have removed the tracking of the time for file creation, deletion, and access. Currently, ext4file only focuses on file-level I/O patterns.

Comment thread libbpf-tools/Makefile Outdated
tcptop \
vfsstat \
wakeuptime \
ext4file \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this new tool ? We already have fsdist/fsslower/filelife/filetop ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ext4file is a tool used to track file-level buffer or direct I/O.
If a certain file is expected to be accessed by direct I/O, ext4file can detect abnormal I/O access.
I have read the source code of the tools you listed above.

  • fsdist focuses on the execution time of operations like read, write, open, and sync, which is different from the issue we are concerned with(Buffer I/O and Direct I/O).
  • Compared to fsdist, fsslower is more powerful. The information it traces includes file names and pays attention to the size of I/O. It also sets a threshold, and if the execution time of an operation is below this threshold, it will skip tracing. Although it tracks file names, it does not achieve file-level tracking, because a file name does not represent a unique file. In addition, it tracks the size of I/O rather than the distribution between buffer and direct, so the results of ext4file can complement those of fsslower.
  • filelife ignores I/O and only focuses on the creation and deletion of files.
  • To prevent excessive output, filetop only displays part of the data, and filetop's I/O tracking cannot further determine whether it is buffer or direct.

ext4file can complement the tools mentioned above and can determine whether a file exhibits unexpected I/O under complex workloads.

@Bojun-Seo

Copy link
Copy Markdown
Contributor

I'm someone who believes that each commit/patch should be self-contained and complete (self-contained atomic unit). I think developers should be able to understand the full context and intent just by reading the commit message alone, without having to dig through the PR description or conversation thread.

Therefore, it would be great if you could include the PR description into the commit message(s). Also, if you revise the patches so that each individual commit/patch maintains its own completeness (rather than scattering fixes across multiple small follow-up commits), it would make the review much easier.

Additionally, it would be helpful to add the answer to question of @chenhengqi directly into the explanation under the Why ext4file section.

@Bojun-Seo Bojun-Seo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I don’t think we need to split this into two commits. How about combining them into one?
  • Too long commit title. Commit title is usually shorter than 70 or 80 characters.
  • I just quickly checked bpf.c for now.

Comment thread libbpf-tools/ext4file.bpf.c Outdated
@@ -0,0 +1,196 @@
// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
// Copyright (c) 2025 Samsung Electronics Co., Ltd.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2026?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed the license title of .bpf.c, .c and .h file.

__type(value, struct file_info_val);
} file_info_map SEC(".maps");

static __always_inline bool str_equal(const char *a, const char *b) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check coding style consistency.

char* a vs char *a

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the coding style into a unified format: char *a

Comment thread libbpf-tools/ext4file.bpf.c Outdated
int BPF_PROG(my_ext4_add_entry, handle_t* handle,
struct dentry* dentry, struct inode* inode)
{
bpf_printk("ext4_add_entry");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using bpf_printk in BPF code can cause performance issues.
I recommend removing it—not just here, but in all other places as well.

You'll see that no other tools except memleak use bpf_printk in their BPF code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deleted the unnecessary bpf_printk.

Comment thread libbpf-tools/ext4file.bpf.c Outdated
int BPF_PROG(my_ext4_file_read_iter,
struct kiocb *iocb, struct iov_iter *to)
{
//bpf_printk("ext4_file_read_iter\n");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove unnecessary comments.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have remove the comments

@niebowen666

Copy link
Copy Markdown
Author
  • I don’t think we need to split this into two commits. How about combining them into one?
  • Too long commit title. Commit title is usually shorter than 70 or 80 characters.
  • I just quickly checked bpf.c for now.

Thank you, get it√

@Bojun-Seo

Copy link
Copy Markdown
Contributor

@niebowen666
If you're not actually trying to close the PR, but rather want to prevent others from reviewing it temporarily while you're still making changes, I recommend changing the PR's status to Draft not Closed.

@niebowen666 niebowen666 reopened this Feb 9, 2026
@niebowen666

Copy link
Copy Markdown
Author

@niebowen666 If you're not actually trying to close the PR, but rather want to prevent others from reviewing it temporarily while you're still making changes, I recommend changing the PR's status to Draft not Closed.

Thanks for your advice.
I have merged the two commits with a shorter commit title.

@niebowen666

niebowen666 commented Feb 24, 2026

Copy link
Copy Markdown
Author

@Bojun-Seo
Hi, Bojun.
I have merged the two commits with a shorter commit title. Anything wrong or unsuitable about my PR?
Thanks!

@Bojun-Seo Bojun-Seo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, please change the title of commit message.

I noticed there are two maps in the bpf.c code. Conceptually, it seems like only one map would be sufficient. The ino_name_map uses the inode as the key and file_info_key as the value, while the file_info_map uses file_info_key as the key and file_info_value as the value. This effectively means that the inode is the ultimate key, and all other information is stored as part of the value in one map.

So, I’m wondering—was there a specific reason for separating them into two maps?

} file_info_map SEC(".maps");

static __always_inline bool str_equal(const char *a, const char *b) {
for (size_t i = 0; i < MAX_FILE_NAME; i++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the value of MAX_FILE_NAME? Where is it defined?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is 255 which is defined in ext4file.h. I set this value based on the definition of NAME_MAX in the Linux kernel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is MAX_FILE_NAME the same as NAME_MAX?
Even if the macro names are different, does it get automatically converted or something?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I found that the NAME_MAX in Linux kernel is set to 255. So I set a new macro MAX_FILE_NAME to 255

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I found that the NAME_MAX in Linux kernel is set to 255.

OK.

So I set a new macro MAX_FILE_NAME to 255

I cannot find the code that sets the new macro MAX_FILE_NAME. Could you tell me the line number of ext4file.h where it is defined?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, NAME_MAX in ext4file.h should be changed to MAX_FILE_NAME. I have fixed it

@niebowen666

Copy link
Copy Markdown
Author

First, please change the title of commit message.

I noticed there are two maps in the bpf.c code. Conceptually, it seems like only one map would be sufficient. The ino_name_map uses the inode as the key and file_info_key as the value, while the file_info_map uses file_info_key as the key and file_info_value as the value. This effectively means that the inode is the ultimate key, and all other information is stored as part of the value in one map.

So, I’m wondering—was there a specific reason for separating them into two maps?

As you can see, ext4file tracks the deletion of files. We envision that users need to frequently create and delete files, and the file descriptor (fd) resources in the kernel are limited, so fd reuse may occur. In this case, an fd may not represent a specific file. Based on the idea, we believe that the existing method of uniquely representing a file is more reasonable.

@Bojun-Seo

Copy link
Copy Markdown
Contributor

First, please change the title of commit message.
I noticed there are two maps in the bpf.c code. Conceptually, it seems like only one map would be sufficient. The ino_name_map uses the inode as the key and file_info_key as the value, while the file_info_map uses file_info_key as the key and file_info_value as the value. This effectively means that the inode is the ultimate key, and all other information is stored as part of the value in one map.
So, I’m wondering—was there a specific reason for separating them into two maps?

As you can see, ext4file tracks the deletion of files. We envision that users need to frequently create and delete files, and the file descriptor (fd) resources in the kernel are limited, so fd reuse may occur. In this case, an fd may not represent a specific file. Based on the idea, we believe that the existing method of uniquely representing a file is more reasonable.

Got it — that makes sense.
By the way, could you include this in the commit message?

@niebowen666

Copy link
Copy Markdown
Author

First, please change the title of commit message.
I noticed there are two maps in the bpf.c code. Conceptually, it seems like only one map would be sufficient. The ino_name_map uses the inode as the key and file_info_key as the value, while the file_info_map uses file_info_key as the key and file_info_value as the value. This effectively means that the inode is the ultimate key, and all other information is stored as part of the value in one map.
So, I’m wondering—was there a specific reason for separating them into two maps?

As you can see, ext4file tracks the deletion of files. We envision that users need to frequently create and delete files, and the file descriptor (fd) resources in the kernel are limited, so fd reuse may occur. In this case, an fd may not represent a specific file. Based on the idea, we believe that the existing method of uniquely representing a file is more reasonable.

Got it — that makes sense. By the way, could you include this in the commit message?

Sure, I have already modified the commit message.

@Bojun-Seo Bojun-Seo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add ext4file on .gitignore file.

Comment thread libbpf-tools/ext4file.c Outdated
#include "trace_helpers.h"
#include "ext4file.h"

#define BG_LIST_NUM 57232

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove dead code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed it.

@niebowen666

Copy link
Copy Markdown
Author

Please add ext4file on .gitignore file.

Do you mean I should add the compiled binary file ext4file (not the source files) to .gitignore? Or something else

@Bojun-Seo

Copy link
Copy Markdown
Contributor

Please add ext4file on .gitignore file.

Do you mean I should add the compiled binary file ext4file (not the source files) to .gitignore? Or something else

I mean, compiled binary file(ext4file) should be added on .gitignore file.

@niebowen666

Copy link
Copy Markdown
Author

Please add ext4file on .gitignore file.

Do you mean I should add the compiled binary file ext4file (not the source files) to .gitignore? Or something else

I mean, compiled binary file(ext4file) should be added on .gitignore file.

Hi Bojun, I have added ext4file on .gitignore(libbpf-tools/.gitignore) file. Is that right?

@Bojun-Seo

Copy link
Copy Markdown
Contributor

Please add ext4file on .gitignore file.

Do you mean I should add the compiled binary file ext4file (not the source files) to .gitignore? Or something else

I mean, compiled binary file(ext4file) should be added on .gitignore file.

Hi Bojun, I have added ext4file on .gitignore(libbpf-tools/.gitignore) file. Is that right?

Yes. The code is correct. But it might be better to merge the last patch into original one.

@niebowen666

Copy link
Copy Markdown
Author

Please add ext4file on .gitignore file.

Do you mean I should add the compiled binary file ext4file (not the source files) to .gitignore? Or something else

I mean, compiled binary file(ext4file) should be added on .gitignore file.

Hi Bojun, I have added ext4file on .gitignore(libbpf-tools/.gitignore) file. Is that right?

Yes. The code is correct. But it might be better to merge the last patch into original one.

OK~ I have merged~

@niebowen666

Copy link
Copy Markdown
Author

Hi, Bojun! @Bojun-Seo
Is there anything wrong about my PR?
Thanks~

@Bojun-Seo

Copy link
Copy Markdown
Contributor

Well, these days, AI code reviews have gotten really good. What do you think about running an AI review first (before human reviewers) and sharing the results?
AI tends to clearly point out what’s good about the PR and what could be improved, which makes it much easier for human reviewers to focus on the important parts. If there are any AI suggestions you decided not to accept, explaining why would be also helpful. It would also be great if you could mention which AI model you used for the review.

… is used to monitor the I/O patterns (buffer or direct) of each file in the target ext4 filesystem, as well as the hint used by each file.

ext4file is used to monitor the I/O patterns (buffer or direct) of each file in the target ext4 filesystem, as well as the hint used by each file.

ext4file is a tool used to track file-level buffer or direct I/O. Currently, in the repository, there exist block-layer tools to monitor I/O patterns of whole disk (such as biopattern, biolatency, etc.) and VFS-layer tools to trace file lifecycle and I/O behavior of files throughout the entire VFS (such as filelife and vfsstat, etc.).

Below is a comparative summary:

| Tool Name     | Layer               | Main Function                                                                                                 | Tracks Filename? |  Differences |
|---------------|---------------------|---------------------------------------------------------------------------------------------------------------|------------------|------------------------|
| biopattern    | Block        | Measures the proportion of random vs. sequential I/O on a storage device                                | No | Can not achieve file-layer tracing(The other bio tools all have this problem) |
| fsdist        | VFS           | Tracks latency distribution of operations like read, write, open, and sync                                    | No | Focus on latency, not I/O pattern |
| fsslower      | VFS           | Traces slow file operations (e.g., long-latency reads/writes), focuses on I/O size                            | Yes| Trace the I/O size and latency, not I/O pattern |
| filelife      | VFS           | Monitors file lifecycle events (creation and deletion)                                                        | Yes| Only focus on file creation and deletion |
| filetop       | VFS          | Shows real-time I/O activity of active files (displays only top entries to avoid verbosity)                   | Yes| There exists no distinction between buffer and direct I/O |
| ext4file      | ext4 Filesystem | Tracks **buffer vs. direct I/O patterns** per file, enables fine-grained file-level monitoring using inode | Yes|  |

Run ext4file before executing your test. You can refer to ./ext4file -h to get the usage of the tool

Show I/O pattern for every file in ext4 filesystem.

Usage: ./ext4file [-h] [-d DIR] [-o FILE] [interval] [count]

Options:
  -h, --help                   Print this help message
  -d DIR, --dir=DIR            Trace the ext4 filesystem mounted on the specified directory
  -o FILE, --output=FILE       Write output to a file (optional; default: stdout)
  interval                     Time interval (in seconds) between reports (default: unlimited)
  count                        Number of reports to generate (default: unlimited)

Examples:
  ./ext4file -d /mnt/ext4                      # Trace I/O patterns of files on the ext4 filesystem mounted at /mnt/ext4
  ./ext4file -d /mnt/ext4 1 10                 # Generate 10 reports, one per second
  ./ext4file -d /mnt/ext4 -o output 1 10       # Generate 10 reports at 1-second intervals, saving output to ./output

The output could be:

root@server:/home/nbw/OpenSource/biohint/libbpf-tools# ./ext4file -d /mnt/ext4File/
EXT4 Filesystem Info: blocks_count=3750232064 blocks_per_group=32768 bg_cnt=114448
Tracing Ext4 read/write... Hit Ctrl-C to end.
2026-01-14 13:58:21
file_name            inode      pa_inode   hint   buffer_read     direct_read     buffer_write    direct_write    delete
test3                83361794   83361793   0      0               0               0               0               False
test2                34         2          2      8               0               1               0               False
dir1                 83361793   2          0      0               0               0               0               False
dir2                 440467457  2          0      0               0               0               0               True
test3                33         2          3      8               0               1               0               False
test1                33         2          5      8               0               1               0               True
test3                440467458  440467457  0      0               0               0               0               True

Below is the detailed explanation of each field in the ext4file output. This tool traces per-file I/O patterns (buffered vs. direct) on ext4 filesystems, providing fine-grained visibility into application behavior.
| Field         | Description   |
|---------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| file_name | The name of the file (without full path). Note: multiple files may share the same name.                                                                                                                     |
| inode     | The inode number of the file. Inode is the unique identifier for a file within the filesystem, even across renames or hard links. This enables accurate tracking of I/O for specific files.                 |
| pa_inode  | The inode number of the file’s parent directory. |
| hint      | The FDP (Flexible Data Placement) hint value associated with the file. FDP is a new NVMe feature that enables the host to guide data placement on the SSD.                                                 |
| buffer_read | Number of buffered read operations performed on the file. Buffered I/O goes through the kernel page cache.                                                                                             |
| direct_read | Number of direct read operations performed on the file. Direct I/O bypasses the page cache.                                                                                                           |
| direct_write | Number of direct write operations performed on the file. Like direct read, it skips the page cache and writes data directly from user space to storage.                                             |
| buffer_write | Number of buffered write operations performed on the file. Data is first written to the page cache and later flushed to disk asynchronously by the kernel.                                         |
| delete    | Indicates whether the file has been unlinked (deleted). If True, the file was removed from the directory but may still be accessible if held open by a process. I/O on such files can indicate resource leaks or long-running file handles. |

This tool is intended for ext4 filesystem developers and performance engineers who need to analyze I/O behavior at the file level.

Was there a specific reason for separating them into two maps?
	- ext4file tracks the deletion of files. We envision that users need to frequently create and delete files, and the file descriptor (fd) resources in the kernel are limited, so fd reuse may occur. In this case, an fd may not represent a specific file. Based on the idea, we believe that the existing method of uniquely representing a file is more reasonable.
@niebowen666

niebowen666 commented Jul 23, 2026

Copy link
Copy Markdown
Author

Hey, Bojun @Bojun-Seo
I have run an AI review using codeLLM pro these days.
prompt: Well, can you take a look at commit 7ab17b34107115521f26cee40788ca6c32db90b0? Help me analyze what this commit means for the whole repository, and then check if there are any areas in this commit that could be optimized or improved.

response:

1. Critical: Memory leak in deletion handling
In my_ext4_unlink (line 83-99) and my_ext4_rmdir (line 102-118), the file_info_map entry is never removed. The ino_name_map entry is conditionally removed (only if names match), but the file_info_map entry remains. For workloads with frequent file creation/deletion, this causes the hash map to grow unboundedly and eventually exhaust the 1M entry limit.
Fix: After setting fivp->fv_delete = true, also call bpf_map_delete_elem(&file_info_map, &fik) to fully clean up. Or at minimum, use a separate "deleted" set.
2. Dead code: unused #define N 16 and #define GROUP 64
Lines 14-15 of ext4file.bpf.c define N and GROUP but they are never referenced anywhere. These should be removed.
3. BPF memory efficiency: consider BPF_F_NO_PREALLOC
Both hash maps declare max_entries, 1000000 with the default pre-allocation flag. For a sparse workload with far fewer active files, this wastes kernel memory. Adding BPF_F_NO_PREALLOC to the map definitions would defer allocation until an entry is actually inserted, saving memory.
SEC(".maps")
struct {
    __uint(type, BPF_MAP_TYPE_HASH);
    __uint(max_entries, 1000000);
    __uint(map_flags, BPF_F_NO_PREALLOC);
    __type(key, u32);
    __type(value, struct file_info_key);
} ino_name_map SEC(".maps");
4. Commit message is excessively verbose
The commit message is ~400 lines long and contains usage documentation, a comparison table, field descriptions, and an internal Q&A exchange. This level of detail belongs in documentation (e.g., a README or man page for the tool), not in the commit message. The commit message itself should be concise (e.g., 2-5 sentences summarizing what the tool does and why it was added).
5. User-space: config.interval default is misleading
ext4file.c:31 sets interval = 100000000 (100 billion nanoseconds = 100 seconds). But the program's behavior is sleep(config.interval) -- sleep() takes seconds, not nanoseconds. So the default effectively means 100 seconds between reports, which is very slow for a monitoring tool. A more intuitive default would be 1 second. The comment says "default: unlimited" but the actual default is just slow.
6. User-space: memory leak for config.dir
In program_configure (line 216), config.dev = malloc(256) allocates memory that is never freed in main()'s cleanup path. Should add free(config.dev) to the cleanup section.
7. User-space: ext4_info_get error path
If ext4_info_get fails (e.g., open() returns negative), the code jumps to cleanup with uninitialized ext4_config values. The fd_ext4_dev < 0 check uses < 0 for error but the cleanup closes only when > 0. A negative fd should trigger early return/error rather than falling through with garbage config values.
8. BPF: str_equal could be simplified
The str_equal function (line 31-37) is correct but unusual. Consider using bpf_strncmp if the kernel supports it, or memcmp + length check, which would be more idiomatic and potentially faster. However, since BPF's bpf_strncmp availability depends on kernel version, the current approach is acceptable.
9. User-space: FD_STDOUT / FD_STDERR sentinel convention
#define FD_STDOUT -1 and #define FD_STDERR -2 use negative values as sentinels, which conflicts with the standard POSIX convention where -1 means an invalid fd. While this works in the targeted_printf function, it's fragile. Using named constants like STD_OUTPUT_FILENO (0) and STD_ERROR_FILENO (2) would be clearer.
10. BPF: No overflow protection on counters
The fv_rw_cnt array uses atomic add (__sync_fetch_and_add), but there's no overflow check. Under heavy I/O, u64 counters could theoretically wrap around after extreme usage (~18.4 x 10^18 operations). This is unlikely in practice but worth noting.

what I did:

1. (reject) The design makes sense. It's because the tool was originally designed to track all files during the time it was allowed to run, including deleted ones.
2. (accept) I have removed the dead code.
3. (accept) I have added the "__uint(map_flags, BPF_F_NO_PREALLOC)" in two map.
4. (reject)The commit message was written based on our communication.
5. (reject) interval is not nanosecond, is second. So we set it as 1000000 seconds to represent "ulimited"
6. (accept) I have added "free(config.dev)" at the end of main(cleanup)
7. (accept) I modified the ext4_info_get function to have a return value, so that the caller can know whether the function executed successfully. I also changed the part in the bpf_initialize_and_load function that calls ext4_info_get.
8. (reject) bpf_strncmp require "const char*"
9. (accept) Changed the definitions of FD_STDOUT and FD_STDERR, and modified the targeted_printf function.
10. (accept) I added a check so that when fv_rw_cnt[] reaches the maximum value of u64, __sync_fetch_and_add will stop.

@Bojun-Seo

Copy link
Copy Markdown
Contributor

Followings are my opinion.

Commit Message

The commit title is still too long. As mentioned in previous reviews, please "shorten the title" to follow the project's conventional commit style.

Additionally, many lines in the commit message body exceed the recommended length. Again, please check conventional commit style.

AI Review Process

I recommend the following process for future PRs:

  • When using AI for review, clearly specify the model (e.g., Claude Sonnet 5, Grok 4, GPT-5.6, etc.). This helps reviewers assess the reliability of the feedback. CodeLLM is an AI-powered code editor, not an LLM model itself
  • It's not necessary to list all "accepted" AI suggestions. It's sufficient to explain the points that were "rejected" and why.
  • If you made changes based on AI feedback, please run the AI review again until no critical issues remain, so reviewers can see the final state.

Free-tier models are already strong enough for code review if given good prompts. You can easily find effective code review prompts by searching or asking an LLM.


Followings are the prompt and the result of your PR reviewed by free version of Grok. I don't know what version is. I just ran it today.

Prompt

Commit message rules

  • Explain the reason why the feature is needed and provide instructions or examples on how to use it in case of feature additions.
  • Contain only the minimum necessary content while remaining sufficiently detailed.
  • Include relevant background knowledge if necessary for understanding the change.
  • Provide explanations for both the "What" and the "Why" when adding magic numbers.
  • Explain why this change is necessary.
  • Write the message in accordance with the established commit message convention by referencing the commit history.

Code review rules

  • Do the function name and its role (internal code and return value) match?
  • Do variable names and macro names accurately reflect their roles?
  • If there are multiple types of exceptions, return distinct errors for each and ensure they are documented with comments.
  • Consider whether all output messages are appropriate for the end-user.
  • If a function has no return value, is it truly unnecessary?
  • Do the return value and return type match?
  • Handle user input with rigorous exception processing.
  • If this change causes documents like README.md to become stale, update them accordingly.

AI Results

I didn't read them, I just copy and paste the results. So, the "I" in the following contents are all AI itself.

Review of PR #5440

The tool idea itself is useful — providing per-file buffer vs direct I/O visibility at the ext4 layer is a nice addition.

However, I have several concerns based on the project’s commit message and code review guidelines.

1. Commit Message (Major Issue)

The commit title and message still do not follow the project's established conventions (please check recent commits in libbpf-tools/).

  • Title is too long and not concise.
  • The body contains a lot of content that should be shortened and better structured.
  • It should clearly explain Why this tool is needed, how it differs from existing tools (filetop, fsslower, etc.), and include usage examples.

Recommendation: Make the commit message self-contained, concise, and follow the style of other tools (e.g. ext4slower, filetop).

2. Code Review

Positive points

  • Tool concept is clear and fills a gap (file-level buffer/direct I/O + FDP hint).
  • Use of inodes for unique file identification is appropriate.

Issues found

  • Function names & roles:
    Most my_ext4_* functions match their purpose well.

  • Variable names:
    Mostly acceptable, but some abbreviations (fik, fivp, fikp) could be clearer (e.g. file_key, file_val).

  • Exception / Error handling:
    Many bpf_map_lookup_elem / update_elem calls do not handle errors explicitly. While common in BPF, critical failures should have better handling or at least comments explaining why it's acceptable.

  • Magic numbers:
    There are several magic numbers (e.g. 1000000 for map size, MAX_FILE_NAME). Please add comments explaining what and why these values were chosen.

  • User input handling:
    The -d (directory) option and other arguments need more rigorous validation. Currently it may not handle invalid paths or permission errors gracefully.

  • Return values:
    All probe functions correctly return 0. Good.

  • Output messages:
    Output format looks user-friendly.

  • Documentation:
    README.md (or tool-specific doc) should be updated if this tool is added to the official list.

Other suggestions

  • Consider splitting large maps or adding LRU logic if memory pressure becomes an issue (1M entries is quite large).
  • str_equal function is a nice addition but could use a more descriptive name like file_name_equal.

Summary

The functionality is valuable, but the commit message needs significant improvement to meet project standards, and some code areas need better error handling, comments for magic numbers, and input validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants