I have two comments in addition to the inline ones:

In D53079#1212648, @glebius wrote:

I trust Alan's expertise here. IMHO, given that this patch is analog to fusefs one, should be fine. But not expert in the area.

I too am confused by the commit message. Won't the column always be shown in json and xml output, if "-s" is used?

Could you please give an example of the before and after output?

Could you give an example of what the output looks like before and after?

In D52914#1209926, @olivier wrote:

In D52914#1209913, @asomers wrote:

The MD_LEN variable is now badly named. I think it's ok to leave its value the same. Most test cases don't require 1 MB of I/O. But you should at least change that name.

So, let’s use DEVICE_IO_LEN then (to clarifies that this constant represents the I/O buffer length used for device tests (16 KB), not the size of the MD device itself which is now 1 MB) ?

The MD_LEN variable is now badly named. I think it's ok to leave its value the same. Most test cases don't require 1 MB of I/O. But you should at least change that name.

After seeing this code in practice it seems quite complicated. I do think that the kernel based approach would be simpler. But an important question is: what file systems already use this option on Linux? How many of them would work with either this implementation, or the hypothetical kernel-based one?

So is -1 being cast to true? Thank you GCC for telling us that. If so, I think we should replace ATF_REQUIRE_MSG with ATF_REQUIRE_EQ_MSG. I think that would be more clear.

What's the problem? Under what conditions is the current code incorrect?

I have a few thoughts:

In D52625#1202173, @markj wrote:

I'm sure this will fix the panic, but I can't easily see if it's correct. For instance, if the unix domain socket code does asynchronous fd reclamation using unp_gc_task, the current thread will be some taskqueue thread. Its PID will probably be zero (but this is an implementation detail). I see some special handling for PID 0 e.g. in fuse_filehandle_get_anyflags(). Will this work the way you want in that case?

• Also add a test case for writes to a file that lies in a sockbuf

What bug exactly does this "fix"? Did you encounter a bug in your work, or did you just discover this problem by inspection? If the former, we should add a test case to tests/sys/fs/fusefs.

In D50826#1193323, @arrowd wrote:

Is this going to be landed?

@arrowd I agree with your change. The only thing I wanted different is a more detailed commit message. Of course, Mark might disagree.

Ok. I'm sure that you'll get the NFS client part done. The rest of it looks fine to me.

In D51808#1183471, @rmacklem wrote:

In D51808#1183460, @asomers wrote:

Here's a stupid question. Is it possible for nfsd to reexport an NFS mountpoint? If so, then nfs_copy_file_range will need to check the flag, too.

Not a stupid question. Linux allows this and they have endless bugs to deal
with because of it. FreeBSD does not and I hope to keep it that way.

Here's a stupid question. Is it possible for nfsd to reexport an NFS mountpoint? If so, then nfs_copy_file_range will need to check the flag, too.

Shouldn't there also be some ZFS code that returns ENOSYS if the cloning requirement cannot be satisfied? And fuse_vnop_copy_file_range cannot ever satisfy that requirement, so it should return ENOSYS just like in vn_generic_copy_file_range

In D51782#1183341, @kevans wrote:

In D51782#1183340, @asomers wrote:

Instead of this, I think a better solution would be for ctld to ignore EEXIST errors. That would not only fix your situation, but would also go some way towards recovering from a ctld that crashed (currently, if ctld crashes the only way to recover is to reboot or to manually remove every target with ctladm). The port::kernel_add() method would need to be be adjusted to return an errno, of course. But the ioctls already do. I'm not sure what error code they currently return; that might need to be adjusted.

I had considered that, but this dummy check wasn't clear: https://cgit.freebsd.org/src/tree/usr.sbin/ctld/ctld.cc#n2038 -> this loop effectively does nothing at startup because all kernel ports are dummies. If the intention was to allow them to persist under the assumption they're being managed elsewhere, it seems like we should also have some mechanism to avoid destroying pre-existing LUNs from the kernel in the next loop -> thus I arrive at maybe they *should* be removed as well.

I'm more than happy to fix this to however people (like you) that actively work on ctld see fit, though.

I note that this seems to revert us to pre https://cgit.freebsd.org/src/commit/usr.sbin/ctld/ctld.c?h=stable/14&id=9215e501b9e7d3f425677ffd1d3f1542c9fbb4dc behavior

Instead of this, I think a better solution would be for ctld to ignore EEXIST errors. That would not only fix your situation, but would also go some way towards recovering from a ctld that crashed (currently, if ctld crashes the only way to recover is to reboot or to manually remove every target with ctladm). The port::kernel_add() method would need to be be adjusted to return an errno, of course. But the ioctls already do. I'm not sure what error code they currently return; that might need to be adjusted.

Is there a way to reproduce this bug using unpatched FreeBSD?

In D51371#1181088, @des wrote:

In D51371#1181080, @asomers wrote:

You don't need to create a thick jail. In fact, you don't need a separate jail file system at all. It's totally fine to create a jail anchored at / .

No, it is absolutely not fine. Both jails run daemons which create pidfiles and sockets in hardcoded locations.

In D51371#1173192, @des wrote:

I would be delighted if someone could suggest a better way to populate a thick(ish) jail than what I'm doing here, btw. The jails weigh in at around 200 MB each on amd64.

The commit message says "jail". But kern.vm_guest doesn't change in a jail. Did you mean to say "in a VM"?

You need to add "LIBADD= xo" to usr.bin/fstat/Makefile. Also, a lot of fields are missing in the html output, for example file_type. Is that intentional?

In D51316#1172010, @arrowd wrote:

Would you like me to check if the test really fails on an unpatched system?

The regression test is at D51316 .

I have tested this patch and confirm that it works. I've also written a minimal reproduction case, which I'll post as a separate review. And while I understand Mark's concern, I tend to agree with arrowd's approach. BMAP should be advisory, and readahead should be optional, so errors in neither should cause the main read to fail.
But I would like to see a better commit message. It should contain enough information to describe the problem without resorting to Bugzilla, since Bugzilla might not be around forever.

I have no idea why Arcanist decided to include all of those extra revisions when I updated the diff. I did not rebase. Only the most recent three revisions are intended to be part of the review. I'll abandon and reopen.

• Abandon the custom kernel module. Rely on existing syscalls instead.
• move the test into the kern directory