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

Merged
merged 5 commits into from
Jun 29, 2024
Merged

Kernel/Ext2FS: Allocate blocks lazily + support writing holes #24149

merged 5 commits into from
Jun 29, 2024

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.

@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
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
@stale stale bot removed the stale label May 29, 2024
@timschumi timschumi self-requested a review June 22, 2024 23:10
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
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.
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
Member

@timschumi timschumi left a comment

Choose a reason for hiding this comment

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

#include <std_disclaimer.h>
/*
* Your warranty is now void.
*
* I am not responsible for bricked devices, dead SD cards,
* thermonuclear war, or you getting fired because the alarm app failed. Please
* do some research if you have any concerns about features included in this ROM
* before flashing it! YOU are choosing to make these modifications, and if
* you point the finger at me for messing up your device, I will laugh at you.
*/

@timschumi timschumi merged commit 1bf7f99 into SerenityOS:master Jun 29, 2024
15 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 29, 2024
@implicitfield implicitfield deleted the ext2fs branch June 29, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ext2FS: Lazy block allocation and holes
3 participants