eventfs: Give files a default of PAGE_SIZE size

Commit Message

Steven Rostedt Jan. 26, 2024, 6:18 p.m. UTC

From: "Steven Rostedt (Google)" <[e-mail]>

The sqlhist utility[1] (or the new trace-cmd sqlhist command[2]) creates
the commands of a synthetic event based on the files in the tracefs/events
directory. When creating these commands for an embedded system, it is
asked to copy the files to the temp directory, tar them up and send them
to the main machine. Ideally, tar could be used directly without copying
them to the /tmp directory. But because the sizes presented by the inodes
are of size '0' the tar just copies zero bytes of the file making it
useless.

By following what sysfs does, and give files a default size of PAGE_SIZE,
it allows the tar to work. No event file is greater than PAGE_SIZE.

Although tar will give an error of:

 tar: events/raw_syscalls/filter: File shrank by 3904 bytes; padding with zeros

Which is consistent to the error it gives to sysfs as well:

~# (cd /sys/ && tar cf - firmware) | tar xf -
 [..]
 tar: firmware/acpi/interrupts/ff_slp_btn: File shrank by 4057 bytes; padding with zeros

But only add size if the file can be open for read. It doesn't make sense
for files that are write-only.

[1] https://trace-cmd.org/Documentation/libtracefs/libtracefs-sqlhist.html
[2] https://trace-cmd.org/Documentation/trace-cmd/trace-cmd-sqlhist.1.html

Link: https://lore.kernel.org/all/[e-mail]/

Signed-off-by: Steven Rostedt (Google) <[e-mail]>
---
 fs/tracefs/event_inode.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Linus Torvalds Jan. 26, 2024, 6:31 p.m. UTC | #1

On Fri, 26 Jan 2024 at 10:18, Steven Rostedt <[e-mail]> wrote:
>
> By following what sysfs does, and give files a default size of PAGE_SIZE,
> it allows the tar to work. No event file is greater than PAGE_SIZE.

No, please. Just don't.

Nobody has asked for this, and nobody sane should use 'tar' on tracefs anyway.

It hasn't worked before, so saying "now you can use tar" is just a
*bad* idea. There is no upside, only downsides, with tar either (a)
not working at all on older kernels or (b) complaining about how the
size isn't reliable on newer ones.

So please. You should *NOT* look at "this makes tar work, albeit badly".

You should look at whether it improves REAL LOADS. And it doesn't. All
it does is add a hack for a bad idea. Leave it alone.

                   Linus

Steven Rostedt Jan. 26, 2024, 6:41 p.m. UTC | #2

On Fri, 26 Jan 2024 10:31:07 -0800
Linus Torvalds <[e-mail]> wrote:

> On Fri, 26 Jan 2024 at 10:18, Steven Rostedt <[e-mail]> wrote:
> >
> > By following what sysfs does, and give files a default size of PAGE_SIZE,
> > it allows the tar to work. No event file is greater than PAGE_SIZE.  
> 
> No, please. Just don't.
> 
> Nobody has asked for this, and nobody sane should use 'tar' on tracefs anyway.
> 
> It hasn't worked before, so saying "now you can use tar" is just a
> *bad* idea. There is no upside, only downsides, with tar either (a)
> not working at all on older kernels or (b) complaining about how the
> size isn't reliable on newer ones.
> 
> So please. You should *NOT* look at "this makes tar work, albeit badly".
> 
> You should look at whether it improves REAL LOADS. And it doesn't. All
> it does is add a hack for a bad idea. Leave it alone.
>

Fine, but I still plan on sending you the update to give all files unique
inode numbers. If it screws up tar, it could possibly screw up something
else. And all the files use to have unique numbers. They are just not unique
in the current -rc release. You have a point that this would just fix this
release and the older kernels would still be broken, but the identical inode
numbers is something I don't want to later find out breaks something.

-- Steve

Linus Torvalds Jan. 26, 2024, 7:06 p.m. UTC | #3

On Fri, 26 Jan 2024 at 10:41, Steven Rostedt <[e-mail]> wrote:
>
> Fine, but I still plan on sending you the update to give all files unique
> inode numbers. If it screws up tar, it could possibly screw up something
> else.

Well, that in many ways just regularizes the code, and the dynamic
inode numbers are actually prettier than the odd fixed date-based one
you picked. I assume it's your birthdate (although I don't know what
the directory ino number was).

               Linus

Steven Rostedt Jan. 26, 2024, 7:37 p.m. UTC | #4

On Fri, 26 Jan 2024 11:06:33 -0800
Linus Torvalds <[e-mail]> wrote:

> On Fri, 26 Jan 2024 at 10:41, Steven Rostedt <[e-mail]> wrote:
> >
> > Fine, but I still plan on sending you the update to give all files unique
> > inode numbers. If it screws up tar, it could possibly screw up something
> > else.  
> 
> Well, that in many ways just regularizes the code, and the dynamic
> inode numbers are actually prettier than the odd fixed date-based one
> you picked. I assume it's your birthdate (although I don't know what
> the directory ino number was).

Yeah, it was. I usually use that when I need a random number. I avoid using
it for passwords though. The odd directory number was the date you pulled
in eventfs ;-)

-- Steve

Patch

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 7be7a694b106..013b8af40a4f 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -363,6 +363,8 @@  static struct dentry *create_file(const char *name, umode_t mode,
 	inode->i_fop = fop;
 	inode->i_private = data;
 	inode->i_ino = ino;
+	if (mode & (S_IRUSR | S_IRGRP | S_IROTH))
+		inode->i_size = PAGE_SIZE;
 
 	ti = get_tracefs(inode);
 	ti->flags |= TRACEFS_EVENT_INODE;