Message ID | 20250210052039.144513-1-me@davidreaver.com (mailing list archive) |
---|---|
Headers | show |
Series | debugfs: Replace dentry with an opaque handle in debugfs API | expand |
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: Do not embed struct dentry into anything else. Do not take over its lifetime rules. For the record: Anything of that sort is going to be vetoed.
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
Greg Kroah-Hartman <gregkh@linuxfoundation.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
On Mon, 10 Feb 2025 08:08:25 -0800 David Reaver <me@davidreaver.com> wrote: > Greg Kroah-Hartman <gregkh@linuxfoundation.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; > }; No it will not be fine. You should not be using dentry at all. I thought this was going to convert debugfs over to kernfs. The debugfs_node should be using kernfs and completely eliminate the use of 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. What caller should ever touch a dentry? What I got from my "conversation" with Linus, is that dentry is an internal caching descriptor of the VFS layer, and should only be used by the VFS layer. Nothing outside of VFS should ever need a dentry. -- Steve
On Mon, Feb 10, 2025 at 11:53:13AM -0500, Steven Rostedt wrote: > No it will not be fine. You should not be using dentry at all. I thought > this was going to convert debugfs over to kernfs. The debugfs_node should > be using kernfs and completely eliminate the use of dentry. I disagree, actually - kernfs is an awful model for anything, sysfs included...
On Mon, 10 Feb 2025 17:00:16 +0000 Al Viro <viro@zeniv.linux.org.uk> wrote: > On Mon, Feb 10, 2025 at 11:53:13AM -0500, Steven Rostedt wrote: > > > No it will not be fine. You should not be using dentry at all. I thought > > this was going to convert debugfs over to kernfs. The debugfs_node should > > be using kernfs and completely eliminate the use of dentry. > > I disagree, actually - kernfs is an awful model for anything, sysfs included... Then what would you suggest? It's the only generic system that is appropriate for control features, where the underlining "files" are actually functions to modify or query information from the kernel. The entire VFS layer is designed for efficient management of some kind of storage device, where the only interaction with the storage device is through VFS. For pseudo file systems like debugfs, sysfs and tracefs, the underlining "storage" is the kernel itself, where we need a way for the "storage" part to work with the kernel. The VFS layer doesn't give that, which is why debugfs and tracefs used dentry as that interface, as the dentry does represent the underlining storage. From what I understand (and Christian can correct me), is that kernfs was created to be that interface of the "storage" connecting back to the kernel. Where the VFS layer deals with the user accessing the file system (read, write, mkdir, etc) but kernfs is the "data storage" that attaches those call back to the kernel to retrieve kernel information or even modify the kernel behavior. -- Steve
On Mon, 10 Feb 2025 12:12:46 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > From what I understand (and Christian can correct me), is that kernfs was > created to be that interface of the "storage" connecting back to the > kernel. Where the VFS layer deals with the user accessing the file system > (read, write, mkdir, etc) but kernfs is the "data storage" that attaches > those call back to the kernel to retrieve kernel information or even modify > the kernel behavior. To further expand on this, what a pseudo file system would need, is a way to describe the layout of the file system, and to have the file_operations defined for each file. And possibly, a way to control what directories are made and what is in the directory when a "mkdir" is performed (as mkdir in pseudo file systems seldom create an empty directory). It would also need to handle the "rmdir" on those directories. Would also need to be able to store permissions too. Debugfs did this with the dentry as the descriptor. But that dentry also requires a static inode behind it and the memory used for all this was really being wasted. Having a separate descriptor that acts like a storage device and saves the file ops is basically all that is needed. It can act like a real file system where no dentry or inode is created until the file system is accessed by the user. And then, this descriptor can be attached to the dentry and have an inode created for it. But after the file is no longer referenced, the dentry and inode can be freed. This is what is lacking with debugfs and tracefs at the moment. I did carve out an "eventfs" from tracefs to do the above, and it saved over 20Megs in memory when it wasn't being accessed. -- Steve
Steven Rostedt <rostedt@goodmis.org> writes: > > No it will not be fine. You should not be using dentry at all. I thought > this was going to convert debugfs over to kernfs. The debugfs_node should > be using kernfs and completely eliminate the use of dentry. > > <snip> > > What caller should ever touch a dentry? What I got from my "conversation" > with Linus, is that dentry is an internal caching descriptor of the VFS > layer, and should only be used by the VFS layer. Nothing outside of VFS > should ever need a dentry. > > -- Steve I agree that just wrapping a dentry shouldn't be the final state for debugfs_node, but this patch series is _only_ trying to introduce debugfs_node as an opaque wrapper/handle. It isn't clear to me that there is consensus on even using kernfs for debugfs. Even if there was consensus, a full conversion to kernfs would take 10x as much code and be extremely difficult to automate. For example, using kernfs would require migrating all of the debugfs users' file_operations to use the kernfs equivalent. I figure any change away from persistent dentry handles for debugfs requires introducing something akin to debugfs_node, so we could get that out of the way first. Thanks, David Reaver