Add notification messages framework #35

inevity · 2017-11-03T06:18:26Z

Now we only impl the NotifyInvalInode,NotifyInvalEntry,
NotifyDelete.

Notify funcs allow the filesystem to invalidate VFS caches.

The statement that 'In a networked fuse file-system, the local
fuse instance may receive changes from a remote system that do not
pass through the local kernel vfs.
When those changes are made the local fuse instance must notify the
local kernel of the change so as to keep the local kernel's cache
in sync. This is accomplished via notify delete or notify
invalidate.
In other words, these notifications only apply if fuse file-system
makes changes to the file-system that are not initiated by the
local kernel: changes initiated by the local kernel must not
involve notify delete or notify invalidate (will deadlock), and
are not required.' is quoted from Jhon Muir.

About fuse notify func history ,please ref link
http://lkml.iu.edu/hypermail/linux/kernel/0906.0/01120.html

jacobsa

Thanks for the context, I understand what this is for now I think.

jacobsa · 2017-11-04T09:47:32Z

connection.go

@@ -241,7 +241,7 @@ func (c *Connection) beginOp(
 	// should not record any state keyed on their ID.
 	//
 	// Cf. https://github.com/osxfuse/osxfuse/issues/208
-	if opCode != fusekernel.OpForget {
+	if opCode != fusekernel.OpForget && opCode < 100 {


Why this change?

For notify ops ,it is issued by the userspace filesystem to
fuse kernel. The ops's kernel opCode is 0 in kernel' viewpoint or kernel not
care whether what the opcode is .But the ops have the notifycode from 1
to 6 now. For adapt the notfiy ops to the BeginOP ,we fake an opcode as
notfiycode + 100. Now opcode range of ops from kernel including origin
from userspace and just from kernel is < 100. The notify ops's opcode
range is [101-106].
Then we can process all ops consistently.
Cf. func (c *Connection) SetNotifyContext(op interface{})
(context.Context, error) {

see pr code.

jacobsa · 2017-11-04T09:47:51Z

connection.go

@@ -411,6 +411,36 @@ func (c *Connection) ReadOp() (ctx context.Context, op interface{}, err error) {
 	}
 }

+func (c *Connection) SetNotifyContext(op interface{}) (context.Context, error) {
+


Please remove the blank line here. (Please run gofmt if you haven't.)

jacobsa · 2017-11-04T09:48:05Z

connection.go

@@ -411,6 +411,36 @@ func (c *Connection) ReadOp() (ctx context.Context, op interface{}, err error) {
 	}
 }

+func (c *Connection) SetNotifyContext(op interface{}) (context.Context, error) {


Please add documentation for all new exported symbols.

jacobsa · 2017-11-04T09:48:43Z

fuseops/ops.go

@@ -845,3 +845,24 @@ type SetXattrOp struct {
 	// simply replace the value if the attribute exists.
 	Flags uint32
 }
+type NotifyPollOp struct {


It's especially important to have good documentation for the new Op structs. See the other ops in this package for an example of the kind of thing I'm looking for.

inevity · 2017-11-05T11:12:24Z

Have added documentation.

fishpadak · 2018-11-01T08:29:25Z

Hello :D Any progress on this PR?

inevity · 2019-01-13T11:43:24Z

Hello :D Any progress on this PR?

Dont know why jacobsa not to merge or give some advice.

stapelberg · 2019-09-18T14:40:55Z

Would you mind rebasing this on the current master branch please? We fixed the travis config in the meantime.

Now we only impl the NotifyInvalInode,NotifyInvalEntry, NotifyDelete. Notify funcs allow the filesystem to invalidate VFS caches. The statement that 'In a networked fuse file-system, the local fuse instance may receive changes from a remote system that do not pass through the local kernel vfs. When those changes are made the local fuse instance must notify the local kernel of the change so as to keep the local kernel's cache in sync. This is accomplished via notify delete or notify invalidate. In other words, these notifications only apply if fuse file-system makes changes to the file-system that are not initiated by the local kernel: changes initiated by the local kernel must not involve notify delete or notify invalidate (will deadlock), and are not required.' is quoted from Jhon Muir. About fuse notify func history ,please ref link http://lkml.iu.edu/hypermail/linux/kernel/0906.0/01120.html .

inevity · 2019-09-20T04:32:53Z

Just have rebased on master branch.

stapelberg

Thanks! This looks like a good feature addition to have (I should probably try using this once it landed).

Aside from the question about the context, I have a few minor comments, but can also just make the changes if that’s okay with you?

stapelberg · 2019-09-21T09:11:29Z

connection.go

+
+	// we should get outmsg from context
+	var key interface{} = contextKey
+	foo := ctx.Value(key)


Can you explain why we need to pull this value out of the context instead of just passing it as a parameter to NotifyKernel?

Just do like the reply func. Of course you can also just pass the value of the context as a parameter to NotifyKernel.

Thanks. The reply mechanism needs to use a context because it passes a value (

fuse/connection.go

Line 406 in 659cc51

ctx = context.WithValue(ctx, contextKey, opState{inMsg, outMsg, op})

) across API boundaries (to the file system implementation and back).

This is not the case here AIUI, so please change the function signature to not take a context, but just take the opState parameter directly.

I rethink this case, even the notify func is diff with the normal op which need reply function. But the context use in notify func is same as the reply func. Just the context is created by our func (s *fileSystemServer) Invalidateentry/inode. And now we call unity all notify op and the normal op.
For the correct context use, we should alway use the context for notify case. see func (s *fileSystemServer) InvalidateInode for opsInFlight.Done.

Maybe i am not understandering the issue on detail.

I rethink this case, even the notify func is diff with the normal op which need reply function. But the context use in notify func is same as the reply func. Just the context is created by our func (s *fileSystemServer) Invalidateentry/inode. And now we call unity all notify op and the normal op.

If I understand correctly, you’re saying having both functions (NotifyKernel and Reply) take arguments via a context is good because they are similar, yes?

While similarity is good, I would argue that in this case, it’s clearer if a context is not used unless absolutely necessary. Since we’re creating a context but then immediately call the function (instead of passing the context somewhere else), I don’t think we need to use a context.

Contexts are used (aside from deadlines and cancellation) to propagate data across API boundaries. In this case, we don’t need to cross API boundaries, so we shouldn’t use a context if we can use a simple variable instead.

inevity · 2019-09-27T09:29:25Z

Can you give some comments about the context commit?
I use context in SetNotifyContext,so this func SetNotifyContext have nothing to modify, only one bug to fix.

Then change NotifyKernel(opstate opState) as this signature.

And the funcs which calling notifykernel in fuseutil/file_system.go Where have some dup code, you can give some detail advice.

inevity · 2019-09-28T02:47:32Z

Wait...

stapelberg

Are you still interested in working on this PR? It’s been open for a while. Perhaps the author of issue #64 would want to carry on?

Here are my design recommendations for this pull request:

Add the InvalidateEntry, InvalidateInode and NotifyDelete functions to the fuse.Connection type.
Directly fill in the corresponding data structure (e.g. fusekernel.NotifyCodeInvalEntryOut) and call c.writeMessage

I thought a bit about whether we need additional locking with a mutex, but I think the individual syscall.Write calls that fuse.Connection.writeMessage makes should already be atomic? Sanity-checks welcome.

stapelberg · 2020-01-27T18:48:59Z

mounted_file_system.go

@@ -24,6 +24,7 @@ type MountedFileSystem struct {
 	// The result to return from Join. Not valid until the channel is closed.
 	joinStatus          error
 	joinStatusAvailable chan struct{}
+	Conn                interface{}


Why does the Conn field use type interface{} instead of *fuse.Connection?

riking · 2020-07-05T06:45:38Z

fuseutil/file_system.go

+	ctx, _ := c.SetNotifyContext(op)
+	var key interface{} = contextKey
+        foo := ctx.Value(key)
+        opstate, ok := foo.(opState)


The formatting here is a mess.

riking · 2020-07-05T06:46:17Z

mount.go

+	InvalidateInode(fuseops.InodeID, int64, int64) error
+	NotifyDelete(fuseops.InodeID, fuseops.InodeID, string) error
+	SetMfs(*MountedFileSystem)
+	GetMfs() *MountedFileSystem
 }


This is a lot of extra methods to be adding to the very simple Server interface, which could plausibly have been proxied in client code..

inevity mentioned this pull request Nov 3, 2017

Notifyfunc impl and add mount subtype #29

Closed

inevity force-pushed the notifyfuncv1 branch from b8928af to c796534 Compare November 3, 2017 06:38

jacobsa reviewed Nov 4, 2017

View reviewed changes

inevity force-pushed the notifyfuncv1 branch from c796534 to 3a29c55 Compare November 5, 2017 11:11

inevity force-pushed the notifyfuncv1 branch from 3a29c55 to 13ad385 Compare September 20, 2019 04:29

stapelberg reviewed Sep 21, 2019

View reviewed changes

inevity force-pushed the notifyfuncv1 branch 2 times, most recently from 42473bf to 0f2c8eb Compare September 27, 2019 10:22

remove context partly

7fd51d8

inevity force-pushed the notifyfuncv1 branch from 0f2c8eb to 7fd51d8 Compare September 27, 2019 11:52

fix build

2ef1aff

inevity force-pushed the notifyfuncv1 branch from bf7c511 to 2ef1aff Compare September 28, 2019 02:19

stapelberg requested changes Jan 27, 2020

View reviewed changes

stapelberg mentioned this pull request Jan 27, 2020

Implementing InvalidateNode/InvalidateEntry #64

Closed

riking reviewed Jul 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notification messages framework #35

Add notification messages framework #35

inevity commented Nov 3, 2017

jacobsa left a comment

jacobsa Nov 4, 2017

inevity Jan 6, 2018

jacobsa Nov 4, 2017

jacobsa Nov 4, 2017

inevity Jan 6, 2018

jacobsa Nov 4, 2017

inevity Jan 6, 2018

inevity commented Nov 5, 2017

fishpadak commented Nov 1, 2018 •

edited

Loading

inevity commented Jan 13, 2019

stapelberg commented Sep 18, 2019

inevity commented Sep 20, 2019

stapelberg left a comment

stapelberg Sep 21, 2019

inevity Sep 23, 2019

stapelberg Sep 23, 2019

inevity Sep 27, 2019

stapelberg Sep 27, 2019

inevity commented Sep 27, 2019

inevity commented Sep 28, 2019

stapelberg left a comment

stapelberg Jan 27, 2020

riking Jul 5, 2020

riking Jul 5, 2020

Add notification messages framework #35

Are you sure you want to change the base?

Add notification messages framework #35

Conversation

inevity commented Nov 3, 2017

jacobsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inevity commented Nov 5, 2017

fishpadak commented Nov 1, 2018 • edited Loading

inevity commented Jan 13, 2019

stapelberg commented Sep 18, 2019

inevity commented Sep 20, 2019

stapelberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inevity commented Sep 27, 2019

inevity commented Sep 28, 2019

stapelberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishpadak commented Nov 1, 2018 •

edited

Loading