Re: Serious performance issues, broken initialization, and a likely fix

Scott Long wrote:
> Ade Lovett wrote:
>> To cut a long story short, the order in which nswbuf is being
>> initialized is completely, totally, and utterly wrong -- this was
>> introduced by revision 1.132 of vm/vnode_pager.c just over 4 years ago.
>>

[snip]

> My vote is to revert rev 1.132 and replace the XXX comment with a more
> detailed explaination of the perils involved.  Do you have any kind of
> easy to run regression test that could be used to quantify this problem
> and guard against it in the future?  Thanks very very much for looking
> into it and providing such a good explaination.

It's not quite as easy as reverting 1.132, since we're talking about at
least two files here, vm/vnode_pager.c and kern/vfs_bio.c

How to handle it properly is a little more complicated since the code
that actually assigns a value to nswbuf is inside
kern_vfs_bio_buffer_alloc() which can be called multiple times as part
of the again goto loop in vm_ksubmap_init().

My personal preference would be to rip out lines 446-494 (RELENG_6) of
kern_vfs_bio_buffer_alloc() into a separate function, which sets nbuf
and nswbuf, and also have vnode_pager_init() call that routine.

As to an easy to run regression test, the code clearly states that there
is a minimum value of nswbuf:

* swbufs are used as temporary holders for I/O, such as paging I/O.
* We have no less then 16 and no more then 256.

However, the check for this is based on:

	options NSWBUF_MIN=<somenumber>

being defined in the kernel configuration file (and propagated to
kernel/opt_swap.h as part of config(8).

Defining a couple of handy constants (in sys/conf.h ?):

	#define NSWBUF_ABSOLUTE_MIN	16
	#define NSWBUF_ABSOLUTE_MAX	256

The current check (based on NSWBUF_MIN) could be rewritten as follows:

#ifdef NSWBUF_MIN
	if (nswbuf < NSWBUF_MIN)
		nswbuf = NSWBUF_MIN;
#endif
	if (nswbuf < NSWBUF_ABSOLUTE_MIN) {
		printf("Warning: nswbuf too low (%d), setting to %d\n",
			nswbuf, NSWBUF_ABSOLUTE_MIN);
		nswbuf = NSWBUF_ABSOLUTE_MIN;
	}
	if (nswbuf > NSWBUF_ABSOLUTE_MAX) {
		printf("Warning: nswbuf too high (%d), setting to %d\n",
			nswbuf, NSWBUF_ABSOLUTE_MAX);
		nswbuf = NSWBUF_ABSOLUTE_MAX;
	}

and then, along with the code moving outlined above, having an undefined
value (or 0) of nswbuf could Never Happen[tm].

Indeed, at this point, where nswbuf is actually used in the RHS of an
assignment, one could even force a panic if it somehow it got missed in
future -- I believe in this case, a full blown panic would be justified,
given the massive loss of performance that occurs with only one swap buffer.

-aDe