Skip to content

Abhishek/enable large reads support#4864

Draft
abhishek10004 wants to merge 4 commits into
abhishek/support_vectored_readsfrom
abhishek/enable_large_reads_support
Draft

Abhishek/enable large reads support#4864
abhishek10004 wants to merge 4 commits into
abhishek/support_vectored_readsfrom
abhishek/enable_large_reads_support

Conversation

@abhishek10004

Copy link
Copy Markdown
Collaborator

Please ensure your PR title follows the format:

type(scope): subject

Example:
feat(api): add user login endpoint

Available types:

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • refactor: A code change that neither fixes a bug nor adds a feature
  • perf: A code change that improves performance
  • test: Adding missing tests or correcting existing tests
  • build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
  • ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
  • chore: Other changes that don't modify src or test files
  • revert: Reverts a previous commit

Description

Link to the issue in case of a bug fix.

Testing details

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

Any backward incompatible change? If so, please explain.

@github-actions

Copy link
Copy Markdown

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Abhishek/enable large reads support". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces 'rapid' and 'non-rapid' bucket type groups to streamline GCSFuse optimizations, updating default parameters like max background requests, congestion thresholds, and FUSE max pages limits accordingly. It also implements a sync.Pool for read buffers to optimize memory usage during large reads on regional buckets when the kernel reader is enabled. The reviewer pointed out a critical issue in cmd/mount.go where post-mount optimized parameters (such as read-ahead, max background, and congestion threshold) are never applied in non-GKE environments because ApplyNonGKE is not called after fuse.Mount succeeds.

Comment thread cmd/mount.go
Comment on lines +150 to +157
// Apply pre mount kernel settings in non-GKE environments for non dynamic mounts when kernel reader is enabled.
if !isDynamicMount(bucketName) && !cfg.IsGKEEnvironment(mountPoint) && newConfig.FileSystem.EnableKernelReader {
kernelparamsManager := kernelparams.NewKernelParamsManager()
if kernelparams.ShouldUpdateMaxPagesLimit(int(newConfig.FileSystem.FuseMaxPagesLimit)) {
kernelparamsManager.SetMaxPagesLimit(int(newConfig.FileSystem.FuseMaxPagesLimit))
kernelparamsManager.ApplyNonGKE(mountPoint)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

While MaxPagesLimit is a system-wide pre-mount setting, other optimized parameters such as MaxReadAheadKb, MaxBackgroundRequests, and CongestionWindowThreshold are per-connection post-mount settings. They can only be applied after the FUSE mount is established and the device major/minor numbers are created. Currently, there is no call to ApplyNonGKE after fuse.Mount in cmd/mount.go, which means these optimized parameters are never applied in non-GKE environments. Please ensure ApplyNonGKE is called with these parameters after fuse.Mount succeeds.

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.

1 participant