Ensure that flush on the mfs root flushes its directory #4509
Conversation
ghost
assigned 
whyrusleeping
force-pushed
the
fix/mfs-flush-free-mem
branch
from
d9058e0
to
4895033
Compare
We may want to add some general sharness tests for stuff like this. That is, upload a file, directory, large, large, small, etc. in one subprocess and repeatedly check memory usage in the other (and see if it goes above some high watermark). Unfortunately, tests like this tend to be flaky (but we could probably set the watermark high enough to avoid false positives). |
Nevermind. I can't read. |
mfs/dir.go
Outdated
| return d.getNode(false) | ||
| } | ||
|
|
||
| func (d *Directory) getNode(clear bool) (node.Node, error) { |
There was a problem hiding this comment.
There has to be a cleaner way to do this.
There was a problem hiding this comment.
yeah, i'm thinking about it. Its pretty obnoxious right now...
There was a problem hiding this comment.
That cross my mind also, but I didn't find it objectionable enough to comment. :)
|
So, while this fixes the memory leak problem, it does bring up another interesting issue. We have the potential for someone to hold a stale reference to a directory, after it has been removed. |
|
One thing we could do for now is have a different call that we only use in the add code since we know we won't run into any of these stale reference problems. |
whyrusleeping
force-pushed
the
fix/mfs-flush-free-mem
branch
from
4895033
to
7616a59
Compare
|
There... thats a slightly better solution that doesnt introduce weirdness around the directory references. In any case though, the mfs directory thing needs work. Otherwise any use of mfs will not clear out its cache. |
whyrusleeping
force-pushed
the
fix/mfs-flush-free-mem
branch
from
7616a59
to
3bab073
Compare
| // them to be garbage collected. | ||
| // CAUTION: Take care not to ever call this while holding a reference to any | ||
| // child directories. Those directories will be bad references and using them | ||
| // may have unintended racy side effects. |
There was a problem hiding this comment.
What exactly do you mean here? How will removing these from the map lead to race conditions?
I think what's throwing me is the term "cache". Is it a write cache? A read cache?
Answered offline. Basically, we could have open file descriptors and we'll lose updates to these files if we remove the references to them from the cache.
mfs/system.go
Outdated
| } | ||
|
|
||
| for _, name := range names { | ||
| dir.Uncache(name) |
There was a problem hiding this comment.
Instead of listing then uncaching, can we just uncache all cached names? That ListNames call could get expensive.
There was a problem hiding this comment.
Specifically, with sharded directories.
There was a problem hiding this comment.
Oh yeah, youre right. This will go through all names, even if they arent cached.
| return fmt.Errorf("invalid mfs structure, root should be a directory") | ||
| } | ||
|
|
||
| if err := dir.Flush(); err != nil { |
There was a problem hiding this comment.
What happens if we concurrently modify this directory? Can we end up uncaching non-flushed entries? Is that an issue?
Should we have a single FlushAndUncache() method?
There was a problem hiding this comment.
Thats basically the purpose of this method. In my comment i make it explicit that you shouldnt use this method concurrently with any other operations since there are deeper issues that could potentially happen.
There was a problem hiding this comment.
Fair enough. I'm working on improving this situation, FYI.
License: MIT Signed-off-by: Jeromy <[email protected]>
whyrusleeping
force-pushed
the
fix/mfs-flush-free-mem
branch
from
3bab073
to
3e6eabb
Compare
|
@Stebalien fixed the 'iterate over every single child' issue. |
During
ipfs add, we periodically call flush on the mfs root to free up unused memory. ApparentlyFlushon the mfs root object wasnt actually callingFlushon its directory, which led to memory not actually being freed. I'm trying to think of the best way to test this.License: MIT
Signed-off-by: Jeromy [email protected]