On 03/03/2014 12:18 PM, Daniel P. Berrange wrote:
> A earlier commit changed the global log buffer so that it only
> records messages that are explicitly requested via the log
> filters setting. This removes the performance burden, and
> improves the signal/noise ratio for messages in the global
> buffer. At the same time though, it is somewhat pointless, since
> all the recorded log messages are already going to be sent to an
> explicit log output like syslog, stderr or the journal. The
> global log buffer is thus just duplicating this data on stderr
> upon crash.
I still wonder if there is value on keeping the ring buffer for when
logging is not stderr. That is, if we dump to syslog by default but
ALSO dump the ring buffer to stderr, it may be easier to find what got
logged by reading stderr rather than having to hunt for the syslog. But
that's a weak argument.
If we commit to patch 1, then the logging ring really is duplicated
overhead, and I'm fine ditching it. You may want to get DV's opinion,
but I'm fine with this patch going in on the premise that patch 1 made
sense.
On the other hand, if patch 1 meets resistance, should we look at ways
to minimize printf() overhead? For example, a LOT of our overhead is
tied to the fact that we malloc() a lot when constructing the log
string. It may be possible to fine-tune things so that most (if not
all) log messages get computed into a static buffer (gnulib's asnprintf
is cool in how it avoids malloc in the common case if you provide a
large enough buffer, while still being robust to larger strings). It
won't kill as much overhead as what you killed by completely ditching
the ring buffer, but may still be enough improvement to merit keeping
the ring buffer around.
I'm not tied to the ring buffer - I still think that targetted logging
is easier to sift through than the signal-to-noise ratio of a ring
buffer dump. I'm just raising the question to make sure we think of the
issues before applying patch 1 and losing what might still be a useful
last-ditch debug aid for hard-to-reproduce crashes.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> daemon/libvirtd-config.c | 3 -
> daemon/libvirtd-config.h | 1 -
> daemon/libvirtd.aug | 1 -
> daemon/libvirtd.c | 2 -
> daemon/libvirtd.conf | 7 -
> daemon/test_libvirtd.aug.in | 1 -
> docs/logging.html.in | 10 --
> src/locking/lock_daemon.c | 2 -
> src/locking/lock_daemon_config.c | 2 -
> src/locking/lock_daemon_config.h | 1 -
> src/locking/test_virtlockd.aug.in | 2 -
> src/locking/virtlockd.aug | 1 -
> src/locking/virtlockd.conf | 7 -
> src/rpc/virnetserver.c | 45 -------
> src/util/virlog.c | 264 +-------------------------------------
> src/util/virlog.h | 2 -
> 16 files changed, 3 insertions(+), 348 deletions(-)
Wow - quite a lot tied up in the log ring buffer.
> @@ -431,7 +429,6 @@ daemonConfigLoadOptions(struct daemonConfig *data,
> GET_CONF_INT(conf, filename, log_level);
> GET_CONF_STR(conf, filename, log_filters);
> GET_CONF_STR(conf, filename, log_outputs);
> - GET_CONF_INT(conf, filename, log_buffer_size);
Does augeas gracefully handle an option that is no longer used by
current libvirtd, but which mattered to older versions? That is, I
think you still have to support the option in the .conf file, and
document that it is now ignored, but guarantee that having it in the
.conf file doesn't cause a syntax error.
> +++ b/daemon/libvirtd.aug
> @@ -64,7 +64,6 @@ module Libvirtd =
> let logging_entry = int_entry "log_level"
> | str_entry "log_filters"
> | str_entry "log_outputs"
> - | int_entry "log_buffer_size"
That is, I don't think you can safely delete this line.
> +++ b/daemon/libvirtd.conf
> @@ -345,13 +345,6 @@
> #log_outputs="3:syslog:libvirtd"
> #
>
> -# Log debug buffer size: default 64
> -# The daemon keeps an internal debug log buffer which will be dumped in case
> -# of crash or upon receiving a SIGUSR2 signal. This setting allows to override
> -# the default buffer size in kilobytes.
> -# If value is 0 or less the debug log buffer is deactivated
> -#log_buffer_size = 64
And that you may need to mention that this variable is present but
ignored for backward compatibility, rather than deleting it altogether.
> +++ b/src/locking/virtlockd.aug
> @@ -27,7 +27,6 @@ module Virtlockd =
> let logging_entry = int_entry "log_level"
> | str_entry "log_filters"
> | str_entry "log_outputs"
> - | int_entry "log_buffer_size"
> | int_entry "max_clients"
Same story.
>
> -static void
> -virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED,
> - void *context ATTRIBUTE_UNUSED)
> -{
> - struct sigaction sig_action;
> - int origerrno;
> -
> - origerrno = errno;
> - virLogEmergencyDumpAll(sig);
We may not be dumping the ring buffer any more, but I _still_ think it's
worth keeping this signal handler and dumping a message that we detected
a fatal crash, as well as pointing the user to go read the contents of
the logging files (and/or rerun libvirtd with more logging enabled). In
other words, delete the ring buffer, but DON'T delete this last-ditch
message to the user.
> @@ -412,23 +384,6 @@ virNetServerPtr virNetServerNew(size_t min_workers,
> sig_action.sa_handler = SIG_IGN;
> sigaction(SIGPIPE, &sig_action, NULL);
>
> - /*
> - * catch fatal errors to dump a log, also hook to USR2 for dynamic
> - * debugging purposes or testing
Deleting USR2 handling makes sense, though.
> -/**
> - * virLogEmergencyDumpAll:
> - * @signum: the signal number
> - *
> - * Emergency function called, possibly from a signal handler.
> - * It need to output the debug ring buffer through the log
> - * output which are safe to use from a signal handler.
> - * In case none is found it is emitted to standard error.
> - */
> -void
> -virLogEmergencyDumpAll(int signum)
> -{
Again, I still think this function is useful - not to dump the ring
buffer, but to mention that we are going down and to check the logs and
give an address where to report the bug.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list