Rather than adding a new result and passing that around, can we instead add a new page_sbusied flag to the fault state? Or, can we just use vm_page_xbusied(fs.m) to check for this case instead of adding new state?

We cannot create a writeable mapping while a page is sbusy. Or at least, we do not permit it today. (See, e.g., the assertion in vm_pageout_flush() which checks that the newly-written pages are not mapped for writing.) In other words, I think this part of the change is wrong.

As an aside, I wonder why we require the contents of pages to be stable while they are being flushed to disk. For filesystems which perform data checksumming, it is maybe required (not for ZFS since it copies the data to be flushed), but most of our filesystems do not care. Similarly, I cannot see why it would matter for swap-backed pages. So long as we do not incorrectly mark pages clean, I cannot see why this is required from a correctness perspective, though it is certainly easier to reason about.

I don't quite follow this. If fs->m is xbusied, then we must have res != FAULT_SOFT_MSHAREDBUSY, so why do we handle the sharedbusy case specially above?

I don't quite follow--what if m_cow->object is mapped directly into another process' address space? Oh, you are also checking m_cow->object->ref_count == 1.

I think this explanation doesn't justify the code. The original problem scenario is that object O has a shadow object O', and O' is mapped by multiple processes.

Then, suppose m_cow is mapped read-only into all of those processes. Suppose some process triggers a copy-on-write fault, causing m_cow to be copied into the shadow object. We must shoot down all of the mappings of m_cow, because it is fully shadowed and may be freed.

I cannot see how that is prevented in the sbusy case. I also do not see how "the sbusy case is only possible when the shadow object is read-mapped" is true. Here we are handling a COW fault, so clearly the shadow object is mapped RW.

I think this observation can be used to simplify the whole comment. I can submit a patch for that after this work is landed.

This place is somewhat worrisome. fs->m_cow is only shared-busy. I can check that first_object->OBJ_ONEMAPPING is clear in the condition for shared-busy path.

If this is a sharedbusy soft fault, then before the unlock can we assert that

1. there is at most one mapping of the m_cow page,
2. that mapping belongs to the current pmap

?

Historically, we have written "COW", not "CoW". I only found one instance of the latter under vm/.

Note that pmap_enter() uses the VM mapping protections to decide whether to map a page RW, so even a read fault may install a RW mapping. In this case, creation of the first writable mapping for a page requires the page to be xbusied, since pmap_remove_write() requires only that the page be sbusy. So, I think this check isn't sufficient, you have to disallow sbusying when (fs->prot & VM_PROT_WRITE) != 0 && fs->object == fs->first_object.

But then, maybe we can simplify the check and sbusy only when fs->object != fs->first_object? Soft faults in the top-level object are mostly handled by vm_fault_soft_fast().

I believe the check here is racy, since fs->first_object is not locked. And the parentheses look wrong.

Why disallow shared busy after the first failed attempt?

Maybe, "Now make sure that racily checked conditions are still valid"

I think this comment about ONEMAPPING is stale?

Since the condition was checked once already.

So in this case we will call vm_fault_busy_sleep(), but why? Why not unbusy the page and fall through to the vm_page_tryxbusy() below?

Shouldn't the predict_true apply to the whole predicate?

I wonder if it's worth maintaining a counter here?

I prefer just rangelocked.

This comment is inaccurate now.

The comment about the object lock is false in the sbusy case.