go-fuse icon indicating copy to clipboard operation
go-fuse copied to clipboard

fs API: Unable to set Ino on root node

Open stevegt opened this issue 3 years ago • 17 comments

Does anyone have any examples of the right way to implement "." and ".." dirents when using the fs api? Or do most callers just not implement those? I've tried several things but this is the closest I've come -- I've not been able to figure out why the inode number isn't showing up:

$ ls -larti /tmp/hello
10 -rw-r--r-- 0 root root 17 Dec 31  1969 file.txt
 ? drwxr-xr-x 0 root root  0 Dec 31  1969 .

That is when using something like this:

    dot := r.NewPersistentInode(ctx,
        &fs.Inode{
        },
        fs.StableAttr{
            Mode: syscall.S_IFDIR,
            Ino:  2,
        })
    r.AddChild(".", dot, false)

Some of the other things I've tried are here, commented out:

package main

import (
	"context"
	"flag"
	"log"
	"os"
	"os/signal"
	"syscall"

	"github.com/hanwen/go-fuse/v2/fs"
	"github.com/hanwen/go-fuse/v2/fuse"
)

type HelloRoot struct {
	fs.Inode
	Attr fuse.Attr
}

var _ = (fs.NodeOnAdder)((*HelloRoot)(nil))

func (r *HelloRoot) OnAdd(ctx context.Context) {

	// dot := r.NewPersistentInode(ctx, r, fs.StableAttr{Mode: syscall.S_IFDIR})
	// r.AddChild(".", dot, false)

	dot := r.NewPersistentInode(ctx,
		&fs.Inode{
			// Attr: fuse.Attr{ Ino: 2, },
		},
		fs.StableAttr{
			Mode: syscall.S_IFDIR,
			Ino:  2,
		})
	r.AddChild(".", dot, false)

	// dot := r.NewPersistentInode(ctx, r)
	// r.AddChild(".", dot, false)

	ch := r.NewPersistentInode(ctx,
		&fs.MemRegularFile{
			Data: []byte("hello big world!\n"),
			Attr: fuse.Attr{
				Mode: 0644,
			},
		}, fs.StableAttr{Ino: 10})
	r.AddChild("file.txt", ch, false)

}

var _ = (fs.NodeGetattrer)((*HelloRoot)(nil))

func (r *HelloRoot) Getattr(ctx context.Context, fh fs.FileHandle, out *fuse.AttrOut) syscall.Errno {
	out.Mode = 0755
	return 0
}

/*
func (r *HelloRoot) Readdir(ctx context.Context) (stream fs.DirStream, errno syscall.Errno) {
	dots := []fuse.DirEntry{
		{Mode: 0755, Name: "."},
		{Mode: 0755, Name: ".."},
	}
	return fs.NewListDirStream(dots), 0
}
*/

func main() {
	debug := flag.Bool("debug", false, "print debug data")
	flag.Parse()
	if len(flag.Args()) < 1 {
		log.Fatal("Usage:\n  hello MOUNTPOINT")
	}

	var server *fuse.Server

	// unmount on exit
	defer umount(server)

	// unmount on SIGINT or SIGTERM
	sig := make(chan os.Signal)
	signal.Notify(sig, os.Interrupt, syscall.SIGTERM)
	go func() {
		<-sig
		umount(server)
		os.Exit(1)
	}()

	opts := &fs.Options{}
	opts.Debug = *debug
	opts.AllowOther = true
	server, err := fs.Mount(flag.Arg(0), &HelloRoot{}, opts)
	if err != nil {
		log.Fatalf("Mount fail: %v\n", err)
	}
	server.Wait()
}

func umount(server *fuse.Server) {
	if server != nil {
		server.Unmount()
	}
}

stevegt avatar Jun 20 '21 02:06 stevegt

why do you want to implement the '.' and '..' entries?

hanwen avatar Jun 21 '21 08:06 hanwen

@stevegt Yes gocryptfs has . and .. , check out https://github.com/rfjakob/gocryptfs/commit/18befda0e6f1a690fa000951df8f9a4a61daebbb

@hanwen xfstests generic/401 breaks with . and .. missing, but I guess other things will explode too, as everything else provides them.

rfjakob avatar Jun 21 '21 08:06 rfjakob

@rfjakob Oh! I hadn't realized you'd switched to the fs API -- I've still been looking at your 1.8.0 code. Oh, this is excellent. Thank you.

@hanwen I'm tilting at the windmill of hoping to write a POSIXish fuse filesystem written in Go for the UI of an open-source project that we've been working on for the last several months (haven't published yet or I'd have a link for you). I'm at the stage of needing to finally decide between go-fuse, bazil, and jacobsa/fuse. You already know this, but go-fuse seems to win on features, performance, and currency, and all we're missing is more docs and reference code for the v2 fs API. Now that I know gocryptfs has been updated to v2, this is going to help a lot, I think.

I'm working a conference this week, but in between or immediately after I'll see what I can learn from the newer gocryptfs implementation, provide a code snippet here and close this issue myself.

stevegt avatar Jun 21 '21 19:06 stevegt

I think it would be easy to make '.' and '..' work correctly. '..' is the parent, so it should be possible to lookup the parent's ino number and populate that in the readdir call.

hanwen avatar Jun 22 '21 15:06 hanwen

I just now realized that recent versions of gocryptfs are also now showing a ? in place of the inode number for the mounted filesystem's root directory "." entry. I've filed an issue over there, but given that I'm running into the same symptom with a toy filesystem, there's a possibility the problem may be here.

As far as I can tell, this line in fs/bridge.go looks like a contributing factor:

     out.Ino = n.stableAttr.Ino

At the time that executes, in both gocryptfs and my toy filesystem, n.stableAttr.Ino is equal to zero, which means if we set Ino earlier in e.g. Getattr(), it's gonna get erased.

@hanwen Is there already a way to set StableAttr.Ino on the root node? It seems like this should be a thing, but after an afternoon of digging around I haven't seen a way to do it yet, given that fs.Inode.stableAttr isn't exported -- is there an accessor somewhere I'm missing?

Here's a version of hello which reproduces the problem -- includes several comments showing a few attempts at a workaround:

package main

import (
	"context"
	"flag"
	"log"
	"os"
	"os/signal"
	"syscall"

	"github.com/hanwen/go-fuse/v2/fs"
	"github.com/hanwen/go-fuse/v2/fuse"
)

type HelloRoot struct {
	fs.Inode
}

var _ = (fs.NodeOnAdder)((*HelloRoot)(nil))

func (r *HelloRoot) OnAdd(ctx context.Context) {

	ch := r.NewPersistentInode(ctx,
		&fs.MemRegularFile{
			Data: []byte("hello big world!\n"),
			Attr: fuse.Attr{
				Mode: 0644,
			},
		}, fs.StableAttr{Ino: 10})
	r.AddChild("hello.txt", ch, false)

	// r.AddChild(".", r.EmbeddedInode(), false)
}

var _ = (fs.NodeGetattrer)((*HelloRoot)(nil))

func (r *HelloRoot) Getattr(ctx context.Context, fh fs.FileHandle, out *fuse.AttrOut) syscall.Errno {
	// fmt.Printf("fh %#v\n", fh)
	out.Attr.Ino = 1
	out.Mode = 0755
	return 0
}

/*
var _ = (fs.NodeLookuper)((*HelloRoot)(nil))

func (r *HelloRoot) Lookup(ctx context.Context, name string, out *fuse.EntryOut) (*fs.Inode, syscall.Errno) {
	if name == "." {
		out.Attr.Ino = 1
		return r.EmbeddedInode(), 0
	}
	return nil, 0
}
*/

func (r *HelloRoot) Readdir(ctx context.Context) (stream fs.DirStream, errno syscall.Errno) {
	entries := []fuse.DirEntry{
		{Mode: syscall.S_IFDIR, Name: "."},
		{Mode: syscall.S_IFDIR, Name: ".."},
	}
	for name, child := range r.Children() {
		entry := fuse.DirEntry{Mode: child.Mode(), Name: name}
		entries = append(entries, entry)
	}
	return fs.NewListDirStream(entries), 0
}

func main() {
	debug := flag.Bool("debug", false, "print debug data")
	flag.Parse()
	if len(flag.Args()) < 1 {
		log.Fatal("Usage:\n  hello MOUNTPOINT")
	}

	var server *fuse.Server

	// unmount on exit
	defer umount(server)

	// unmount on SIGINT or SIGTERM
	sig := make(chan os.Signal)
	signal.Notify(sig, os.Interrupt, syscall.SIGTERM)
	go func() {
		<-sig
		umount(server)
		os.Exit(1)
	}()

	opts := &fs.Options{}
	opts.Debug = *debug
	opts.AllowOther = true
	server, err := fs.Mount(flag.Arg(0), &HelloRoot{}, opts)
	if err != nil {
		log.Fatalf("Mount fail: %v\n", err)
	}
	server.Wait()
}

func umount(server *fuse.Server) {
	if server != nil {
		server.Unmount()
	}
}

stevegt avatar Jul 05 '21 23:07 stevegt

Checking the ./example directory, looks like the symptom is limited to the fs API. Examples using nodefs instead do show an inode number for the root node:

example imports root inode number
autounionfs nodefs, pathfs 1
hello fs "?"
loopback fs "?"
memfs nodefs 1
multizip fs "?"

The symptom is orthogonal to whether the filesystem implements "." -- just run ls -lid MOUNTPOINT. I'll rename this issue.

stevegt avatar Jul 06 '21 00:07 stevegt

The inode number "1" in the above nodefs examples comes from this stanza in fsmount.go:

	if out.Ino == 0 {
		out.Ino = nodeId
	}

I don't see anything equivalent in the fs API code -- I'm likely missing something, but the only places I see out.Ino explicitly set are two locations, both of which set it to n.stableAttr.Ino, and a third location which sets it to b.automaticIno.

The automatic inode numbering is apparently not kicking in for the root node, and more grepping still hasn't shown me a way to set the root node's stableAttr.Ino explicitly.

stevegt avatar Jul 06 '21 01:07 stevegt

I see a few possible solutions:

  1. Simplest would be to wrap the out.Ino assignment in the following if statement.
    • This would avoid overwriting a non-zero Ino when it's been set by Getattr.
    • This doesn't solve the problem of not being able to set StableAttr.Ino on the root inode.
    • This doesn't solve the problem of automatic numbering not working for the root node.
    • This breaks cases where we might want to overwrite a non-zero out.Ino with whatever is in StableAttr.Ino -- but is that a valid concern? Is there a known use case for overwriting a non-zero out.Ino with StableAttr.Ino?
    • This check will execute for all nodes, not just root.
		if out.Ino == 0 {
			out.Ino = n.stableAttr.Ino
		}
  1. Export fs.Inode.stableAttr to enable users to set rootNode.StableAttr.Ino before mounting the filesystem.

    • But this would also expose the ability for callers to change "stable" bits on a live node.
  2. Add a constructor to enable setting fs.Inode.stableAttr bits on the root node.

    • I looked at reworking NewPersistentInode to support this, but given that it expects a non-nil ctx and bridge, it would need extensive changes.
    • The way this would work would be to add a function in fs/inode.go that looks something like:
                func NewRootInode(InodeEmbedder, StableAttr) (InodeEmbedder)

As far as I can tell, (1) would fix gocryptfs, since @rfjakob is already setting out.Ino in the root node's Getattr. But I don't know if the caveats in (1) would break anything else.

Exporting fs.Inode.stableAttr as in (2) would require touching 30 lines of code and has that "stable" concern but otherwise looks straightforward.

So far it feels to me like (3) is the right long-term solution -- it closes what appears to me to be a gap in the fs API -- we have NewInode and NewPersistentInode to set StableAttr on non-root nodes, but those methods can't be called for a root node because they need a ctx and a non-nil bridge.

Am I missing anything?

stevegt avatar Jul 06 '21 18:07 stevegt

Could we call Getattr on the root node to populate stableAttr ?

rfjakob avatar Jul 06 '21 18:07 rfjakob

Could we call Getattr on the root node to populate stableAttr ?

Unless I'm misunderstanding something, we can't say rootNode.stableAttr.Ino = 42 from inside Getattr because stableAttr isn't exported. I'm feeling a bit thick though -- how could there not be a way of setting StableAttr on the root node?

stevegt avatar Jul 06 '21 18:07 stevegt

I meant, could bridge call getattr once on mount

rfjakob avatar Jul 06 '21 18:07 rfjakob

I meant, could bridge call getattr once on mount

That then would be an option (4), calling Inode.Getattr() from someplace during mount and using the resulting out values to populate the root node's StableAttr. That call to Inode.Getattr() could not be done from the existing rawBridge.getattr() though, because the existing rawBridge.getattr() always discards the out.Ino it gets from Inode.Getattr(). Option (1) would fix the latter.

I'm starting to think we're going to end up with a combination of either (1) and (3) or (1) and (4).

stevegt avatar Jul 06 '21 19:07 stevegt

Although it occurs to me that (3) alone, skipping (1), is the least invasive of the four options and least likely to break or inadvertently change the behavior of existing callers.

stevegt avatar Jul 06 '21 19:07 stevegt

Looking at this a couple of months later -- after not looking at the code in the elapsed time, my brain sees (3) as easiest to understand conceptually as a caller, and likely easiest to implement and maintain. I've been on a detour chasing several other things in the interim, but will implement (3) when I get back to this if there are no objections and if someone else doesn't do it first.

stevegt avatar Sep 18 '21 18:09 stevegt

couldn't we provide stableAttr for the root node in the mount options?

hanwen avatar Sep 18 '21 19:09 hanwen

couldn't we provide stableAttr for the root node in the mount options?

You mean pass Ino in as a field in the Options struct in Mount(..., options *Options) and then assign it to StableAttr.Ino in NewNodeFs instead of what going on here? (I'll call that option (4) for discussion.) https://github.com/hanwen/go-fuse/blob/24a1dfe6b4f8d478275d5cf671d982c4ddd8c904/fs/bridge.go#L291

Or do you mean pass in an entire StableAttr struct as a field in the mount options? Calling this option (5).

Either makes sense to me -- I was focused on constructors when writing options 1-3 above and didn't think about the mount phase as an opportunity to set Ino.

stevegt avatar Oct 06 '21 23:10 stevegt

I've created PR #404 implementing option (5) above -- StableAttr field in Options. Has an example that works, but no new test cases 'cause I haven't yet been able to grok how to follow the go-fuse testing conventions, and didn't want to dive into figuring that out if we actually want option (4) instead.

stevegt avatar Oct 07 '21 01:10 stevegt

implemented in 615a0a7. Thanks @adlternative !

hanwen avatar Feb 05 '23 22:02 hanwen

implemented in 615a0a7. Thanks @adlternative !

Thanks for review too! :-)

adlternative avatar Feb 06 '23 03:02 adlternative