Are my inline comments visible ? since at the bottom it page it lists them with a "Unsubmitted" label, as in this patch needs revision ? or as in other people can't see them ?

Yes. You must've figured out by now, but in Phabricator, unlike on Github, you must click "Submit" for inline comments to be visible. And BTW if you prefer Github PRs to Phabricator, you can use those too. Both are acceptable ways to submit patches to FreeBSD.

I think that this will be a nice feature. Have you considered how to test it? I think it could all be integrated into the existing fusefs test suite. Or alternatively, the negative tests at least could use atf-sh, with a suitable fuse binary installed from ports.

Works for me. Thanks for the fast fix.

This looks a lot better. In addition to the inline comments, I have two general ones:

This is a good start. In addition to the above comments, though, there needs to be a paired test for FTS_PHYSICAL. It isn't worth submitting a test for one without the other.

Yes, I like this version better.

The real tension is between in.body.read.size being unsigned and read() returning signed. We could change size1 and size2 to unsigned numbers, but then we'd have to cast them when checking the output of read, as is done on line 1521. I don't have a strong preference for which, but I think you can remove the "size1 >= 0" check. We can't change the type of fuse_read_in::size, because that's part of the FUSE API.

LGTM

• Remove an excessive vnode lock assertion

In D55230#1276682, @kib wrote:

Am I understand you right: after entering VOP_READ() for fusefs, and having the vnode shared-locked, VOP_READ() might discover that the attributes changed and e.g. the node size increased? So it has to set delayed setsize, but needs to read some block after the current EOF? If yes, this is fine. For instance, nfs operates similarly.

The vnode pager size is effectively used only for one purpose: it prevents the page fault from mapping pages beyond the EOF. For instance, UFS uses pages with negative (but effectively, very high positive) page indexes as the storage for indirect blocks content. And the pager check for EOF exactly prevents mapping of the metadata from the indirect blocks into userspace.

So I believe it should work fine for fusefs.

• Respond to markj's comments
• Assert vnode lock before checking lock type
• Use vn_delay_setsize to avoid lock upgrade in fuse_vnode_setsize

In D55230#1271915, @kib wrote:

D55595 should help with the vnode_pager_setsize(), in particular, it should allow to avoid the lock upgrade.

LGTM.

I'm with @emaste . And in fact, I already have a branch that enables these tests. They don't all pass, however.

@kib is there anything else you'd like me to do for this PR, short of rewriting everything so the vnode lock isn't held while waiting for the daemon?

@antranigv_freebsd.am I see a few warnings when I use libxo's warn option:

> sudo /usr/obj/usr/home/somers/src/freebsd.org/src/amd64.amd64/usr.bin/procstat/procstat  --libxo=json,pretty,warn cs (pgrep ntp)
procstat: invalid XML tag name: '98363'
{
  "__version": "2", 
  "procstat": {
    "cs": {
      "98363": {
procstat: invalid XML tag name: '100222'
procstat: key field emitted after normal value field: 'thread_id'
        "100222": {
          "process_id": 98363,
          "thread_id": 100222,
          "command": "ntpd",
          "thread_name": "-",
          "cpu": -1,
          "cpu_set_id": 1,
          "cpu_set": "0-15"procstat: invalid XML tag name: '100222'
procstat: invalid XML tag name: '98363'

• 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?