Closed sbauersfeld closed 6 months ago
I definitely like that this PR is a lot shorter compared to #144 :)
The fact that these callbacks will block recycling of the inMessage and outMessage buffers is not immediately obvious and I don't yet know of a good way to add a test to guarantee this behavior.
Not sure I follow. Why do you want to guarantee that the callback blocks recycling?
Could you provide an example implementation of such a callback, too? It’s hard to evaluate code in a vacuum, and while the hook mechanism seems fine on a technical level, I think it would be good to expand in the documentation regarding how users would typically use this mechanism, and maybe also what users should/shouldn’t do in terms of safety.
Thanks
Why do you want to guarantee that the callback blocks recycling?
We want the filesystem implementation to be able to control when these messages are recycled so that we can continue to use the WriteFile data after the kernel response but before the buffer is recycled.
For WriteFile, this lets us continue to use the data from the message even after the kernel reply. An example Callback implementation for WriteFile would be:
func (fs *myFileSystem) WriteFile(ctx context.Context, op *fuseops.WriteFileOp) (err error) {
go func() {
// transfer op.Data in background
fs.mu.Lock()
fs.uploadComplete = true
fs.mu.Unlock()
fs.cond.Broadcast()
}()
op.Callback = func() {
// op.Data is valid until after this function returns, at which point op.Data will be recycled for future operations
fs.mu.Lock()
defer fs.mu.Unlock()
for fs.uploadComplete == false {
fs.cond.Wait()
}
}
}
For ReadFile, we don't need to block the recycling of the message buffer, but we need to get the callback after the kernel reply. An example Callback implementation for ReadFile would be:
func (fs *myFileSystem) ReadFile(ctx context.Context, op *fuseops.ReadFileOp) (err error) {
op.Data = append(op.Data, fs.getData(op))
op.Callback = func() {
// the fs data buffers can be recycled when this function is called, since the kernel response is done
fs.recycleDataBuffer()
}
}
Could you provide an example implementation of such a callback, too?
I can inline the above examples if you'd like! And also add some more documentation
Hi @stapelberg any more comments on this?
Hi @stapelberg have you had a chance to take a look at my most recent comments? https://github.com/jacobsa/fuse/pull/147#issuecomment-1660807389
Sorry for letting this linger for so long. The last few months have not allowed me to spend much time on open source maintenance.
The explanations you provided in the PR are satisfying, so I merged the change.
Thank you for the contribution!
Thank you for the review @stapelberg. I will make good use of this change soon!
An alternative to https://github.com/jacobsa/fuse/pull/144
One downside to the MessageProvider introduced in https://github.com/jacobsa/fuse/pull/144 is that the FUSE server will need to be able to link a FUSE operation (ReadFile, WriteFile, etc) with the calls to
putInMessage
andputOutMessage
so that the FUSE server knows which Write / Read has completed. Presumably, this forces the FUSE server to maintain some map with the FuseId as the key.In contrast, the approach in this PR allows the FUSE server to specify the Callback function directly in the ReadFile or WriteFile operation, so the FUSE server does not need to maintain a map of FuseIds.
Like the other PR, this change allows us to both get notified when a response is sent to the kernel and also block the recycling of the inMessage and outMessage buffers. This change is slightly preferable for us because it allows us to skip the map with FuseIds. This also seems to be a less intrusive change than the other PR (for example, we don't need to expose the buffer package).
However, there are a couple of downsides to this change: