Message-ID: <CAGSQo00Pj0qF90712K7xACNEvr2e0q=98b8-0VUcXLD5V+oDhg@mail.gmail.com>
Date: Wed, 14 May 2025 15:08:21 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Danilo Krummrich <dakr@...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:54 AM Benno Lossin <lossin@...nel.org> wrote:
>
> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich 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.
> >
> > 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.
> >
> > 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.
>
> Yeah that's whats normally done in Rust anyways. But I think that
> lifetimes bite us in this case:
>
>     let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
>
>     let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
>     // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
>
>     /* code for creating the file etc */
>
>     Ok(Self { _debugfs: debugfs, _sub: sub })
>     // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
>
> This code won't compile, since we can't store the "root" dir in the same
> struct that we want to store the subdir, because the subdir borrows from
> the root dir.
>
> Essentially this would require self-referential structs like the
> `ouroboros` crate [1] from user-space Rust. We should rather have the
> `.keep()` function in the API than use self-referential structs.
>
> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html

Yes, this is also an issue when it comes to attaching data to debugfs
if you look at the WIP patchset [1]. If we do end up deciding to try
to do self-referential structs for users (I'd rather not, but I
understand I don't have the deciding vote here), I'd strongly suggest
considering self_cell [2] as a model instead of ouroboros - there are
structural UB issues [3] with ouroboros that are unlikely to be
resolved, and the author of ouroboros encourages use of self_cell
instead [4]

[1]: https://lore.kernel.org/all/20250506-debugfs-rust-attach-v2-0-c6f88be3890a@google.com/
[2]: https://docs.rs/self_cell/latest/self_cell/
[3]: https://github.com/rust-lang/miri/issues/2844#issuecomment-1510577943
[4]: https://github.com/someguynamedjosh/ouroboros/issues/88

>
> Another problem that only affects complicated debugfs structures is that
> you would have to store all subdirs & files somewhere. If the structure
> is dynamic and changes over the lifetime of the driver, then you'll need
> a `Vec` or store the dirs in `Arc` or similar, leading to extra
> allocations.

Yep, this is part of why I tried to follow the "Build it, then hold
the needed handles to keep it alive" approach rather than keeping the
entire structure around.

>
> > 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.
>
> I agree with this, I want that `ManuallyDrop` & `forget` are only used
> rarely mostly for low-level operations.

Sure, I'll re-instate `Dir::keep` and `File::keep` as aliases for
`ManuallyDrop::new` in the next version. TBD whether we add the bit
that Danilo suggested to make `Dir::keep` not exist for root-level
directories.

>
> ---
> Cheers,
> Benno

Powered by blists - more mailing lists