Received-SPF: pass (google.com: domain of linux-kernel+bounces-40670-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 26 Jan 2024 16:26:26 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>, Linux Trace Devel
 <linux-trace-devel@vger.kernel.org>, Masami Hiramatsu
 <mhiramat@kernel.org>, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
 Christian Brauner <brauner@kernel.org>, Ajay Kaher
 <ajay.kaher@broadcom.com>, Geert Uytterhoeven <geert@linux-m68k.org>,
 linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH] eventfs: Have inodes have unique inode numbers
Message-ID: <20240126162626.31d90da9@gandalf.local.home>
In-Reply-To: <CAHk-=wgZEHwFRgp2Q8_-OtpCtobbuFPBmPTZ68qN3MitU-ub=Q@mail.gmail.com>
References: <20240126150209.367ff402@gandalf.local.home>
	<CAHk-=wgZEHwFRgp2Q8_-OtpCtobbuFPBmPTZ68qN3MitU-ub=Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 26 Jan 2024 12:25:05 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Steven,
>  stop making things more complicated than they need to be.
> 
> And dammit, STOP COPYING VFS LAYER FUNCTIONS.
> 
> It was a bad idea last time, it's a horribly bad idea this time too.
> 
> I'm not taking this kind of crap.
> 
> The whole "get_next_ino()" should be "atomic64_add_return()". End of story.

I originally wrote it that way, and thought to myself that the VFS version
is "faster" and switched to that.

My fault for being too much into micro-optimizations.

> 
> You arent' special. If the VFS functions don't work for you, you don't
> use them, but dammit, you also don't then steal them without
> understanding what they do, and why they were necessary.
> 
> The reason get_next_ino() is critical is because it's used by things
> like pipes and sockets etc that get created at high rates, the the
> inode numbers most definitely do not get cached.

Yes, I understood why it's optimized, and took it because it's been there
since 2010 and figured it's pretty solid.

> 
> You copied that function without understanding why it does what it
> does, and as a result your code IS GARBAGE.
> 
> AGAIN.
> 
> Honestly, kill this thing with fire. It was a bad idea. I'm putting my
> foot down, and you are *NOT* doing unique regular file inode numbers
> uintil somebody points to a real problem.
> 
> Because this whole "I make up problems, and then I write overly
> complicated crap code to solve them" has to stop,.

If I had just used the atomic_add_return() is it really that overly
complicated? Yes, I copied from VFS because I figured if they put in the
effort to make it faster then why not use that, even though it was overkill.

> 
> No more. This stops here.
> 
> I don't want to see a single eventfs patch that doesn't have a real
> bug report associated with it. And the next time I see you copying VFS
> functions (or any other core functions) without udnerstanding what the
> f*ck they do, and why they do it, I'm going to put you in my
> spam-filter for a week.
> 
> I'm done. I'm really *really* tired of having to look at eventfs garbage.

So we keep the same inode number until something breaks with it, even
though, using unique ones is not that complicated?

I'd be happy to change that patch to what I originally did before deciding
to copy get_next_ino():

unsigned int tracefs_get_next_ino(int files)
{
	static atomic_t next_inode;
	unsigned int res;

	do {
		res = atomic_add_return(files + 1, &next_inode);

		/* Check for overflow */
	} while (unlikely(res < files + 1));

	return res - files;
}

If I knew going back and copying over get_next_ino() was going to piss you
off so much, I wouldn't have done that.

Not to mention that the current code calls into get_next_ino() and then
throws it away. That is, eventfs gets its inode structure from tracefs that
adds the inode number to it using the VFS get_next_ino(). That gets thrown
away by the single inode assigned. This just makes it more likely that the
global get_inode_ino() is going to overflow due to eventfs, even though
eventfs isn't even using them.

I only did the one inode number because that's what you wanted. Is it that
you want to move away from having inode numbers completely? At least for
pseudo file systems? If that's the case, then we can look to get people to
start doing that. First it would be fixing tools like 'tar' to ignore the
inode numbers.

Otherwise, I really rather keep it the way it has always been. That is,
each file has its own unique inode number, and not have to deal with some
strange bug report because it's not. Is there another file system that has
just one inode number?

-- Steve