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

[PR1] Support to keep ReadDir response in kernel cache #1897

Merged
merged 22 commits into from
May 25, 2024
Merged

Conversation

raj-prince
Copy link
Collaborator

@raj-prince raj-prince commented May 9, 2024

Description

  • Added a configuration under FileSystem - kernel-dir-cache-ttl-secs

    • if 0 - means no kernel caching.
    • if -1 - means cache for life-time
    • Otherwise, kernel will cache the readdir response and invalidated based on the kernel-dir-cache-ttl-secs value.
  • Supported cli flag --kernel-dir-cache-ttl-secs, whose value will take precedence over config.

  • With this support list response as part of ls command will be saved in the kernel and invalidated based on the kernel-dir-cache-ttl-secs.

  • Kernel doesn't keep list response partially, either complete response or none.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - Done.
  2. Unit tests - Added unit test for newly added method in DirInode interface.
  3. Integration tests - NA

@raj-prince raj-prince changed the title Readdir cache poc changes [PR1] Support to keep ReadDir response in kernel cache May 21, 2024
@raj-prince raj-prince changed the title [PR1] Support to keep ReadDir response in kernel cache Complete change to Support to keep ReadDir response in kernel cache May 22, 2024
@raj-prince raj-prince changed the title Complete change to Support to keep ReadDir response in kernel cache [PR1] Support to keep ReadDir response in kernel cache May 23, 2024
@raj-prince raj-prince self-assigned this May 23, 2024
@raj-prince raj-prince added the execute-integration-tests Run only integration tests label May 23, 2024
@raj-prince raj-prince marked this pull request as ready for review May 23, 2024 04:38
@raj-prince raj-prince requested a review from a team as a code owner May 23, 2024 04:39
@raj-prince raj-prince requested a review from sethiay May 23, 2024 04:39
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 74.28571% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 61.28%. Comparing base (d086815) to head (f11d677).

Files Patch % Lines
internal/fs/fs.go 16.66% 4 Missing and 1 partial ⚠️
internal/fs/inode/base_dir.go 28.57% 4 Missing and 1 partial ⚠️
internal/fs/inode/dir.go 63.63% 4 Missing ⚠️
flags.go 89.47% 1 Missing and 1 partial ⚠️
main.go 0.00% 1 Missing ⚠️
tools/mount_gcsfuse/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1897      +/-   ##
==========================================
+ Coverage   60.76%   61.28%   +0.52%     
==========================================
  Files         130      130              
  Lines       12397    12458      +61     
==========================================
+ Hits         7533     7635     +102     
+ Misses       4526     4480      -46     
- Partials      338      343       +5     
Flag Coverage Δ
unittests 61.28% <74.28%> (+0.52%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flags.go Outdated Show resolved Hide resolved
flags.go Outdated Show resolved Hide resolved
internal/config/config_util.go Outdated Show resolved Hide resolved
internal/fs/fs.go Outdated Show resolved Hide resolved
internal/fs/fs.go Outdated Show resolved Hide resolved
internal/fs/fs.go Outdated Show resolved Hide resolved
internal/fs/fs.go Outdated Show resolved Hide resolved
internal/fs/inode/base_dir_test.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/fs.go Show resolved Hide resolved
internal/config/mount_config.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
internal/config/mount_config.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@sethiay sethiay left a comment

Choose a reason for hiding this comment

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

Since we are upgrading jacobsa/fuse, so please run the performance tests also.

Copy link
Collaborator

@gargnitingoogle gargnitingoogle left a comment

Choose a reason for hiding this comment

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

@raj-prince I've reviewed the full code here and added some comments.

This also needs some integration tests or new configurations added in the existing integration tests. Please add.

internal/config/config_util.go Outdated Show resolved Hide resolved
internal/config/config_util.go Outdated Show resolved Hide resolved
flags.go Outdated Show resolved Hide resolved
flags.go Outdated Show resolved Hide resolved
internal/config/config_util.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
internal/util/util_test.go Outdated Show resolved Hide resolved
internal/util/util_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
flags_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gargnitingoogle gargnitingoogle left a comment

Choose a reason for hiding this comment

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

Forgot to mark Request changes earlier.

Copy link
Collaborator

@gargnitingoogle gargnitingoogle left a comment

Choose a reason for hiding this comment

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

Added some more changes

internal/util/util.go Outdated Show resolved Hide resolved
internal/fs/inode/dir_test.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/inode/dir_test.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
internal/fs/fs.go Outdated Show resolved Hide resolved
internal/fs/fs.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Outdated Show resolved Hide resolved
@raj-prince raj-prince added the execute-perf-test Execute performance test in PR label May 24, 2024
Copy link
Collaborator

@gargnitingoogle gargnitingoogle left a comment

Choose a reason for hiding this comment

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

Minor comments. I don't mind handling them in a future PR to help this PR along.

internal/config/config_util_test.go Outdated Show resolved Hide resolved
flags_test.go Outdated Show resolved Hide resolved
internal/config/yaml_parser_test.go Outdated Show resolved Hide resolved
internal/config/yaml_parser_test.go Outdated Show resolved Hide resolved
internal/fs/inode/dir.go Show resolved Hide resolved
Copy link
Collaborator

@gargnitingoogle gargnitingoogle left a comment

Choose a reason for hiding this comment

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

LGTM

@raj-prince
Copy link
Collaborator Author

raj-prince commented May 24, 2024

Perf test result:
image

Although triggered again since the previous build aborted due to unavailable executor.

@raj-prince
Copy link
Collaborator Author

Perf looks good:
image

@raj-prince raj-prince merged commit 89392be into master May 25, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execute-integration-tests Run only integration tests execute-perf-test Execute performance test in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants