New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TmpFS #454
TmpFS #454
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool! Let's improve some things :)
Kernel/FileSystem/TmpFS.cpp
Outdated
return m_root_inode->identifier(); | ||
} | ||
|
||
void TmpFS::register_inode(NonnullRefPtr<TmpFSInode> inode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be
void TmpFS::register_inode(TmpFSInode& inode)
We're not passing ownership of the TmpFSInode
, it just needs to be a live, non-null object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, I was thinking of a different refcounting scheme (where refcount is stored by the smart pointer outside of the object)
LOCKER(m_lock); | ||
ASSERT(identifier.fsid() == fsid()); | ||
|
||
auto it = m_inodes.find(identifier.index()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be written as
return m_inodes.get(identifier.index()).value_or(nullptr);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice shortcut, but it doesn't seem to work here; m_inodes
stores NonullRefPtr<Inode>
s, so we cannot use nullptr
as a default value; the conversion to RefPtr<Inode>
is happening after we get it from the map.
Kernel/FileSystem/TmpFS.cpp
Outdated
struct timeval now; | ||
kgettimeofday(now); | ||
|
||
InodeMetadata metadata {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to default-initialize objects like this:
InodeMetadata metadata {}; | |
InodeMetadata metadata; |
Kernel/FileSystem/TmpFS.cpp
Outdated
void TmpFSInode::one_ref_left() | ||
{ | ||
// Destroy ourselves. | ||
static_cast<TmpFS&>(fs()).unregister_inode(identifier()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add an override for fs()
in TmpFSInode
that does the casting for us:
TmpFS& fs() { return static_cast<TmpFS&>(Inode::fs()); }
const TmpFS& fs() const { return static_cast<const TmpFS&>(Inode::fs()); }
That way we'll always get the right type when we call fs()
inside TmpFSInode
Kernel/FileSystem/TmpFS.h
Outdated
private: | ||
TmpFS(); | ||
|
||
RefPtr<TmpFSInode> m_root_inode { nullptr }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to initialize this explicitly; a RefPtr
is always nullptr
by default.
Kernel/FileSystem/TmpFS.h
Outdated
InodeMetadata m_metadata; | ||
InodeIdentifier m_parent; | ||
|
||
ByteBuffer m_content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the most important change to make: this should be a KBuffer
.
ByteBuffer
uses kmalloc()
for its storage, taking memory from the tiny little kernel heap.
KBuffer
takes memory from the global page allocator, which has lots more of it :)
This is an FS that stores all of its contents directly in memory. It's mounted on /tmp by default.
Done, plus some other tweaks, please review again 😄 |
I think this look good enough to merge, but we'll probably need to rethink how truncation works if we have multiple processes mmapping the file. |
This is an FS that stores all of its contents directly in memory.
It's mounted on
/tmp
by default.