[WIP] Invalidate Unlinked FSNodes from the cache
#52
Conversation
ghost
assigned 
|
This now passes the |
|
(Ignoring |
| // TODO: This could actually transform the directory into a | ||
| // new root, but it wouldn't work with a `File`. | ||
| func (d *Directory) RemoveParent() { | ||
| d.inode.parent = nil |
There was a problem hiding this comment.
This isn't going to be thread-safe.
There was a problem hiding this comment.
Yes, I'm not even sure if we should even be manipulating the internal inode.
| @@ -69,6 +69,7 @@ type FSNode interface { | |||
| GetNode() (ipld.Node, error) | |||
| Flush() error | |||
| Type() NodeType | |||
| RemoveParent() | |||
There was a problem hiding this comment.
Exposing RemoveParent on a public interface probably isn't a good idea (it's an internal function only meant to be called by Unlink. We could either:
- Define a private interface.
- Call it
Unlink()and have files unlink themselves (the child would need access to the parent somehow).
There was a problem hiding this comment.
- Define a private interface.
Yes, that sounds good, I'm abusing the FSNode interface here (because in my mental model FSNode just means child of a directory -either another dir or a file- but I'm thinking in OOP terms and I don't think I can have a public interface with private methods).
Yes, this is more of a PoC to test an idea that would help clean up this internal "cache". |
Actually, we can probably handle renames in the same way (just make sure to update cache/parent on rename). I think we really are going to need a lock around the "parent" (or are going to need to use an atomic value) regardless of what we do. As for cleaning up the cache, I really think the only solution is a |
|
Ok, if this solution makes sense (since I probably won't have time to implement it) I'm going to assign it to @nitishm who I think we'll be interested in. I'm going to close this now as it was a fast PoC to test an idea but we shouldn't cherry-pick these commits into the final PR I think, just use them as a starting point. @nitishm This is just a heads-up, don't bother doing anything here until I create a well documented issue describing the problem and the potential solution coded here, I won't be very active in MFS from now on but @Stebalien will be able to guide you through it. |
I can try. Really, my only knowledge of MFS comes from hacking on ipfs/kubo#4514. I'm mostly just groping blindly and reading code. |
So do the rest of us.. but you tend to have better judgement 😄 |
Inspired by #48 (comment).
Provisional (informal) description:
The only place that should modify the internal bookkeeping of issued
FSNodes (e.g., when callingChild), alias the "cache", to remove entries should beUnlink.If you need to lower the memory usage because you requested too many nodes (a.k.a. listed the entire MFS) remove the entire MFS reference (I'm not sure this is a perfect guarantee, we don't know when all the
FSNodes are reclaimed by GC).When removed from our internal bookkeeping, nullify the
FSNodestructure issued, this is implemented here by setting theparentreference tonilto make sure it doesn't have access to upper layers to propagate its changes. (This is just a proposed solution, we could add an explicitunlinkedfield or similar).