Conflict with Loopback OpenDir, SetLk, and FileHandles

[manninglucas]

I'm using a server backed by the loopback filesystem to perform the following operations:

rx 60: OPENDIR n4 
tx 60:     OK, {Fh 1 }
rx 62: SETLK n4 *fuse.LkIn: &{{88 32 62 4 {{0 0} 8} 0} 1 8 {0 18446744073709551615 0 4292862304} 0 0} 
tx 62:     95=operation not supported

The SetLk method returns ENOTSUP even though loopbackFile implements the SetLker interface. OPENDIR returns a valid file handle in its response, so this seems like unexpected behavior. Digging further, it looks like Opendir does not register a FileHandle the same way Open does, it just closes it with the closes the fd right after opening. So, when rawBridge's SetLk looks for a file, it doesn't find one associated with the inode and returns ENOTSUP (even though it should be).

An easy solution would be to extend the loopbackNode to implement NodeSetlker, but we still need to get the correct file descriptor somehow. Would it be possible to modify or extend the NodeOpendirer interface to include a method that returns a FileHandle like open? Then we can register this fileHandle with b.registerFile and the issue resolves itself. I already have a patch in the works if this is acceptable.

[hanwen]

Would it be possible to modify or extend the NodeOpendirer interface to include a method that returns a FileHandle like open?

it's possible, but it would be backward incompatible, so if we can avoid it, that would be better.

One of the reasons for the current interface (single readdir callwhich returns a dirstream) is that it allows taking a single consistent snapshot of the dir, and that it is easy to implement seeking correctly (see the comment in rawBridge.setStream). I fear that this may become impossible if the DirStream is exposed through OpenDir instead.

Why do you want to lock directories?

[hanwen]

@rfjakob FYI - Jakob did a lot of stress testing of the current API, and I wonder what pitfalls he sees if we start returning filehandles from Opendir.

[hanwen]

one alternative is to have loopbackNode.SetLk open the directory to get a file descriptor just for the purpose of locking.

[manninglucas]

Why do you want to lock directories?

I'm working on implementing FUSE in gvisor, which aims to implement most of Linux system call interface. I've been using this package to help test the correctness of my implementation, and one our internal tests locks an open directory file and checks for success, since it's allowed on Linux.

In my prototype change, I added an interface called NodeOpenDirFher that defines a method OpenDirFh, which works the same way as the normal OpenDir but returns a FileHandle in addition to a syscall.Errno. Then, RawBridge checks if the node implements OpenDirFh, and if it does, registers the file handle returned from that. Otherwise it just uses the OpenDir method as normal.

I think this satisfies the backwards compatibility requirement — only those who need OpenDir to return a FileHandle will implement the NodeOpenDirFher interface, and everyone else is unaffected.

Having SetLk Open the file itself like you mentioned would also work well with fewer total changes. I could do either, just let me know which is preferable.

[manninglucas]

On a side note, I think there is a bug in rawBridge SetLk on this line (same applies for SetLkw). I think that should be f.file.(FIleSetLker) not n.ops.(FileSetLker), like it is in GetLk. This can be addressed in the same PR if we decide to make one of the changes discussed.

[hanwen]

oh, you're a googler! If you don't mind, can you loop me in on internal bug(s) for this? I usually work on go-fuse in my free time, but if this is related to Google business, I prefer to do it in work time.

[hanwen]

re. bug: yes, that looks wrong. I guess this code does not have enough test coverage.

[hanwen]

Having SetLk Open the file itself like you mentioned would also work well with fewer total changes.

I think I'd rather have this. NodeOpenDirFher (aside from the ugly name) will be confusing, because it makes people wonder if they should implement NodeOpener or NodeOpenDirFher.

ps. I prefer to use gerrit for review (see CONTRIBUTING)

[rfjakob]

The upside I see for NodeOpenDirFher is that it also fixes xfstests generic/035 ( #55 ), and that the *at() syscall family (i.e. openat and friends) ran again go-fuse filesystems could become safer w.r.t. to concurrent modifications.

[manninglucas]

After looking into this more, I've realized that gVisor's FUSE implementation is incorrect. Linux does not send FUSE SetLk requests to open dir files, it only does it for regular files. See https://github.com/torvalds/linux/blob/master/fs/fuse/file.c#L3204 vs
https://github.com/torvalds/linux/blob/master/fs/fuse/dir.c#L1952