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

sysfs: always reopen directory before readdir #2355

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Conversation

haoqixu
Copy link
Contributor

@haoqixu haoqixu commented Jan 3, 2025

As POSIX states, if a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir() returns an entry for that file is unspecified.

So there is no guarantee that files added after opendir() will be visible in readdir(). We need to reopendir() to get the new state of the directory before readdir().

Btrfs was modified to track the last index number in opendir() and make readdir() never process beyond that index number (torvalds/linux@9b378f6). This change was introduced in Linux 6.4.12 (see https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.4.12). I think this change is the reason of the test failures in #2335.

I noticed that there is a similar commit in tmpfs (torvalds/linux@64a7ce7).

I can reproduce the testing failures on both btrfs and tmpfs filesystems on archlinux with 6.12.7 kernel

The behavior is not a bug, because the POSIX specification explicitly allows it. The PR sets reopenDir to true in newOsFile to always reopen directory before readdir.

Fixes #2335

For more information, here are some references:

@haoqixu haoqixu requested a review from mathetake as a code owner January 3, 2025 10:32
@james-lawrence
Copy link

could this also have been the cause of bad file errors I see infrequently?

@evacchi
Copy link
Contributor

evacchi commented Jan 10, 2025

if this is the right behavior, probably worth adding a test to make sure that we won't change it in the future :)

@ncruces
Copy link
Collaborator

ncruces commented Jan 15, 2025

if this is the right behavior, probably worth adding a test to make sure that we won't change it in the future :)

I think the tests in #1087 already do this. I have some nits regarding the comment, but I'll address them myself later.

@ncruces ncruces merged commit a40f1b5 into tetratelabs:main Jan 15, 2025
48 of 50 checks passed
ncruces added a commit that referenced this pull request Jan 15, 2025
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.

Failing written-after-open tests
5 participants