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/Ext2FS: Allocate blocks lazily + support writing holes #24149

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

Conversation

implicitfield
Copy link
Contributor

... and also do a thorough refactor to be able to accomplish that, because the old implementation was extremely dependent on all blocks being sequential. There's a fairly long explanation of that in the relevant commit message.

This also accidentally fixes various situations where the filesystem would get slightly corrupted, such as when running the second of the following two commands:

dd if=/usr/lib/libweb.so.serenity of=foo.bin bs=1M count=50
truncate -s 35M foo.bin

Here's a more complete script that I used to test the correctness of this PR:

# test direct + singly indirect + doubly indirect blocks
dd status=none if=/usr/lib/libweb.so.serenity of=foo.bin bs=1M count=50
dd status=none if=/usr/lib/libweb.so.serenity of=/tmp/foo.bin bs=1M count=50
sha512sum /tmp/foo.bin | sed 's,/tmp/,,' | sha512sum -c

rm /tmp/*.bin

# test doubly indirect + triply indirect blocks
dd status=none if=/usr/lib/libweb.so.serenity of=bar.bin bs=1M count=50 seek=4096
dd status=none if=bar.bin bs=1M skip=4000 of=/tmp/bar.bin
dd status=none if=/usr/lib/libweb.so.serenity of=/tmp/foo.bin bs=1M count=50 seek=96
sha512sum /tmp/foo.bin | sed 's,foo,bar,' | sha512sum -c

Closes #6236.

This removes removes the need to know the last block in advance, and
makes the loops slightly easier to reason about by depending on fewer
conditions.
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 28, 2024
Kernel/FileSystem/Ext2FS/FileSystem.cpp Outdated Show resolved Hide resolved
Kernel/FileSystem/Ext2FS/Inode.cpp Outdated Show resolved Hide resolved
Kernel/FileSystem/Ext2FS/Inode.cpp Outdated Show resolved Hide resolved
Kernel/FileSystem/Ext2FS/Inode.cpp Outdated Show resolved Hide resolved
Kernel/FileSystem/Ext2FS/Inode.cpp Outdated Show resolved Hide resolved
Kernel/FileSystem/Ext2FS/Inode.cpp Show resolved Hide resolved
Since we now only store blocks that are actually allocated, it is
entirely valid for the block list to be empty, so this commit lifts the
restrictions on accessing inodes with an empty block list.
This is a large commit, since this is essentially a complete rewrite of
the key low-level functions that handle reading/writing blocks. This is,
however, a necessary prerequisite of being able to write holes.

The previous version of `flush_block_list()` (along with its numerous
helper functions) was entirely reliant on all blocks being sequential.
In contrast to the previous implementation, the new version
of `flush_block_list()` simply writes out the difference between the old
block list and the new block list by calculating the correct indirect
block(s) to update based on the relevant block's logical index.

`compute_block_list()` has also been rewritten, since the estimated
amount of meta blocks was incorrectly calculated for files with holes as
a result of the estimated amount of blocks being a function of the file
size. Since it isn't possible to accurately compute the shape of the
block list without traversing it, we no longer try to perform such a
computation, and instead simply search through all of the allocated
indirect blocks.

`compute_block_list_with_meta_blocks()` has also been removed in favor
of the new `compute_meta_blocks()`, since meta blocks are fundamentally
distinct from data blocks due to there being no mapping between any
logical block index and the physical block index.
With this change, we no longer preallocate blocks when an inode's size
is updated, and instead only allocate the minimum amount of blocks when
the inode is actually written to.
Copy link

stale bot commented May 22, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label May 22, 2024
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 stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ext2FS: Lazy block allocation and holes
2 participants