Message-ID: <86ldud3hqe.fsf@davidreaver.com>
Date: Mon, 10 Feb 2025 08:08:25 -0800
From: David Reaver <me@...idreaver.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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

Greg Kroah-Hartman <gregkh@...uxfoundation.org> writes:

>
> 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.
>

No problem, and thank you for taking a look!

> 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.
>

Ack, I'll do something different in v2.

For my own education: what goes wrong with lifecycles with this embed?
Feel free to point me at a doc or something.

Also, Al and Greg, would wrapping a pointer be fine?

	struct debugfs_node {
		struct dentry *dentry;
	};

I was trying to do the simplest thing possible so the bulk of the change
was mechanical. Wrapping a pointer is slightly more complicated because
we have to deal with memory allocation, but it is still totally doable.

> 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.
>

Understood. I considered exposing the underlying dentry as a "dirty
backdoor" around the opaque wrapper, so I was trying to minimize it :)
I'm happy to undo some of these wrappers though, it will make the change
simpler.

> 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.
>

Yeah this is a great idea, and could even be done in a few patches
outside of this large migration patch series if necessary. I'll
investigate.

> 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?
>

I'll take another look at the relay code as well and see if I can remove
the pointers.

> 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?
>

I agree with this sentiment, and I discussed this in the cover letter a
bit under the section "#includes and #defines". The need for peppering
temporary #defines (for intermediate commits) and forward declarations
around is my least favorite part of this patch series.

I am indeed sure they are needed in most cases. I'll give a few examples
for both the temporary #defines Coccinelle adds and the forward
declarations that replace the #defines in the last commit:

1. If you remove the forward declaration (or the corresponding temporary
   #define in the Coccincelle commit) in
   drivers/gpu/drm/xe/xe_gsc_debugfs.h, you get this compilation error:

   drivers/gpu/drm/xe/xe_gsc_debugfs.h:12:57: error: ‘struct debugfs_node’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
      12 | void xe_gsc_debugfs_register(struct xe_gsc *gsc, struct debugfs_node *parent);

   gcc does not like implicitly-defined types inside of function
   arguments. As far as I can tell, we only get this error for function
   arguments; this is apparently okay for a top-level declaration, like:

   struct debugfs_node *my_root_node;


2. In the Coccinelle commit, if you remove the #define debugfs_node from
   include/linux/fault-inject.h, you get errors of this sort:

   mm/fail_page_alloc.c:55:13: error: assignment to ‘struct dentry *’ from incompatible pointer type ‘struct debugfs_node *’ [-Werror=incompatible-pointer-types]
      55 |         dir = fault_create_debugfs_attr("fail_page_alloc", NULL,
         |             ^

   Because the #define is not in scope, the compiler is assuming we are
   implicitly defining a new type.


The Coccinelle script adds a forward declaration of struct debugfs_node
wherever there was one for struct dentry. This is just a heuristic I
found that seemed to do the job and was easy to automate.

I originally did this whole patch series in reverse, where we
immediately make struct debugfs_node, migrate debugfs internals, and
migrate all users of the API, but that leads to one very large commit
and appeared harder to review to me. I went with this intermediate
#define idea so the commits could be split up and each commit would
compile, but I don't like the little bit of extra complexity it adds.

I'm open to any other migration ideas folks have! I'm not tied to these
two plans at all.

Thanks,
David Reaver

Powered by blists - more mailing lists