In D56165#1334631, @kib wrote:

In D56165#1334568, @asomers wrote:

In D56165#1285144, @kib wrote:

As was discussed elsewhere, fuse server which times out the responses could cause lock cascades in VFS. This would have global consequences for the whole system.

Until the vnodes are locked around communication with userspace, I do not think this change is appropriate.

I think kib means "unlocked around communication with userspace". But I do not see how such a change could possibly work.

Fuse needs to make some way to ensure liveness of the inode (not vnode) around calls to the userspace.

In D56165#1285144, @kib wrote:

As was discussed elsewhere, fuse server which times out the responses could cause lock cascades in VFS. This would have global consequences for the whole system.

Until the vnodes are locked around communication with userspace, I do not think this change is appropriate.

LGTM

Respond to @markj's latest comments

The new dtrace probes are just to help develop these test cases. I'll remove them before I commit.

• Convert tests from sh to C++
• Fix a mutex recursion panic

In D56605#1321861, @ngie wrote:

This version of pjdfstest requires a pjdfstest user, which we currently
don't have. The current plan is for the pjdfstest package to create the
user upon installation.

What about just using the tests user in the upstream port?

% id tests
uid=977(tests) gid=977(tests) groups=977(tests)

In D56605#1321860, @ngie wrote:

Dang -- not bad. Are there any known data creation races that need to be managed in the Rust implementation? How much "tire kicking" has the Rust version gotten so far? How about A/B test code coverage from the kernel?

In D56605#1321843, @ngie wrote:

I'm sorry -- y'all are right about the 2 implementations being able to coexist. 15 seconds is really short, but that's done on an in-memory filesystem, whereas our tests are run on physical device/virtual compute (so vSphere, etc) backed devices with a WITNESS/INVARIANTS enabled kernel (runtime is ~10 minutes). I'm not sure what the kernel options were for the 15 second run, but regardless, it would probably be at least an order of magnitude longer than the runtime for the new pjdfstest implementation.

In D56605#1321359, @ngie wrote:

It's a regression for folks that use the tool in-tree as-designed because:

1. The existing tool does not support the configuration format required by the rust tool. Execution is driven by prove/kyua .

In D56605#1321284, @ngie wrote:

In D56605#1321277, @asomers wrote:

...

It's all yours: https://github.com/asomers/py-pjdfstest .

Thank you!

Also, I want to caution you that Python has an additional handicap: it's C interface is much more awkward than Rust's. The Rust standard library is simple enough that for many things, we use it directly to test the file system. And for other stuff, it's pretty easy to call Rust's libc bindings. But the Python version needs to define a Python-C interface for pretty much everything that touches the file system.

Yeah, the CFFI part is kind of annoying, but it's a part of the joy of doing C bindings in python, unless you have a thing for SWIG or doing everything with the raw python-C APIs *shudders*.

BTW, what version of Rust is OneFS stuck on? If it isn't too old, we may be able to make pjdfstest work on it.

1.74.1, which was released in 12/2023. I don't honestly know if you want to deal with that pain :(.

In D56605#1321251, @ngie wrote:

I can't get into details too much, but yes.. ~12 hours if I'm lucky because it has to bootstrap using the stage 1 compiler instead of rebuilding everything with the stage 2 compiler (which FreeBSD can do).

In D56605#1321232, @ngie wrote:

This forces all end-users of pjdfstest to build/install the new rust application/binding instead of the simple C application in tree. I would argue that's a regression.

Dell is stuck on an old version of rust for various reasons and upgrading the builder image/toolchain requires a proverbial act of Congress (I love that compiling a new toolchain requires a builder host with 768GB+ of RAM/12 CPUs in order to get the operation done sometime within a day of when operation is started). This is part of the reason why I wasn't on the bandwagon of adopting rust in everything in base, despite the fact that I really like rust as a language in concept.

In D56605#1321232, @ngie wrote:

This forces all end-users of pjdfstest to build/install the new rust binding instead of the simple C application in tree.
I think it would be a good idea to add a build option to build/run either one or the other. Otherwise, some consumers will be broken by this change because not every has access to the rust toolchain.

Ok, git arc worked after squashing my commits.

• fixup: use LTO_UNSAFE instead of WITHOUT_LTO

In D57609#1321156, @markj wrote:

In D57609#1321155, @asomers wrote:

Great. And BTW, I'll apply @arrowd 's suggestion too. I can't update the diff. Lately, arc diff --update doesn't seem to work very well. Even if I haven't rebased my branch, that command complains that it's got far too many revisions to upload. I don't know why.

Have you tried git-arc? It's in tools/tools/git and in the ports tree as devel/freebsd-git-arc. I haven't used arc directly in a long time.

Great. And BTW, I'll apply @arrowd 's suggestion too. I can't update the diff. Lately, arc diff --update doesn't seem to work very well. Even if I haven't rebased my branch, that command complains that it's got far too many revisions to upload. I don't know why.

In D57609#1320673, @markj wrote:

I applied this patch, built a pjdfstest-0.2.2 test package and installed it in a VM. Now pjdfstest just complains about a missing user:

root@freebsd:/usr/tests/sys/fs/pjdfstest # kyua test
tmpfs:tmpfs  ->  failed: atf-check failed; see the output of the test for details  [0.110s]
ufs:ufs1  ->  failed: atf-check failed; see the output of the test for details  [0.190s]
ufs:ufs2_nosu  ->  failed: atf-check failed; see the output of the test for details  [0.153s]
ufs:ufs2_su  ->  failed: atf-check failed; see the output of the test for details  [0.158s]
ufs:ufs2_suj  ->  failed: atf-check failed; see the output of the test for details  [0.172s]
zfs:zfs  ->  sysctl_register_oid: can't re-use a leaf (vfs.zfs.metaslab.condense_pct)!
ZFS filesystem version: 5
ZFS storage pool version: features support (5000)
failed: atf-check failed; see the output of the test for details  [0.882s]
zfs:zfs_nullfs  ->  failed: atf-check failed; see the output of the test for details  [0.196s]
p9fs:p9fs  ->  skipped: No p9fs shares configured  [0.069s]

Results file id is usr_tests_sys_fs_pjdfstest.20260616-144227-803467
Results saved to /root/.kyua/store/results.usr_tests_sys_fs_pjdfstest.20260616-144227-803467.db

0/8 passed (0 broken, 7 failed, 1 skipped)
root@freebsd:/usr/tests/sys/fs/pjdfstest # pjdfstest
error: pjdfstest: no such user
root@freebsd:/usr/tests/sys/fs/pjdfstest # kyua debug tmpfs:tmpfs
Executing command [ mkdir mnt ]
Executing command [ mount -t tmpfs none mnt ]
Executing command [ pjdfstest -c pjdfstest.toml -p mnt ]
Fail: incorrect exit status: 1, expected: 0
stdout:

stderr:
error: pjdfstest: no such user

Files left in work directory after failure: mnt, pjdfstest.toml
tmpfs:tmpfs  ->  failed: atf-check failed; see the output of the test for details

In D56605#1320614, @markj wrote:

In D56605#1320471, @asomers wrote:

It took a while, but I found the problem. It's this commit: https://github.com/freebsd/freebsd-ports/commit/83a19a60d13fe . For some reason, that commit forces panic=abort for any Rust port that uses LTO, which is most of them. I don't know why diizzy included that line. But I can confirm that removing that line fixes the problem.

But why is pjdfstest panicking in the first place?

It took a while, but I found the problem. It's this commit: https://github.com/freebsd/freebsd-ports/commit/83a19a60d13fe . For some reason, that commit forces panic=abort for any Rust port that uses LTO, which is most of them. I don't know why diizzy included that line. But I can confirm that removing that line fixes the problem.

I'll take a look at the UFSv1 failure.

In D57512#1318003, @fuz wrote:

Can we change it to say -1 so it still parses as a number? n/a looks like something scripts will dislike.

I just committed this. But I raised the PORTVERSION to 0.2.1, which no longer requires the third unprivileged user. I didn't update the differential revision because I had to rebase to fix Poudriere, and Phabricator doesn't seem able to cope with rebases very well.

Before:

55880 zones, Maximum LBA 0xda47ffff (3662151679)
Zone lengths are the same, types vary
  Start LBA  Length       WP LBA      Zone Type      Condition      Sequential             Reset
          0,  65536, 0xffffffffffffffff,  Conventional,           NWP,     Sequential,  No Reset Needed
    0x10000,  65536, 0xffffffffffffffff,  Conventional,           NWP,     Sequential,  No Reset Needed

After:

55880 zones, Maximum LBA 0xda47ffff (3662151679)
Zone lengths are the same, types vary
  Start LBA  Length       WP LBA      Zone Type      Condition      Sequential             Reset
          0,  65536,         N/A,  Conventional,           NWP,     Sequential,  No Reset Needed
    0x10000,  65536,         N/A,  Conventional,           NWP,     Sequential,  No Reset Needed

It occurs to me that this might be a good use case for an out-of-tree kernel driver written in Rust. It's easy to test, it's small, and it's not wanted within the tree.

In D57483#1316905, @imp wrote:

I'll ignore the trend we've had towards retiring older-format support generally...

In D57481#1316903, @jlduran wrote:

In D57481#1316901, @asomers wrote:

Ahh, our comments crossed in the ether. But now that I see what you did, I request leaving "pkg install" in. IMHO it's a very important feature of the installerscript that it can install packages.

OK, In that case I must diagnose to see what is failing.

Believe it or not, there's a practical reason for this. I wasn't just tinkering around for fun, honestly.

Ahh, our comments crossed in the ether. But now that I see what you did, I request leaving "pkg install" in. IMHO it's a very important feature of the installerscript that it can install packages.

What's this about "removing the pkg install step"? I don't see anything removed.

In D57326#1313561, @des wrote:

In D57326#1313548, @asomers wrote:

In D57326#1313547, @des wrote:

In D57326#1313542, @asomers wrote:

Why?

options was never correct, it has always been instr. I assume whoever first wrote the page copy-pasted fts_children() and forgot to adjust options to instr.

Crucially, functions that take an options argument accept multiple value bitwise-or'ed together, while fts_set() accepts only one value at a time.

The prototype at the top of the man page documents the argument as being named "options". And the header file does not contradict that. So "options" is definitely the name of the argument, and change e030e4e73fe7 was correct. Whether it _should_ be named that is a different question. I would not describe this change of yours as a reversion.

I just explained to you that the prototype at the top of the page is incorrect. The header does not name the argument. The code names the argument instr and always has; options was never a reasonable name for this argument.

In D57326#1313547, @des wrote:

In D57326#1313542, @asomers wrote:

Why?

options was never correct, it has always been instr. I assume whoever first wrote the page copy-pasted fts_children() and forgot to adjust options to instr.

Crucially, functions that take an options argument accept multiple value bitwise-or'ed together, while fts_set() accepts only one value at a time.

Why?

In D57318#1313384, @jlduran wrote:

Hmm, I was about to submit a change that:

--- a/usr.sbin/bsdinstall/scripts/script
+++ b/usr.sbin/bsdinstall/scripts/script
@@ -50,6 +50,7 @@ f_include $BSDCFG_SHARE/variable.subr
 ############################################################ GLOBALS

 : ${TMPDIR:="/tmp"}
+: ${DISTRIBUTIONS=""}; export DISTRIBUTIONS

 #
 # Strings that should be moved to an i18n file and loaded with f_include_lang()

In my opinion is a "better" fix.

In D56717#1313354, @jlduran wrote:

Sorry for not testing this thoroughly, I just realized that in order for DISTRIBUTIONS to work, the variable must be exported (see inline). Otherwise the user should "manually" export it. If this is going to be the case, then the manual page should be updated to include export DISTRIBUTIONS in the example.