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.

Sign up for GitHub

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

Jump to bottom

fix(mm): 修复fat文件系统的PageCache同步问题 #1005

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

fix(mm): 修复fat文件系统的PageCache同步问题 #1005

wants to merge 19 commits into from
+513 −143

Conversation

MemoryShore
Copy link
Collaborator

修复fat文件系统中调用read/write对文件进行读写时与缓存中的内容不同步的问题

@dragonosbot
Copy link

@MemoryShore: no appropriate reviewer found, use r? to override

@dragonosbot dragonosbot added A-fs Area: 文件系统 S-等待审查 Status: 等待assignee以及相关方的审查。 labels Oct 21, 2024
@github-actions github-actions bot added the Bug fix A bug is fixed in this pull request label Oct 21, 2024
@fslongjin
Copy link
Member

fslongjin commented Oct 21, 2024 •
edited
Loading

这样的写法我感觉过度的把pagecache的逻辑跟fat本身的逻辑进行了耦合，而且耦合的很紧密啊（都涉及到了FAT写入文件的具体实现的函数那里了）。我认为这样写的不好，得把page cache的逻辑抽取出来。毕竟将来会有不同的文件系统。

@dragonosbot author

@dragonosbot dragonosbot added S-等待作者修改 Status: 这正在等待作者的一些操作（例如代码更改或更多信息）。 and removed S-等待审查 Status: 等待assignee以及相关方的审查。 labels Oct 21, 2024
@MemoryShore
Copy link
Collaborator Author

MemoryShore commented Oct 21, 2024 •
edited
Loading

这样的写法我感觉过度的把pagecache的逻辑跟fat本身的逻辑进行了耦合，而且耦合的很紧密啊（都涉及到了FAT写入文件的具体实现的函数那里了）。我认为这样写的不好，得把page cache的逻辑抽取出来。毕竟将来会有不同的文件系统。

@dragonosbot author

我感觉也没有很耦合吧，pagecache的read和write就只接收一个offset和buf而已，别的文件系统也能用？
@fslongjin

@fslongjin
Copy link
Member

fslongjin commented Oct 22, 2024 •
edited
Loading

这耦合程度很深吧，你想想假如有10种文件系统，pagecache如果改动涉及写入的地方，就要改10个文件系统里面都改。而且，如果有10种不同的文件系统，你这里这段代码得复制10遍。从这个角度来看，这里是缺少抽象的。

@MemoryShore

@fslongjin
Copy link
Member

貌似我好像没找到脏页回写机制？

@MemoryShore
Copy link
Collaborator Author

貌似我好像没找到脏页回写机制？

page_writeback函数

fslongjin
fslongjin reviewed Nov 3, 2024
View reviewed changes
kernel/src/filesystem/vfs/file.rs
@@ -128,6 +138,7 @@ impl FileMode {
pub struct PageCache {
Copy link
Member

Choose a reason for hiding this comment

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

貌似没看到PageCache结构体被drop得时候，释放页面的逻辑？是有什么机制保证不会内存泄露吗？

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

我想的是页面释放由PageManager管理

Copy link
Member

Choose a reason for hiding this comment

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

我想的是页面释放由PageManager管理

emm，我没太理解，比如一个inode被删除的时候，它对应的page cache是怎么被释放的？因为Inode已经被Drop了

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

确实有点问题，我再改一下

fslongjin
fslongjin requested changes Nov 3, 2024
View reviewed changes
Copy link
Member

@fslongjin fslongjin left a comment

Choose a reason for hiding this comment

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

shrink_list函数里面应该是产生了use after free了？怎么先释放了内存，才从page_cache删除掉？然后还page write back. 这个应该是有问题的。
image

@fslongjin
Copy link
Member

为啥inode read的地方，又有处理直接读写的代码？这样相当于改变了read_at的定义了。而且ramfs这些，本身就不支持cache的。它read direct就会失败。感觉实现的不太对头

fslongjin
fslongjin reviewed Nov 10, 2024
View reviewed changes
kernel/src/process/exec.rs
@@ -121,7 +121,7 @@ impl ExecParam {
let inode = ROOT_INODE().lookup(file_path)?;

// 读取文件头部，用于判断文件类型
let file = File::new(inode, FileMode::O_RDONLY)?;
let file = File::new(inode, FileMode::O_RDONLY | FileMode::O_DIRECT)?;
Copy link
Member

Choose a reason for hiding this comment

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

为啥这里要加DIRECT?我感觉不用吧

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

不DIRECT的话会访问pagecache，然后在pagecache中创建页面，这个过程要用到分配器会给当前AddressSpace加锁，但是加载elf的时候又会加锁导致死锁
image

image

而且加锁的位置在load的最开始的地方，我在想这样是必须的吗？

Copy link
Member

Choose a reason for hiding this comment

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

加载程序的话，有page cache肯定快点的。因此我感觉不要direct. 加载elf那里并不是必须一开始就对user vm加锁的。

fslongjin
fslongjin reviewed Nov 10, 2024
View reviewed changes
kernel/src/filesystem/fat/fs.rs
match &guard.inode_type {
FATDirEntry::File(f) | FATDirEntry::VolId(f) => {
let r = f.read(
&guard.fs.upgrade().unwrap(),
&mut buf[0..len],
offset as u64,
);
guard.update_metadata();
Copy link
Member

Choose a reason for hiding this comment

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

这个为啥删了？这个是用来更新读取时间的。（只是现在还没实现）

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个方法会把inode记录的size跟磁盘的size同步导致错误，我看看是不是要修改一下

Copy link
Member

Choose a reason for hiding this comment

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

这个方法会把inode记录的size跟磁盘的size同步导致错误，我看看是不是要修改一下

啊哈，这又是什么bug哈哈

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

就是说，如果启用pagecache的话inode的size跟fat文件的size不是保持一致的，但是这个方法会直接让inode的size更新为fat的size导致用户态获取的文件大小不正确
image

Copy link
Member

Choose a reason for hiding this comment

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

哦哦，那最终回写的时候就会把它弄成一致的，对吧？

Copy link
Collaborator Author

@MemoryShore MemoryShore Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

对的，不过如果需要更新读取时间的话感觉不能直接删掉，而是要修改一下方法的逻辑，不能直接把size同步

Copy link
Member

Choose a reason for hiding this comment

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

对的，不过如果需要更新读取时间的话感觉不能直接删掉，而是要修改一下方法的逻辑，不能直接把size同步

确实

fslongjin
fslongjin reviewed Nov 10, 2024
View reviewed changes
kernel/src/filesystem/fat/fs.rs Outdated Show resolved Hide resolved
kernel/src/filesystem/vfs/mod.rs Outdated Show resolved Hide resolved
fslongjin
fslongjin reviewed Nov 10, 2024
View reviewed changes
kernel/src/mm/page.rs Outdated Show resolved Hide resolved
@dragonosbot dragonosbot added the A-driver Area: 驱动程序 label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jomocool Jomocool Awaiting requested review from Jomocool

@fslongjin fslongjin Awaiting requested review from fslongjin

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-driver Area: 驱动程序 A-fs Area: 文件系统 Bug fix A bug is fixed in this pull request S-等待作者修改 Status: 这正在等待作者的一些操作（例如代码更改或更多信息）。
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MemoryShore @dragonosbot @fslongjin