Message-ID: <aC2HacSU7j5d9bzu@pollux>
Date: Wed, 21 May 2025 09:57:29 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Benno Lossin <lossin@...nel.org>,
	Matthew Maurer <mmaurer@...gle.com>,
	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>,
	Trevor Gross <tmgross@...ch.edu>,
	"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 Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@...nel.org> wrote:
> > > >> 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.
> > > 
> > > Is there no refcount that we can use instead of borrowing? I guess not,
> > > since one can call `debugfs_remove`. What about a refcount on the rust
> > > side? or is debugfs not used for "debugging" and needs to have the
> > > performance of no refcount?
> > 
> > debugfs should never have any performance issues (i.e. you don't use it
> > for performant things.)
> > 
> > So refcount away!  That should never be an issue.
> 
> What I imagine would be the ideal API for Rust is the following:
> 
> * For each file / dir you create, you get a Rust object that owns it.
> 
> * When you destroy one of these Rust objects, it disappears from the
>   file system. I.e., dropping a directory removes things recursively.
> 
> * If you remove a directory before the removing objects inside it, then
>   the Rust objects become "ghost" objects that are still usable, but not
>   visible in the file system anywhere. I.e. calling methods on them
>   succeed but are no-ops.

If we would want to keep an entry alive as long as there are more leaves, we'd
obviously need to reference count things.

But what do we need reference counting for with this logic? Shouldn't this be
also possible without?

> * Possibly we have a way to drop a Rust object without removing it from
>   the file system. In this case, it can never be accessed from Rust
>   again, and the only way to remove it is to drop its parent directory.

I'm not sure about this one.

IIUC, this basically brings back the "keep() logic", which I still think is a
footgun and should be avoided.

Also, we only needed this, since due to the borrowing design we couldn't store
parent and child nodes in the same structure. With reference counting (or the
logic above) this goes away.

I wrote the following in a previous conversation [1].

--

What I see more likely to happen is a situation where the "root" directory
(almost) lives forever, and hence subsequent calls, such as

	root.subdir("foo").keep()

effectively are leaks.

One specific example for that would be usb_debug_root, which is created in the
module scope of usb-common and is used by USB host / gadget / phy drivers.

The same is true for other cases where the debugfs "root" is created in the
module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively leak memory everytime a
device is unplugged (or unbound in general).

--

[1] https://lore.kernel.org/lkml/aCR9cD7OcSefeaUm@pollux/

> This way, you can drop foo/ before dropping bar/ and baz/ without that
> having to be unsafe.
> 
> Whether that's best implemented by calling dget/dput on the dentry or by
> having Rust keep track of a separate Rust-only refcount, I don't know.
> But I think this is the API we want.
> 
> Thoughts?
> 
> Alice

Powered by blists - more mailing lists