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

Document ownership and lifetime of buffers passed to callbacks #725

Open
mrvn opened this issue Jan 27, 2023 · 7 comments
Open

Document ownership and lifetime of buffers passed to callbacks #725

mrvn opened this issue Jan 27, 2023 · 7 comments

Comments

@mrvn
Copy link
Contributor

mrvn commented Jan 27, 2023

The struct fuse_lowlevel_ops has callbacks passing buffers to the callback:

  1. write
  2. ioctl
  3. write_buf
  4. retrieve_reply

The ownership and lifetime of these buffers is not documented.

My understanding is that these buffers are references into the const struct fuse_buf *buf passed to fuse_session_process_buf and as such ownership remains with libfuse and the lifetime of the buffer ends with the call to fuse_reply_* for the relevant request.

This should be documented to avoid confusion and data corruption for filesystems that want to implement caching or async writeback. For that use case is it best to use the splice interface and read the data into buffers allocated by the filesystem instead of having to memcpy the fuse buffers into filesystem buffers before the fuse_reply_* call?

Note: The same issue applies to the high level interface.

@bsbernd
Copy link
Collaborator

bsbernd commented Jan 27, 2023

No it does not end with fuse_reply_*. Buffer belong to struct fuse_worker::fuse_buf - i.e. this is a per thread buffer. If you do not pass memory to another thread - no issue. But if you handled it async, pass the req to another thread and let the fuse work threads continue - memory might be overwritten or freed. See fuse_do_work() in fuse_loop_mt.c.
I'm working on a uring interface, where buffers are associated with the ring - buffers belong to the req then and are only released with the req by fuse_reply_*. This involves severe kernel changes - I cannot make any predictions when this will be upstream available.

@mrvn
Copy link
Contributor Author

mrvn commented Jan 27, 2023

Note: For the ocaml interface I'm writing my own request loop. Threads have to be registered with the ocaml GC and that doesn't work well with fuse creating threads internally on the fly. So allocation and reuse of the fuse_buf could be handled differently there. Might even have them GC controlled then.

That said would it be possible for the buffers to be detachable from the request? For caching work the FS might want to keep buffers longer than the request.

@bsbernd
Copy link
Collaborator

bsbernd commented Jan 27, 2023

Check out libfuse-3.12 and above - it still creates threads on the fly, but typically does not destruct them anymore. You do need to switch to the 3.12 API, to get that.

In my uring branch there are two options - either one single ring or one ring per core, each ring has a thread assigned, you can further distribute by submitting with events to threads or coroutines...

Regarding long life buffers - might be not difficult with the traditional /dev/fuse posix IO, but is much harder with the ring. Why don't you just use the page cache, if you need to keep the buffer?

@trapexit
Copy link
Collaborator

trapexit commented Jan 27, 2023

The page cache can slow down max throughput in some usecases but this ownership/lifecycle of buffers is also a issue in other usecases. Imagine one where you'd like to process the writes out of band without another copying of the buffer. I was considering if I could get away with completely async processing of writes as a way to reduce latency between write calls back to the client. However, in my case I've almost entirely rewritten libfuse2 for my purposes so was planning on adding more control over ownership.

@bsbernd
Copy link
Collaborator

bsbernd commented Jan 27, 2023

speeding up page cache is another topic - kernel work, I need to check with recent kernels. As I said, with the ring patches buffers are assigned per ring-req - a large ring might be sufficient for a small cache (uring allows max 32768 entries, I think). Anyway @mrvn please submit a patch to document current behavior.

@trapexit
Copy link
Collaborator

I was only referring to the page cache because you asked "why not just use the page cache"? The perf hit could be a reason.

@Nikratio Nikratio changed the title Ownership and lifetime of buffers is unclear Document ownership and lifetime of buffers passed to callbacks Feb 17, 2023
@Nikratio
Copy link
Contributor

Coming back to the original subject of the bug - pull requests to more explicitly document the current semantics are welcome :-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants