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 size of symlinks and enable kernel symlink cache #2285

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

Conversation

nbdd0121
Copy link

@nbdd0121 nbdd0121 commented Aug 5, 2024

Description

Symlink inodes are expected to have size equal to the length of its target. This is defined in POSIX spec.

FUSE's symlink cache also expects this, so fixing this allows the enabling of the symlink cache. Note that since symlink targets are not mutable, the cache is always valid while the inode is valid, so there is no need for any TTL or invalidation logic.

This is expected to bring significant time saving for workloads that make use of symlinks (especially for a directory symlink, since all ops inside the symlink directory would require reading the symlink at least once).

Link to the issue in case of a bug fix.

Fix #2273

Testing details

  1. Manual:
    • Mount a bucket on two mount points (with separate GCSFuse processes)
    • Create a symlink in mount1
    • Stat and see its size is correct and point to the correct target in mount2.
    • Remove and recreate the symlink in mount1 to a different target
    • Stat and see its size is correct and point to the correct target in mount2.
  2. Unit tests - NA
  3. Integration tests - I didn't add a test because I'm unfamiliar with this project (and go in general)'s test setup and it seems that the test infra is being reworked on.

Symlink inodes are expected to have size equal to the length of its
target. This is defined in POSIX spec.

FUSE's symlink cache also expects this, so fixing this prepares the
enabling of the symlink cache.

Signed-off-by: Gary Guo <[email protected]>
This would provide significant savings when symlink'ed path is
frequently accessed.

Since symlink target is not mutable, this is always safe regardless TTL
settings. (When the symlink is removed and re-created, its generatiton
would change so a different inode is created).

Signed-off-by: Gary Guo <[email protected]>
@nbdd0121 nbdd0121 requested review from kislaykishore and a team as code owners August 5, 2024 12:03
@nbdd0121 nbdd0121 requested a review from sethiay August 5, 2024 12:03
@nbdd0121
Copy link
Author

nbdd0121 commented Aug 5, 2024

This improves perf by 10% in our workload on top of #2269.

@raj-prince
Copy link
Collaborator

raj-prince commented Aug 12, 2024

Thanks @nbdd0121 for the contribution!!

Changes look good to me, but requires a couple of changes to make it mergeable.

(a) We need to guard the symlink caching under a flag like, --experimental-enable-symlink-cache (by default disabled). Keeping this experimental as we haven't performed all the testing.
(b) Could you please add unit test corresponding to size changes - here?
(c) If possible, could you please write the simple composite test for both true and false value of --experimental-enable-symlink-cache? You may refer this sample PR: #2036

Note: Adding an experimental flag would be trivial if you do this after 1 week. We are making the changes to simplify those path.

@kislaykishore kislaykishore removed their request for review August 14, 2024 07:11
@raj-prince
Copy link
Collaborator

raj-prince commented Sep 12, 2024

@nbdd0121
Copy link
Author

Tried to write a test but adding the test to symlink_test.go doesn't seem trivial to me. The test that exists there don't use NewSymlinkInode. I tried to just use inode.NewSymLinkInode but then I can't call inode.Attribute because I don't have a ctx?

(My experience is mostly with C and Rust and I have no experience with Go at all.)

@ashmeenkaur
Copy link
Collaborator

Hi @nbdd0121,

Sorry for the delay here! We were busy putting together a formal development guide, and it's finally ready: https://github.com/GoogleCloudPlatform/gcsfuse/blob/master/docs/dev_guide.md#dev-guide

Please give it a look and follow the instructions – looking forward to get these changes merged.

Thanks!

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.

Size reported is wrong for symlinks
3 participants