[GIT,PULL] tracing: Fixes for 6.6-rc2

  Linus,

Tracing fixes for 6.6-rc2:

- Fix the "bytes" output of the per_cpu stat file
  The tracefs/per_cpu/cpu*/stats "bytes" was giving bogus values as the
  accounting was not accurate. It is suppose to show how many used bytes are
  still in the ring buffer, but even when the ring buffer was empty it would
  still show there were bytes used.

- Fix a bug in eventfs where reading a dynamic event directory (open) and then
  creating a dynamic event that goes into that diretory screws up the accounting.
  On close, the newly created event dentry will get a "dput" without ever having
  a "dget" done for it. The fix is to allocate an array on dir open to save what
  dentries were actually "dget" on, and what ones to "dput" on close.

[
   Note, the show_event_dentries was critical in debugging the above ;-)
]

Please pull the latest trace-v6.6-rc2 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace-v6.6-rc2

Tag SHA1: 3164684ad125d90cdb6c96cddb00676649831d52
Head SHA1: ef36b4f92868d66908e235980f74afdfb9742d12


Steven Rostedt (Google) (1):
      eventfs: Remember what dentries were created on dir open

Zheng Yejian (1):
      ring-buffer: Fix bytes info in per_cpu buffer stats

----
 fs/tracefs/event_inode.c   | 87 +++++++++++++++++++++++++++++++++++++---------
 kernel/trace/ring_buffer.c | 28 ++++++++-------
 2 files changed, 85 insertions(+), 30 deletions(-)
---------------------------
  

On Sat, 23 Sept 2023 at 16:14, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> - Fix a bug in eventfs where reading a dynamic event directory (open) and then
>   creating a dynamic event that goes into that diretory screws up the accounting.

Honestly, I'm getting more and more convinced that you just need to
stop this eventfs stuff.

This is just *incredibly* ugly:

  /*
   * This just sets the file->private_data back to the cursor and back.
   */
  static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx)
  {
        struct dentry_list *dlist = file->private_data;
        int ret;

        file->private_data = dlist->cursor;
        ret = dcache_readdir(file, ctx);
        dlist->cursor = file->private_data;
        file->private_data = dlist;
        return ret;
  }

I guess it works by the f_pos locking magic, but by christ is this
ugly beyond words.

Honestly, now, are the eventfs changes *really* making up for this
kind of continual "this is crazy" garbage? We had the whole "this is
undebuggable" discussion, now there's stuff like this.

Were people even *aware* of the f_pos locking, or did this just happen to work?

And that's entirely ignoring the disgusting thing that is that
"allocate an array of every dentry we looked at" issue. Which honestly
also looks disgusting.

I beg of you: think hard about just reverting all the eventfs changes
that moved away from using dentries natively, and instead started
doing these *incredibly* hacky and ugly things.

Really.

               Linus
  

The pull request you sent on Sat, 23 Sep 2023 19:14:20 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace-v6.6-rc2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5edc6bb321d970c77d666a6cf8eeb060f2d18116

Thank you!
  

On Sun, 24 Sep 2023 14:09:04 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 23 Sept 2023 at 16:14, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > - Fix a bug in eventfs where reading a dynamic event directory (open) and then
> >   creating a dynamic event that goes into that diretory screws up the accounting.  
> 
> Honestly, I'm getting more and more convinced that you just need to
> stop this eventfs stuff.
> 
> This is just *incredibly* ugly:
> 
>   /*
>    * This just sets the file->private_data back to the cursor and back.
>    */
>   static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx)
>   {
>         struct dentry_list *dlist = file->private_data;
>         int ret;
> 
>         file->private_data = dlist->cursor;
>         ret = dcache_readdir(file, ctx);
>         dlist->cursor = file->private_data;
>         file->private_data = dlist;
>         return ret;
>   }
> 
> I guess it works by the f_pos locking magic, but by christ is this
> ugly beyond words.
> 
> Honestly, now, are the eventfs changes *really* making up for this
> kind of continual "this is crazy" garbage? We had the whole "this is
> undebuggable" discussion, now there's stuff like this.

I never said "this is undebuggable". That show_event_dentries just
showed ref counts and what was allocated. Not that it was "undebuggable".
When ls is done on eventfs, the dentries are created. When the memory
is tight, any dentry that is not open should be reclaimed. The
show_event_dentries was to see if they were reclaimed or are still
around and taking up memory. It is also showing the ref counts, where
you can see if closing the file/directory would decrement the ref count
properly. It was used here because I found a way to dec a dentry
without first upping the refcount, as I explain below.

The purpose of eventfs here is that the /sys/kernel/tracing/events
directory currently allocates dentries and inodes for all existing
events in the kernel. Since there are now over a thousand events in the
kernel, and each event has several files created for them, and these
files are seldom looked at, why should they be allocated when not used?

These dentries and inodes are allocated for the top level directory and
wasting memory for most users. When an instance is created it makes
matters even worse.

 mkdir /sys/kernel/trace/instance/foo

Which creates an entire copy of the events directory to maintain state
(you can enable events for this instance and not for other instances),
all those dentries and inodes for the events are allocated again (like
20MB worth).

And we plan on using many instances in production, we rather not waste
all that memory. That was the reason for doing this in the first place.

Now, I presented this at LSFMM where I learned about /proc doing
something similar (and you pointed that out too) but when I looked at
that code I couldn't figure out how to easily make that work with
tracefs, so this work came out instead.

I'm not an FS developer so there may be a better way to do this. I
would be happy to hear about better alternatives.

> 
> Were people even *aware* of the f_pos locking, or did this just happen to work?

I looked at the implementation of dcache_dir_open(), dcache_readdir()
and dcache_dir_close() and saw how it allocated, modified, and freed
the file->private_data / cursor. I came to the conclusion that if
there wasn't protection around them then it would not work. In fact,
it's abstracted out enough that I could have just simply copied the
code and just use my struct dlist_entry directly. But instead of
copying of that code, I did it this way to reuse the existing code.

Would you prefer that I just copy that code (there's really not much)
and implement it less "ugly" by using my own cursor? Which would
require that f_pos locking magic just like any other implementation of
.iterate_shared and friends.

> 
> And that's entirely ignoring the disgusting thing that is that
> "allocate an array of every dentry we looked at" issue. Which honestly
> also looks disgusting.

How is it disgusting? How else would you do it?

You open a directory to read the files which creates the dentries and
ups their ref counts, in the mean time, a new file is created (it's OK
not to see it, as it's similar to RCU, only new opens will see the new
file), but when you close it, you only do the dput on the files you
opened and not the new file. The current code used just the link list
of all the files which also included the new file that wasn't updated
on open.

> 
> I beg of you: think hard about just reverting all the eventfs changes
> that moved away from using dentries natively, and instead started
> doing these *incredibly* hacky and ugly things.

And bring back all that wasted memory?

If there's a better way to achieve the same thing, I'll happily do it.

-- Steve
  

On Mon, 25 Sept 2023 at 07:53, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> These dentries and inodes are allocated for the top level directory and
> wasting memory for most users. When an instance is created it makes
> matters even worse.

So honestly, dentries are *tiny*. They are probably not any bigger
than the backing data structures you use to keep track of the names in
teh first place.

The memory cost is likely almost from the inodes, which are indeed
pretty big and fat.

> If there's a better way to achieve the same thing, I'll happily do it.

Do you actually *need* individual inodes for each of the entries in tracefd?

Because I think the *big* memory savings would be to use one single
inode for every file (make directories have individual inodes,
anything else will confuse user-space 'pwd' etc mightily)

Then you'd

 (a) have the actual tracefs-specific data in dentry->d_fsdata

 (b) use "-inode->i_op->getattr()" to make the stat() info look
different for different files (if you even care, but you might)

and I think that would be most of it.

You might hit some gotcha somewhere - things like "dcache_readdir()
will take the inode number just from the inode, and there's no
callback for it", so if you care about showing unique inode numbers to
user space, we might need to help you out some way.

But that "reuse a single inode" is actually a very traditional
pattern, even if I suspect that pattern doesn't remain anywhere. /proc
used to do things like that, iirc.

                        Linus