[PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[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 <scott@perlcode.org>
---
--- Documentation/accounting/getdelays.c	2008-05-15 09:00:12.000000000 -0600
+++ Documentation/accounting/getdelays.c	2008-06-05 02:23:57.000000000 -0600
@@ -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;

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Balbir Singh]

Scott Wiersdorf wrote:
> 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 <scott@perlcode.org>
> ---
> --- Documentation/accounting/getdelays.c	2008-05-15 09:00:12.000000000 -0600
> +++ Documentation/accounting/getdelays.c	2008-06-05 02:23:57.000000000 -0600
> @@ -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");

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

>  }
> 
>  /*
> @@ -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"

> +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?

> +	}
> +	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);
> +						}

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

>  						if (!loop)
>  							goto done;
>  						break;
> 


-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Scott Wiersdorf]

On Fri, Jun 06, 2008 at 09:16:13AM +0530, Balbir Singh wrote:
> >  #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");
> 
> Please mention that old data will be lost and that SIGUSR1 will take affect
> after some data is received.

See below.

> 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!

> > +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>

> > +						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 <scott@perlcode.org>

---
--- Documentation/accounting/getdelays.c	2008-05-15 09:00:12.000000000 -0600
+++ Documentation/accounting/getdelays.c	2008-06-06 14:34:37.000000000 -0600
@@ -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;

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Andi Kleen]

Scott Wiersdorf <scott@perlcode.org> writes:

> 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

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Scott Wiersdorf]

On Fri, Jun 06, 2008 at 03:59:09PM +0200, Andi Kleen wrote:
> Scott Wiersdorf <scott@perlcode.org> writes:
> 
> > 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
-- 
Scott Wiersdorf
<scott@bluehost.com>

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Andi Kleen]

Scott Wiersdorf wrote:
> On Fri, Jun 06, 2008 at 03:59:09PM +0200, Andi Kleen wrote:
>> Scott Wiersdorf <scott@perlcode.org> writes:
>>
>>> 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.

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

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Scott Wiersdorf]

On Fri, Jun 06, 2008 at 03:59:09PM +0200, Andi Kleen wrote:
> Scott Wiersdorf <scott@perlcode.org> writes:
> 
> > 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
-- 
Scott Wiersdorf
<scott@bluehost.com>

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Andi Kleen]

Scott Wiersdorf wrote:
> On Fri, Jun 06, 2008 at 03:59:09PM +0200, Andi Kleen wrote:
>> Scott Wiersdorf <scott@perlcode.org> writes:
>>
>>> 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?)

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

> 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

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Scott Wiersdorf]

On Sat, 2008-06-07 at 03:13 +0200, Andi Kleen wrote:
> Scott Wiersdorf wrote: 
> > 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

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Balbir Singh]

Scott Wiersdorf wrote:
> On Sat, 2008-06-07 at 03:13 +0200, Andi Kleen wrote:
>> Scott Wiersdorf wrote: 
>>> 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.

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.

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [PATCH 2.6.25-4] getdelays.c: signal handling for log rotation

[Andrew Morton]

On Fri, 6 Jun 2008 14:47:04 -0600
Scott Wiersdorf <scott@bluehost.com> wrote:

> On Fri, Jun 06, 2008 at 03:59:09PM +0200, Andi Kleen wrote:
> > Scott Wiersdorf <scott@perlcode.org> writes:
> > 
> > > 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).
> 

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.