Message-ID: <2025021048-thieving-failing-7831@gregkh>
Date: Mon, 10 Feb 2025 08:08:19 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: David Reaver <me@...idreaver.com>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Christian Brauner <brauner@...nel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, cocci@...ia.fr,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/6] debugfs: Replace dentry with an opaque handle in
 debugfs API

On Sun, Feb 09, 2025 at 09:20:20PM -0800, David Reaver wrote:
> Overview
> ========
> 
> This patch series replaces raw dentry pointers in the debugfs API with
> an opaque wrapper struct:
> 
> 	struct debugfs_node {
> 		struct dentry dentry;
> 	};
> 
> Intermediate commits rely on "#define debugfs_node dentry" to migrate
> debugfs users without breaking the build. The final commit introduces
> the struct and updates debugfs internals accordingly.
> 
> Why an RFC?
> ===========
> 
> This is a large change, and I expect a few iterations -- unless this
> entire approach is NACKed of course :) Any advice is appreciated, and
> I'm particularly looking for feedback on the following:
> 
> 1. This change touches over 1100 files. Is that okay? I've been told it
>    is because the patch series does "one thing", but it is a lot of
>    files to touch across many systems.
> 
> 2. The trickiest part of this migration is ensuring a declaration for
>    struct debugfs_node is in scope so we don't get errors that it is
>    being implicitly defined, especially as different kernel
>    configurations change which headers are transitively included. See
>    "#includes and #defines" below. I'm open to any other migration
>    strategies.
> 
> 3. This change is mostly automated with Coccinelle, but I'm really
>    contorting Coccinelle to replace dentry with debugfs_node in
>    different kinds of declarations. Any Coccinelle advice would be
>    appreciated.
> 
> Purpose/Background
> ==================
> 
> debugfs currently relies on dentry to represent its filesystem
> hierarchy, and its API directly exposes dentry pointers to users. This
> tight coupling makes it difficult to modify debugfs internals. A dentry
> and inode should exist only when needed, rather than being persistently
> tied to debugfs. Some kernel developers have proposed using an opaque
> handle for debugfs nodes instead of dentry pointers [1][2][3].
> 
> Replacing dentry with debugfs_node simplifies future migrations away
> from dentry. Additionally, a declaration with debugfs_node is more
> self-explanatory -- its purpose is immediately clear, unlike dentry,
> which requires further context to understand its role as a debugfs
> dentry.

First off, many thanks for attempting this, I didn't think it was ready
to even be attempted, so it's very nice to see this.

That being said, I agree with Al, we can't embed a dentry in a structure
like that as the lifecycles are going to get messy fast.

Also, your replacement of many of the dentry functions with wrappers
seems at bit odd, ideally you would just return a dentry from a call
like "debugfs_node_to_dentry()" and then let the caller do with it what
it wants to, that way you don't need to wrap everything.

And finally, I think that many of the places where you did have to
convert the code to save off a debugfs node instead of a dentry can be
removed entirely as a "lookup this file" can be used instead.  I was
waiting for more conversions of that logic, removing the need to store
anything in a driver/subsystem first, before attempting to get rid of
the returned dentry pointer.

As an example of this, why not look at removing almost all of those
pointers in the relay code?  Why is all of that being stored at all?

Oh, also, all of those forward declarations look really odd, something
feels wrong with needing that type of patch if we are doing things
right.  Are you sure it was needed?

thanks,

greg k-h

Powered by blists - more mailing lists