Skip to content
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

Kernel/Filesystem+Tests: Fix RAMFS block count reporting, introduce a test #25077

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sdomi
Copy link
Contributor

@sdomi sdomi commented Oct 6, 2024

I tried making the test for this as generic as possible, so we can extend it onto more FSes (once we start testing those more widely in the future). Currently it runs against RAMFS itself and Ext2FS, because that was the easiest to set up.

L21 from TestFileSystemReportedSize.cpp isn't an EXPECT_EQ due to the macro complaining about not being able to compare between two types; maybe this is something we should look into fixing in the future?

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 6, 2024
@timschumi
Copy link
Member

timschumi commented Oct 6, 2024

L21 from TestFileSystemReportedSize.cpp isn't an EXPECT_EQ due to the macro complaining about not being able to compare between two types; maybe this is something we should look into fixing in the future?

I'm not sure if this is fixable unless we want to go into templated functions instead of macros. We assign the left-hand side and the right-hand side to auto variables first, and C++ isn't smart enough to restrict the types based on the following comparison (instead taking the type from the immediate).

In this particular case, EXPECT_EQ(st.st_blocks, 0ull); should make it work.

EDIT: Remainder of the review is pending, once I'm back at my PC.

@@ -122,6 +122,7 @@ ErrorOr<void> RAMFSInode::ensure_allocated_blocks(size_t offset, size_t io_size)
}
}
clean_allocated_blocks_on_failure.disarm();
m_metadata.block_count = m_blocks.size();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it fix the issue actually? I don't see where we set the block size for RAMFS so it will essentially multiply the block count by 0, isn't it correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block size is assumed to be 512/1024/"whatever the user puts in" by du. I don't think POSIX has a facility to retrieve the filesystem block size?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addendum: Ah, yeah, there is st_blksize. If we have that implemented then we should make sure that it's set.

Copy link
Contributor Author

@sdomi sdomi Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

st_blksize is being set by two classes down the line, in BlockBasedFileSystem.h (precisely here). I checked, and for our purposes it's always 512. Is this enough, or should we for some reason set it again in this Inode.cpp?

Copy link
Member

@supercomputer7 supercomputer7 Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RAMFS is not inheriting from BlockBasedFileSystem, so setting this there is probably not affecting us anyway.

Each data block is using a 128 KiB space, so 512 bytes is probably incorrect anyway.

struct DataBlock {
public:
using List = Vector<OwnPtr<DataBlock>>;
static ErrorOr<NonnullOwnPtr<DataBlock>> create();
constexpr static size_t block_size = 128 * KiB;

In #19867, I actually reduced this to 4 KiB. But 128 KiB is way too big to do anything useful with it.
Maybe change the block size to PAGE_SIZE:

constexpr static size_t data_block_size = PAGE_SIZE;

and then change st_blksize by setting it to that size as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, mb. I'm not sure where it gets the 512 from then (I did verify that it does)

will push a fix later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants