Re: [Bcache v14 16/16] bcache: Debug and tracing code

On Tue, 2012-06-12 at 10:24 -0700, Kent Overstreet wrote:

On Tue, Jun 12, 2012 at 09:50:58AM -0700, Joe Perches wrote:

quoted

On Tue, 2012-06-12 at 08:39 -0700, Kent Overstreet wrote:

quoted

diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c

quoted

+static void dump_bset(struct btree *b, struct bset *i)
+{
+	for (struct bkey *k = i->start; k < end(i); k = bkey_next(k)) {
+		printk(KERN_ERR "block %zu key %zu/%i: %s", index(i, b),
+		       (uint64_t *) k - i->d, i->keys, pkey(k));

Add #define pr_fmt and use pr_<level> not printk everywhere.

I've got the pr_fmt, but I don't want to use it here because it's
dumping a btree node (100s of lines) so the bcache: would be redundant,
but more importantly I don't want lines getting truncated.

quoted

Doesn't this throw a gcc warning for argument mismatch?

No, why?

"(uint64_t *)k - i->d" is what type again?
What is a %zu?

Isn't that a mismatch?

quoted

quoted

+static int debug_seq_show(struct seq_file *f, void *data)
+{
+	static const char *tabs = "\t\t\t\t\t";

Seems a _very_ odd use.

It is a strange hack.

The idea is that we want to indent more as we recurse; we could build up
a new string of tabs each time we recurse that's got one more tab than
our parent's, but that'd be a pain in the ass and it'd use more stack
space (though that should be fine here), so instead it's just
decrementing the pointer to the tab string to produce a string with one
more tab.

It's a nice idea, but that's not what's happening
as I believe you reference tabs only once as &tabs[4]

I'm not opposed to taking it out if you know cleaner way that isn't
ridiculously verbose. But this code needs to be rewritten to not use
single_open() (which I tihnk is going to be a pain in the ass) so it's
not really at the top of my list.

quoted

quoted

+	uint64_t last = 0, sectors = 0;
+	struct cache_set *c = f->private;
+
+	struct btree_op op;
+	bch_btree_op_init_stack(&op);
+
+	btree_root(dump, c, &op, f, &tabs[4], &last, &sectors);
+

Why not just:

	btree_root(dump, c, &op, "\t", &last, &sectors);