The control flow here would be less confusing to me if this i == 0 case handling was moved out of the loop:

restart:
if (object->flags & OBJ_DEAD) {
    ...
}
swblk_iter_init(&blks, object);
sb = swblk_start(&blks, pi);
swp_pager_init_freerange(&range);
for (i = 0;;) {
    ...
}

and anywhere the loop drops the VM object look, instead of i = 0; continue;, goto restart;.

This assignment could be moved into the loop header.

Unrelated to this diff, but, how is it safe to dereference sb->p while srcobject is unlocked? That seems wrong.

Similarly, I don't think it's safe to access sb after reacquiring the lock. Am I missing something?

Would it be possible to have an explicit iter_invalidate operation which resets the path but leaves the current index and limit alone?

Where swap_pager_copy is called in vm_object_split, all the pages are xbusied first.

Where swap_pager_copy is called in vm_object_collapse, vm_object_collapse_scan does a "check for busy page" and calls vm_object_collapse_scan_wait for non-busy objects, which does ... I have no idea.

I can change this to save the sb values before releasing the lock, but I don't know that to be correct. I can change this to do a big goto restart after reacquiring the lock. But I'd just be making wild stabs in the dark.

Sure.

What we really want here is a swap pager operation which returns the next pindex for which there is no swap block. Absent that, I'm not sure it's really worth maintaining an iterator here. (I'm trying to argue here that swap_pager_find_least() should keep its old interface, since I think that permits some simplification of the iterator interface.)

Can we have two nested loops instead?

If m == NULL here, then the object lock was dropped and the iterator is invalid.

This should go into the #ifdef _KERNEL block, together with the functions that reference it.

The first part of this loop is searching for the smallest pindex >= pi for which there is a resident page or an assigned swap block. I wonder if it'd make sense to have a combined interface which searches for this pindex, since the vm_page scan will eventually also make use of iterators. (And even more radically, I wonder if it'd be possible and sensible to use the same trie for both resident pages and swap blocks.)

In other words, I wonder if we should just leave this loop as it was before for now.

@alc was suggesting this reuse just yesterday. He observed that some nodes would have both pages and swap blocks.

The indentation here is wrong.

Is it safe to access sb here, after the object lock was dropped and reacquired? Perhaps the existence of an xbusy page in the object at that pindex is sufficient to provide that guarantee.

Indeed, we'd need to come up with some encoding for the keys which handles this overlap. I don't have any suggestions offhand. One comment is that instead of storing vm_page pointers, we could likely compress them into a (segid, offset) tuple which should in general require less than 64 bits.

The comment does not match the code.

Yes.

And swblk_next -> swblk_iter_next too.

Added a KASSERT to justify the comment.

A clerical error. Restored.

Why is this before the call to swblk_iter_next()? Alternatively, does this variable need to exist? Couldn't restart do swblk_iter(&blks, object, blks.index);?

This is not style(9).

From a performance standpoint, I'm skeptical of using iterators in cases like this where there will be 0 or 1 calls to swblk_iter_next(). If there are 0 calls, the cost of setting up the iterator is wasted cycles. If there is 1 call, the savings from the old implementation, i.e., walking the trie again, which should be almost all dcache hits, will be offset by the icache misses for executing a new function, swblk_iter_next(). In contrast, the old implementation simply used the same function twice.

The two callers of swap_pager_find_least both call it from within loops, and an earlier version of this had those functions passing an iterator into this function, rather than having this function create it's own every time. But @markj rejected that usage.

Don't you need to reset the page iterator too?

To be clear, my position was that we should try to avoid exposing the iterator interface directly and instead provide a function which can be used to interleave iteration of the swap block and VM object tries, since that's what those callers are doing.