Closing, since superseded by an upstream fix.

In D33233#1025445, @imp wrote:

There's almost no difference between sync; shutdown and just shutdow other than more typing. The latter does a sync first thing anyway (in the system call) so you may start the io a smidgen earlier with the former... but sync doesn't guarantee the io will be done before shutdown starts... so even doing them on sepatate lines might only gain you the time it takes you to type shutdown at the expense of typing 5 extra characters.

In D33233#1025440, @imp wrote:

Since reboot and shutdown system calls were added just after v7, the sync 3 times dance is a placebo. It was marginally useful on v7 before powering off. It's existed in lore as being necessary when it hasn't been. And since all it does is schedule IO you can't use it to know data is on the disk.

Late to the party, but I find this passage rather obscure:

It replaced the one per line sync as a substitute for waiting.

What does that refer to exactly? To typing only a single sync, as evoked in the previous paragraph ("The shutdown procedure involved running...")?
If yes, I would suggest replacing the whole paragraph:

Typing sync three times (one line each) was a placebo that would generally suffice in 7th edition machines that were otherwise quiesced system. It replaced the one per line sync as a substitute for waiting.

with something like:
"""
Typing sync three times (one line each) would generally suffice in 7th edition machines that were otherwise quiesced system for all dirty buffers to reach the disks. This placebo procedure was in effect just a substitute for waiting.
"""
Or did you mean something else?

I don't see any more path where the pointer to the new struct thread won't be initialized before the new thread is started, so I think this completely fixes the initial problem. Moreover, there most probably won't be any noticeable performance impact, since kproc_create() in the end calls fork1() with RFSTOPPED, so the only difference is now that the new process will be released only very slightly later.

In D44906#1023893, @markj wrote:

Oh hmm, not unrelated, but at a glance I think it is superseded by the privport check. That is, all code paths which lead there first have to go through the privport check. But I could be wrong about this.

There's also NFS_REQRSVPORT, but it is unrelated I guess?

In D44867#1022984, @kib wrote:

Having one (or more) syscalls that differ only in small semantic details makes the overall kernel/user interface much more hard to grasp.

I don't think that, as it stands, the changes here now introduce new LORs or additional recursion, so good to go.

In D44867#1022914, @kib wrote:

This review adds pthread_sigqueue(3) as requested. Right now it is not tested and unfinished: it lacks man pages update and test case, which I add after the issue below is discussed.

As usual, I prefer not to add a new syscall that only differs a small bit from sigqueue(2), where we need to be able to directly designate a thread in the current process. One option is to silently allow to pass tids from the current process, but since one of the supported use cases for sigqueue(2) seems to be a process liveness check for sig=0 (same as for kill(2)), I decided not to. Instead, two high bits in the signum are carved for flags, one meaning the tid from curproc is allowed, and the other is reserved for future. This still gives small ABI incompatibility, but the typical wrong use of signum by passing large invalid signal numbers should be still catched.

In D44788#1022769, @jah wrote:

• Fix fdvp lock recursion during file copy-up; use ERELOOKUP to simplify

The main problem that I see with these changes is that they lead to dropping support for FSes with non-recursive locking (see some of the inline comments). I think there are also some minor problems in the locking/relookup logic (see the inline comments as well).

Going to be superseded by a backport of FreeBSD-EN-23:18.openzfs to OpenZFS 2.1.x and FreeBSD's stable/13.

Hi Seigo,

@zlei: Responsed to your comment above in D44552. Thanks.

Responding to your comment in D44581 here:

In D44581#1017675, @kib wrote:

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.

In D44581#1017709, @kib wrote:

There is a wart in ELF, called copy relocation. When a binary references data object defined in dso, static linker allocates the symbol in the binary and issues the copy relocation, to copy the content of the dso at this symbol to binary. The dso is resolved to reference the binary' object, not its own instance. This was done to emulate the semantic of the .a archives, as everything in ELF.

I'm a priori OK with that as a stopgap measure, although I don't really know how still-running processes interact with the suspend/resume mechanism (which problems can this cause? I can imagine a bunch of them, but not sure if they are impacting).

In D44581#1017690, @kib wrote:

This is a no-starter at all. It would increase the sys_errlist[] size which is de-facto ABI break with all the consequeneces.

I agree with this simple, stopgap measure. It is indeed what FreeBSD-EN-23:18.openzfs achieves, in a much simpler way.

In D44581#1017637, @glebius wrote:

Is there any good reasons to keep in kernel only rather than making it a BSD extension, placing it a few lines above in the errno.h right under #ifndef _POSIX_SOURCE?It is a good error for kldload(1) to return to a user, as long as strerror() supports it.

Although I spent quite some time to understand the signal and ptrace machinery (earlier; I have spent some more for this review), I still don't understand it globally (and probably some details as well). I have a couple of remarks and questions that might improve our common understanding (at worst, hoping answers and info will at least improve mine).

In D44594#1017003, @zlei wrote:

Another option could be refactor linker_file_unload() into two functions. One for normal unload and another is for abnormal ones such as unloading partially linked file.
Almost all abnormal unload requires the flag LINKER_UNLOAD_FORCE and at that moment the reference count of file should be right one. The logic will be much clear.

In D44552#1017019, @zlei wrote:

Rechecked this, CURRENT and stable/14 vfs_byname_kld() has translated error to ENODEV. As for stable/13 it will be leaked.

These changes plug holes indeed.

Is it possible instead to reuse LINKER_FILE_LINKED, by setting this flag later in linker_load_file() (after the if (modules && TAILQ_EMPTY(&lf->modules))), and testing for it? This would also improve the behavior of linker_find_file_by_id() by preventing it from reporting a loaded module that is immediately unloaded by the just-mentioned if.

In D44552#1016580, @zlei wrote:

Move translating errno EUNSDEP from kern_kldload() to sys_kldload()

Great! More cruft removed, including one call to unionfs_relookup(). I have a few suggestions.

Thanks!

In D44101#1006672, @brooks wrote:

That is technically true. It was exposed by abd529cebab9018d0fa9b328cdaaab3a51d87b44 so it was exposed for nearly 15 minutes. :)

I agree with your reasoning. Overall looks fine. Thanks! I have a couple of suggestions.

In D44101#1006706, @kib wrote:

In D44101#1006654, @olce wrote:

Today, its implementation performs (almost) the same as the POSIX-specified sched_yield(), which should be used instead.

It is quite different from sched_yield(). yield() tries to keep thread off cpu as much as possible, while sched_yield() re-schedules immediately.

It seems that a stub was exposed by libc up to 0db2fac06ab70163, unless this is an artifact of the svn or git import.