Skip to contentSkip to conversation

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

PATCH In review J. Bruce Fields 5 participants 20 messages Jul 27, 2012 – Aug 6, 2012

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

J. Bruce Fields

author
#0 lore raw

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

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>
---
 utils/gssd/gssd_main_loop.c |   52 +++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 22 deletions(-)
utils/gssd/gssd_main_loop.c +30 -22
index c18e12c..8886eb6 100644
@@ -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);
 		}
 	}
Signature · 1 line
1.7.9.5
#1 lore raw

J. Bruce Fields wrote:

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

Use Yoda conditions must we?

author
#2 lore raw
Quoted 6 lines · Jim Rees

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.

#3 lore raw
Quoted 18 lines · J. Bruce Fields
Quoted 6 lines · Jim Rees

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));
	 }
Quoted 1 line

?

Better... IMHO.. but what's going to mean when we see that in some log?

steved

author
#4 lore raw
Quoted 21 lines · Steve Dickson
Quoted 18 lines · J. Bruce Fields
Quoted 6 lines · Jim Rees

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));
	 }
Quoted 2 lines
Quoted 1 line

?

Better... IMHO.. but what's going to mean when we see that in some log?

Beats me.

--b.

author
#5 lore raw
Quoted 22 lines · J. Bruce Fields
Quoted 21 lines · Steve Dickson
Quoted 18 lines · J. Bruce Fields
Quoted 6 lines · Jim Rees

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));
	 }
Quoted 4 lines
Quoted 2 lines
Quoted 1 line

?

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.

#6 lore raw
Quoted 36 lines · J. Bruce Fields
Quoted 22 lines · J. Bruce Fields
Quoted 21 lines · Steve Dickson
Quoted 18 lines · J. Bruce Fields
Quoted 6 lines · Jim Rees

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));
	 }
Quoted 4 lines
Quoted 2 lines
Quoted 1 line

?

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.

Quoted 6 lines

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.

author
#7 lore raw
Quoted 4 lines · Steve Dickson
Quoted 2 lines · J. Bruce Fields

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.

#8 lore raw

PATCH 1/4 rpc.gssd: simplify signal handling

From: "J. Bruce Fields" <[email protected]>

We're not actually using the extra sa_sigaction parameters.

Signed-off-by J. Bruce Fields <[email protected]>
---
 utils/gssd/gssd_main_loop.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
utils/gssd/gssd_main_loop.c +5 -6
index c18e12c..9954ffb 100644
@@ -61,9 +61,9 @@ extern int pollsize;
 
 static volatile int dir_changed = 1;
 
-static void dir_notify_handler(int sig, siginfo_t *si, void *data)
+static void dir_notify_handler(int sig)
 {
-	printerr(2, "dir_notify_handler: sig %d si %p data %p\n", sig, si, data);
+	printerr(2, "dir_notify_handler: sig %d\n", sig);
 
 	dir_changed = 1;
 }

@@ -183,13 +183,12 @@ void
 gssd_run()
 {
 	int			ret;
-	struct sigaction	dn_act;
+	struct sigaction	dn_act = {
+		.sa_handler = dir_notify_handler
+	};
 	sigset_t		set;
 
-	/* Taken from linux/Documentation/dnotify.txt: */
-	dn_act.sa_sigaction = dir_notify_handler;
 	sigemptyset(&dn_act.sa_mask);
-	dn_act.sa_flags = SA_SIGINFO;
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
 	/* just in case the signal is blocked... */
Signature · 1 line
1.7.9.5
#9 lore raw

Re: [PATCH 1/4] rpc.gssd: simplify signal handling

Quoted 8 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

We're not actually using the extra sa_sigaction parameters.

Signed-off-by J. Bruce Fields <[email protected]>
---
 utils/gssd/gssd_main_loop.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Committed...

steved.

Quoted 34 lines
utils/gssd/gssd_main_loop.c +5 -6
index c18e12c..9954ffb 100644
@@ -61,9 +61,9 @@ extern int pollsize;
 
 static volatile int dir_changed = 1;
 
-static void dir_notify_handler(int sig, siginfo_t *si, void *data)
+static void dir_notify_handler(int sig)
 {
-	printerr(2, "dir_notify_handler: sig %d si %p data %p\n", sig, si, data);
+	printerr(2, "dir_notify_handler: sig %d\n", sig);
 
 	dir_changed = 1;
 }

@@ -183,13 +183,12 @@ void
 gssd_run()
 {
 	int			ret;
-	struct sigaction	dn_act;
+	struct sigaction	dn_act = {
+		.sa_handler = dir_notify_handler
+	};
 	sigset_t		set;
 
-	/* Taken from linux/Documentation/dnotify.txt: */
-	dn_act.sa_sigaction = dir_notify_handler;
 	sigemptyset(&dn_act.sa_mask);
-	dn_act.sa_flags = SA_SIGINFO;
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
 	/* just in case the signal is blocked... */
#10 lore raw

PATCH 2/4 rpc.gssd: don't call printerr from signal handler

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

printerr() isn't actually safe to call from a signal handler. It might be possible to make it so, but I think this is the only case in nfs-utils where we try to, and I'm not convince it's worth it.

This fixes a bug that would eventually cause mounts to hang when gssd is run with -vv.

Signed-off-by J. Bruce Fields <[email protected]>
---
 utils/gssd/gssd_main_loop.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
utils/gssd/gssd_main_loop.c +1 -3
index 9954ffb..6914687 100644
@@ -61,10 +61,8 @@ extern int pollsize;
 
 static volatile int dir_changed = 1;
 
-static void dir_notify_handler(int sig)
+static void dir_notify_handler(__attribute__((unused))int sig)
 {
-	printerr(2, "dir_notify_handler: sig %d\n", sig);
-
 	dir_changed = 1;
 }
Signature · 1 line
1.7.9.5
#11 lore raw

Re: [PATCH 2/4] rpc.gssd: don't call printerr from signal handler

Quoted 8 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

printerr() isn't actually safe to call from a signal handler. It might be possible to make it so, but I think this is the only case in nfs-utils where we try to, and I'm not convince it's worth it.

This fixes a bug that would eventually cause mounts to hang when gssd is run with -vv.

Yes, I've seen this hang. gssd gets stuck on a futex() with -vv or higher.

Quoted 28 lines
Signed-off-by J. Bruce Fields <[email protected]>

---
utils/gssd/gssd_main_loop.c | 4 +---

1 file changed, 1 insertion(+), 3 deletions(-)
utils/gssd/gssd_main_loop.c +1 -3
index 9954ffb..6914687 100644
@@ -61,10 +61,8 @@ extern int pollsize;
 
 static volatile int dir_changed = 1;
 
-static void dir_notify_handler(int sig)
+static void dir_notify_handler(__attribute__((unused))int sig)
 {
-	printerr(2, "dir_notify_handler: sig %d\n", sig);
-
 	dir_changed = 1;
 }
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

#12 lore raw

Re: [PATCH 2/4] rpc.gssd: don't call printerr from signal handler

In reply to J. Bruce Fields
Quoted 10 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

printerr() isn't actually safe to call from a signal handler. It might be possible to make it so, but I think this is the only case in nfs-utils where we try to, and I'm not convince it's worth it.

This fixes a bug that would eventually cause mounts to hang when gssd is run with -vv.

Signed-off-by J. Bruce Fields <[email protected]>

Committed...

steved.

Quoted 20 lines
---
 utils/gssd/gssd_main_loop.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
utils/gssd/gssd_main_loop.c +1 -3
index 9954ffb..6914687 100644
@@ -61,10 +61,8 @@ extern int pollsize;
 
 static volatile int dir_changed = 1;
 
-static void dir_notify_handler(int sig)
+static void dir_notify_handler(__attribute__((unused))int sig)
 {
-	printerr(2, "dir_notify_handler: sig %d\n", sig);
-
 	dir_changed = 1;
 }
#13 lore raw

PATCH 3/4 rpc.gssd: handle error to open toplevel directory

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Reverse the sense of the test here, and also add debugging and cleanup in the error case.

(Though the lack of cleanup isn't currently a problem in practice since we'll eventually exit in this case.)

Signed-off-by J. Bruce Fields <[email protected]>
---
 utils/gssd/gssd_main_loop.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
utils/gssd/gssd_main_loop.c +6 -4
index 6914687..142c8c5 100644
@@ -121,11 +121,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;
Signature · 1 line
1.7.9.5
#14 lore raw

Re: [PATCH 3/4] rpc.gssd: handle error to open toplevel directory

Quoted 9 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Reverse the sense of the test here, and also add debugging and cleanup in the error case.

(Though the lack of cleanup isn't currently a problem in practice since we'll eventually exit in this case.)

Signed-off-by J. Bruce Fields <[email protected]>

Committed...

steved.

Quoted 26 lines
---
 utils/gssd/gssd_main_loop.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
utils/gssd/gssd_main_loop.c +6 -4
index 6914687..142c8c5 100644
@@ -121,11 +121,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;
#15 lore raw

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

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Use ppoll instead.

(This breaks compatibility with kernels before 2.6.16 and glibc before 2.4).

Signed-off-by J. Bruce Fields <[email protected]>
---
 utils/gssd/gssd_main_loop.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
utils/gssd/gssd_main_loop.c +5 -7
index 142c8c5..2d20fa2 100644
@@ -186,13 +186,14 @@ gssd_run()
 	struct sigaction	dn_act = {
 		.sa_handler = dir_notify_handler
 	};
-	sigset_t		set;
+	sigset_t		set, emptyset;
 
 	sigemptyset(&dn_act.sa_mask);
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
-	/* just in case the signal is blocked... */
+	sigemptyset(&emptyset);
 	sigemptyset(&set);
+	/* just in case the signal is blocked... */
 	sigaddset(&set, DNOTIFY_SIGNAL);
 	sigprocmask(SIG_UNBLOCK, &set, NULL);
 

@@ -210,16 +211,13 @@ gssd_run()
 				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 = ppoll(pollarray, pollsize, NULL, &emptyset);
 		if (ret < 0) {
 			if (errno != EINTR)
 				printerr(0,
 					 "WARNING: error return from poll\n");
 		} else if (ret == 0) {
-			/* timeout */
+			printerr(0, "WARNING: unexpected timeout\n");
 		} else { /* ret > 0 */
 			scan_poll_results(ret);
 		}
Signature · 1 line
1.7.9.5
#16 lore raw

Re: [PATCH 4/4] rpc.gssd: don't call poll() twice a second.

Quoted 6 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Use ppoll instead.

(This breaks compatibility with kernels before 2.6.16 and glibc before 2.4).

I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.

Quoted 52 lines
Signed-off-by J. Bruce Fields <[email protected]>

---
utils/gssd/gssd_main_loop.c | 12 +++++-------

1 file changed, 5 insertions(+), 7 deletions(-)
utils/gssd/gssd_main_loop.c +5 -7
index 142c8c5..2d20fa2 100644
@@ -186,13 +186,14 @@ gssd_run()
 	struct sigaction	dn_act = {
 		.sa_handler = dir_notify_handler
 	};
-	sigset_t		set;
+	sigset_t		set, emptyset;
 
 	sigemptyset(&dn_act.sa_mask);
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
-	/* just in case the signal is blocked... */
+	sigemptyset(&emptyset);
 	sigemptyset(&set);
+	/* just in case the signal is blocked... */
 	sigaddset(&set, DNOTIFY_SIGNAL);
 	sigprocmask(SIG_UNBLOCK, &set, NULL);
 

@@ -210,16 +211,13 @@ gssd_run()
 				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 = ppoll(pollarray, pollsize, NULL, &emptyset);
 		if (ret < 0) {
 			if (errno != EINTR)
 				printerr(0,
 					 "WARNING: error return from poll\n");
 		} else if (ret == 0) {
-			/* timeout */
+			printerr(0, "WARNING: unexpected timeout\n");
 		} else { /* ret > 0 */
 			scan_poll_results(ret);
 		}
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

#17 lore raw

Re: [PATCH 4/4] rpc.gssd: don't call poll() twice a second.

Quoted 11 lines · Chuck Lever
Quoted 6 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Use ppoll instead.

(This breaks compatibility with kernels before 2.6.16 and glibc before 2.4).

I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.

I was assuming we could drop support for older systems.

If not, then'd I'd much prefer to go back to the self-pipe trick than to deal with autoconf and ifdef'ing out all this stuff.

--b.

Quoted 60 lines
Quoted 53 lines
Signed-off-by J. Bruce Fields <[email protected]>

---
utils/gssd/gssd_main_loop.c | 12 +++++-------

1 file changed, 5 insertions(+), 7 deletions(-)
utils/gssd/gssd_main_loop.c +5 -7
index 142c8c5..2d20fa2 100644
@@ -186,13 +186,14 @@ gssd_run()
 	struct sigaction	dn_act = {
 		.sa_handler = dir_notify_handler
 	};
-	sigset_t		set;
+	sigset_t		set, emptyset;
 
 	sigemptyset(&dn_act.sa_mask);
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
-	/* just in case the signal is blocked... */
+	sigemptyset(&emptyset);
 	sigemptyset(&set);
+	/* just in case the signal is blocked... */
 	sigaddset(&set, DNOTIFY_SIGNAL);
 	sigprocmask(SIG_UNBLOCK, &set, NULL);
 

@@ -210,16 +211,13 @@ gssd_run()
 				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 = ppoll(pollarray, pollsize, NULL, &emptyset);
 		if (ret < 0) {
 			if (errno != EINTR)
 				printerr(0,
 					 "WARNING: error return from poll\n");
 		} else if (ret == 0) {
-			/* timeout */
+			printerr(0, "WARNING: unexpected timeout\n");
 		} else { /* ret > 0 */
 			scan_poll_results(ret);
 		}
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

#18 lore raw

Re: [PATCH 4/4] rpc.gssd: don't call poll() twice a second.

Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Chuck Lever
Quoted 2 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Use ppoll instead.

(This breaks compatibility with kernels before 2.6.16 and glibc before 2.4).

I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.

I was assuming we could drop support for older systems.

We went to a lot of trouble to get the IPv6 stuff to build correctly on older systems, and we still heard complaints.

Quoted 2 lines

If not, then'd I'd much prefer to go back to the self-pipe trick than to deal with autoconf and ifdef'ing out all this stuff.

This patch is a tiny change. I don't think autoconf in this case is onerous. I'm happy to help you with it.

Quoted 62 lines

--b.

Quoted 49 lines
Quoted 49 lines
Signed-off-by J. Bruce Fields <[email protected]>

---
utils/gssd/gssd_main_loop.c | 12 +++++-------

1 file changed, 5 insertions(+), 7 deletions(-)
utils/gssd/gssd_main_loop.c +5 -7
index 142c8c5..2d20fa2 100644
@@ -186,13 +186,14 @@ gssd_run()
 	struct sigaction	dn_act = {
 		.sa_handler = dir_notify_handler
 	};
-	sigset_t		set;
+	sigset_t		set, emptyset;
 
 	sigemptyset(&dn_act.sa_mask);
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
-	/* just in case the signal is blocked... */
+	sigemptyset(&emptyset);
 	sigemptyset(&set);
+	/* just in case the signal is blocked... */
 	sigaddset(&set, DNOTIFY_SIGNAL);
 	sigprocmask(SIG_UNBLOCK, &set, NULL);
 

@@ -210,16 +211,13 @@ gssd_run()
 				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 = ppoll(pollarray, pollsize, NULL, &emptyset);
 		if (ret < 0) {
 			if (errno != EINTR)
 				printerr(0,
 					 "WARNING: error return from poll\n");
 		} else if (ret == 0) {
-			/* timeout */
+			printerr(0, "WARNING: unexpected timeout\n");
 		} else { /* ret > 0 */
 			scan_poll_results(ret);
 		}
Signature · 1 line
1.7.9.5
Quoted 11 lines
Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

#19 lore raw

Re: [PATCH 4/4] rpc.gssd: don't call poll() twice a second.

Quoted 16 lines · Chuck Lever
Quoted 13 lines · J. Bruce Fields
Quoted 11 lines · Chuck Lever
Quoted 6 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Use ppoll instead.

(This breaks compatibility with kernels before 2.6.16 and glibc before 2.4).

I think I prefer seeing this wrapped with some autoconf machinery to allow nfs-utils to continue to build on earlier systems.

Quoted 8 lines
Quoted 1 line

I was assuming we could drop support for older systems.

We went to a lot of trouble to get the IPv6 stuff to build correctly on older systems, and we still heard complaints.

Quoted 2 lines

If not, then'd I'd much prefer to go back to the self-pipe trick than to deal with autoconf and ifdef'ing out all this stuff.

This patch is a tiny change. I don't think autoconf in this case is onerous. I'm happy to help you with it.

I think it would be work the effort... Let me know if I can help...

steved.

Quoted 74 lines
Quoted 63 lines

--b.

Quoted 49 lines
Quoted 49 lines
Signed-off-by J. Bruce Fields <[email protected]>

---
utils/gssd/gssd_main_loop.c | 12 +++++-------

1 file changed, 5 insertions(+), 7 deletions(-)
utils/gssd/gssd_main_loop.c +5 -7
index 142c8c5..2d20fa2 100644
@@ -186,13 +186,14 @@ gssd_run()
 	struct sigaction	dn_act = {
 		.sa_handler = dir_notify_handler
 	};
-	sigset_t		set;
+	sigset_t		set, emptyset;
 
 	sigemptyset(&dn_act.sa_mask);
 	sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
 
-	/* just in case the signal is blocked... */
+	sigemptyset(&emptyset);
 	sigemptyset(&set);
+	/* just in case the signal is blocked... */
 	sigaddset(&set, DNOTIFY_SIGNAL);
 	sigprocmask(SIG_UNBLOCK, &set, NULL);
 

@@ -210,16 +211,13 @@ gssd_run()
 				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 = ppoll(pollarray, pollsize, NULL, &emptyset);
 		if (ret < 0) {
 			if (errno != EINTR)
 				printerr(0,
 					 "WARNING: error return from poll\n");
 		} else if (ret == 0) {
-			/* timeout */
+			printerr(0, "WARNING: unexpected timeout\n");
 		} else { /* ret > 0 */
 			scan_poll_results(ret);
 		}
Signature · 1 line
1.7.9.5
Quoted 11 lines
Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com