Re: dentry bloat.

[Linus Torvalds]

On Sat, 8 May 2004, Andrew Morton wrote:
>   */
>  struct qstr {
>  	const unsigned char *name;
> -	unsigned int len;
>  	unsigned int hash;
> -};
> +	unsigned short len;
> +} __attribute__((packed));

This would make me nervous.

Since we don't use this thing in an array, this may be right. But on the
other hand, if this thing is preceded by an "unsigned short" in a
structure (or "int" on a 64-bit architecture), I think you just made
"name" be unaligned, which can cause serious problems. Because I think
"packed" _also_ means that it doesn't honor the alignment of the thing
when laying it out in structures.

Also, in your previous patch (which I'm not as convinced might be wrong), 
the d_qstr pointer removal makes me worry:

-       struct qstr * d_qstr;           /* quick str ptr used in lockless lookup and concurrent d_move */

I thought the point of d_qstr was that when we do the lockless lookup,
we're guaranteed to always see "stable storage" in the sense that when we
follow the d_qstr, we will always get a "char *" + "len" that match, and
we could never see a partial update (ie len points to the old one, and
"char *" points to the new one).

In particular, think about the "d_compare(parent, qstr, name)" / 
"memcmp(qstr->name, str, len)" part - what if "len" doesn't match str, 
because a concurrent d_move() is updating them, and maybe we will compare 
past the end of kernel mapped memory or something?

(In other words, the "move_count" check should protect us from returning a 
wrong dentry, but I'd worry that we'd do something that could cause 
serious problems before we even get to the "move_count" check).

Hmm?

Btw, I'd love to be proven wrong, since I hate that d_qstr, and I think 
your d_qstr removal patch otherwise looked wonderful.

		Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/