Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace fsslower: Add support for statfs #2234

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Conversation

eiffel-fl
Copy link
Member

Hi.

This PR adds support to trace statfs syscalls:

$ sudo ./ig trace fsslower -m 0 -c test                  francis/fsslower-statfs *% u=
RUNTIME.CONTAINERNAME                 PID        COMM             T         BYTES       OFFSET          LAT FILE                         
test                                  283280     bash             O             0            0            6 df                           
test                                  283280     bash             R           256            0            5 df                           
test                                  283280     bash             R           728           64            2 df                           
test                                  283280     bash             R            28          792            1 df                           
test                                  283280     bash             O             0            0            1 ld-linux-x86-64.so.2         
test                                  283280     bash             R            64            0            2 ld-linux-x86-64.so.2         
test                                  283280     bash             R           504           64            1 ld-linux-x86-64.so.2         
test                                  283280     df               O             0            0            5 ld.so.cache                  
test                                  283280     df               O             0            0            2 libc.so.6                    
test                                  283280     df               R           832            0            4 libc.so.6                    
test                                  283280     df               R           784           64            1 libc.so.6                    
test                                  283280     df               R           784           64            2 libc.so.6                    
test                                  283280     df               S             0            0            7 diff                         
test                                  283280     df               S             0            0            3 hosts
# Other terminalroot@45b252881c0d:/# df
Filesystem                 1K-blocks      Used Available Use% Mounted on
...

Best regards.

Copy link

@jepio jepio left a comment

Choose a reason for hiding this comment

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

looks ok to me

Comment on lines 18 to 28
struct data_key {
__u32 tid;
enum fs_file_op op;
};

Copy link

Choose a reason for hiding this comment

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

why are you making op part of the key? do you expect probes to be interleaved for a thread?

Copy link
Member Author

Choose a reason for hiding this comment

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

From my test, it does not work using only tid as it would conflict between open and statfs.

@jepio
Copy link

jepio commented Nov 17, 2023 via email

@eiffel-fl
Copy link
Member Author

can you add that information as a comment or in the commit message (saying that statfs entry can occur from within an open call)

On Fri, Nov 17, 2023, 16:49 eiffel-fl @.> wrote: @.* commented on this pull request. ------------------------------ In pkg/gadgets/trace/fsslower/tracer/bpf/fsslower.bpf.c <#2234 (comment)> : > +struct data_key { + __u32 tid; + enum fs_file_op op; +}; + From my test, it does not work using only tid as it would conflict between open and statfs. — Reply to this email directly, view it on GitHub <#2234 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABXINVWSKEHVZOW2RCJDHQ3YE6BPXAVCNFSM6AAAAAA7OPA5RSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMZXGMZDKNJWGU . You are receiving this because you commented.Message ID: @.***>

I rather added a comment in the struct declaration.

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Copy link
Member

@alban alban left a comment

Choose a reason for hiding this comment

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

It looks good to me from code reading.

It would be good to have an image-based gadget for this.

Comment on lines +21 to +24
* We need to take into account the operation to avoid losing some of
* them.
* Indeed, it is possible to enter statfs syscall while already being in
* an open one.
Copy link
Member

Choose a reason for hiding this comment

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

I am curious if you have more details why open() would call statfs().

@eiffel-fl eiffel-fl merged commit 702152d into main Jan 8, 2024
53 checks passed
@eiffel-fl eiffel-fl deleted the francis/fsslower-statfs branch January 8, 2024 12:25
@eiffel-fl
Copy link
Member Author

Thank you for the reviews!

@eiffel-fl
Copy link
Member Author

It would be good to have an image-based gadget for this.

This is sadly not possible with the current state of image-based gadget as we need code to do the abstraction between the given function and the actual tracepoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants