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

feat(auto-balance): disk in pressure #104

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

c3y1huang
Copy link
Contributor

Which issue(s) this PR fixes:

Issue longhorn/longhorn#4105

What this PR does / why we need it:

Implement a new function to sync data contents by local files.

Special notes for your reviewer:

None

Additional documentation or context

longhorn/longhorn-engine#1003 (review)

@c3y1huang c3y1huang self-assigned this Jul 1, 2024
@c3y1huang c3y1huang force-pushed the feat-auto-balance-node-disks branch 2 times, most recently from ebe4de4 to 02fa133 Compare July 1, 2024 06:46
@c3y1huang c3y1huang marked this pull request as ready for review July 1, 2024 08:07
@c3y1huang c3y1huang requested review from derekbit and a team July 1, 2024 08:07
}

fileSize := fileInfo.Size()
if fileSize%Blocks != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Blocks is the block size here and a bit weird. We can rename it to block size in the future.

@derekbit
Copy link
Member

derekbit commented Jul 1, 2024

Did a quick review.

What syncContent() does

  1. Open the source file once and get a source file descriptor
  2. Create the target file once and get a target file descriptor
  3. Get data interval layout from the source file
  4. Initiate multiple workers (numSyncWorkers) for concurrently reading from the source file and writing to the target file.

We don't have any synchronization in the implementation. The concurrent readers and writers on a single source file descriptor and a single target file descriptor can indeed introduce race conditions.

@c3y1huang
Copy link
Contributor Author

Did a quick review.

What syncContent() does

  1. Open the source file once and get a source file descriptor
  2. Create the target file once and get a target file descriptor
  3. Get data interval layout from the source file
  4. Initiate multiple workers (numSyncWorkers) for concurrently reading from the source file and writing to the target file.

We don't have any synchronization in the implementation. The concurrent readers and writers on a single source file descriptor and a single target file descriptor can indeed introduce race conditions.

Wouldn't they be on different interval due to the fileIntervalChannel?

@derekbit
Copy link
Member

derekbit commented Jul 1, 2024

Wouldn't they be on different interval due to the fileIntervalChannel?

Each worker seeks the file descriptor to a specific offset and reads or writes data, so multiple workers will race.

After discussing with @c3y1huang, we decided to use a single worker for now. We can improve it by using multiple workers in the future.

Using a single worker might not be a bad approach because:

  • Sequential read and write operations usually perform well.
  • We don't consume the entire disk IOPS.

cc @innobead

@c3y1huang c3y1huang requested a review from derekbit July 1, 2024 09:32
sparse/local.go Outdated Show resolved Hide resolved
@c3y1huang c3y1huang force-pushed the feat-auto-balance-node-disks branch from d8c812c to 268dc63 Compare July 2, 2024 02:55
@c3y1huang c3y1huang requested a review from derekbit July 2, 2024 02:56
sparse/test/local_test.go Outdated Show resolved Hide resolved
@c3y1huang c3y1huang force-pushed the feat-auto-balance-node-disks branch 3 times, most recently from fb6853e to 5ce7457 Compare July 2, 2024 03:59
c3y1huang added 3 commits July 2, 2024 12:45
longhorn/longhorn-4105

Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-4105

Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-4105

Signed-off-by: Chin-Ya Huang <[email protected]>
@c3y1huang c3y1huang force-pushed the feat-auto-balance-node-disks branch from 5ce7457 to d4118bb Compare July 2, 2024 04:49
@c3y1huang c3y1huang requested a review from derekbit July 2, 2024 04:53
@derekbit derekbit merged commit f1b882f into longhorn:master Jul 2, 2024
3 checks passed
@c3y1huang c3y1huang deleted the feat-auto-balance-node-disks branch July 2, 2024 08:23
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.

2 participants