Message-ID: <aBujjJN9nz6Ib_lt@polis>
Date: Wed, 7 May 2025 20:16:44 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Matthew Maurer <mmaurer@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...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>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Timur Tabi <ttabi@...dia.com>, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH WIP 1/2] rust: debugfs: Add interface to build debugfs
 off pinned objects

On Mon, May 05, 2025 at 10:32:39AM -0700, Matthew Maurer wrote:
> On Sun, May 4, 2025 at 9:58 AM Danilo Krummrich <dakr@...nel.org> wrote:
> >
> > On Sat, May 03, 2025 at 12:43:59AM +0000, Matthew Maurer wrote:
> > > +/// A DebugFS directory combined with a backing store for data to implement it.
> > > +#[pin_data]
> > > +pub struct Values<T> {
> > > +    dir: Dir<'static, false>,
> > > +    // The order here is load-bearing - `dir` must be dropped before `backing`, as files under
> > > +    // `dir` may point into `backing`.
> > > +    #[pin]
> > > +    backing: T,
> > > +    // Since the files present under our directory may point into `backing`, we are `!Unpin`.
> > > +    #[pin]
> > > +    _pin: PhantomPinned,
> > > +}
> >
> > This only ever allows attaching data to the root directory, correct? What if I
> > want to remove (or replace) a file or a subdir? Then I'd be left with the data
> > for this specific file (or subdir) until the root is finally removed.
> 
> The intended way to deal with this is that your debugfs root has
> structures or handles that let you compute what all your debugfs files
> should see, not necessarily fully populated if you want something
> dynamic. For example, one of your entries could be
> `Arc<Mutex<Vec<Record>>>`, and this could get updated elsewhere
> without thinking about DebugFS - you just need to know the shape of
> all the handles DebugFS will need.

Well sure, but what if we can't initialize them (yet) at the time we create the
debugfs root?

Also, what if the data that should be exposed through a file in debugfs is not
supposed to live for the full lifetime of the debugfs root (which might be
forever)?

> It'd be pretty easy for me to relax this to allow attachments to
> subtrees. I'd just need to do `Values<'a, T>` and `Dir<'a,  false>`.
> It'd also make the argument for the safety of the builder interface
> slightly more complicated (things that statically outlive the lifetime
> of the dir would also be usable in the builder, which is safe, but
> trickier to argue for).
> 
> You wouldn't be able to do nested attachments, but you could do
> attachments starting with any subtree. So for example, you could do:
> ```
> let root = Dir::new(c_str!("foo"));
> let foo = root.subdir(c_str!("bar")).owning();
> let bar = root.subdir(c_str!("baz")).owning();
> let foo_values = Values::attach(foo_data, foo);
> let bar_values = Values::attach(bar_data, foo);
> ```
> 
> The tricky business here (and why I didn't do it in my initial draft)
> is that because `foo_values` and `bar_values` are restricted to live
> no longer than `root` (since they will be invalidated if `root` is
> dropped), it is hard to store these objects in a module context or
> similar object, which would make it tricky to use. Attaching to a root
> directory on the other hand is not tricky, because their lifecycle
> isn't dependent on some other object.
> 
> A legal way of using this kind of scoped interface (but not
> immediately obvious to me how to design the safe interface to let
> users build it) might look like
> 
> ```
> struct MyDebugFsInfo<T> {
>   // RawValues is a fictitious type that would be like `Values`, but
> with the actual lifetime parameter erased.
>   subdirs: Vec<RawValues<T>>,
>   // Order matters, root must be released last because earlier values
> are borrows
>   root: Dir<'static, false>,
> }
> 
> impl<T> MyDebugFsInfo<T> {
>   fn new_subdir(&mut self, name: &CStr, value: T) {
>     let subdir = self.root.subdir(name);
>     // SAFETY: We don't allow the root to be destroyed before our structure, so
>     let val = unsafe { RawValues::attach(value, self.root.subdir(name)) };
>     self.subdirs.push(val)
>   }
>   fn get<'a>(&'a self, idx: usize) -> &Values<'a, T> {
>      let x = self.subdirs[idx];
>      // SAFETY: We know all our subdirs are children of the root we
> own, so if we have a read-only reference to ourselves, that's an upper
> bound on how long the value struct can live
>      unsafe { RawValues::ascribe(x) }
>   }
> }
> ```

Yes, I'm aware of this possibility and its complexity. That's why I think per
file would suit better.

> If I wanted to do better than this in terms of ergonomics, I think I'd
> need some kind of refcounted wrapper where holding a subdir prevents
> the parent dir from being cleaned up, which I could add, but would be
> a Rust abstraction layer on top of the C one.

I don't think something like that is desirable.

> >
> > It would also require Option<V> (where V is the type of a field in T), if I
> > don't have an instance of V yet, when the root directory is created.
> 
> `Option<V>` won't actually save you - once you create a `Values<T>`,
> you are intentionally unable to mutate `T` anymore, because it may be
> read at any point in time after that by someone reading a debugfs
> file, so you can't do a potentially racing mutation. If you wanted a
> value to be attached to a debugfs, but didn't have one, you'd need to
> do `Mutex<Option<V>>`.

Sure, or some other kind of synchronization. I left that aside, since it's not
relevant for the point I wanted to make. Which is that with the per root data
approach we'd need to work with Option types when the data isn't yet known when
the root directory is created and if we want to be able to free the data before
the root directory is removed (which might be never).

> >
> > I think we should store the data per file, rather than per root directory.
> 
> This doesn't match what at least seems to be a common usage of
> debugfs, where a debugfs directory is used to represent an object with
> each file in the directory representing one of the properties on the
> object.

Why not treat File as container, i.e. make the property of an object a File<T>
with T being the property?

> This also means that we need to store N handles in a Rust driver (one
> per live file) compared to 1 handle in a C driver.

I can't follow what you mean here. Files in a C driver do need a handle to the
data they expose too, no?

> 
> ===
> 
> I do think that the normal idiomatic way of doing this in Rust would
> be to attach refcounted handles (e.g. similar to `ForeignOwnable`) to
> inodes, and when the inode is destroyed, have that refcount be
> deleted, but we don't currently have any `release` implementation for
> a DebugFS inode afaict, so everything has to be externally scoped via
> this attached pinned data setup I've been producing.

Why do you think that this would be "the normal idimatic way" of doing this?

Does this imply that you think tying the lifetime of the object exposing a value
to the value itself wouldn't be idiomatic for Rust?

Powered by blists - more mailing lists