getdelays.c: signal handling for log rotation

PATCH In review Scott Wiersdorf 5 participants 11 messages Jun 5, 2008 – Jun 30, 2008

Share & tools

Scott Wiersdorf

author

2008-06-05 20:08 UTC #0 lore raw

PATCH 1/1 getdelays.c: signal handling for log rotation

This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.

Signed-off-by Scott Wiersdorf <[email protected]>

---

accounting/getdelays.c +31 -6

@@ -50,6 +50,7 @@ int dbg;
 int print_delays;
 int print_io_accounting;
 int print_task_context_switch_counts;
+volatile sig_atomic_t reopen_log = 0;
 __u64 stime, utime;
 
 #define PRINTF(fmt, arg...) {			\

@@ -80,6 +81,7 @@ static void usage(void)
 	fprintf(stderr, "  -l: listen forever\n");
 	fprintf(stderr, "  -v: debug on\n");
 	fprintf(stderr, "  -C: container path\n");
+	fprintf(stderr, "\nSend USR1 to reopen the logfile if -w is used.\n");
 }
 
 /*

@@ -231,6 +233,30 @@ void print_ioacct(struct taskstats *t)
 		(unsigned long long)t->cancelled_write_bytes);
 }
 
+void catch_usr1(int sig)
+{
+	reopen_log = 1;
+	signal(sig, catch_usr1);
+}
+
+int reopen_logfile(int fd, char *logfile)
+{
+	if (fd) {
+		PRINTF("USR1 received. Closing logfile.\n");
+		close(fd);
+	}
+	fd = open(logfile, O_WRONLY | O_CREAT | O_TRUNC,
+		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	if (fd == -1) {
+		perror("Cannot open output file\n");
+		exit(1);
+	}
+
+	reopen_log = 0;
+
+	return fd;
+}
+
 int main(int argc, char *argv[])
 {
 	int c, rc, rep_len, aggr_len, len2, cmd_type;

@@ -320,12 +346,8 @@ int main(int argc, char *argv[])
 	}
 
 	if (write_file) {
-		fd = open(logfile, O_WRONLY | O_CREAT | O_TRUNC,
-			  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
-		if (fd == -1) {
-			perror("Cannot open output file\n");
-			exit(1);
-		}
+		fd = reopen_logfile(fd, logfile);
+		signal(SIGUSR1, catch_usr1); /* only set when write_file is set */
 	}
 
 	if ((nl_sd = create_nl_socket(NETLINK_GENERIC)) < 0)

@@ -444,6 +466,9 @@ int main(int argc, char *argv[])
 								err(1,"write error\n");
 							}
 						}
+						if (reopen_log) {
+							fd = reopen_logfile(fd, logfile);
+						}
 						if (!loop)
 							goto done;
 						break;

Balbir Singh

2008-06-06 03:47 UTC #1 lore raw

Quoted 22 lines · Scott Wiersdorf

This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.
Signed-off-by Scott Wiersdorf <[email protected]>
---
accounting/getdelays.c +1 -0
@@ -50,6 +50,7 @@ int dbg;
 int print_delays;
 int print_io_accounting;
 int print_task_context_switch_counts;
+volatile sig_atomic_t reopen_log = 0;
 
 
 
__u64 stime, utime;
#define PRINTF(fmt, arg...) { \

accounting/getdelays.c static void usage(void) · line 84

Quoted 4 lines

accounting/getdelays.c +1 -0

@@ -80,6 +81,7 @@ static void usage(void)
 	fprintf(stderr, "  -l: listen forever\n");
 	fprintf(stderr, "  -v: debug on\n");
 	fprintf(stderr, "  -C: container path\n");
+	fprintf(stderr, "\nSend USR1 to reopen the logfile if -w is used.\n");

Please mention that old data will be lost and that SIGUSR1 will take affect after some data is received.

accounting/getdelays.c void print_ioacct(struct taskstats *t) · line 241

Quoted 13 lines

accounting/getdelays.c +6 -0

 }
 
 /*

@@ -231,6 +233,30 @@ void print_ioacct(struct taskstats *t)
 		(unsigned long long)t->cancelled_write_bytes);
 }
 
+void catch_usr1(int sig)
+{
+	reopen_log = 1;
+	signal(sig, catch_usr1);
+}
+

Aren't we better of using the newer sigaction primitives? IIRC, signal can be racy. The man page states "Avoid its use"

accounting/getdelays.c void print_ioacct(struct taskstats *t) · line 246

Quoted 5 lines

accounting/getdelays.c +5 -0

+int reopen_logfile(int fd, char *logfile)
+{
+	if (fd) {
+		PRINTF("USR1 received. Closing logfile.\n");
+		close(fd);

So sending USR1 causes data to be lost?

Quoted 32 lines

accounting/getdelays.c +15 -6

+	}
+	fd = open(logfile, O_WRONLY | O_CREAT | O_TRUNC,
+		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	if (fd == -1) {
+		perror("Cannot open output file\n");
+		exit(1);
+	}
+
+	reopen_log = 0;
+
+	return fd;
+}
+
 int main(int argc, char *argv[])
 {
 	int c, rc, rep_len, aggr_len, len2, cmd_type;

@@ -320,12 +346,8 @@ int main(int argc, char *argv[])
 	}
 
 	if (write_file) {
-		fd = open(logfile, O_WRONLY | O_CREAT | O_TRUNC,
-			  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
-		if (fd == -1) {
-			perror("Cannot open output file\n");
-			exit(1);
-		}
+		fd = reopen_logfile(fd, logfile);
+		signal(SIGUSR1, catch_usr1); /* only set when write_file is set */
 	}
 
 	if ((nl_sd = create_nl_socket(NETLINK_GENERIC)) < 0)

accounting/getdelays.c int main(int argc, char *argv[]) · line 471

Quoted 6 lines

accounting/getdelays.c +3 -0

@@ -444,6 +466,9 @@ int main(int argc, char *argv[])
 								err(1,"write error\n");
 							}
 						}
+						if (reopen_log) {
+							fd = reopen_logfile(fd, logfile);
+						}

This seems way of the 80 character space. You have braces that are not required.

Quoted 4 lines

accounting/getdelays.c +0 -0

 						if (!loop)
 							goto done;
 						break;

Signature · 4 lines

	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Scott Wiersdorf

2008-06-06 21:04 UTC #2 lore raw

Quoted 9 lines · Balbir Singh

Review static void usage(void) · line 84
Quoted 6 lines
#define PRINTF(fmt, arg...) { \
(quoted diff) +1 -0
@@ -80,6 +81,7 @@ static void usage(void)
 	fprintf(stderr, "  -l: listen forever\n");
 	fprintf(stderr, "  -v: debug on\n");
 	fprintf(stderr, "  -C: container path\n");
+	fprintf(stderr, "\nSend USR1 to reopen the logfile if -w is used.\n");
Please mention that old data will be lost and that SIGUSR1 will take affect after some data is received.

See below.

Quoted 2 lines

Aren't we better of using the newer sigaction primitives? IIRC, signal can be racy. The man page states "Avoid its use"

I've rewritten this to use the new sigaction (bottom of this email). Thanks!

Quoted 10 lines

Review static void usage(void)

Quoted 8 lines

(quoted diff) +8 -0

+int reopen_logfile(int fd, char *logfile)
+{
+	if (fd) {
+		PRINTF("USR1 received. Closing logfile.\n");
+		close(fd);
+	}
+	fd = open(logfile, O_WRONLY | O_CREAT | O_TRUNC,
+		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);

So sending USR1 causes data to be lost?

Yes, if you don't move the old file out of the way first. Sending a USR1 can also be used to rotate the log:

  # ./getdelays -m0,1 -l -w logfile &
  (time passes)
  # mv logfile logfile.0
  # kill -USR1 <pid>

Or to truncate the log:

  # ./getdelays -m0,1 -l -w logfile &
  (time passes)
  # kill -USR1 <pid>

Quoted 5 lines

Review
Quoted 3 lines
(quoted diff) +3 -0
+						if (reopen_log) {
+							fd = reopen_logfile(fd, logfile);
+						}
This seems way of the 80 character space. You have braces that are not required.

Yeah, I'm all for saving one line ;-) (fixed in this new patch)

Signed-off-by Scott Wiersdorf <[email protected]>

---

accounting/getdelays.c +35 -6

@@ -50,6 +50,8 @@ int dbg;
 int print_delays;
 int print_io_accounting;
 int print_task_context_switch_counts;
+volatile sig_atomic_t reopen_log;
+struct sigaction sig_usr1;
 __u64 stime, utime;
 
 #define PRINTF(fmt, arg...) {			\

@@ -80,6 +82,8 @@ static void usage(void)
 	fprintf(stderr, "  -l: listen forever\n");
 	fprintf(stderr, "  -v: debug on\n");
 	fprintf(stderr, "  -C: container path\n");
+	fprintf(stderr, "\nSend USR1 to reopen and truncate the logfile (if \
+-w is used). New log is\ncreated after next netlink datum is received.\n");
 }
 
 /*

@@ -231,6 +235,30 @@ void print_ioacct(struct taskstats *t)
 		(unsigned long long)t->cancelled_write_bytes);
 }
 
+void catch_usr1(int sig)
+{
+	reopen_log = 1;
+	sigaction(SIGUSR1, &sig_usr1, NULL);
+}
+
+int reopen_logfile(int fd, char *logfile)
+{
+	if (fd) {
+		PRINTF("USR1 received. Closing logfile.\n");
+		close(fd);
+	}
+	fd = open(logfile, O_WRONLY | O_CREAT | O_TRUNC,
+		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	if (fd == -1) {
+		perror("Cannot open output file\n");
+		exit(1);
+	}
+
+	reopen_log = 0;
+
+	return fd;
+}
+
 int main(int argc, char *argv[])
 {
 	int c, rc, rep_len, aggr_len, len2, cmd_type;

@@ -320,12 +348,11 @@ int main(int argc, char *argv[])
 	}
 
 	if (write_file) {
-		fd = open(logfile, O_WRONLY | O_CREAT | O_TRUNC,
-			  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
-		if (fd == -1) {
-			perror("Cannot open output file\n");
-			exit(1);
-		}
+		fd = reopen_logfile(fd, logfile);
+		sig_usr1.sa_handler = catch_usr1;
+		memset (&sig_usr1.sa_mask, 0x00, sizeof(sigset_t));
+		sig_usr1.sa_flags = SA_RESTART;
+		sigaction(SIGUSR1, &sig_usr1, NULL);
 	}
 
 	if ((nl_sd = create_nl_socket(NETLINK_GENERIC)) < 0)

@@ -444,6 +471,8 @@ int main(int argc, char *argv[])
 								err(1,"write error\n");
 							}
 						}
+						if (reopen_log)
+							fd = reopen_logfile(fd, logfile);
 						if (!loop)
 							goto done;
 						break;

Andi Kleen

2008-06-06 13:59 UTC #3 lore raw

In reply to Scott Wiersdorf

Quoted 5 lines · Scott Wiersdorf

This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.

You could do the same by sending SIGSTOP; copy file; truncate file; SIGCONT

-Andi

Scott Wiersdorf

2008-06-06 21:05 UTC #4 lore raw

Quoted 9 lines · Andi Kleen

Quoted 5 lines · Scott Wiersdorf
This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.
You could do the same by sending SIGSTOP; copy file; truncate file; SIGCONT

I believe it, but you can miss a lot of important stuff between STOP and CONT, especially on a busy system. With a single handler that reopens the file, you'll miss at most one netlink datum packet, and most of the time you'll miss nothing.

Scott

Signature · 2 lines

Scott Wiersdorf
<[email protected]>

Andi Kleen

2008-06-07 01:29 UTC #5 lore raw

Quoted 8 lines · Scott Wiersdorf

Quoted 7 lines · Andi Kleen
Quoted 5 lines · Scott Wiersdorf
This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.

Quoted 4 lines

Quoted 1 line
You could do the same by sending SIGSTOP; copy file; truncate file; SIGCONT
I believe it, but you can miss a lot of important stuff between STOP and CONT, especially on a busy system.

Do you have data on that or are you guessing?

I suspect the later, I'm sceptical of your claim. A lot of events fit into the netlink socket buffer.

-Andi

Scott Wiersdorf

2008-06-06 21:05 UTC #6 lore raw

In reply to Andi Kleen

Quoted 9 lines · Andi Kleen

Quoted 5 lines · Scott Wiersdorf
This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.
You could do the same by sending SIGSTOP; copy file; truncate file; SIGCONT

Actually, I was wrong in my previous reply. Sorry for my error. The above will work fine (no data loss; I guess the data queues somewhere in some magic way?)

I still think a single handler is elegant enough though, and works better with many log rotation systems that want to send a single signal to a pid (it's what we need where I'm working now, hence the patch).

Scott

Signature · 2 lines

Scott Wiersdorf
<[email protected]>

Andi Kleen

2008-06-07 01:30 UTC #7 lore raw

Quoted 8 lines · Scott Wiersdorf

Quoted 7 lines · Andi Kleen
Quoted 5 lines · Scott Wiersdorf
This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.

Quoted 5 lines

Quoted 1 line
You could do the same by sending SIGSTOP; copy file; truncate file; SIGCONT
Actually, I was wrong in my previous reply. Sorry for my error. The above will work fine (no data loss; I guess the data queues somewhere in some magic way?)

netlink has a socket buffer like all other buffer. I think it's around 128K.

Quoted 4 lines

I still think a single handler is elegant enough though, and works better with many log rotation systems that want to send a single signal to a pid (it's what we need where I'm working now, hence the patch).

Well in general it would be cool if there was nicer free userland for the obscure but useful delay accounting. The code in Documentation is really not much more than a example. Do you have something downloadable?

-Andi

Scott Wiersdorf

2008-06-09 14:27 UTC #8 lore raw

Quoted 9 lines · Andi Kleen

Quoted 4 lines · Scott Wiersdorf
I still think a single handler is elegant enough though, and works better with many log rotation systems that want to send a single signal to a pid (it's what we need where I'm working now, hence the patch).
Well in general it would be cool if there was nicer free userland for the obscure but useful delay accounting. The code in Documentation is really not much more than a example. Do you have something downloadable?

I don't. I'm really not much of a C programmer; I'm just hacking on this file for our company because we have a use for it. I'm all for it going into userland, but don't have the time to be responsible for that.

Scott

Balbir Singh

2008-06-09 14:43 UTC #9 lore raw

Quoted 6 lines · Scott Wiersdorf

Quoted 5 lines · Andi Kleen
Quoted 4 lines · Scott Wiersdorf
I still think a single handler is elegant enough though, and works better with many log rotation systems that want to send a single signal to a pid (it's what we need where I'm working now, hence the patch).

Quoted 7 lines

Quoted 3 lines
Well in general it would be cool if there was nicer free userland for the obscure but useful delay accounting. The code in Documentation is really not much more than a example. Do you have something downloadable?
I don't. I'm really not much of a C programmer; I'm just hacking on this file for our company because we have a use for it. I'm all for it going into userland, but don't have the time to be responsible for that.

Andi,

I've come across http://people.suug.ch/~tgr/taskstats/. It seems like a good implementation. The code in Documentation/accounting is to show a prototype of the implementation and to keep it updated as we add new features.

Signature · 4 lines

	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Andrew Morton

2008-06-30 19:54 UTC #10 lore raw

In reply to Scott Wiersdorf

Quoted 11 lines · On Fri, 6 Jun 2008 14:47:04 -0600 Scott Wiersdorf <[email protected]> wrote:

Quoted 9 lines · Andi Kleen
Quoted 5 lines · Scott Wiersdorf
This adds a USR1 signal handler to getdelays.c, which causes getdelays to close its logfile and reopen it (if '-w logfile' is specified). This is useful in situations when getdelays is running for a long time (i.e, the log file growing) and you need to rotate the logs but don't want to lose any log data.
You could do the same by sending SIGSTOP; copy file; truncate file; SIGCONT

Quoted 9 lines

Actually, I was wrong in my previous reply. Sorry for my error. The above will work fine (no data loss; I guess the data queues somewhere in some magic way?)
I still think a single handler is elegant enough though, and works better with many log rotation systems that want to send a single signal to a pid (it's what we need where I'm working now, hence the patch).

I've somewhat lost track of the discussion here. Could you please send a new patch which reflects the changes whcih you decided to make?

Thanks.