Add readahead for pagecache #848

Sun12551 · 2024-05-15T13:25:47Z

This pull request introduces a new feature to the page cache system - a readahead mechanism implemented as ondemand_readahead, replacing the existing commit_page implementation. The enhanced functionality now identifies sequential I/O patterns and performs readahead accordingly.

Before this modification, the bandwidth for sequential read/write operations in exFAT was approximately 50MiB/s. With this update, the corresponding bandwidth can now be boosted to around 200MiB/s, showcasing a significant performance improvement in handling sequential I/O operations.

yingdi-shan

This PR introduces readahead, which can help to improve the sequential read performance across all filesystems.

yingdi-shan · 2024-05-17T03:59:40Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ pub fn set_ra_page(&mut self, page: usize) {
+ self.ra_pages = if page > Self::MAX_RA_PAGES {
+ Self::MAX_RA_PAGES
+ } else if page < Self::INIT_RA_PAGES {
+ Self::INIT_RA_PAGES
+ } else {
+ page
+ };
+ }


What is the purpose of this function?

This function allows users of the page cache to set the maximum size of the readahead window. I have renamed it to set_max_window_size.

yingdi-shan · 2024-05-17T04:01:18Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+
+ pub fn new() -> Self {
+ Self {
+ ra_start: Self::INVALID_PAGE_IDX,


Instead of using a constant to represent invalid state, I recommend using an Option.

yingdi-shan · 2024-05-17T04:05:44Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ if nreqs == 0 {
+ return false;
+ }


The logic does not follow the name of this function.

yingdi-shan · 2024-05-17T04:07:36Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ Some(self.ra_start..self.ra_start + self.ra_size)
+ }
+
+ fn is_all_req_completed(&self) -> bool {


We can use has_completed_all_requests.

yingdi-shan · 2024-05-17T04:13:51Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ Ok(())
+ }
+
+ pub fn should_readahead(&mut self, idx: usize, npages: usize) -> bool {


npages represents the maximal number of pages for readahead, maybe we should consider changing its name for better clarity.

yingdi-shan · 2024-05-18T11:00:42Z

LGTM. Thanks for your contribution.

tatetian · 2024-06-04T07:25:21Z

@liqinggd Could you take a look at this PR? Your inputs are highly appreciated.

tatetian · 2024-06-04T07:40:23Z

Read-ahead is an important I/O optimization technique. Good job on bring it to Asterinas!

My concern is about the downside of read-ahead. It has been shown that the read-ahead mechanism introduced in this PR can bring significant performance boost to sequential reads of ramfs. But what about its impact on the performance of other workload patterns such as random reads and writes? What about its impact on the performance of Ext2 and Ramfs? I think we need real numbers to demonstrate that the read-ahead mechanism only incurs negligible overheads to the "bad" cases.

Ramfs is a particularly interesting case because ramfs is not expected to enjoy any performance benefit from the read-ahead mechanism, but it also utilizes the page cache mechanism. In light of this, I think read-ahead should be a feature that can be opted in by file systems. Individual file systems should decide if read-ahead makes sense for it. Another benefit of making read-ahead optional feature is to enable turning it on/off easily to evaluate its performance impact.

liqinggd · 2024-06-07T06:28:16Z

kernel/aster-nix/src/fs/utils/page_cache.rs

@@ -85,16 +87,179 @@ impl Debug for PageCache {
 }
 }



Please add some comments to explain the structure

liqinggd · 2024-06-07T06:38:34Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ }
+
+ /// Set the maximum readahead window size.
+ pub fn set_max_window_size(&mut self, size: usize) {


This API gives the user a surprise because the user cannot set the size as one wanted

I removed the limit on the maximum window size and now the user is free to set it. The previous maximum window size (MAX_WINDOW_SIZE) was changed to the default maximum size (DEFAULT_MAX_SIZE).

liqinggd · 2024-06-07T06:48:04Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ pub fn wait_for_prev_readahead(
+ &mut self,
+ pages: &mut MutexGuard<LruCache<usize, Page>>,
+ force: bool,


Why add a force argument here? It makes the logic of the code confusing. I suggest to remove this argument.

The force parameter is used to indicate whether to wait for the current request to complete. Maybe because I have written some logic that does not belong to this function, making this function a little confused. I've removed that part of the logic.

liqinggd · 2024-06-07T06:54:33Z

kernel/aster-nix/src/fs/utils/page_cache.rs

@@ -85,16 +87,179 @@ impl Debug for PageCache {
 }
 }

+struct ReadaheadWindow {
+ /// Window start.
+ start: usize,


If it is an index, rename it to start_idx. I suggest to use Range<usize> here to represent the window.

liqinggd · 2024-06-07T06:56:05Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ /// Window size.
+ size: usize,
+ /// Look ahead position in current window.
+ lookahead_index: usize,


I can not see the benefit of lookahead_index, it is just the start of the range.

The lookahead_index indicates where the readahead is triggered. We set the lookahead_index to the start of the window for now. This should be adjustable by the user, which is a TODO here.

liqinggd · 2024-06-07T06:57:41Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ })
+ }
+
+ pub fn update(&self, max_size: usize, max_page: usize) -> Option<Self> {


This API gives me a surprise, because update means an in-place modification.

I have renamed it to next.

liqinggd · 2024-06-07T06:59:46Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+}
+
+impl ReadaheadWindow {
+ pub fn new(start_page: usize, init_size: usize, max_page: usize) -> Option<Self> {


I think the constructor just use Range as the input argument is enough. The argument max_page is wired.

liqinggd · 2024-06-07T07:01:25Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ pub fn new() -> Self {
+ Self {
+ ra_window: None,
+ max_size: Self::MAX_WINDOW_SIZE,


I suggest this value is a default window size.

liqinggd · 2024-06-07T07:11:25Z

kernel/aster-nix/src/fs/utils/page_cache.rs

+ Ok(())
+ }
+
+ pub fn should_readahead(&mut self, idx: usize, max_page: usize) -> bool {


The function is intended solely to determine whether readahead should be performed; the operation of updating the window should be placed where readahead is actually executed later on. It seems peculiar that a function meant for making a judgment call would also require a mutable reference (&mut).

Fixed. I moved the window update logic to set_up_window, which will be called after should_readhead decided to conduct the readahead.

yingdi-shan reviewed May 17, 2024

View reviewed changes

Sun12551 force-pushed the readahead branch from 7677275 to a60258f Compare May 17, 2024 15:33

Sun12551 force-pushed the readahead branch from a60258f to 59ad6cc Compare May 21, 2024 08:07

yingdi-shan approved these changes May 23, 2024

View reviewed changes

Sun12551 force-pushed the readahead branch from 64f17fd to 59ad6cc Compare May 28, 2024 03:40

Sun12551 force-pushed the readahead branch 3 times, most recently from 859a862 to d0d3f2d Compare June 6, 2024 08:08

liqinggd reviewed Jun 7, 2024

View reviewed changes

Add readahead for pagecache

6b7971d

Sun12551 force-pushed the readahead branch from d0d3f2d to 6b7971d Compare June 10, 2024 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add readahead for pagecache #848

Add readahead for pagecache #848

Sun12551 commented May 15, 2024 •

edited

yingdi-shan left a comment

yingdi-shan May 17, 2024

Sun12551 May 17, 2024 •

edited

yingdi-shan May 17, 2024

Sun12551 May 17, 2024

yingdi-shan May 17, 2024

Sun12551 May 17, 2024

yingdi-shan May 17, 2024

Sun12551 May 17, 2024

yingdi-shan May 17, 2024

Sun12551 May 17, 2024

yingdi-shan commented May 18, 2024

tatetian commented Jun 4, 2024

tatetian commented Jun 4, 2024 •

edited

liqinggd Jun 7, 2024

liqinggd Jun 7, 2024

Sun12551 Jun 10, 2024

liqinggd Jun 7, 2024

Sun12551 Jun 10, 2024 •

edited

liqinggd Jun 7, 2024

Sun12551 Jun 10, 2024

liqinggd Jun 7, 2024

Sun12551 Jun 10, 2024

liqinggd Jun 7, 2024

Sun12551 Jun 10, 2024

liqinggd Jun 7, 2024

Sun12551 Jun 10, 2024

liqinggd Jun 7, 2024

liqinggd Jun 7, 2024

Sun12551 Jun 10, 2024

Add readahead for pagecache #848

Are you sure you want to change the base?

Add readahead for pagecache #848

Conversation

Sun12551 commented May 15, 2024 • edited

yingdi-shan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sun12551 May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yingdi-shan commented May 18, 2024

tatetian commented Jun 4, 2024

tatetian commented Jun 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sun12551 Jun 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sun12551 commented May 15, 2024 •

edited

Sun12551 May 17, 2024 •

edited

tatetian commented Jun 4, 2024 •

edited

Sun12551 Jun 10, 2024 •

edited