From c022f6f9c6efe7199d0a05b6cb3cd1e253ca784e Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Thu, 12 Dec 2024 12:49:33 +0100 Subject: [PATCH 1/2] mfs: clean cache on sync There have been multiple reports of people complaining that MFS slows down to a crawl and triggers OOM events when adding many items to a folder. One underlying issue is that the MFS Directory has a cache which can grown unbounded. All items added to a directory are cached here even though they are already written to the underlying Unixfs folder on mfs writes. The cache serves Child() requests only. The purpose is the a Directory can then give updated information about its children without being "up to date" on its own parent: when a new write happens to a folder, the parent folder needs to be notified about the new unixfs node of their child. With this cache, this notification can be delayed until a Flush() is requested, one of the children triggers a bubbling notification, or the parent folder is traversed. When the parent folder is traversed, it will write cached entries of its childen to disk, including that of the current folder. In order to write the current folder it has triggered a GetNode() on it, which in turn has written all the cached nodes in the current folder to disk. This cascades. In principle, leafs do not have children and should not need to be rewritten to unixfs, but without caching the TestConcurrentWrites tests start failing. Caching provides a way for concurrent writes to access a single reference to a unixfs node, somehow making things work. Apart from cosmetic changes, this commits allows resetting the cache when it has been synced to disk. Other approaches have been tested, like fully removing the cache, or resetting it when it reaches certain size. All were insatisfactory in that MFS breaks in one way or other (i.e. when setting maxCacheSize to 1). Setting size to ~50 allows tests to pass, but just because no tests are testing with 50+1 items. This change does not break any tests, but after what I've seen, I can assume that concurrent writes during a Flush event probably result in broken expectations, and I wouldn't be surprised if there is a way to concatenate writes, lookups and flushes on references to multiple folders on the tree and trigger errors where things just work now. The main problem remains: the current setup essentially keeps all mfs in memory. This commit could allevaite the fact that reading the directory object triggers as many unixfs.AddChild() as nodes in the directory. After this, AddChild() would be triggered only on nodes cached since the last time. I don't find a reliable way of fixing MFS and I have discovered multiple gotchas into what was going to be a quick fix. --- CHANGELOG.md | 2 ++ mfs/dir.go | 50 +++++++++++++++----------------------------------- 2 files changed, 17 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2f2a10ee..aa6a512cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ The following emojis are used to highlight certain changes: ### Fixed +* `mfs`: directory cache is now cleared every time the directory node is read, somewhat limiting unbounded growth and time to sync it to the underlying unixfs. + ### Security ## [v0.25.0] diff --git a/mfs/dir.go b/mfs/dir.go index 38302ac39..690aa9f98 100644 --- a/mfs/dir.go +++ b/mfs/dir.go @@ -99,10 +99,11 @@ func (d *Directory) localUpdate(c child) (*dag.ProtoNode, error) { d.lock.Lock() defer d.lock.Unlock() - err := d.updateChild(c) + err := d.unixfsDir.AddChild(d.ctx, c.Name, c.Node) if err != nil { return nil, err } + // TODO: Clearly define how are we propagating changes to lower layers // like UnixFS. @@ -125,31 +126,10 @@ func (d *Directory) localUpdate(c child) (*dag.ProtoNode, error) { // TODO: Why do we need a copy? } -// Update child entry in the underlying UnixFS directory. -func (d *Directory) updateChild(c child) error { - err := d.unixfsDir.AddChild(d.ctx, c.Name, c.Node) - if err != nil { - return err - } - - return nil -} - func (d *Directory) Type() NodeType { return TDir } -// childNode returns a FSNode under this directory by the given name if it exists. -// it does *not* check the cached dirs and files -func (d *Directory) childNode(name string) (FSNode, error) { - nd, err := d.childFromDag(name) - if err != nil { - return nil, err - } - - return d.cacheNode(name, nd) -} - // cacheNode caches a node into d.childDirs or d.files and returns the FSNode. func (d *Directory) cacheNode(name string, nd ipld.Node) (FSNode, error) { switch nd := nd.(type) { @@ -219,7 +199,12 @@ func (d *Directory) childUnsync(name string) (FSNode, error) { return entry, nil } - return d.childNode(name) + nd, err := d.childFromDag(name) + if err != nil { + return nil, err + } + + return d.cacheNode(name, nd) } type NodeListing struct { @@ -361,29 +346,24 @@ func (d *Directory) AddChild(name string, nd ipld.Node) error { return err } - err = d.unixfsDir.AddChild(d.ctx, name, nd) - if err != nil { - return err - } - - return nil + return d.unixfsDir.AddChild(d.ctx, name, nd) } -func (d *Directory) sync() error { +func (d *Directory) syncCache(clean bool) error { for name, entry := range d.entriesCache { nd, err := entry.GetNode() if err != nil { return err } - err = d.updateChild(child{name, nd}) + err = d.unixfsDir.AddChild(d.ctx, name, nd) if err != nil { return err } } - - // TODO: Should we clean the cache here? - + if clean { + d.entriesCache = make(map[string]FSNode) + } return nil } @@ -408,7 +388,7 @@ func (d *Directory) GetNode() (ipld.Node, error) { d.lock.Lock() defer d.lock.Unlock() - err := d.sync() + err := d.syncCache(true) if err != nil { return nil, err } From fbe02294e50c04095612f57bf4047713a939d030 Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Mon, 16 Dec 2024 13:12:21 +0100 Subject: [PATCH 2/2] mfs: clean directory cache on flush Flushing represents a sort of "I'm done" with the folder. It is less likely that old references to the folder are used after it has been flushed. As such, we take the chance to clean up the cache then. --- CHANGELOG.md | 2 +- mfs/dir.go | 10 +++++++--- mfs/file.go | 3 +-- mfs/root.go | 11 +---------- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa6a512cf..e0e1bcf95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ The following emojis are used to highlight certain changes: ### Fixed -* `mfs`: directory cache is now cleared every time the directory node is read, somewhat limiting unbounded growth and time to sync it to the underlying unixfs. +* `mfs`: directory cache is now cleared on Flush(), liberating the memory used by the otherwise ever-growing cache. References to directories and sub-directories should be renewed after flushing. ### Security diff --git a/mfs/dir.go b/mfs/dir.go index 690aa9f98..581f446a6 100644 --- a/mfs/dir.go +++ b/mfs/dir.go @@ -323,7 +323,7 @@ func (d *Directory) Unlink(name string) error { } func (d *Directory) Flush() error { - nd, err := d.GetNode() + nd, err := d.getNode(true) if err != nil { return err } @@ -349,7 +349,7 @@ func (d *Directory) AddChild(name string, nd ipld.Node) error { return d.unixfsDir.AddChild(d.ctx, name, nd) } -func (d *Directory) syncCache(clean bool) error { +func (d *Directory) cacheSync(clean bool) error { for name, entry := range d.entriesCache { nd, err := entry.GetNode() if err != nil { @@ -385,10 +385,14 @@ func (d *Directory) Path() string { } func (d *Directory) GetNode() (ipld.Node, error) { + return d.getNode(false) +} + +func (d *Directory) getNode(cacheClean bool) (ipld.Node, error) { d.lock.Lock() defer d.lock.Unlock() - err := d.syncCache(true) + err := d.cacheSync(cacheClean) if err != nil { return nil, err } diff --git a/mfs/file.go b/mfs/file.go index aff025db6..700244321 100644 --- a/mfs/file.go +++ b/mfs/file.go @@ -270,10 +270,9 @@ func (fi *File) setNodeData(data []byte) error { } fi.nodeLock.Lock() - defer fi.nodeLock.Unlock() fi.node = nd parent := fi.inode.parent name := fi.inode.name - + fi.nodeLock.Unlock() return parent.updateChildEntry(child{name, fi.node}) } diff --git a/mfs/root.go b/mfs/root.go index e584b6e06..e332f2da3 100644 --- a/mfs/root.go +++ b/mfs/root.go @@ -170,16 +170,7 @@ func (kr *Root) Flush() error { func (kr *Root) FlushMemFree(ctx context.Context) error { dir := kr.GetDirectory() - if err := dir.Flush(); err != nil { - return err - } - - dir.lock.Lock() - defer dir.lock.Unlock() - - clear(dir.entriesCache) - - return nil + return dir.Flush() } // updateChildEntry implements the `parent` interface, and signals