Message-ID: <20240116131228.3ed23d37@gandalf.local.home>
Date: Tue, 16 Jan 2024 13:12:28 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Christian Brauner <brauner@...nel.org>, Al Viro <viro@...iv.linux.org.uk>,
 kernel test robot <oliver.sang@...el.com>, Ajay Kaher <akaher@...are.com>,
 Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
 linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] eventfs: Create dentries and inodes at dir open

On Tue, 16 Jan 2024 09:55:15 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> [ html crud because I still don't have power or real Internet, just trying
> to keep an eye on things on my phone. Mailing lists removed to avoid
> bounces, please put them back in replies that don't have my horrible
> formatting ]
> 
> No.
> 
> Christ, you're making things worse again
> 
> The reason for the bug is that you're still messing with the dentries at
> readdir() time.

I may have deleted the comment, but the only reason I created the
inodes/destries is to keep the consistent inode number as it's created at
the time the inodes and dentries are.

Ah I did delete the comment, but it is still applicable :-/

       /*
-        * Need to create the dentries and inodes to have a consistent
-        * inode number.
+        * Need to make a struct eventfs_dent array, start by
+        * allocating enough for just the files, which is a fixed
+        * array. Then use realloc via add_entry() for the directories
+        * which is stored in a linked list.
         */

So if for some reason, user space did a getdents() and used the inode
numbers to match what it found, they would likely be the same.

> 
> Just stop it. Readdir should not be creating dentries. Readdir should not
> be even *looking* at dentries. You're not a virtual filesystem, the
> dentries are just caches for filename lookup, and have *nothing* to do with
> readdir.

Actually, it's not looking at them. I did it as a way to just have the
inode numbers be consistent.

	dents[cnt].ino = d->d_inode->i_ino;

Yes, that's the only reason I create them. The dentry/inode is not used for
anything outside of that.

> 
> So just iterate over your own internal data structures in readdir. DO NOT
> CREATE DENTRIES OR INODES FOR READDIR.
> 
> I've told you before, and I'll tell you again: either you are a real and
> proper virtual filesystem and you let the vfs layer manage *everything*,
> and the dentries and inodes are all you have. Or you are a *real*
> filesystem and you maintain your own data structures and the dentries and
> inodes are just the in-memory caches.
> 
> This "do both" is UNACCEPTABLE.
> 

The dentries were created for the inode numbers so that I did not need to
add them to meta data. They are generated at creation time.

I don't know how important inode numbers are. If they can all just have
random inode numbers and it doesn't break user space, where an inode number
will be one value at one read and another shortly after, is that going to
cause a problem?

Maybe I can just use a hash to generate he inode numbers from the name?
Hopefully there will be no collisions. Then I don't need the dentry
creation at all.

I do realize that if the dentries get freed due to reclaim and recreated,
their inode numbers will be different. But for a virtual file system, I
don't know how important having consistent inode numbers is.

-- Steve

Powered by blists - more mailing lists