[PATCH] winbindd: control number of winbindd's client connections

Jeremy Allison jra at samba.org
Thu Jun 4 12:46:44 MDT 2015


On Thu, Jun 04, 2015 at 08:13:14AM +0300, Uri Simchoni wrote:
> > On Wed, Jun 03, 2015 at 09:12:06AM +0300, Uri Simchoni wrote:
> >> This patch handles a case we've encountered in which winbindd opened
> >> client connections up to the process limit on open file descriptors.
> >>
> >
> > One thing that we should take into account: If we happen to
> > have winbind requests that do multiple steps to children,
> > like for example getpwnam or more extremely getgrgroup with
> > group expansion we do not check between two child requests
> > whether the client has given up in the meantime. We might
> > want to measure if that's an issue and drop client
> > connections earlier, saving a lot of work inside winbind.
> >
> > Have you thought about that, or do you have a gut feeling if
> > that could be an issue?
> >
> > Volker
> >
> 
> That's a good catch.
> It's true that while winbindd is working on a client request, it never
> looks back at the client connection to see whether the client is still
> there - it notices this only when trying to send the response back.
> I considered polling the client, but noticed that we have
> remove_timed_out_clients(), so I figured I'd raise the client timeout
> to 300 sec and let the server give up first (by default - after 60
> sec).
> 
> I ASSUME that removing a client this way also cancels any jobs on
> behalf of the client, because otherwise the program would crash when
> completing the request (verifying this is indeed so required me to dig
> deeper into the RPC infra and I didn't do it -
> remove_timed_out_clients() is not new).
> 
> What I DIDN'T notice was that remove_timed_out_clients() is being
> called only when a new connection is to be accepted (i.e. to free some
> connections) and not as a general server timeout mechanism.
> 
> So I propose to also remove_timed_out_clients() periodically.

Ok Uri, can you post a follow-up patch please ?

I'd really like to get this changeset into master.

The only change I'd like to see is changing the

+       int completed_requests;                   /* Number of requests completed on this connection */

to unsigned int, as counts can't be negative.

Cheers,

	Jeremy.


More information about the samba-technical mailing list