This is updated to use while loop and depend on outer for loop.
New code looks simpler.

Basically, the idea is to advance array index based on the number of page-ins completed. With new code, a single page-in operation can pull pages back more than the size of d[] array.

New variable here is now 'i' and this must be 0 when code reaches here.

In order for this 'i' to be greater than 0, we must have paged in more than we thought we paged out, based on our sb->d[].

This function is only called by "swapoff" command to detach a swap device.

Should I add something like KASSERT( i > 0, "we paged-in more than we had paged-out" )?

If 'npages' is set within a KASSERT expression, then it won't be set if assertions aren't turned on, and the program will be broken.

I suggested earlier that you just return '1' in this case; you haven't commented on that.

If you invoked this as
npages = vm_page_grab_pages(object, pindex, VM_ALLOC_NORMAL | VM_ALLOC_NOWAIT, ma, SWB_NPAGES);
would it work the same? Would it let you drop the call to swap_pager_haspage?

If you know that i must be 0, then you should add KASSERT(i == 0 ...).

The changes to this file may be good or bad, but they don't seem related to the purpose of this change, so they should be part of a different patch.

If the value of 'a' matters here, then that value can't be set in a KASSERT expression, since if asserts are turned off, 'a' won't be set.

This line seems to choose to use the idea I suggested, which you said didn't work.

The name 'error' is confusing. How about 'haspage'?
Otherwise, you seem to be saying that you expect an error and the program should terminate if there is no error.

Is the value of npages returned form vm_page_grab_pages always going to match the value of npages passed in?

swap_pager_haspage() call here is to find how many EXTRA and adjacent pages available behind the current one. The return value is passed back to the caller via the function variable, npages here, and return code indicates if the function call is successful or not.

At this line of code, we know pindex was paged out and we know we need to take at least one page back from the device. So, actually and after iterations of thinking and discussion, I think our best action is npages = 1 and continue.

I originally added KASSERT because the piece I used as a reference had KASSERT. At this point, I think we'd better drop it.

Okay, I will add KASSERT.

The npages passed in is the maximum. It may return lower number.

On the other hand, I had had few extra condition checks to see how it would work. This call here always returned the same number.

Why is this call needed ?

To be honest, I don't know. My objective was to call swap_pager_getpages() with more than 1 page to page-in faster. I kept the rest of operations same.

Existing code calls swp_pager_force_dirty() equivalent if (m->valid == VM_PAGE_BITS_ALL). Otherwise, swp_pager_force_launder() is called for each of vm_page_t m.

Sorry, I misread the code, I thought that only one page is dirtied. The previous pages in the run are dirtied with the call to swp_pager_force_launder().

The reason for dirtying is that clean pages in swap objects are either zero or have a copy in swap, by definition of clean. So if we are removing the swap, we must ensure that the user content is not going away.

The "else" clause should be reconsidered. swap_pager_haspage(object, pindex)
should never return FALSE, because the caller has determined that there is, in fact, an allocated swap block at pindex of object. And, in fact, if ever it could have returned FALSE, npages should then be set to zero.

More generally, I was surprised to see swap_pager_haspage() called here. The caller to this function already has found the sb structure from which npages will be computed, so I was surprised that it wasn't computed by the caller and passed in.

There is a space after ma[i] that should be deleted.

Indeed, "the caller has determined that there is, in fact, an allocated swap block at pindex of object" is the true statement and thus old code didn't call swap_pager_haspage() but rather called vm_page_grab() to prepare vm_object_t. I added this call to also find how many EXTRA pages continues after the one we know so that we can call swap_pager_getpages() for more than one page.

It sounds like we need to add a panic() if swap_pager_haspage() returns false, instead. Or should I KASSERT?

It seems like this line is now part of a different patch being reviewed elsewhere. In that case, please remove it from here.

Move this line back before the call to vm_page_get_pages().

Deep inside of swp_pager_force_pagein() we will release the object lock, allowing concurrent activity to potentially deallocate sb. So, subsequent lines should not use sb. Instead, the loop should be terminated early and the trie lookup repeated on the old "pi" value.

I don't see the need for "i = 0" here.

n_blks must not exceed MAXPHYS / PAGE_SIZE.