Kostik, what does Bruce say about proper SYSCTL style? Is the CTLFLAG_RW supposed to be on the first line and the indentation of the continuation lines four spaces? (Currently, the new SYSCTL's are following the style of the existing ones in this file.)

This comment is confusing IMO. Due to the backing store free, the page is marked dirty, and not queued. But since the page is not worth keeping in memory (it was in swap after all), it should go into laundry.

I am only sure about the 4-spaces indent and the new line before description. I used line break after the CTLFLAG* flags, but indeed Bruce might said that line break should be used before.

What code is supposed to re-queue the page after flush ? I see that the patch added a call to vm_page_deactivate_noreuse() for write completion in the swap pager, but I failed to find something that would re-queue pages for the local vnode pager.

I think this is a valid problem in that there is no general mechanism to requeue the pages. The buffer cache takes care of this for some filesystems it seems, but I don't see how it would work for ZFS.

Thank you for pointing out the buffer cache involvement there, indeed, for filesystems using buffer cache it happens automatically. But IMO relying of the pager behaviour is wrong, and ZFS should not know such peculiarities of VM.

I would expect that e.g. completion code for vm_pageout_flush() take care of the re-queue, esp. because it already handles it for some error cases.

Even for filesystems which use the buffer cache, I don't see anything that automatically unwires the buf's pages once the write completes. vnode_pager_generic_putpages() specifies IO_VMIO, which is translated to B_RELBUF by ext2 and UFS, but it seems that this should really be enforced by generic code.

Unfortunately, it doesn't seem possible for vm_pageout_flush() or even the vnode pager to specify a completion handler - VOP_PUTPAGES provides no mechanism to do so. Am I missing something?

We do not need to force unwire. It is enough for pages to be queued laterm when the buffer is recycled. Buffer cache size is limited to the fixed amount, so the count of pages participating in the VMIO buffers and not visible to the page daemon is limited. OTOH, pages that not queued because they were missed are effectively unswappable until the owning object is destroyed.

VOP_PUTPAGES() is synchronous, more, the typical operation of the vnode pager marks the page clean before the buffer write is initiated. It is, so to say, migrate the dirtyness from the pages to buffer. I mean that vm_pageout_flush() could re-queue the pages after the pager returned.

Forcing an unwire is not strictly necessary, but in the case of laundering, the pages have gone through LRU and are eligible for reclamation. It seems strange to let them exert pressure on the bufspace and go through the buffer cache's own LRU. I think UFS' current behaviour of specifying B_RELBUF is correct.

VOP_PUTPAGES is not synchronous by default - one needs to specify VM_PAGER_PUT_SYNC, and this is only done when v_free_count < v_pageout_free_min. Even if VOP_PUTPAGES allowed one to specify an iodone handler like VOP_GETPAGES_ASYNC does, I don't see a way to implement it as a generic vnode method. vop_stdputpages() currently just calls VOP_WRITE, which also doesn't provide notifications for async writes.

If you want to modify the bufcache behaviour WRT unwiring of the laundered pages, then vfs_vmio_unwire() looks like a proper place. It already tried to free pages or affect their LRU position on unwire in several cases, so one more case is not too outstanding.

I tried to express that VOP_PUTPAGES() is synchronous from the VM PoV: the page is marked clean outright, even before the write is scheduled somehow in the io subsystem. It is io level which records the need of performing write, and e.g. for clustering allowed (async putpages in VM terms), the dirty buffer may sit on the dirty queue until buffer or syncer daemons care about it. But from the VM look, the page is clean after the successful return from vm_pager_putpages(), and sometimes even earlier. So vm_pageout_fault() can do whatever re-queuing attempts it finds suitable, after the pager call.

Do we use VM_PAGER_PEND anywhere besides the swap pager?

To Kostik's point, I think that I agree. We should remove the vm_page_dequeue() calls from vm_pageout_cluster() and instead call vm_page_deactivate_noreuse() in vm_pageout_flush() when the pager returns VM_PAGER_OK.

However, there is one catch. We shouldn't automatically call vm_page_deactivate_noreuse() when vm_pageout_flush() is called by msync(), or in general any caller besides the laundry thread. I think it would suffice to test whether the page is in the laundry queue, and only call vm_page_deactivate_noreuse() if it is. That way, we would also handle the case where msync() is performed on a page in the laundry queue.

Turning to Mark's point, we ought to tell the buffer cache to immediately release the buffer and perform vm_page_deactivate_noreuse() on the pages. However, I don't think that any of the existing flags that vm_pageout_flush() can pass to vm_pager_put_pages() accomplishes the latter. Am I wrong?

Ok, I understand the suggestion now. I think that queuing the page using vm_page_deactivate_noreuse() if it was on the laundry queue is a reasonable policy, and we can use the B_NOREUSE flag to effect this in the buffer cache.

It does indeed seem like we need to add a new VM_PAGER_PUT_* flag to signal our intent to VOP_PUTPAGES, and it also needs to be plumbed through VOP_WRITE somehow for the generic PUTPAGES implementation.

If we add a new VM_PAGER_PUT_* flag, then we actually don't need to test whether the page is in the laundry queue: vm_pageout_flush() takes the pager flags as a parameter, so we can just set the flag in vm_pageout_cluster() and use that to determine where to queue. That way, msync and so on will be unaffected.

VM_PAGER_PEND only appears to be set in the swap pager.

That way, msync and so on will be unaffected.

We might have a page in the laundry queue that is actually laundered by the msync(2) call. In that case, we would want the page to be moved to the inactive queue. If the page has been referenced while in the laundry queue, there shouldn't be a problem with having used vm_page_deactivate_noreuse() on the page because vm_pageout_scan() will see the reference and not reclaim the page.

Here is the proposed patch:

Index: vm/vm_pageout.c
===================================================================
--- vm/vm_pageout.c     (revision 307753)
+++ vm/vm_pageout.c     (working copy)
@@ -405,7 +405,6 @@ vm_pageout_cluster(vm_page_t m)
         */
        vm_page_assert_unbusied(m);
        KASSERT(m->hold_count == 0, ("page %p is held", m));
-       vm_page_dequeue(m);
        vm_page_unlock(m);
 
        mc[vm_pageout_page_count] = pb = ps = m;
@@ -448,7 +447,6 @@ more:
                        ib = 0;
                        break;
                }
-               vm_page_dequeue(p);
                vm_page_unlock(p);
                mc[--page_base] = pb = p;
                ++pageout_count;
@@ -474,7 +472,6 @@ more:
                        vm_page_unlock(p);
                        break;
                }
-               vm_page_dequeue(p);
                vm_page_unlock(p);
                mc[page_base + pageout_count] = ps = p;
                ++pageout_count;
@@ -550,6 +547,10 @@ vm_pageout_flush(vm_page_t *mc, int count, int fla
                    ("vm_pageout_flush: page %p is not write protected", mt));
                switch (pageout_status[i]) {
                case VM_PAGER_OK:
+                       vm_page_lock(mt);
+                       if (vm_page_in_laundry(mt))
+                               vm_page_deactivate_noreuse(mt);
+                       vm_page_unlock(mt);
                case VM_PAGER_PEND:
                        numpagedout++;
                        break;

Looks fine.

Seems right to me. I can work on the corresponding buffer cache change, but that's probably not a prerequisite to merging PQ_LAUNDRY?

No, I don't think it's a prerequisite.

This ought to have a more accurate description. Suggestions?

"pages eligible for laundering"?

Should the "2" here be "VM_INACT_SCAN_INTERVAL"?

Yes. That name doesn't really make sense though - it's a rate.

How about:

diff --git a/sys/vm/vm_pageout.c b/sys/vm/vm_pageout.c
index d09dccb..c996797 100644
--- a/sys/vm/vm_pageout.c
+++ b/sys/vm/vm_pageout.c
@@ -155,11 +155,9 @@ static struct kproc_desc vm_kp = {
 SYSINIT(vmdaemon, SI_SUB_KTHREAD_VM, SI_ORDER_FIRST, kproc_start, &vm_kp);
 #endif

-/* Sleep intervals for pagedaemon threads, in subdivisions of one second. */
-#define        VM_LAUNDER_INTERVAL     10
-#define        VM_INACT_SCAN_INTERVAL  2
-
-#define        VM_LAUNDER_RATE         (VM_LAUNDER_INTERVAL / VM_INACT_SCAN_INTERVAL)
+/* Pagedaemon activity rates, in subdivisions of one second. */
+#define        VM_LAUNDER_RATE         10
+#define        VM_INACT_SCAN_RATE      2

 int vm_pageout_deficit;                /* Estimated number of pages deficit */
 u_int vm_pageout_wakeup_thresh;
@@ -1149,7 +1147,7 @@ vm_pageout_laundry_worker(void *arg)
                 */
                if (shortfall > 0) {
                        in_shortfall = true;
-                       shortfall_cycle = VM_LAUNDER_RATE;
+                       shortfall_cycle = VM_LAUNDER_RATE / VM_INACT_SCAN_RATE;
                        target = shortfall;
                } else if (!in_shortfall)
                        goto trybackground;
@@ -1211,7 +1209,7 @@ trybackground:
                                target = 0;
                        }
                        launder = vm_background_launder_rate * PAGE_SIZE / 1024;
-                       launder /= VM_LAUNDER_INTERVAL;
+                       launder /= VM_LAUNDER_RATE;
                        if (launder > target)
                                launder = target;
                }
@@ -1225,7 +1223,7 @@ dolaundry:
                         */
                        target -= min(vm_pageout_launder(domain, launder,
                            in_shortfall), target);
-                       pause("laundp", hz / VM_LAUNDER_INTERVAL);
+                       pause("laundp", hz / VM_LAUNDER_RATE);
                }

                /*
@@ -2001,7 +1999,7 @@ vm_pageout_worker(void *arg)
                         */
                        mtx_unlock(&vm_page_queue_free_mtx);
                        if (pass >= 1)
-                               pause("psleep", hz / 2);
+                               pause("psleep", hz / VM_INACT_SCAN_RATE);
                        pass++;
                } else {
                        /*

I agree. Commit your proposed change.

I don't think that we ever consciously chose between vm_page_deactivate() and vm_page_deactivate_noreuse() here. Using vm_page_deactivate() will preserve the contents of this "failed page" from reclamation for a little longer. Is there actually a reason to prefer that?

I can't see any good reason either way. It looks like the failure modes that lead to VM_PAGER_BAD are transient (e.g. vnode is being reclaimed) and will lead to the page being freed by another mechanism.

After sleeping on the question and your response, I have a slight preference for using the _noreuse option. I'm also going to condition the _noreuse call on whether the page is in the laundry, like we did for the OK and PEND cases. This way, msync() pages will remain in their current queue, unless they were in the laundry.