Provide same Node instance for same file not to make overlayfs confused. #19

ktock · 2019-09-13T01:24:30Z

Fixes: #16

CRFS currently doesn't support to merge the layers using overlayfs.

CRFS generates different "Node" instance every time "Lookup" is called. This behaviour makes bazil/fuse assign different "Node IDs"(used by FUSE) to inodes every time even if these "Lookups" point to same file because bazil/fuse caches "Node IDs" keyed by a "Node" instance (not an inode number etc.). Most time (when we don't use overlayfs etc.) it is fine.

However, when dentry cache revalidation is executed and the dentry is expired (by default, set to 1 min in bazil/fuse.), FUSE "lookups" the original inode and it doesn't allow different Node IDs to same inode and concludes the cache as invalid. Unfortunately, overlayfs doesn't allow dentry caches being invalid and returns ESTALE.

This commit solves this issue and make CRFS support overlayfs by cache node
instances in CRFS once it looked-up and using it when the same name is looked
up.

Currently, CRFS returns different "Node" instances everytime "Lookup" is called. This behaviour makes bazil/fuse assign different "Node ID"(used by FUSE) to inodes everytime even if these inodes are describing same file.(The reason is that bazil/fuse caches "Node IDs" keyed by a "Node" insatance(not an inode number etc.)) Most time(when we don't use overlayfs etc.) it is fine, but overlayfs doesn't allow this behaviour when it revalidates dentry cache, so returns ESTALE. As a result, CRFS currently doesn't support to merge the layers using overlayfs. This commit solves this issue and make CRFS support overlayfs by cache node instances in CRFS once it lookedup and useing it when the same name is looked up. Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

bradfitz

Thanks!

bradfitz · 2019-09-13T14:53:15Z

crfs.go

@@ -901,6 +903,7 @@ type node struct {
 	te *stargz.TOCEntry
 	sr *stargz.Reader
 	f  *os.File // non-nil if root & in debug mode
+	children map[string]fspkg.Node // Remenber child nodes once looked up.


typo: remember

But more important is to document what the keys look like. It might make sense to use a before-line comment and spell it all out:

// children maps from previously-looked up base names (like "foo.txt") to
// the *node that was previously returned. This prevents FUSE inode numbers
// from getting out of sync
child map[string]*node

Also, I'd make the value type just be *node. It's slightly faster and smaller, but it's also more clear because that's the only type we'll have there, right?

Thank you for your review. I fixed it on b076f18.

bradfitz · 2019-09-13T14:54:45Z

crfs.go

 	e, ok := n.te.LookupChild(name)
 	if !ok {
 		return nil, fuse.ENOENT
 	}
-	return &node{n.fs, e, n.sr, nil}, nil
+
+    	c := &node{n.fs, e, n.sr, nil, make(map[string]fspkg.Node)}


Arguably positional initializing was already overdue before, but now it really is. Switch these to named fields here and drop the nil entry.

Thank you for your review. I fixed it on b076f18.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

bradfitz · 2019-09-14T04:19:21Z

crfs.go

@@ -966,11 +973,24 @@ func (h *nodeHandle) ReadDirAll(ctx context.Context) (ents []fuse.Dirent, err er
 //
 // See https://godoc.org/bazil.org/fuse/fs#NodeStringLookuper
 func (n *node) Lookup(ctx context.Context, name string) (fspkg.Node, error) {
+    	if c, ok := n.child[name] ; ok {


This looks like a data race. The node.Lookup can be called from concurrent goroutines and no mutex is protecting this map read/write.

Right. I added mutex to protect children maps in e18ee07.

That's not enough. This read is also a race.

Thank you for your review. I fixed it on 39ae89f.

Run gofmt. No space before ;.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

ktock · 2019-09-19T22:56:21Z

@bradfitz Could I get any comments?

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

bradfitz · 2019-09-25T18:27:54Z

crfs.go

@@ -901,6 +903,12 @@ type node struct {
 	te *stargz.TOCEntry
 	sr *stargz.Reader
 	f  *os.File // non-nil if root & in debug mode
+
+	mu sync.Mutex // For children maps.


mu sync.Mutex // guards child, below

bradfitz · 2019-09-25T18:28:14Z

crfs.go

@@ -901,6 +903,12 @@ type node struct {
 	te *stargz.TOCEntry
 	sr *stargz.Reader
 	f  *os.File // non-nil if root & in debug mode
+
+	mu sync.Mutex // For children maps.
+	// children maps from previously-looked up base names (like "foo.txt") to


// child maps ...

(The field is named "child", not "children")

bradfitz · 2019-09-25T18:28:34Z

crfs.go

@@ -966,11 +973,24 @@ func (h *nodeHandle) ReadDirAll(ctx context.Context) (ents []fuse.Dirent, err er
 //
 // See https://godoc.org/bazil.org/fuse/fs#NodeStringLookuper
 func (n *node) Lookup(ctx context.Context, name string) (fspkg.Node, error) {
+    	if c, ok := n.child[name] ; ok {


Run gofmt. No space before ;.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

ktock · 2019-10-06T01:43:20Z

@bradfitz Could I get any comments?

googlebot added the cla: yes Signed a CLA label Sep 13, 2019

bradfitz suggested changes Sep 13, 2019

View reviewed changes

Fix to make comments, types and initialization easier to understand

b076f18

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

bradfitz reviewed Sep 14, 2019

View reviewed changes

Avoid race of node children maps

e18ee07

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

ktock requested a review from bradfitz September 20, 2019 08:55

Protect all read/write operations to node children maps from race.

39ae89f

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

ktock mentioned this pull request Sep 25, 2019

x/build: speed up large container start-up times without pre-pulling containers into VMs (CRFS) golang/go#30829

Open

bradfitz suggested changes Sep 25, 2019

View reviewed changes

ktock added 2 commits September 26, 2019 13:54

Format code with gofmt.

2a9eb81

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

Fix comments

c979e09

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

ktock requested a review from bradfitz September 26, 2019 05:03

bradfitz merged commit ed98e0b into google:master Oct 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide same Node instance for same file not to make overlayfs confused. #19

Provide same Node instance for same file not to make overlayfs confused. #19

ktock commented Sep 13, 2019

bradfitz left a comment

bradfitz Sep 13, 2019

ktock Sep 14, 2019

bradfitz Sep 13, 2019

ktock Sep 14, 2019

bradfitz Sep 14, 2019

ktock Sep 14, 2019

bradfitz Sep 20, 2019

ktock Sep 21, 2019

bradfitz Sep 25, 2019

ktock commented Sep 19, 2019

bradfitz Sep 25, 2019

bradfitz Sep 25, 2019

bradfitz Sep 25, 2019

ktock commented Oct 6, 2019

Provide same Node instance for same file not to make overlayfs confused. #19

Provide same Node instance for same file not to make overlayfs confused. #19

Conversation

ktock commented Sep 13, 2019

bradfitz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktock commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktock commented Oct 6, 2019