Thanks for looking into this.

I think the correct change is to test
		if (atomic_read(&svsk->sk_inuse) > 1 || test_bit(SK_BUSY, &svsk->sk_flags))
or even
		if (atomic_read(&svsk->sk_inuse) != 1 || test_bit(SK_BUSY, &svsk->sk_flags))

sk_inuse contains a bias of '1' until SK_DEAD is set.  So a normal,
active socket will have an inuse count of 1 or more.  If it is exactly
1, then either it is SK_DEAD (in which case there is nothing for this
code to do), or it has no users, in which case it is appropriate to
close the socket if it is old.
Note that this test is for "the socket should not be closed", so we
test if it is *not* 1, or > 1.

The tests are needed because we don't want to close a socket that
might be inuse elsewhere.  The SK_BUSY bit combined with the sk_inuse
count combine to check if the socket is in use at all or not.

You change effectively disabled the test, as sk_inuse is never <= 0
(except when SK_DEAD is set).

This bug has been present since 
commit aaf68cfbf2241d24d46583423f6bff5c47e088b3
Author: NeilBrown <neilb@suse.de>
Date:   Thu Feb 8 14:20:30 2007 -0800

(i.e. it is my fault).
So it is in 2.6.21 and later and should probably go to .stable for .21
and .22.

Bruce:  for you :-)
---------------------------
Correctly close old nfsd/lockd sockets.

From: NeilBrown <neilb@suse.de>

Commit aaf68cfbf2241d24d46583423f6bff5c47e088b3 added a bias
to sk_inuse, so this test for an unused socket now fails.  So no
sockets gets closed because they are old (they might get closed
if the client closed them).

This bug has existed since 2.6.21-rc1.

Thanks to Wolfgang Walter for finding and reporting the bug.


Cc: Wolfgang Walter <wolfgang.walter@studentenwerk.mhn.de>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./net/sunrpc/svcsock.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
--- .prev/net/sunrpc/svcso...
[ message continues ]

What is it that ensures svsk->sk_inuse isn't incremented or SK_BUSY set
after that test?  Not all the code that does either of those is under
the same serv->sv_lock lock that this code is.

--b.
-

This should not matter - SK_CLOSED may be set at any time.

svc_age_temp_sockets only detaches the socket, sets SK_CLOSED and then 
enqueues it. If SK_BUSY is set its already enqueued and svc_sock_enqueue 
ensures that it is not enqueued twice.

Regards,
-- 
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
-

Oh, got it.  And the list manipulation is safe thanks to sv_lock.  Neat,
thanks.  Can you verify that this solves your problem?

--b.
-

Patch works fine here.

Regards,
-- 
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
-

Great, thanks again!

--b.
-

I'll test it tomorrow. So friday morning I'll know and mail you for sure.

Regards,
-- 
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
-