Re: serious performance regression in .41

[christos%astron.com@localhost (Christos Zoulas)]

In article <llj90f$9k6$1%ger.gmane.org@localhost>,
Christos Zoulas <christos%astron.com@localhost> wrote:
>In article <20140521164755.885AE14A5BF%mail.netbsd.org@localhost>,
>Mindaugas Rasiukevicius  <rmind%netbsd.org@localhost> wrote:
>>christos%zoulas.com@localhost (Christos Zoulas) wrote:
>>> Since my last upgrade (I think the last working has been .24), I've been
>>> getting very bad performance running "sync" and choppy interactive
>>> response. It seems something bad happens with locking when there are too
>>> many objects in the buffer cache, because rebooting makes the problem go
>>> away for a while. Here's the lockstat output of sync (which in my 16GB
>>> machine takes ~11sec):
>>
>>The new vfs_vnode_iterator*() mechanism acquires and releases mntvnode_lock
>>on every iteration cycle; see vfs_vnode_iterator_next().  That seems like a
>>significant overhead if there are many vnodes.  In your case, that seems to
>>be more than 150k calls!  As a side effect, such increased overhead probably
>>results in a higher contention against vfs_insmntque() and / or concurrent
>>sync/vflush() calls.  I think the old behaviour should be restored, that is:
>>keep the mntvnode_lock while iterating and drop it only when we find a vnode
>>to sync or recycle.
>
>This is not just it. It vget()s and vrele()s each node during
>vflush()!  Even if they are not needed. I flipped the locking order
>than there was some significant improvement, but the bulk of the
>performance issue is still there. If this does not get fixed soon,
>it should be backed out. It kills the machine.

Here's an ugly fix (but internally only ugly fix):

http://www.netbsd.org/~christos/mntvnode_lock.diff

I also think that:

    bool vfs_vnode_iterator_next(struct vnode_iterator *, struct vnode **);

should be changed to the simpler:

    struct vnode *vfs_vnode_iterator_next(struct vnode_iterator *);

Since it returns NULL/false or non-NULL/true.

christos