Re: [PATCH 1/3] pseudo: Handle too many files deadlock

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Wed, 2017-06-14 at 15:51 +0200, Gary Thomas wrote:
> On 2017-06-14 15:42, Richard Purdie wrote:
> > 
> > If we have large amounts of parallelism, pseudo can end up with too
> > many open connections and will no longer accept further
> > connections,
> > hanging. This patch works around that by closing some clients,
> > allowing
> > turnover of connections and unblocking the system. The downside is
> > a small
> > but theoretical window of data loss. This is likely better than
> > locking
> > up entirely though. Discussions with Peter are onging about how we
> > could
> > better fix this.
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >   .../pseudo/files/toomanyfiles.patch                | 62
> > ++++++++++++++++++++++
> >   meta/recipes-devtools/pseudo/pseudo_1.8.2.bb       |  1 +
> >   2 files changed, 63 insertions(+)
> >   create mode 100644 meta/recipes-
> > devtools/pseudo/files/toomanyfiles.patch
> > 
> > diff --git a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> > b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> > new file mode 100644
> > index 0000000..14a8975
> > --- /dev/null
> > +++ b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> > @@ -0,0 +1,62 @@
> > +Currently if we max out the maximum number of files, pseudo can
> > deadlock, unable to
> > +accept new connections yet unable to move forward and unblock the
> > other processes
> > +waiting either.
> > +
> > +Rather than hang, when this happens, close out inactive
> > connections, allowing us
> > +to accept the new ones. The disconnected clients will simply
> > reconnect. There is
> > +a small risk of data loss here sadly but its better than hanging.
> > +
> > +RP
> > +2017/4/25
> > +
> > +Upstream-Status: Pending [Peter is aware of the issue]
> > +
> > +Index: pseudo-1.8.2/pseudo_server.c
> > +==================================================================
> > =
> > +--- pseudo-1.8.2.orig/pseudo_server.c
> > ++++ pseudo-1.8.2/pseudo_server.c
> > +@@ -581,6 +581,7 @@ pseudo_server_loop(void) {
> > + 	int rc;
> > + 	int fd;
> > + 	int loop_timeout = pseudo_server_timeout;
> > ++	int hitmaxfiles;
> > +
> > + 	clients = malloc(16 * sizeof(*clients));
> > +
> > +@@ -597,6 +598,7 @@ pseudo_server_loop(void) {
> > + 	active_clients = 1;
> > + 	max_clients = 16;
> > + 	highest_client = 0;
> > ++	hitmaxfiles = 0;
> > +
> > + 	pseudo_debug(PDBGF_SERVER, "server loop started.\n");
> > + 	if (listen_fd < 0) {
> > +@@ -663,10 +665,18 @@ pseudo_server_loop(void) {
> > + 						message_time.tv_u
> > sec -= 1000000;
> > + 						++message_time.tv
> > _sec;
> > + 					}
> > ++				} else if (hitmaxfiles) {
> > ++					/* In theory there is a
> > potential race here where if we close a client,
> > ++					   it may have sent us a
> > fastop message which we don't act upon.
> > ++					   If we don't close a
> > filehandle we'll loop indefinitely thought.
> > ++					   Only close one per
> > loop iteration in the interests of caution */
> > ++					close_client(i);
> > ++					histmaxfiles = 0;
> Typo?  s/histmaxfiles/hitmaxfiles/

I wondered if anyone would spot that! :)

Its fixed in the version on master-next.

Cheers,

Richard