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

Set FUSE entry expiration time #2269

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

Conversation

nbdd0121
Copy link

@nbdd0121 nbdd0121 commented Aug 2, 2024

Description

Currently entry expiration is not set, which means that the dentry is thrown away after each use, causing significant overhead since every lookup operation needs to call into userspace.

Entry captures the mapping from file name to inode. The kernel list cache option allows readdir response to be cached, which is also dentry, so I reuse the same TTL for entry expiration timeout. This is conservative, and I am pretty sure there can be further performance improvement by setting the expiration in more cases (as the object generation number is also cached).

There are a few other places where dentry is returned, e.g. create or mkdir, but since the cache is more useful for readonly/read-mostly FS, I didn't bother setting the dentry expiration time for these calls.

This change brings significant performance improvement due for readonly workloads.

  • I tested the change with a bucket that have 6000 files, using du -hs on the directory (it will stat all the files.). I ran the command once to populate the cache, and then time the second run. The change reduces time taken by the command from 532ms to 72ms.
  • I also tested the change in a production compilation workload. The time taken for each run reduces from 7 minutes to 1 minute.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - NA

Currently entry expiration is not set, which means that the dentry is
thrown away after each use, causing significant overhead since every
lookup operation needs to call into userspace.

Entry captures the mapping from file name to inode. The kernel list
cache option allows readdir response to be cached, which is also
dentry, so I reuse the same TTL for entry expiration timeout.
This is conservative, and I am pretty sure there can be further
performance improvement by setting the expiration in more cases (as the
object generation number is also cached).

There are a few other places where dentry is returned, e.g. create or
mkdir, but since the cache is more useful for readonly/read-mostly FS, I
didn't bother setting the dentry expiration time for these calls.

This change brings significant performance improvement due for readonly
workloads.
* I tested the change with a bucket that have 6000 files, using `du -hs`
  on the directory (it will stat all the files.). I ran the command once
  to populate the cache, and then time the second run. The change
  reduces time taken by the command from 532ms to 72ms.
* I also tested the change in a production compilation workload. The
  time taken for each run reduces from 7 minutes to 1 minute.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
@raj-prince
Copy link
Collaborator

Thanks @nbdd0121 for the contribution!!!

One concern here is, GCSFuse maintains lookupCount for every inode, which is incremented on every LookUpInode and used at various filesystem operations. We need to measure the impact as caching the dentry leads to inconsistent lookupCount.

Also, we have this optimization in our plan, let me discuss with the team.

This can bring perf improvement in compilation workload where the
compiler searches for headers.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
@nbdd0121
Copy link
Author

nbdd0121 commented Aug 5, 2024

While the dentry is cached, FUSE will not issue the forget op. So the lookup count should still be in a consistent state. When the entry is evicted or re-validation says it is points to a different inode, FUSE will then issue forget op so the lookup count is only decremented by then.

@ashmeenkaur
Copy link
Collaborator

ashmeenkaur commented Nov 26, 2024

Hey @nbdd0121, apologies for the delay in responding to this PR.

Can you please reimplement these changes using an experimental hidden flag experimental-enable-lookup-cache to control this new dentry cache functionality? We can reuse the ttl from the existing metadata-cache-ttl-secs flag.

Here is our dev guide for your reference:
https://github.com/GoogleCloudPlatform/gcsfuse/blob/master/docs/dev_guide.md#dev-guide

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants