The vnode lock is held, and at least for usual filesystems like UFS it is held in exclusive mode. Then the vn_io_fault() code cannot run in parallel.

For ZFS and tmpfs it is not true, ZFS causes only shared mode vnode lock held, while tmpfs pages belong to swap object. So io really can occur in parallel but I think this is mostly harmless, although indeed not too pretty. It might cause partial or contradictory content to persist if system crashes, but I suspect that ZFS provides some internal synchronization there (I do not know for sure).

I'm skeptical that the hold_count tests serve any useful purpose. Moreover, if we want to say, "hands off this page because it's undergoing I/O,", we have other ways of saying that. It shouldn't be the role of hold_count.

It looks like the hold_count tests were added when the page holding mechanism was introduced in r1549. I believe that the tests are not needed for correctness; my guess is that we wanted to avoid writing "in-flux" dirty pages that are perhaps likely to be re-dirtied soon again anyway. That makes sense as a rationale for testing hold_count in the PQ_LAUNDRY scan, but not so much in the clustering code I think.

I've had a chance to think some more about this.

Consider kern_physio.c calling vmapbuf(). In other words, places where we hold a mapped page; create a temporary mapping to that page in the kernel address space (or simply use the direct map); and then modify the page through the temporary mapping.

Now, suppose that that page is dirty and we are about to launder it. The holder's temporary mapping bypasses the write protection that is applied to the page by vm_pageout_flush().

So, if we are not paying attention to the hold count in the laundry code, how do we know whether the holder's write through the temporary mapping occurred before or after the page was written to disk? Skipping held pages in the laundry was probably, once upon a time the simplest approach to this potential problem.

Rhetorical question: Why did I say, "once upon a time"? Well, before the introduction of vm_page_lock(), the clustering code held the page queue lock, which by the way was also required to hold a page. So, I think that there was actually some ordering guarantee between these operations provided by the page queue lock. However, now, I don't see anything blocking a holder from jumping in right after the clustering code completed. Moreover, I'm not convinced that all holders are checking for, for example, the busy state of a page before writing to it.

To be clear, my initial statement that the hold count served no purpose is clearly contradicted by what I wrote above. :-) However, that doesn't mean that we should keep them. As I said in the previous paragraph, I don't see how we could eliminate the potential races. I think that we need to audit the holders and take appropriate steps in the holders to synchronize with possible laundering.

I started performing such an audit and quickly discovered r253939 and r253953. I'll admit that I don't really understand the reasoning for the revert: 1) the vget() call in vm_fault_hold() uses LK_NOWAIT to avoid sleeping on the vnode lock with the fault page busied, presumably because vm_object_clean(), for example, may sleep on a busy page with the object's vnode lock held; 2) vm_fault_hold() will sleep forever if it finds that the page is resident and already busied. What am I misunderstanding?

Mentioned revision replaced hold with busy.

Busy state is de-facto a sleepable lock. This lock is after the vnode lock in the global order. Besides mentioned vm_fault_hold() and vm_object_page_clean(), this order is followed by buffer cache and misc. implementations of VOP_GETPAGE() in filesystems.

For one example where the referenced change would break things, consider exec: there, exec_map_first_page() busies the first executable page, while the binary vnode is locked. But then the busy state is converted to the hold, because the binary's vnode lock is dropped several times during execve(2) flow. Keeping the page busy would contradict the order when relocking, causing deadlocks.

Another example is the vfs_vnops.c, where deadlock avoidance code hold pages, then unlock the vnode to access the userspace in uiomove(). Before vn_io_fault(), the userspace was accessed while vnode and buffer locks were held, causing well-known deadlock (I call it UPS deadlock by Stephan Uphoff who provided a code to reliably reproduce the bug, see tools/test/upsdl).

Yet another example is the tmpfs io, where again, if we would busy tmpfs node object' pages around uiomove() in uiomove_object_page(), we risk faulting on them if userspace asked for io to/from tmpfs node mapping and deadlocking.

I believe that similar examples existed for GEM GTT or CPU mappings in current drm2 code.

There should be more cases, but I think that the list is good enough. It would work to replace hold with wire, but this was not done because hold is much cheaper. It will break if replacing hold with busy. Hold and wire are non-sleeping, while busying is sleepable.

Thanks for the explanation. Reading through the vm_page_hold() callers in the tree, we seem to have two cases: 1) vm_page_hold() is called while the object is locked and after we guaranteed that the page is unbusied by anyone else, and 2) pmap_extract_and_hold(), where the page is found in a pmap. The laundry thread already synchronizes properly with case 1 since it holds the object lock throughout vm_pageout_cluster() and busies the pages before releasing the lock. There's nothing synchronizing a concurrent vm_pageout_cluster() and pmap_extract_and_hold(VM_PROT_WRITE), I think, since the laundry thread calls pmap_remove_write() after dropping the page lock. As Alan noted, before r207410 we handled case 2 properly.

I'm wondering if we can handle synchronization of case 2 with laundering by simply moving the pmap_remove_write() call in vm_pageout_flush() to under the page lock in vm_pageout_cluster(). I note that the other caller of vm_pageout_flush(), vm_object_page_collect_flush(), already removes write mappings.

Yes, indeed this may work. The interaction with the vm_fault_soft_fast() case should be similar.

vm_fault_soft_fast() checks the busy state and then (possibly) holds the page without dropping the object lock at any point in between. Isn't that sufficient?

Fast path only check for busy state when when mapping is for write. If it maps for read, then busy is fine, in fact this behavior is modelled after vm_map_pmap_enter().

But the fact that a write access would cause page fault which would wait for busy should make your change in D12084 correct.

Returning to this discussion, I see some vm_fault_quick_hold_pages() callers that immediately wire and unhold the returned pages (see zbuf_sfbuf_get() for instance). That suggests that we really do need the wire_count checks in vm_pageout_cluster(), since we otherwise won't detect the existence of an unmanaged writeable mapping. Alternately, perhaps such callers should be modified to avoid unholding the page and instead simply unhold at the same time as unwiring.

Why should we care about unmanaged writes ? It is the caller choice, otherwise it would busy or otherwise protected the page state.

Also, what do you mean by checking for wired in vm_pageout_cluster() ? Do you mean that wired pages should be excluded from laundry ? Then we put the user data at risk, e.g. the writeable file which is mapped and happen to wired still must be msynced periodically.

In case of bpf, the pages wired should be normal pages (usually).

I think wiring would count as "otherwise protected the page state," but after this change it might not: vm_pageout_cluster() skips pages not in the laundry queues, and if wiring a page always removes it from its page queue, then we won't attempt to launder it. With this change, the laundry thread skips wired pages in its queue scan, but may still cluster with wired pages.

Right, I meant that I should perhaps restore the wire_count != 0 next to the hold_count != 0 checks in vm_pageout_cluster(). Note that vm_pageout_cluster() is used only by the laundry thread, so I believe your comment about msync doesn't apply. vm_object_page_collect_flush() doesn't check hold_count either.

With this change, the laundry thread skips wired pages in its queue scan, but may still cluster with wired pages.

Ah, I understand.

The "by yet another map" is archaic, and we should just delete it. Essentially, it is implying that wiring only happens through mappings.

Aren't we implicitly changing the behavior here? Specifically, calling this function on a lazily wired page, i.e., one that hasn't been removed from the active queue, will now have its act_count updated.

While we are here, please convert this to the newer form: vm_page_assert_locked(m).

"Initialize act_count."

Stylistically, I don't think that we need a blank line here.

Again, since we're here, should we go ahead and change the return type to bool?

Repeating myself (and to be clear this isn't a complaint :-)), I don't think that we gain anything (in terms of the replacement policy) by requeueing active pages. Do you?

I don't, no. We just need to set PGA_REFERENCED, I think.

Indeed... it's not obvious to me that the new behaviour is incorrect though. The page daemon will ignore act_count when it visits this page, if it's still wired. Otherwise, vm_page_unwire(PQ_ACTIVE) will call vm_page_activate() anyway, so we're guaranteed to have act_count = ACT_INIT, and if we unwire into another queue, the value of act_count doesn't matter. So unless I'm missing something, the difference in behaviour is inconsequential.

If we call vm_page_unwire(PQ_NONE) on a page in a queue, we leave it in the queue. I can't think of any specific scenarios where that would lead to a problem, though.

I think that we should remove the page from the queue.

To be clear, I've looked at all of the vm_page_unwire(PQ_NONE) calls that we have today, and l don't see a problem with any of them because they all immediately call vm_page_free() or vm_page_deactivate{,_noreuse}().

However, someday I might do something like this:

vm_page_lock(m);
vm_page_unwire(m, PQ_NONE);
vm_page_unlock(m);

expecting afterwards that the page can't be reclaimed by the page daemon because it is either wired or not in a page queue. But, if we lazily leave the page enqueued when the wire count transitions to zero, the page daemon will see no reason not to reclaim the page.

Put another way, not removing the page represents a subtle change in semantics that I would consider surprising.

This would appear to deactivate an active page, and not just enqueue a page that is not in any queue. If the page is referenced, it will be reactivated, so I don't think that there is any real harm to the replacement strategy's ordering, just pointless shuffling of the page between queues.

Ditto.

I think we want to deactivate pages in the laundry as well, so in the latest revision I just avoid deactivating active pages.

I have done something with the buffer cache recently which just sets a flag when something needs to be re-queued. This could be useful here to avoid locking the pagequeue for something that is very frequently referenced. It simply implements a second chance algorithm when the flag is seen.

If we really must repeat this check so many times I would prefer that it was an inline with a comment. Alternatively, does a wire_count > 0 imply a hold count? Should we bump hold when we bump wired?

Right now, a scan of PQ_INACTIVE or PQ_LAUNDRY moves referenced pages to PQ_ACTIVE. Are you suggesting adding a second type of reference flag that simply means "requeue"?

I can't see any reason we can't bump the hold count with the wire count.

Yes, that's right. In the buf case I set a flag that is protected by the buf lock. buf_recycle() will move the buf to the tail and clear the flag if it is set. It is not perfect lru, it is second chance, but it eliminates a ton of lock activity for things like indirect blocks which are accessed very frequently.

The active queue already does basically the same thing when it discovers page activity. It clears ACTIVE and requeues the page. The same flag could be used when we hold and release the page. Right now hold doesn't move the page in the LRU like wired does. If we had such a flag it could do so without touching the pq locks.

I think it would be reasonable to set such a flag in all of vm_page_unwire(), vfs_vmio_unwire(), and sendfile_free_page(). The only other vm_page_requeue() calls in the tree are vm_swapout_object_deactivate_pages(), and those apply only to pages in PQ_ACTIVE. (I'm not sure that the requeue is really crucial in that case either.)

I'll work on adding such a flag, but that belongs in a separate diff.

One possible issue comes from the fact that hold_count is only 16 bits wide. I don't think it would be hard to construct a scenario that triggers an overflow if we bump hold_count along with wire_count, so for now I'd like to hold off on making that change.

I'd like to simplify the situation we have with hold/wire/busy mechanisms. This change makes holds and wires more similar than they were, so it seems like a step in the right direction. I'll also write a block comment for vm_page.h that describes these mechanisms and the relationships between them.

I added a block comment.

I'm ok with adding an inline for testing hold_count > 0 || wire_count > 0, but am not sure what it should be called. vm_page_is_referenced() isn't great since it's unrelated to vm_page_reference().

I am not sure I follow the distinction between page structure and page content. Either of hold/wire or busy prevent page reclaim.

Also, I believe that the description makes things more confusing than clear by mixing hold/wire and busy. I agree that hold and wire are very similar, esp. after the patch in this review. While busy mechanism' purpose is quite different.

I believe you mean pmap_extract_and_hold() and vm_fault_quick_hold_pages(). The order between wait for busy and the vnode lock there is only a symptom, really we do not want to block other page accesses while driver or filesystem hold the user page as the source of data io.

It is useful to note that the page identity might change during the sleep, so the waiter needs to re-lookup the page in the object after the wait.

I just wanted to avoid confusing the counters with the page's reference bits. I'll reword this.

I described the busy lock in a separate paragraph. I can remove the reference to it here, but I had kept it because of the note that hold_count is used in place of the busy lock in some areas.

May be, 'contains two counters, each of which prevents the page reuse' Then you avoid the word reference and do not need to explain that this is not PGA_REFERENCED or m->active.

Again, remove 'reference' there.

I removed two uses of "reference," but I think the remaining usage is clear from context.