IterableDir does not iterate over all contents created after opening (Linux BTRFS)

[ianprime0509]

Zig Version

0.12.0-dev.286+b0d9bb0bb

Steps to Reproduce and Observed Behavior

Caveat: this may not necessarily be a bug if this is acceptable platform-specific behavior, but it may be an opportunity for enhanced documentation if it is acceptable. Related to #17064 as the apparent underlying cause of that issue (this pattern appears within Package.zig: #17064 (comment)).

Given the following Zig code:

const std = @import("std");

pub fn main() !void {
    var dir = try std.fs.cwd().openIterableDir("test", .{});
    defer dir.close();

    try dir.dir.writeFile("1", "1");
    try dir.dir.writeFile("2", "2");

    var iter = dir.iterate();
    while (try iter.next()) |entry| {
        std.debug.print("{s}\n", .{entry.name});
    }
}

Running this code with an initially empty test directory yields the following output on my machine:

1

Subsequent runs, after test has already been populated with both files, yield the following output:

1
2

Output of uname -a:

Linux toolbox 6.4.12-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Aug 23 17:46:49 UTC 2023 x86_64 GNU/Linux

Expected Behavior

The code should always output

1
2

even if those files were not both present when the IterableDir was opened, or a note should be added to IterableDir's documentation mentioning the platform-specific behavior.

In support of the view that this is not a bug (at least in Zig), the following C code reproduces the same behavior of excluding 2 from the output of the first run on an initially empty test directory:

#import <dirent.h>
#import <errno.h>
#import <stdio.h>

int main(void) {
  DIR *dir = opendir("test");
  if (!dir) {
    perror("open test");
    return 1;
  }

  FILE *file;
  if (!(file = fopen("test/1", "w"))) {
    perror("open 1");
    return 1;
  }
  fwrite("1", 1, 1, file);
  fclose(file);

  if (!(file = fopen("test/2", "w"))) {
    perror("open 2");
    return 1;
  }
  fwrite("2", 1, 1, file);
  fclose(file);

  errno = 0;
  struct dirent *entry;
  while ((entry = readdir(dir))) {
    printf("%s\n", entry->d_name);
    errno = 0;
  }
  if (errno != 0) {
    perror("read test");
    return 1;
  }
  return 0;
}

[ianprime0509]

Another data point and observation: I am only able to reproduce this on BTRFS: I created a temporary Ext4 filesystem for testing and I am unable to reproduce the issue there.

Possibly related: torvalds/linux@9b378f6 This commit was included in the 6.4.12 kernel released on August 23, which is not too long before the first reported occurrence of the associated package manager bug I'm aware of: zigtools/zls#1428

[theoparis]

Another data point and observation: I am only able to reproduce this on BTRFS: I created a temporary Ext4 filesystem for testing and I am unable to reproduce the issue there.

Possibly related: torvalds/linux@9b378f6 This commit was included in the 6.4.12 kernel released on August 23, which is not too long before the first reported occurrence of the associated package manager bug I'm aware of: zigtools/zls#1428

I also use BTRFS, and I just tried downgrading to the linux-lts (6.1.51) kernel on Arch Linux and it solved the issue.

[xdBronch]

for the sake of additional data points im on xfs + linux 6.4.11 and have yet to come across the PM bug

[squeek502]

Would this potentially be an upstream Linux / BTRFS bug, then?

[mikdusan]

IMO not a bug.

For linux/bsd/darwin getdents or similar is used in the underlying implementation. Related, the nature of the call is similar to the posix readdir call and 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.

I'd caution against any guarantees (to seeing dir changes after the dir is opened) for zig std beyond "unspecified" because there are lots of different underlying platforms and fs in use.

[ianprime0509]

Thanks, I looked over the man pages for getdents and readdir initially and didn't see any mention of guarantees (or lack thereof) in this scenario, but I didn't think to check if POSIX specified any behavior here.

Based on my reading of POSIX, then, calling rewinddir should have the same effect as reopening the directory, but even that does not seem to be the case with BTRFS after the change I linked above, so this may be a combination of an upstream bug and a change of behavior within the bounds of the spec.

In any case, this certainly doesn't seem to be a Zig bug, so maybe the issue needs to be retagged as a documentation enhancement or similar (if there were a portable way to "reload" the directory state, as it seems rewinddir should do, that might also be a helpful addition to the stdlib, but given the above I'm not sure if such a thing exists). Alternatively, I'll close the issue if it no longer belongs here.

[squeek502]

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.

Zig does the equivalent of rewinddir during the first next call, so it seems like this could still be a bug:

zig/lib/std/fs.zig

Lines 656 to 659 in f33bb02

	if (self.first_iter) {
	std.os.lseek_SET(self.dir.fd, 0) catch unreachable; // EBADF here likely means that the Dir was not opened with iteration permissions
	self.first_iter = false;
	}

@ianprime0509 could you test your C code reproduction with a rewinddir call added before the first readdir call?

[ianprime0509]

@squeek502 yes, unfortunately it still does not work even when using rewinddir. I sent an email to the linux-btrfs mailing list to see if I could get some feedback there as well: https://lore.kernel.org/linux-btrfs/YR1P0S.NGASEG570GJ8@ianjohnson.dev/T/#u

[daurnimator]

One thing off the top of my head is that officially, you need to fsync the directory containing a file when you add/remove a file from that directory.

Searching around for docs I found https://lwn.net/Articles/457667/ which mentions the requirement:

The more subtle usages deal with newly created files, or overwriting existing files. A newly created file may require an fsync() of not just the file itself, but also of the directory in which it was created (since this is where the file system looks to find your file). This behavior is actually file system (and mount option) dependent. You can either code specifically for each file system and mount option combination, or just perform fsync() calls on the directories to ensure that your code is portable.

---

I gave this a try:

const std = @import("std");

pub fn main() !void {
    var dir = try std.fs.cwd().openIterableDir("test", .{});
    defer dir.close();

    try dir.dir.writeFile("1", "1");
    try dir.dir.writeFile("2", "2");
    try std.os.fsync(dir.dir.fd);

    var iter = dir.iterate();
    while (try iter.next()) |entry| {
        std.debug.print("{s}\n", .{entry.name});
    }
}

And it still prints just 1 for me. So it wasn't this issue.

[ianprime0509]

A patchset was submitted (not by me) to the linux-btrfs mailing list which I've confirmed resolves this issue: https://lore.kernel.org/linux-btrfs/cover.1694260751.git.fdmanana@suse.com/

[leecannon]

@ianprime0509 It might be a good idea to include btrfs in the title of this issue.

[ianprime0509]

@leecannon good idea, thanks, I added a note in the title to clarify that this is limited to Linux BTRFS.

[kassane]

A patchset was submitted (not by me) to the linux-btrfs mailing list which I've confirmed resolves this issue: https://lore.kernel.org/linux-btrfs/cover.1694260751.git.fdmanana@suse.com/

Maybe fixed on LK 6.5-rc7:

• fix infinite loop in readdir(), could happen in a big directory when files get renamed during enumeration.
  Filipe Manana (1): btrfs: fix infinite directory reads

Reference:
https://lore.kernel.org/lkml/cover.1692453407.git.dsterba@suse.com/T/

[squeek502]

Maybe fixed on LK 6.5-rc7:

That fix infinite directory reads commit is likely the cause of this bug, not the fix. It's the commit linked in this comment. The fix in https://lore.kernel.org/linux-btrfs/cover.1694260751.git.fdmanana@suse.com/ is still pending AFAICT.

[andrewrk]

Thanks for reporting this upstream, @ianprime0509! Looks like your efforts have resulted in a kernel fix.

[andrewrk]

I'm going to close this because there is nothing actionable for Zig to do, and no workaround is planned to be implemented. There is nothing to do but wait until the affected kernel versions stop being in circulation.

Short summary of the situation, for troubleshooters:

• Bug only affects btrfs file system on linux
• Bug introduced in 6.4.12 kernel
• Bug fixed in 6.5.6

Recommend for btrfs users to stick with < 6.4.12 or >= 6.5.6 kernel versions to avoid this bug.

[ianprime0509]

FYI, the patch that fixes this is included in the recently released Linux 6.5.6: https://lwn.net/Articles/946853/

btrfs: refresh dir last index during a rewinddir(3) call

Apparently, some distros (including Arch) have been including this patch separately in their kernels recently as well. Hopefully, distros tracking the 6.5.x kernel series will pick this up soon, and we might start seeing fewer reports of this.

[squeek502]

Awesome job getting this reported and ultimately fixed, @ianprime0509!

[perillo]

Currently, the code in Package/Fetch.zig closes the IterableDir and then open it again.
I think this is not very efficient: why not implement what rewinddir does in IterableDir.reset()? In this case reset can be always be called, just to be safe.

[ianprime0509]

@perillo IterableDir already does have equivalent logic when performing the first step of an iteration:

zig/lib/std/fs.zig

Line 656 in 6fd1c64

std.os.lseek_SET(self.dir.fd, 0) catch unreachable; // EBADF here likely means that the Dir was not opened with iteration permissions

(this logic is also used when calling next after reset). The issue here was a bug in BTRFS for certain Linux kernels where using rewinddir or lseek to reset to the beginning of the directory didn't actually work, and the only way to completely reset the directory state was to fully reopen the file descriptor.

The workaround in question is only intended to be used on such kernels, when explicitly enabled:

zig/src/Package/Fetch.zig

Lines 400 to 406 in 6fd1c64

	if (builtin.os.tag == .linux and f.job_queue.work_around_btrfs_bug) {
	// https://github.com/ziglang/zig/issues/17095
	tmp_directory.handle.close();
	const iterable_dir = cache_root.handle.makeOpenPathIterable(tmp_dir_sub_path, .{}) catch
	@panic("btrfs workaround failed");
	tmp_directory.handle = iterable_dir.dir;
	}

and for all other systems, the logic is exactly as you suggest (lseek is used to reset the directory state before iterating to compute the hash).

[perillo]

@ianprime0509 Thanks for the clarification and sorry for the late response.