diff mbox

rpc.gssd: don't call poll() twice a second

Message ID 20120727210247.GC6388@fieldses.org (mailing list archive)
State New, archived
Headers show

Commit Message

J. Bruce Fields July 27, 2012, 9:02 p.m. UTC 
From: "J. Bruce Fields" <bfields@redhat.com>

Use the self-pipe trick instead.

(Alternatively, we could use ppoll.  That wasn't supported before
2.6.16, whereas gss was introduced in 2.5.  I was trying to be
conservative about compatibility with older kernels, but maybe we don't
care at this point.)

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 utils/gssd/gssd_main_loop.c |   52 +++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

Comments

Jim Rees July 27, 2012, 11:08 p.m. UTC | #1 
J. Bruce Fields wrote:

  +	if (1 != write(pipefd[1], "!", 1))
  +		printerr(2, "weird; maybe an interrupt?");

Use Yoda conditions must we?
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields July 29, 2012, 5:48 p.m. UTC | #2 
On Fri, Jul 27, 2012 at 07:08:53PM -0400, Jim Rees wrote:
> J. Bruce Fields wrote:
> 
>   +	if (1 != write(pipefd[1], "!", 1))
>   +		printerr(2, "weird; maybe an interrupt?");
> 
> Use Yoda conditions must we?

Yeah, yeah.  How about:

	 static void something_changed(void)
	 {
	-	if (1 != write(pipefd[1], "!", 1))
	-		printerr(2, "weird; maybe an interrupt?");
	+	if (write(pipefd[1], "!", 1) != 1)
	+		printerr(0, "%s writing to pipe", strerror(errno));
	 }
 
?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Dickson July 29, 2012, 11:09 p.m. UTC | #3 
On 07/29/2012 01:48 PM, J. Bruce Fields wrote:
> On Fri, Jul 27, 2012 at 07:08:53PM -0400, Jim Rees wrote:
>> J. Bruce Fields wrote:
>>
>>   +	if (1 != write(pipefd[1], "!", 1))
>>   +		printerr(2, "weird; maybe an interrupt?");
>>
>> Use Yoda conditions must we?
> 
> Yeah, yeah.  How about:
> 
> 	 static void something_changed(void)
> 	 {
> 	-	if (1 != write(pipefd[1], "!", 1))
> 	-		printerr(2, "weird; maybe an interrupt?");
> 	+	if (write(pipefd[1], "!", 1) != 1)
> 	+		printerr(0, "%s writing to pipe", strerror(errno));
> 	 }
>  
> ?
Better... IMHO..  but what's going to mean when we see that in some log?

steved
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields July 30, 2012, 8:59 p.m. UTC | #4 
On Sun, Jul 29, 2012 at 07:09:30PM -0400, Steve Dickson wrote:
> 
> 
> On 07/29/2012 01:48 PM, J. Bruce Fields wrote:
> > On Fri, Jul 27, 2012 at 07:08:53PM -0400, Jim Rees wrote:
> >> J. Bruce Fields wrote:
> >>
> >>   +	if (1 != write(pipefd[1], "!", 1))
> >>   +		printerr(2, "weird; maybe an interrupt?");
> >>
> >> Use Yoda conditions must we?
> > 
> > Yeah, yeah.  How about:
> > 
> > 	 static void something_changed(void)
> > 	 {
> > 	-	if (1 != write(pipefd[1], "!", 1))
> > 	-		printerr(2, "weird; maybe an interrupt?");
> > 	+	if (write(pipefd[1], "!", 1) != 1)
> > 	+		printerr(0, "%s writing to pipe", strerror(errno));
> > 	 }
> >  
> > ?
> Better... IMHO..  but what's going to mean when we see that in some log?

Beats me.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields July 30, 2012, 10:10 p.m. UTC | #5 
On Mon, Jul 30, 2012 at 04:59:49PM -0400, J. Bruce Fields wrote:
> On Sun, Jul 29, 2012 at 07:09:30PM -0400, Steve Dickson wrote:
> > 
> > 
> > On 07/29/2012 01:48 PM, J. Bruce Fields wrote:
> > > On Fri, Jul 27, 2012 at 07:08:53PM -0400, Jim Rees wrote:
> > >> J. Bruce Fields wrote:
> > >>
> > >>   +	if (1 != write(pipefd[1], "!", 1))
> > >>   +		printerr(2, "weird; maybe an interrupt?");
> > >>
> > >> Use Yoda conditions must we?
> > > 
> > > Yeah, yeah.  How about:
> > > 
> > > 	 static void something_changed(void)
> > > 	 {
> > > 	-	if (1 != write(pipefd[1], "!", 1))
> > > 	-		printerr(2, "weird; maybe an interrupt?");
> > > 	+	if (write(pipefd[1], "!", 1) != 1)
> > > 	+		printerr(0, "%s writing to pipe", strerror(errno));
> > > 	 }
> > >  
> > > ?
> > Better... IMHO..  but what's going to mean when we see that in some log?
> 
> Beats me.

Looking at it a little more: actually, if gssd is slow to process these
events then in theory they could pile up, and we could eventually get
EAGAIN/WOUDBLOCK.

Which wouldn't be a problem, except that now we're modifying errno in a
signal handler.  So the signal handler should be saving and restoring
errno.

And also: I noticed one of the reasons gssd hasn't been completely
reliable for me is that we already have a printerr() in the signal
handler, and printerr() doesn't appear to be reentrant.

Eh, I'm leaning toward just using ppoll.  According to the man page that
requires kernel >= 2.6.16, glibc >= 2.4.  Is that OK?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Dickson July 31, 2012, 4:50 p.m. UTC | #6 
On 07/30/2012 06:10 PM, J. Bruce Fields wrote:
> On Mon, Jul 30, 2012 at 04:59:49PM -0400, J. Bruce Fields wrote:
>> On Sun, Jul 29, 2012 at 07:09:30PM -0400, Steve Dickson wrote:
>>>
>>>
>>> On 07/29/2012 01:48 PM, J. Bruce Fields wrote:
>>>> On Fri, Jul 27, 2012 at 07:08:53PM -0400, Jim Rees wrote:
>>>>> J. Bruce Fields wrote:
>>>>>
>>>>>   +	if (1 != write(pipefd[1], "!", 1))
>>>>>   +		printerr(2, "weird; maybe an interrupt?");
>>>>>
>>>>> Use Yoda conditions must we?
>>>>
>>>> Yeah, yeah.  How about:
>>>>
>>>> 	 static void something_changed(void)
>>>> 	 {
>>>> 	-	if (1 != write(pipefd[1], "!", 1))
>>>> 	-		printerr(2, "weird; maybe an interrupt?");
>>>> 	+	if (write(pipefd[1], "!", 1) != 1)
>>>> 	+		printerr(0, "%s writing to pipe", strerror(errno));
>>>> 	 }
>>>>  
>>>> ?
>>> Better... IMHO..  but what's going to mean when we see that in some log?
>>
>> Beats me.
> 
> Looking at it a little more: actually, if gssd is slow to process these
> events then in theory they could pile up, and we could eventually get
> EAGAIN/WOUDBLOCK.
> 
> Which wouldn't be a problem, except that now we're modifying errno in a
> signal handler.  So the signal handler should be saving and restoring
> errno.
> 
> And also: I noticed one of the reasons gssd hasn't been completely
> reliable for me is that we already have a printerr() in the signal
> handler, and printerr() doesn't appear to be reentrant.
> 
> Eh, I'm leaning toward just using ppoll.  According to the man page that
> requires kernel >= 2.6.16, glibc >= 2.4.  Is that OK?
I would think so.... 

steved.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields July 31, 2012, 8:59 p.m. UTC | #7 
On Tue, Jul 31, 2012 at 12:50:42PM -0400, Steve Dickson wrote:
> On 07/30/2012 06:10 PM, J. Bruce Fields wrote:
> > Eh, I'm leaning toward just using ppoll.  According to the man page that
> > requires kernel >= 2.6.16, glibc >= 2.4.  Is that OK?
> I would think so.... 

OK.  With ppoll, and split up into little pieces this time.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
index c18e12c..8886eb6 100644
--- a/utils/gssd/gssd_main_loop.c
+++ b/utils/gssd/gssd_main_loop.c
@@ -57,15 +57,18 @@ 
 extern struct pollfd *pollarray;
 extern int pollsize;
 
-#define POLL_MILLISECS	500
+static int pipefd[2];
 
-static volatile int dir_changed = 1;
+static void something_changed(void)
+{
+	if (1 != write(pipefd[1], "!", 1))
+		printerr(2, "weird; maybe an interrupt?");
+}
 
 static void dir_notify_handler(int sig, siginfo_t *si, void *data)
 {
 	printerr(2, "dir_notify_handler: sig %d si %p data %p\n", sig, si, data);
-
-	dir_changed = 1;
+	something_changed();
 }
 
 static void
@@ -80,7 +83,7 @@  scan_poll_results(int ret)
 		if (i >= 0 && pollarray[i].revents) {
 			if (pollarray[i].revents & POLLHUP) {
 				clp->gssd_close_me = 1;
-				dir_changed = 1;
+				something_changed();
 			}
 			if (pollarray[i].revents & POLLIN)
 				handle_gssd_upcall(clp);
@@ -93,7 +96,7 @@  scan_poll_results(int ret)
 		if (i >= 0 && pollarray[i].revents) {
 			if (pollarray[i].revents & POLLHUP) {
 				clp->krb5_close_me = 1;
-				dir_changed = 1;
+				something_changed();
 			}
 			if (pollarray[i].revents & POLLIN)
 				handle_krb5_upcall(clp);
@@ -123,11 +126,13 @@  topdirs_add_entry(struct dirent *dent)
 	}
 	snprintf(tdi->dirname, PATH_MAX, "%s/%s", pipefs_dir, dent->d_name);
 	tdi->fd = open(tdi->dirname, O_RDONLY);
-	if (tdi->fd != -1) {
-		fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
-		fcntl(tdi->fd, F_NOTIFY,
-		      DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
+	if (tdi->fd == -1) {
+		printerr(0, "ERROR: failed to open %s\n", tdi->dirname);
+		free(tdi);
+		return -1;
 	}
+	fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
+	fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
 
 	TAILQ_INSERT_HEAD(&topdirs_list, tdi, list);
 	return 0;
@@ -185,6 +190,7 @@  gssd_run()
 	int			ret;
 	struct sigaction	dn_act;
 	sigset_t		set;
+	char			buf;
 
 	/* Taken from linux/Documentation/dnotify.txt: */
 	dn_act.sa_sigaction = dir_notify_handler;
@@ -202,26 +208,28 @@  gssd_run()
 
 	init_client_list();
 
+	ret = pipe2(pipefd, O_NONBLOCK);
+	if (ret == -1)
+		return;
+	pollarray[0].fd = pipefd[0];
+	pollarray[0].events = POLLIN;
+
 	printerr(1, "beginning poll\n");
 	while (1) {
-		while (dir_changed) {
-			dir_changed = 0;
-			if (update_client_list()) {
-				/* Error msg is already printed */
-				exit(1);
-			}
-		}
-		/* race condition here: dir_changed could be set before we
-		 * enter the poll, and we'd never notice if it weren't for the
-		 * timeout. */
-		ret = poll(pollarray, pollsize, POLL_MILLISECS);
+		ret = poll(pollarray, pollsize, -1);
 		if (ret < 0) {
 			if (errno != EINTR)
 				printerr(0,
 					 "WARNING: error return from poll\n");
 		} else if (ret == 0) {
-			/* timeout */
+			/* timeout?? */
 		} else { /* ret > 0 */
+			if (pollarray[0].revents) {
+				if (1 != read(pipefd[0], &buf, 1))
+					printerr(2, "weird; maybe an interrupt?");
+				if (update_client_list())
+					exit(1);
+			}
 			scan_poll_results(ret);
 		}
 	}