Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to create Notifier objects when handling requests #303

Merged
merged 2 commits into from
Oct 19, 2024

Conversation

colinmarc
Copy link
Contributor

This is often useful in poll implementations. It also comes into play in #300, because your initialization and setup (when you can call Session::notifier) is often separate from creating the session.

@cberner
Copy link
Owner

cberner commented Oct 14, 2024

This doesn't seem like the right place to create a Notifier to me. Do you have an example of how you're using it?

@colinmarc
Copy link
Contributor Author

Sure, for example here: https://github.com/colinmarc/southpaw/blob/main/src/fs.rs#L350-L354

Each open (which results in an async read) gets its own notifier, so I don't have to pass them around everywhere.

@cberner
Copy link
Owner

cberner commented Oct 16, 2024

I haven't actually used the notification interface myself, but my understanding is that a Notifier is a communication channel that can push messages to the kernel for updates to various inodes, dir entries...etc. Because it's not specific to the inode being opened in open() adding it to the Request seems off to me.

If it was available in Filesystem::init() would that solve your case? Looking at the code again, I'm not sure that Session::notifier() is the right place to add it.

@colinmarc
Copy link
Contributor Author

colinmarc commented Oct 17, 2024

If it was available in Filesystem::init() would that solve your case? Looking at the code again, I'm not sure that Session::notifier() is the right place to add it.

I guess so, but I'd just be wrapping it in an Arc and passing it around some more. IMO Notifiers should be cheap, cloneable (they aren't Clone right now) and readily available. And I think the semantics should match FUSE's as closely as possible.

If you'd like to provide a tighter interface, instead, we could create a scoped object, something like

struct PollHandle {
    pub ph: u64
    notifier: Notifier
}

impl PollHandle {
    pub fn notify(self) -> io::Result<()> {
        self.notifier.poll(self.ph)
    }
}

And pass it in instead of ph: u64 to the poll() call.

@cberner
Copy link
Owner

cberner commented Oct 18, 2024

That does seem better to me. I haven't used notification myself, but as I understand it there are two distinct use cases:

  1. inode and directory cache management. For this case the current interface seems more or less right to me, because you need a Notifier client that you can hold on to and push notifications to the kernel independent of any requests coming in. Although passing it into init() might be better than retrieving it from Session
  2. responding to poll() requests. In this case I agree the current interface isn't ideal. I like your suggestion of passing in a PollHandle that can be notified.

Are there any other cases I'm missing?

The thing I want to avoid is adding a notifier to all requests, because many requests don't need a notifier

This allows filesystems to respond directly to poll requests without
having saved a notifier at session creation time.
@colinmarc
Copy link
Contributor Author

Updated to add the PollHandle type.

Copy link
Owner

@cberner cberner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@cberner cberner merged commit bbba3db into cberner:master Oct 19, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants