swap_pager: deoptimize swapoff_object

[dougm]

Summary

Undo a change in swap_pager_swapoff_object that assumed a swap block would be valid after reacquiring a lock, when that validity cannot be assumed.

[alc]

No, this is not what @markj meant.

[dougm]

In D46753#1065827, @alc wrote:

No, this is not what @markj meant.

Maybe not.

Here's what I see. In stable/14, there's this code (https://cgit.freebsd.org/src/plain/sys/vm/swap_pager.c?h=stable/14):

			vm_object_pip_add(object, 1);
			rahead = SWAP_META_PAGES;
			rv = swap_pager_getpages_locked(object, &m, 1, NULL,
			    &rahead);
			if (rv != VM_PAGER_OK)
				panic("%s: read from swap failed: %d",
				    __func__, rv);
			VM_OBJECT_WLOCK(object);
			vm_object_pip_wakeupn(object, 1);
			vm_page_xunbusy(m);

			/*
			 * The object lock was dropped so we must restart the
			 * scan of this swap block.  Pages paged in during this
			 * iteration will be marked dirty in a future iteration.
			 */
			break;
		}

The two lines @markj has highlighted aren't there. I put them in main. Assuming that they are a problem, and that stable/14 is correct, I seek to restore correctness by removing them. I thought that including them was an optimization. I assume that in stable/14 the next iteration will likely walk the same swapblk and find that page and process it properly, and that this change would have the same effect.

[markj]

In D46753#1065828, @dougm wrote:

In D46753#1065827, @alc wrote:

No, this is not what @markj meant.

Maybe not.

It is. :)

I meant to point out that the update of &sb->d[i] following reacquisition of the VM object lock seems wrong.

P.S. I am traveling until Oct 3 and will be slower than usual to reply to reviews before then.

Here's what I see. In stable/14, there's this code (https://cgit.freebsd.org/src/plain/sys/vm/swap_pager.c?h=stable/14):

			vm_object_pip_add(object, 1);
			rahead = SWAP_META_PAGES;
			rv = swap_pager_getpages_locked(object, &m, 1, NULL,
			    &rahead);
			if (rv != VM_PAGER_OK)
				panic("%s: read from swap failed: %d",
				    __func__, rv);
			VM_OBJECT_WLOCK(object);
			vm_object_pip_wakeupn(object, 1);
			vm_page_xunbusy(m);

			/*
			 * The object lock was dropped so we must restart the
			 * scan of this swap block.  Pages paged in during this
			 * iteration will be marked dirty in a future iteration.
			 */
			break;
		}

The two lines @markj has highlighted aren't there. I put them in main. Assuming that they are a problem, and that stable/14 is correct, I seek to restore correctness by removing them. I thought that including them was an optimization. I assume that in stable/14 the next iteration will likely walk the same swapblk and find that page and process it properly, and that this change would have the same effect.

That is my understanding as well.

sys/vm/swap_pager.c

1955

I think the assertion can and should remain.

[dougm]

Let the assertion remain.