Fix some regressions in ext2_write #26
Conversation
New blocks for an inode are allocated when data blocks
should be written and not enough blocks are allocated yet
or during allocating new direntries in ext2_allocate_direntry.
When writing the first time to a newly created file (inode.size==0),
new blocks must be allocated and the inode size is erroneously set to
(blocks_count / fs->blocks_per_block_count) * fs->block_size
regardless of the actually written data.
int fd = creat("foo", 0660);
write(fd, "foo", 3);
This code snippet will create a new inode with size 4096 instead
of the correct size 3.
ext2_write_inode_data will correctly set the size to 3.
However, ext2_allocate_block called by ext2_write_inode_block will
set the inode size to 4096.
This is fixed by removing the size update during ext2_allocate_block.
If there is enough space in an inode but the file grows the
inode is never written back to the device and thus the new size is
not persisted.
int fd = creat("foo", 660);
write(fd, "foo", 3); // New blocks are allocated and inode written back
write(fd, "\n", 1); // size is adapted but not written back
close(fd);
// Reading the file afterwards will only return "foo".
The code snippet reproduces the bug.
This is fixed by always writing the inode back to the device if
the size is updated.
The vfs_file_t->length field is initialized but never used afterwards. It seams correct to updated it after the inode size possibly changed.
I will check, and get back to this pull request. |
|
Let me start by saying that I really appreciate your help and contribution. That said, from my understanding (which could be wrong), the size of an inode is always a multiple of the size of a block. That is why, in: // Update the size.
inode->size = (blocks_count / fs->blocks_per_block_count) * fs->block_size;we multiply by That said, I've checked out your PR and encountered some subtle problems (you'll see why I said subtle). So, with the code from the PR, here is how I reproduce the problem.
-> % mkdir test
-> % cd test
-> % touch test.txt
$ fsck.ext2 -f rootfs.imgWhat I obtain is the following: e2fsck 1.46.5 (30-Dec-2021)
Pass 1: Checking inodes, blocks, and sizes
Inode 103, i_size is 4096, should be 8192. Fix\<y>? no
Pass 2: Checking directory structure
Second entry 'test.txt' (inode=104) in directory inode 103 should be '..'
Entry '..' in /home/user/test (103) is duplicate '..' entry.
Entry '..' in /home/user/test (103) is duplicate '..' entry.
Entry '..' in /home/user/test (103) is a link to directory /home/user (85).
Pass 3: Checking directory connectivity
'..' in /home/user/test (103) is \<The NULL inode> (0), should be /home/user (85).
Pass 4: Checking reference counts
Pass 5: Checking group summary information
rootfs: ********** WARNING: Filesystem still has errors **********
rootfs: 104/8192 files (0.0% non-contiguous), 2326/8192 blocksSo, some things are breaking inside the filesystem. While with the current version ( e2fsck 1.46.5 (30-Dec-2021)
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
rootfs: 107/8192 files (0.0% non-contiguous), 2330/8192 blocksBeware, this PR is still highlighting a problem in the original // Reading the file afterwards will only return "foo".
stat(filename, &st);
printf("File size is: %u\n", st.st_size); // Here it shows 4096.shows that the size is always |
|
Found the problem. The changes you made are correct. I just need to set the size of every new directory (created with I'll pull the request and then make the small change that should fix everything (LoL, famous last words...). |
You are welcome! Thank you for creating and maintaining MentOS!
In my understanding the But I am by no means a filesystem expert, and have only shallowly skimmed through the linux kernel documentation and https://e2fsprogs.sourceforge.net/ext2intro.html. A more general questionWhat contributions are you willing to merge upstream? I am a German teacher planning to use MentOS to teach basic OS-concepts like file permissions and user separation. |
Anything that could help teach an aspect of OSs. The general idea behind MentOS, is that it ultimately should be 1) somewhat correct, 2) easy to approach, 3) somewhat similar to Linux.
I am glad to hear it was of some help to others :) Separate pull-requests would make it easier for me to test and integrate them. It is even better if they are done on the If you encounter any issues or have suggestions to make MentOS easier to use in a lecture, feel free to open an issue so I can focus my time on it. |
The first patch fixes a problem with a to big file size when creating new files.
@Galfurian you know the ext2 way better than me. Is it safe to remove the
inode.sizeupdate fromext2_allocate_inode_block?The second patch ensures that the file size is correctly persisted for consecutive writes.
More in-depth explanations and reproducers can be found in the commit messages.
The third patch is optional and has currently no observable effect, because
vfs_file_t->lengthis never used currently.