Skip to content

libct: close the mount source fd ASAP!#5177

Open
lifubang wants to merge 3 commits intoopencontainers:mainfrom
lifubang:refactor-mounts
Open

libct: close the mount source fd ASAP!#5177
lifubang wants to merge 3 commits intoopencontainers:mainfrom
lifubang:refactor-mounts

Conversation

@lifubang
Copy link
Member

@lifubang lifubang commented Mar 16, 2026

This commit factors out setupAndMountToRootfs without changing any
logic. Use "Hide whitespace changes" during review to focus on the
actual changes.

The refactor ensures the mount source file descriptor is closed via
defer in each loop iteration, reducing the total number of open FDs
in runc. This helps avoid hitting the file descriptor limit under
high concurrency or when handling many mounts.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Close mount source file descriptors as soon as they are no longer needed during rootfs setup, reducing peak FD usage in runc (especially under high mount counts / concurrency).

Changes:

  • Refactors mount source-fd acquisition logic into a new initMountEntry helper.
  • Moves lifetime management of the mount source FD into mountToRootfs to close it immediately after each mount is processed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kolyshkin kolyshkin requested review from rata March 16, 2026 18:19
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

Just curious, are you seeing a lot fds and is causing issues for you?

@lifubang
Copy link
Member Author

Just curious, are you seeing a lot fds and is causing issues for you?

This can fail under a restrictive nofile (max open files) limit when using many idmapped mounts. The second commit demonstrates the issue; without the fix in the first commit, the test fails.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

lgtm

@kolyshkin
Copy link
Contributor

Guess you'd want 1.5 backport @lifubang ?

@lifubang
Copy link
Member Author

Guess you'd want 1.5 backport @lifubang ?

Yes, it looks like a small bug.

@lifubang lifubang added this to the 1.5.0-rc.2 milestone Mar 19, 2026
@lifubang lifubang added the backport/1.5-todo A PR in main branch which needs to be backported to release-1.5 label Mar 19, 2026
@lifubang lifubang requested review from cyphar and rata March 19, 2026 08:16
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

Thanks! Let's tune the error handling and simplify a little bit the code

Signed-off-by: lifubang <lifubang@acmcoder.com>
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

For review purposes, it would be nice to split the second commit into two

  1. Factor out setupAndMountToRootfs.
  2. Implement the fix.

Otherwise it is harder to review what exactly are you changing since you also move the code around.

Still LGTM though

@kolyshkin kolyshkin requested a review from rata March 19, 2026 23:28
@cyphar
Copy link
Member

cyphar commented Mar 20, 2026

Yeah I agree @kolyshkin, it took me a couple of read-throughs to see what was being changed.

This commit factors out setupAndMountToRootfs without changing any
logic. Use "Hide whitespace changes" during review to focus on the
actual changes.

The refactor ensures the mount source file descriptor is closed via
defer in each loop iteration, reducing the total number of open FDs
in runc. This helps avoid hitting the file descriptor limit under
high concurrency or when handling many mounts.

Signed-off-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
@lifubang
Copy link
Member Author

2. Implement the fix.

In fact, no second fix is needed. As @rata suggested, factoring out setupAndMountToRootfs is sufficient to ensure the mount source file descriptor is closed in each loop iteration.

You can use “Hide whitespace changes” to make the review easier.

@kolyshkin
Copy link
Contributor

  1. Implement the fix.

In fact, no second fix is needed. As @rata suggested, factoring out setupAndMountToRootfs is sufficient to ensure the mount source file descriptor is closed in each loop iteration.

You can use “Hide whitespace changes” to make the review easier.

Thank you for clarification! Still LGTM )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.5-todo A PR in main branch which needs to be backported to release-1.5 kind/refactor refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants