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

[Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>]

On Tue, Jun 12, 2012 at 09:50:58AM -0700, Joe Perches wrote:
> On Tue, 2012-06-12 at 08:39 -0700, Kent Overstreet wrote:
> 
> > diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
> 
> > +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.

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

No, why?

> 
> > +static void vdump_bucket_and_panic(struct btree *b, const char *m, va_list args)
> > +{
> 
> m should be renamed fmt

Agreed.

> 
> > +	struct bset *i;
> > +
> > +	console_lock();
> > +
> > +	for_each_sorted_set(b, i)
> > +		dump_bset(b, i);
> > +
> > +	vprintk(m, args);
> > +
> > +	console_unlock();
> > +
> > +	panic("at %s\n", pbtree(b));
> > +}
> > +
> > +static void dump_bucket_and_panic(struct btree *b, const char *m, ...)
> > +{
> 
> here too.
> 
> > +	va_list args;
> > +	va_start(args, m);
> > +	vdump_bucket_and_panic(b, m, args);
> > +	va_end(args);
> > +}
> > +
> > +static void __maybe_unused
> > +dump_key_and_panic(struct btree *b, struct bset *i, int j)
> > +{
> > +	long bucket = PTR_BUCKET_NR(b->c, node(i, j), 0);
> > +	long r = PTR_OFFSET(node(i, j), 0) & ~(~0 << b->c->bucket_bits);
> > +
> > +	printk(KERN_ERR "level %i block %zu key %i/%i: %s "
> > +	       "bucket %llu offset %li into bucket\n",
> 
> coalesce formats please.

The "block %zu key %i/%i" part? I think I can do that.

> 
> > +	       b->level, index(i, b), j, i->keys, pkey(node(i, j)),
> > +	       (uint64_t) bucket, r);
> > +	dump_bucket_and_panic(b, "");
> > +}
> 
> []
> 
> > +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.

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.

> 
> > +	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);
> 
> Please don't be lazy when modifying code.
> 
> []
> 
> > diff --git a/drivers/md/bcache/debug.h b/drivers/md/bcache/debug.h
> []
> > +#define KEYHACK_SIZE 80
> > +struct keyprint_hack {
> > +	char s[KEYHACK_SIZE];
> > +};
> 
> structures named _hack are generally a bad idea.

Heh. Well, I don't try to hide my terrible hacks. Ugly stuff should be
ugly.

If you missed what it's for, it lets you do
printk("some key: %s\n", pkey(k));

which is very handy.

IMO it ought to be a vsnprintf extension, except that there's no plugin
mechanism to do that so it could be specific to bcache. I'd love to
implement that (shouldn't be very hard), but in the meantime this gets
the job done.