Message-ID: <CAGSQo0204_Dz65G33pAN3_iY=ejPXJRdAiB4ioM-nvMkAh0YXQ@mail.gmail.com>
Date: Wed, 14 May 2025 14:55:02 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Benno Lossin <lossin@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Sami Tolvanen <samitolvanen@...gle.com>, Timur Tabi <ttabi@...dia.com>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 4/4] rust: samples: Add debugfs sample

On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > > +impl kernel::Module for RustDebugFs {
> > > +    fn init(_this: &'static ThisModule) -> Result<Self> {
> > > +        // Create a debugfs directory in the root of the filesystem called "sample_debugfs".
> > > +        let debugfs = Dir::new(c_str!("sample_debugfs"));
> > > +
> > > +        {
> > > +            // Create a subdirectory, so "sample_debugfs/subdir" now exists.
> > > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> > > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> >
> > I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > support of the wrapped type. But in this case, `Dir` is sometimes
> > intended to not be dropped, so I'd rather have a `.keep()` function I
> > saw mentioned somewhere.
>
> I agree, if we really want to "officially" support to forget() (sub-)directories
> and files in order to rely on the recursive cleanup of the "root" directory, it
> should be covered explicitly by the API. I.e. (sub-)directories and files should
> have some kind of keep() and / or forget() method, to make it clear that this is
> supported and by design and won't lead to any leaks.

OK, I can do this with `.keep()` just being equivalent to
`ManuallyDrop::new`. Since we now only have "by default, things are
dropped", rather than two states, we shouldn't need a `.destroy()`
method.

>
> Consequently, this would mean that we'd need something like your proposed const
> generic on the Dir type, such that keep() / forget() cannot be called on the
> "root" directory.

Just to ensure I understand correctly, your opinion is that on
balance, additional complexity in the API types are worth making it
less ergonomic to suppress drop on a root directory, keeping in mind
that they still *can* suppress drop on that directory. You don't want
the extra API variants to do anything else special other than change
the ergonomics of `.keep()` (e.g. don't have subdirectories be default
pre-`.keep()`'d).

>
> However, I really think we should keep the code as it is in this version and
> just don't provide an example that utilizes ManuallyDrop and forget().
>
> I don't see how the idea of "manually dropping" (sub-)directories and files
> provides any real value compared to just storing their instance in a driver
> structure as long as they should stay alive, which is much more intuitive
> anyways.

We can't easily do this, because dropping a root directory recursively
drops everything underneath it. This means that if I have

foo/
  - bar/
  - baz/

Then my directory handle for `bar` have to be guaranteed to outlive my
directory handle for `foo` so that I know it's didn't get deleted
under me. This is why they have a borrow onto their parent directory.
This borrow means that you can't (without `unsafe`, or something like
`yoke`) keep handles to `foo` and `bar` in the same struct.

I could arguably make the subdir handle creator *take ownership* of
the root handle, which would mitigate this issue, but if we want to
allow creation of arbitrarily shaped trees, we'd either end up rapidly
reproducing something like my builder interface from the follow-up
series which I understand folks aren't enthused about, or we'd end up
with Rust having a mirror struct of some kind, e.g.

```
struct Tree {
  name: CString,
  // Order is load bearing, any children must be dropped before their parent
  children: KVec<Tree>
  root: Dir,
}

impl Tree {
  fn subdir(&mut self, name: &CStr) -> &mut Tree {
    if let Some(x) = children.iter_mut().find(|tree| tree.name == name) {
      x
    } else {
      // This is an internal function, similar to Dir::create
      // SAFETY: The structure promises all children will be released
before the parent directory is destroyed.
      self.children.push(unsafe { Tree::sys_subdir(self.dir, name) })
      self.children.last_mut().unwrap()
    }
}
```

This structure has the downside that we now need to track the file
structure in Rust too, not just in the main kernel. This gets more
complex when we add Files, but follows the same approximate structure.

If we don't track the filenames, you have no way to reference
subdirectories again later. If we do track them, we're reproducing
dentry structure less efficiently in Rust. Additionally, because this
needs to track filenames, this now means that `CONFIG_DEBUG_FS=n` is
no longer free.

>
> It either just adds complexity to the API (due to the need to distingish between
> the "root" directory and sub-directories) or makes the API error prone by
> providing a *valid looking* option to users to leak the "root" directory.
>
> - Danilo

Powered by blists - more mailing lists