Re: [Patch 1/3] CLD: End-to-end verbosity

2010-04-06 Thread Jeff Garzik

On 03/31/2010 08:43 PM, Pete Zaitcev wrote:

diff --git a/server/server.c b/server/server.c
index 3208e0f..2d68ee6 100644
--- a/server/server.c
+++ b/server/server.c
@@ -55,7 +55,7 @@ static struct argp_option options[] = {
  Store database environment in DIRECTORY.  Default: 
  CLD_DEF_DATADIR },
{ debug, 'D', LEVEL, 0,
- Set debug output to LEVEL (0 = off, 2 = max) },
+ Set debug output to LEVEL (0 = off, 1 = debugging) },
{ stderr, 'E', NULL, 0,
  Switch the log to standard error },
{ foreground, 'F', NULL, 0,
@@ -64,6 +64,8 @@ static struct argp_option options[] = {
  Bind to UDP port PORT.  Default:  CLD_DEF_PORT },
{ pid, 'P', FILE, 0,
  Write daemon process id to FILE.  Default:  CLD_DEF_PIDFN },
+   { verbose, 'v', NULL, 0,
+ Enable the session-level verbosity },
{ strict-free, 1001, NULL, 0,
  For memory-checker runs.  When shutting down server, free local 
  heap, rather than simply exit(2)ing and letting OS clean up. },



As is hinted by the current code's debugging switch being an integer 
'level' value, the server [and client?] has increasing levels of 
verbosity.  The debug levels are


0: key messages affecting server operation, only
1: debugging output enabled, sans per-packet output
2: debugging output enabled, including per-packet output

ie. clearly ordered by increasing value == increased verbosity.

As is clearly illustrated when I cut the patch down to the above 
snippet, the user interface you have created gives the user two knobs 
for log verbosity, and it is not clear to a casual user which knob 
controls which sets of messages.  That makes for a -more- confusing user 
interface, because the user must constantly ask themselves the question 
do I need debug?  or verbose?  I don't know!


Additionally, this interface changes runs counter to other tools, which 
increase verbosity with added -v switches -- analagous to the existing 
integer-based debug level interface.


If it is truly your desire to permit fine-grained selection of certain 
classes of messages, then don't dick around!  Go ahead and create a 
bitmap log mask which permits fine-grained selection of various 
messages, much like netif_msg_* and netif_msg_init() in the kernel's 
include/linux/netdevice.h.


Having two switches, -d and -v, for different, undocumented classes of 
message just increases confusion.  Put yourself in the mind of a user 
trying to figure out which is which.


I readily admit the __internal implementation__ resulting from your 
patches is a useful cleanup, but at a macro level, it merely increases 
logging user interface confusion.


Jeff


--
To unsubscribe from this list: send the line unsubscribe hail-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Patch 1/7] tabled: make two dump displays uniform

2010-04-06 Thread Jeff Garzik

On 04/01/2010 09:51 PM, Pete Zaitcev wrote:

From: Jeff Garzikjgar...@pobox.com
Subject: Re: Tabled issues
Date: Mon, 29 Mar 2010 15:32:33 -0400



I asserted that the standard stats dump facility must dump
all available statistics.  That does not exclude other methods
of stat(us) dumping.  Your patch added new stats to the HTML-pretty
version of output, but failed to add the new stats to the standard
stat dump facility.


Your wish is my command.

Signed-off-by: Pete Zaitcevzait...@redhat.com

---
  server/replica.c |   28 +
  server/server.c  |   47 ++
  server/status.c  |   22 +--
  server/storage.c |   50 +
  server/tabled.h  |3 ++
  5 files changed, 117 insertions(+), 33 deletions(-)


applied, thanks.  I will endeavor to make the stats dump more like nfs4d 
in the future, FWIW.



--
To unsubscribe from this list: send the line unsubscribe hail-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Patch 2/7] tabled: fix the endless recusion when reading long objects

2010-04-06 Thread Jeff Garzik

On 04/01/2010 09:51 PM, Pete Zaitcev wrote:

At certain network and disk speeds, tabled can blow its stack by
filling it with (essentially) endless recursion:

#2  0x0040c077 in cli_write_free (cli=value optimized out, tmp=
 0x7bb910, done=value optimized out) at server.c:397
#3  0x0040ca55 in cli_writable (cli=0x686e90) at server.c:525
#4  0x0040da65 in cli_write_start (cli=0x686e90) at server.c:561
#5  0x00408ad5 in object_get_poke (cli=0x686e90) at object.c:1039
#6  0x0040c077 in cli_write_free (cli=value optimized out, tmp=
 0x7bb8d0, done=value optimized out) at server.c:397
#7  0x0040ca55 in cli_writable (cli=0x686e90) at server.c:525
#8  0x0040da65 in cli_write_start (cli=0x686e90) at server.c:561
#9  0x00408ad5 in object_get_poke (cli=0x686e90) at object.c:1039
#10 0x0040c077 in cli_write_free (cli=value optimized out, tmp=
 0x7bb890, done=value optimized out) at server.c:397

The fix is to deliver callbacks only from the top level.

Callbacks must be delivered every time a send is completed,
which amounts to every call to is_writeable(). Since there
is a large number of callers to it, we found it advantageous
to run callbacks from every source of events. In other words,
every function that is passed to event_set must invoke
cli_write_run_compl. Mind that storage.c contains calls to
event_set.

Signed-off-by: Pete Zaitcevzait...@redhat.com

---
  server/object.c |4 +++
  server/server.c |   52 +++---
  server/tabled.h |6 +
  3 files changed, 50 insertions(+), 12 deletions(-)


applied 2-7


--
To unsubscribe from this list: send the line unsubscribe hail-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Patch 1/7] tabled: make two dump displays uniform

2010-04-06 Thread Pete Zaitcev
On Tue, 06 Apr 2010 12:57:57 -0400
Jeff Garzik j...@garzik.org wrote:

 applied, thanks.  I will endeavor to make the stats dump more like nfs4d 
 in the future, FWIW.

I was going to look into it too, but there's now some stuff
I urgently need to address in Darcy's report on tabled's faults.
So I only wrote nfs4 into the endless TODO.

-- Pete
--
To unsubscribe from this list: send the line unsubscribe hail-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Patch 1/3] CLD: End-to-end verbosity

2010-04-06 Thread Jeff Garzik

On 04/06/2010 11:32 PM, Pete Zaitcev wrote:

On Tue, 06 Apr 2010 10:40:33 -0400
Jeff Garzikj...@garzik.org  wrote:


The debug levels are

0: key messages affecting server operation, only
1: debugging output enabled, sans per-packet output
2: debugging output enabled, including per-packet output


The previous patch did just that:
Why did you reject it?


That's a damned good question.  I have no idea.  Did I ever reply to 
that patch?  It looks like I fscked up and missed it?


Jeff



--
To unsubscribe from this list: send the line unsubscribe hail-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [Patch 1/3] CLD: End-to-end verbosity

2010-04-06 Thread Pete Zaitcev
On Wed, 07 Apr 2010 00:36:32 -0400
Jeff Garzik j...@garzik.org wrote:

 On 04/06/2010 11:32 PM, Pete Zaitcev wrote:
  On Tue, 06 Apr 2010 10:40:33 -0400
  Jeff Garzikj...@garzik.org  wrote:
 
  The debug levels are
 
 0: key messages affecting server operation, only
 1: debugging output enabled, sans per-packet output
 2: debugging output enabled, including per-packet output
 
  The previous patch did just that:
  Why did you reject it?
 
 That's a damned good question.  I have no idea.  Did I ever reply to 
 that patch?  It looks like I fscked up and missed it?

Apparently no reply (by e-mail):
http://marc.info/?l=hail-develm=126575343714155w=2

I recall you said that users hate bitmasks on IRC or in other thread,
so I thought that -D and -v would be right (effectively a bitmask but
no hexadecimal at least).

-- Pete
--
To unsubscribe from this list: send the line unsubscribe hail-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html