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

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

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

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

这样的写法我感觉过度的把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

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

@dragonosbot author

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

@fslongjin
Copy link
Member

fslongjin commented Oct 22, 2024

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

@MemoryShore

@fslongjin
Copy link
Member

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

@MemoryShore
Copy link
Collaborator Author

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

page_writeback函数

@MemoryShore MemoryShore requested a review from fslongjin October 27, 2024 17:03
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

image
page的new方法改为私有吧,然后有两个地方是手动Page new的,也改改

kernel/src/filesystem/vfs/file.rs Outdated Show resolved Hide resolved
kernel/src/mm/fault.rs Outdated Show resolved Hide resolved
Comment on lines 502 to 505
if self.shared {
shm_manager_lock().free_id(&self.shm_id.unwrap());
}
unsafe {
Copy link
Member

Choose a reason for hiding this comment

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

这里我感觉不对:
一个shm有多个page. page释放的时候,不应该去改shm,释放shm的id。

并且shared的页面也不一定是共享内存吧

@Jomocool 麻烦看看?

Copy link
Collaborator

Choose a reason for hiding this comment

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

第501行已经断言映射计数是0了,所以问题应该不大。
然后shared目前就只判断是否为共享页的。
deallocate_page_frames已经实现了释放shm_id,这里其实不需要释放了
不过要注意的是得保证同一块shm下的所有page必须一起释放,不能单独回收

kernel/src/mm/page.rs Outdated Show resolved Hide resolved
kernel/src/perf/bpf.rs Outdated Show resolved Hide resolved
kernel/src/mm/page.rs Outdated Show resolved Hide resolved
kernel/src/mm/page.rs Outdated Show resolved Hide resolved
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.

然后pagecache感觉可以创建一个单独的文件来写会好些?

kernel/src/filesystem/vfs/file.rs Outdated Show resolved Hide resolved
@MemoryShore MemoryShore requested a review from Jomocool November 27, 2024 15:31
Comment on lines 1170 to 1178
let mut page_guard = page.write_irqsave();
page_guard.remove_vma(self);

// 如果物理页的vma链表长度为0并且未标记为不可回收,则释放物理页.
// TODO 后续由lru释放物理页面
if page_guard.can_deallocate() {
if let PageType::Shm(shm_id) = page_guard.page_type() {
shm_manager_lock().free_id(shm_id);
}
Copy link
Member

Choose a reason for hiding this comment

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

我感觉这个会对同一个shmid,进行多次释放吧。这个应该不对?

@Jomocool

Copy link
Collaborator

Choose a reason for hiding this comment

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

我感觉这个会对同一个shmid,进行多次释放吧。这个应该不对?

@Jomocool

的确,这里可能会导致同一个shmid被多次释放

Copy link
Collaborator

@Jomocool Jomocool left a comment

Choose a reason for hiding this comment

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

shm_id重复释放

Comment on lines 463 to 470

// 映射计数减少
kernel_shm.decrease_count();

// 释放shm_id
if kernel_shm.map_count() == 0 && kernel_shm.mode().contains(ShmFlags::SHM_DEST) {
shm_manager_guard.free_id(shm_id);
}
Copy link
Member

Choose a reason for hiding this comment

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

这样不对吧?如果进程没有shmdt就退出了,那么映射计数就错了。产生资源泄露。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这是上一个commit的,已经outdated了
@fslongjin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-driver Area: 驱动程序 A-fs Area: 文件系统 Bug fix A bug is fixed in this pull request S-等待作者修改 Status: 这正在等待作者的一些操作(例如代码更改或更多信息)。
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants