Re: [PATCH] eventfs: Have inodes have unique inode numbers

From: Steven Rostedt <rostedt@goodmis.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux Trace Devel <linux-trace-devel@vger.kernel.org>,
	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
Date: Sun, 28 Jan 2024 17:17:33 -0500	[thread overview]
Message-ID: <20240128171733.2ba41226@rorschach.local.home> (raw)
In-Reply-To: <CAHk-=whJ56_YdH-hqgAuV5WkS0r3Tq2CFX+AQGJXGxrihOLb_Q@mail.gmail.com>

On Sun, 28 Jan 2024 14:07:49 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sun, 28 Jan 2024 at 13:43, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > That's just wrong.
> >
> > Either you look things up under your own locks, in which case the SRCU
> > dance is unnecessary and pointless.
> >
> > Or you use refcounts.
> >
> > In which case SRCU is also unnecessary and pointless.  
> 
> So from what I can see, you actually protect almost everything with
> the eventfs_mutex, but the problem is that you then occasionally drop
> that mutex in the middle.
> 
> The one valid reason for dropping it is the readdir callback, which
> does need to write to user space memory.
> 
> But no, that's not a valid reason to use SRCU. It's a very *bad*
> reason to use SRCU.
> 
> The thing is, you can fix it two ways:
> 
>  - either refcount things properly, ie when you do that lookup under your lock:
> 
>         mutex_lock(&eventfs_mutex);
>         ei = READ_ONCE(ti->private);
>         if (ei && ei->is_freed)
>                 ei = NULL;
>         mutex_unlock(&eventfs_mutex);
> 
>    you just go "I now have a ref" to the ei, and you increment the
> refcount like you should, and then you dcrement it at the end when
> you're done.
> 
> Btw, what's with the READ_ONCE()? You have locking.
> 
> The other option is to simply re-lookup the ei when you re-get the
> eventfs_mutext anyway.
> 
> Either of those cases, and the SRCU is entirely pointless. It  really
> looks wrong, because you seem to take that eventfs_mutex everywhere
> anyway.

The original code just used the mutex, but then we were hitting
deadlocks because we used the mutex in the iput() logic. But this could
have been due to the readdir logic causing the deadlocks.

A lot of the design decisions were based on doing the dentry creation
in the readdir code. Now that it's no longer there, I could go back and
try taking the eventfs_mutex for the entirety of the lookup and see if
lockdep complains again about also using it in the iput logic.

Then yes, we can get rid of the SRCU as that was added as a way to get
out of that deadlock.

-- Steve