• Respond to markj's comments

The odd thing here is that VOP_WRITE should always be called with the vnode exclusively locked. So it shouldn't be possible for it to race with a separate operation, like VOP_READ, that might call fuse_internal_cache_attrs . Do you have a stack trace for either thread involved with this race?

This looks like a real problem. But how did you find it? Was it just virtio-fs, or did you find it with a regular fuse file system too? If either case, do you know if FUSE_ASYNC_READ was set? And do you know what values are typically used for the entry and attribute cache?

See https://reviews.freebsd.org/D55230 for a more complete solution.

In D55167#1261343, @cracauer wrote:

In D55167#1261331, @asomers wrote:

In D55167#1261330, @cracauer wrote:

The reproducing case is timing dependent. I came up with the discrepancy when a user on the forums interrupted a cp operation with a du and cp bailed out.

Do you at least know what fuse file system he was using, and what options it was mounted with?

NTFS. I just asked the user for flags.

In D55167#1261330, @cracauer wrote:

The reproducing case is timing dependent. I came up with the discrepancy when a user on the forums interrupted a cp operation with a du and cp bailed out.

This is probably correct. However, I'd like to see two other things:

• A reproduction case
• An audit of the rest of the base system for other utilities that make the same mistake, like install(1).

FYI I've proposed a GSoC project to capsicumize the fts(3) family of functions. That would enable finishing the OpenRsync port.
https://wiki.freebsd.org/SummerOfCodeIdeas?highlight=%28fts_open%29#Capsciumize_fts.283.29

I have no idea what is double-release.

I'm going to reopen this in a few days, rebased and with a different approach.

In D55145#1260918, @guest-seuros wrote:

Sure i can close this diff.

But can you tell me what won't work ? I use 16-Current and have INVARIANTS

I'm not very confident in this area, but I think arrowd's latest patch should work. But to check for double-releases, he should add this patch to the test suite:

diff --git a/tests/sys/fs/fusefs/destroy.cc b/tests/sys/fs/fusefs/destroy.cc
index 0c8e2fd22a50..b3e7f8bc5fff 100644
--- a/tests/sys/fs/fusefs/destroy.cc
+++ b/tests/sys/fs/fusefs/destroy.cc
@@ -136,6 +136,17 @@ TEST_F(AutoUnmount, dup)
        assert_unmounted();
 }

Thanks for your attention in this area. As a matter of fact, I've been working on this exact same problem. My first attempt at a solution was close to this one, but ultimately it won't work. Today I'm going to try a different solution, based on mutex protection rather than the vnode lock. I'll update Bugzilla when I have a working patch, so you can subscribe to this bug to find out.
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=283391

Interesting find. I'll review this next week, after I'm home from vacation.

Thanks for working on this! I was actually working on the exact same problem myself last week. And I came up with a very similar solution. But then I realized that it wouldn't work :( . The cached attrs also need to be modified in paths where the vnode lock cannot be upgraded. So my plan is to add a mutex just to protect them. I started work on it last week. But now I'm on vacation, so I can't resume work for a few more days. But thanks for your interest in fuse.

It was not my intention to enable new tests in this commit, but I accidentally enabled the Sctp test. It probably should've been enabled before, but never was. And it works fine now.

It looks good to me. And I see that with --libxo=json, these errors append a {}. Are there any other cases where they do something differently than err?

I agree with Ed on all of these points.

As long as the existing test suites pass, I'm happy.

In D54897#1255061, @kib wrote:

I think this is fine, and I have this change in some intermediate version of the patch.
But then I noted that e.g. inotify(2) is not there (unless I mistaken), so I decided that it is up to upstream to add missed syscalls, and then import a new version.
You might add inotify as well while there.

I suggest a few wording changes. Also, fork->vfork, pdfork->pdrfork, and execfd->procdescp.

I didn't read the assembly code (I haven't had to write x86 assembly since University), but otherwise it looks good to me. And thanks for adding the P686 example, too.

In D54850#1253849, @kib wrote:

Is this the only place where the update of the cache under the shared vnode lock occurs?
Or even better, can you point me to the single entry point of the cache update code?

Approved, but don't forget to bump the man page's .Dd date.

Thanks for writing the man page too. I've made a few comments, mostly to make the English more grammatical and idiomatic.

This looks great, @kib! If you commit it, I'll follow up with the test suite. A few questions:

Wow! This looks much simpler than the original userspace-based implementation. I like it. I wrote some tests, too. I can commit those separately if you like, after you commit the main code. The only thing that's missing is a mention of the new option in mount_fusefs.8 . Could you please add that?

I'm afraid that with this latest patch, I get an instapanic on boot in pid 1.

Sorry, you're right @jlduran , as js just informed me elsewhere. My source tree was out of date. Sorry for the noise.

I had to build an aarch64 VM just to test the RFSPAWN change, but it works now. It probably works on amd64 too, but I can't test it without writing assembly code.

Add test cases

The new syscalls mostly work nicely. I've just found a few bugs. In addition to my inline comments, pdwait never returns a pid. It always returns 0. Is that deliberate? Also, it's unclear to me when a child ought to be reaped. I would think that it should be reaped as soon as pdwait or waitpid returns it (though the pid shouldn't be recycled until the process descriptor is closed). However, I'm seeing that the child doesn't get reaped until close(). That is, pdwait() will return the process multiple times, as if WNOWAIT were set. Is that deliberate?

Rather than add a new syscall for pdwait, what would you say to adding another idtype for waitid and wait6? We already have those syscalls, and their existing interface nicely allows for different kinds of ids. We would just need to add a P_PIDFD idtype value. That's also what Linux does.

• Style, suggested by des@
• Fix the encoded width of tcp-state, suggested by @des

LGTM. But you should wrap the long line in your commit message . And add something to the commit message indicating that the problem is with the "show" subcommand.

I can't reproduce this behavior with "sesutil map --libxo=json,pretty". What is the right way to trigger the bug?

Again, 300s -> 400s is not much of an increase. Might want to go more.

This looks fine. But as the default timeout is already 300s, I suggest increasing it even more. If it were me, I would double it.

Is it kosher to have two gitrefs in one entry?

In D53357#1241293, @guest-svmhdvn wrote:

Sorry to bring this back, but I'm noticing intermittency again on latest 16-CURRENT HEAD. I can reproduce it locally on amd64, and I'm seeing it fail on aarch64 in jenkins with the same result: https://ci.freebsd.org/view/Test/job/FreeBSD-main-aarch64-test/1857/testReport/sys.fs.fusefs/bad_server/main/

I needed a large count (at least 500+ iterations on my local system) to reproduce it:

$ bricoler run freebsd-src-regression-suite --param freebsd-src:url=/usr/src --param freebsd-src:branch= --param freebsd-src-regression-suite:hypervisor=bhyve --param freebsd-src-regression-suite:memory=4096 --param freebsd-src-regression-suite:ncpus=2 --param freebsd-src-regression-suite:parallelism=1 --param freebsd-src-regression-suite:count=600 --param freebsd-src-regression-suite:tests='sys/fs/fusefs/bad_server:main'

Standard output:
[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from BadServer
[ RUN      ] BadServer.UnknownUnique
[       OK ] BadServer.UnknownUnique (1 ms)
[ RUN      ] BadServer.ShortWrite
/home/siva/src/fbsdsrcgit/tests/sys/fs/fusefs/mockfs.cc:987: Failure
Value of: res >= static_cast<ssize_t>(sizeof(in.header)) || m_quit
  Actual: false
Expected: true

[  FAILED  ] BadServer.ShortWrite (0 ms)
[ RUN      ] BadServer.ErrorWithPayload
[       OK ] BadServer.ErrorWithPayload (0 ms)
[----------] 3 tests from BadServer (2 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test suite ran. (2 ms total)
[  PASSED  ] 2 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] BadServer.ShortWrite

 1 FAILED TEST
===> Failed tests
sys/fs/fusefs/bad_server:main  ->  failed: Returned non-success exit status 1  [0.006s]

@asomers could you try to reproduce?

LGTM, but you should also create a real Phabricator account now.

@jrtc27's suggestion works. Though there are other problems too, that I haven't solved yet.

• Respond to Jessica's feedback.

LGTM.