On 05/14/2013 10:59 AM, Dave Chinner wrote:
> On Sun, May 12, 2013 at 10:13:30PM +0400, Glauber Costa wrote:
>> From: Dave Chinner <dchinner@xxxxxxxxxx>
>>
>> [ glommer: don't reintroduce double decrement of nr_unused_dentries,
>> adapted for new LRU return codes ]
>> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
>> Signed-off-by: Glauber Costa <glommer@xxxxxxxxxx>
>> ---
>
> I'm seeing a panic on startup in d_kill() with an invalid d_child
> list entry with this patch. I haven't got to the bottom of it yet.
>
> .....
>
>> void shrink_dcache_sb(struct super_block *sb)
>> {
>> - LIST_HEAD(tmp);
>> -
>> - spin_lock(&sb->s_dentry_lru_lock);
>> - while (!list_empty(&sb->s_dentry_lru)) {
>> - list_splice_init(&sb->s_dentry_lru, &tmp);
>> -
>> - /*
>> - * account for removal here so we don't need to handle it later
>> - * even though the dentry is no longer on the lru list.
>> - */
>> - this_cpu_sub(nr_dentry_unused, sb->s_nr_dentry_unused);
>> - sb->s_nr_dentry_unused = 0;
>> -
>> - spin_unlock(&sb->s_dentry_lru_lock);
>> - shrink_dcache_list(&tmp);
>> - spin_lock(&sb->s_dentry_lru_lock);
>> - }
>> - spin_unlock(&sb->s_dentry_lru_lock);
>> + list_lru_dispose_all(&sb->s_dentry_lru, shrink_dcache_list);
>> }
>> EXPORT_SYMBOL(shrink_dcache_sb);
>
> And here comes the fun part. This doesn't account for the
> dentries that are freed from the superblock here.
>
> So, it needs to be something like:
>
> void shrink_dcache_sb(struct super_block *sb)
> {
> unsigned long disposed;
>
> disposed = list_lru_dispose_all(&sb->s_dentry_lru,
> shrink_dcache_list);
>
> this_cpu_sub(nr_dentry_unused, disposed);
> }
>
> But, therein lies a problem. nr_dentry_unused is a 32 bit counter,
> and we can return a 64 bit value here. So that means we have to bump
> nr_dentry_unused to a long, not an int for these per-cpu counters to
> work.
>
> And then there's the problem that the sum of these counters only
> uses an int. Which means if we get large numbers of negative values
> on different CPU from unmounts, the summation will end up
> overflowing and it'll all suck.
>
> So, Glauber, what do you reckon? I've never likes this stupid
> hand-rolled per-cpu counter stuff, and it's causing issues. Should
> we just convert them to generic per-cpu counters because they are
> 64bit clean and just handle out-of-range sums in the /proc update
> code?
>
I am not against it, per se.
But assuming that whoever did it did it for a reason, can't we just
propagate 64 bit quantities everywhere? Moving to long instead of int
should already give us some room, and is actually a sensible thing to
do: There is no reason whatsoever to keep using ints around like this.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
