• Fix handling of explicit VNETs passed to $V()
• Improve the README
• Add a simplistic selftest module

In D52733#1204957, @olce wrote:

In D52733#1204931, @markj wrote:

How so? Allocations within an object are serialized by the VM object write lock.

I didn't check initially, was just addressing your concern in D52441.

concurrent allocations might lead to lost increases of the iterator index

If this looks ok, I'll extend it to support PCPU and DPCPU variables. And probably counter(9) too.

This is required for D50825.

Have a hard-coded list in kern.post.mk

• Remove unneeded changes.

• Fix vnet.py to work with kernel modules.
• Move scripts to sys/tools/gdb/.
• Install scripts to /usr/lib/debug/boot/<kernel name>/gdb
• Modify kernel-gdb.py to automatically load those scripts.

Looks mostly ok to me, thank you.

In D52628#1203111, @mjg wrote:

This is finished, it just has extra parts to trigger defered drop + a printf that it happened. I'll upload a cleaned up variant shortly.

Did you verify you get the printfs from vdrop_doomed_process in your testcase?

This patch seems to fix the panics I referred to in my revision. Will you finish this patch?

Remove some unintended modifications.

In D52625#1202183, @asomers wrote:

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?

The FUSE protocol says that every request sent to the server must include the pid, gid, and uid responsible. That's so the server can do access control itself, if it wants to. The entire concept of a fuse_filehandle exists solely to support that requirement. However, it's bullshit. It's a terrible antifeature, because there are many situations in which the kernel can't possibly set those accurately. Most obviously, if a write is coming from the writeback cache. So FUSE daemons can never rely on those fields. Yet, we try to set them anyway, for compliance's sake.

Regarding this particular bug, I'm happy as long as it does not panic. This is another one of those cases where we can't possibly set the uid, gid, or pid to their "correct" values.

There's another stupid detail in the protocol too: fhid. When the kernel does FUSE_OPEN, the server may return an fhid. It's an opaque 64-bit number. But we're supposed to set it in any future request that involves the same open file. It's easily possible for two processes (or even the same process) to open a file multiple times, and we're supposed to keep track of which fhid is needed by each. That's very difficult to due given the design of our VHS. It's easy in Linux, which is probably why it was added to the fuse protocol in the first place. The main purpose, again, is server-side access control. But we have a little optimization. If two different file descriptors have the same uid, pid, gid, and access mode, then we only store one file handle for both. That's because the server almost certainly would treat them the same, for access control decisions. IIRC, that "pid == 0" expression is there precisely because of situations where we don't know the true pid. In such a case, we don't want fuse_filehandle_get_anyflags to match on pid.

So to reiterate, I'm happy as long as it doesn't panic. I'm not upset that the pid field won't be "correct".

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?

The implementation seems ok to me.

In D52542#1201909, @quentin.thebault_defenso.fr wrote:

In D52542#1201906, @markj wrote:

tap_init() will open the device and add it to the mevent loop, which uses kevent() under the hood. But, ng_device doesn't implement d_kqfilter, i.e., ngd_cdevsw doesn't have a d_kqfilter implementation, in contrast with tap devices. How does bhyve know when to read packets from the device?

Please have a look at parent revision D52541, which adds kevent support to ng_device.

tap_init() will open the device and add it to the mevent loop, which uses kevent() under the hood. But, ng_device doesn't implement d_kqfilter, i.e., ngd_cdevsw doesn't have a d_kqfilter implementation, in contrast with tap devices. How does bhyve know when to read packets from the device?

In D52608#1201803, @olce wrote:

It is not safe to modify the vnode structure after releasing one's
reference. Modify vput() to avoid this.

This part I don't understand, as I don't see how it matches the change. If you're the last one dropping a reference, it's safe to modify the vnode, as vput_final() will do, and both versions do not differ in this respect (the hold count is still non-zero, and is decremented at the end of vput_final()). In the original code, you even had the additional "guarantee" of holding the vnode lock. What do you mean exactly by "modifying the vnode structure"?

In D52245#1201575, @kib wrote:

In D52245#1201483, @markj wrote:

Looking at vunref() for a bit and taking fdesc_pathconf() as an example:

vref(vp);
VOP_UNLOCK(vp);
error = kern_fpathconf(...);
vn_lock(vp, LK_SHARED | LK_RETRY);
vunref(vp);
return (error);

The vnode can be vgone()d while the lock is dropped. Suppose usecount == 2 and the vunref() decrements it to 1. Then the VOP_UNLOCK in the VOP_PATHCONF caller can unlock a freed vnode. I don't see a way to fix this without either acquiring the vnode lock in freevnode() in order to provide a barrier (and this sounds deadlock-prone), or changing the VOP contract to permit returning with the vnode unlocked if it is doomed.

I do not understand what you are saying there. Which specific VOP_PATHCONF() would access the freed vnode?

In D52245#1201577, @mjg wrote:

In D52245#1201483, @markj wrote:

In D52245#1201325, @mjg wrote:

Note I proposed a fix to the panic which is orthogonal to the vput change.

So I slept on it. Per my previous remark about possible extra locking *and* not completely taking care of the issue anyway due to vunref, I don't think there is any benefit of this going in (and there is a visible loss)

What about something like the following (ignoring vunref() for a moment)?

if (refcount_release_if_last(&vp->v_usecount))

This would reduce scalability as it would replace fetchadd with a fcmpset loop.

In D52245#1201325, @mjg wrote:

Note I proposed a fix to the panic which is orthogonal to the vput change.

So I slept on it. Per my previous remark about possible extra locking *and* not completely taking care of the issue anyway due to vunref, I don't think there is any benefit of this going in (and there is a visible loss)

Is the proper fix "just" to plumb the acpi softc through acpi_wake_prep_walk()->AcpiWalkNamespace()->acpi_wake_prep()->...? That doesn't seem too painful.

In D52245#1201035, @kib wrote:

IMO the bug is there:
vput()

	if (!refcount_release(&vp->v_usecount)) {
		VOP_UNLOCK(vp);
		return;
	}

vput() released the reference, but still operates on the vnode, unlocks it.

In D52245#1193915, @kib wrote:

If we are in freevnode(), there _must_ be no other users for its pointer. Hopefully, if trylock fails, we should see the lock owner in lk_lock, and then see what it does.

In D52045#1199301, @kib wrote:

In D52045#1193293, @kib wrote:

From my current PoV, this should be more or less finished implementation. In particular, I reviewed all filters for the need of special copy handling.

Ping?

In D52575#1200462, @christos wrote:

In D52575#1200461, @markj wrote:

MFC after: 2 days

Why do you use such short MFC timeouts for non-urgent things? It takes time for other folks to update and test changes, which is the purpose of having the MFC period.

In this case because I thought the change is simple enough. I can bump it to 1-2 weeks if you want.

MFC after: 2 days