Re: ext2 FS corruption with 2.5.59.

[Anton Blanchard <anton@samba.org>]

 
Hi,

> +static inline void i_size_write(struct inode * inode, loff_t i_size)
> +{
> +#if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> +#ifdef __ARCH_HAS_GET_SET_64BIT
> +	set_64bit((unsigned long long *) &inode->i_size, (unsigned long long) i_size);
> +#else
> +	inode->i_size_version2++;
> +	wmb();
> +	inode->i_size = i_size;
> +	wmb();
> +	inode->i_size_version1++;
> +	wmb(); /* make it visible ASAP */
> +#endif
> +#elif BITS_PER_LONG==64 || !defined(CONFIG_SMP)
> +	inode->i_size = i_size;
> +#endif
> +}

That last wmb is suspect. We dont put an wmb after a spinlock to "make it
visible". If you think of an wmb as an ordering tag that propagates out
through the cpu and storage hierarchy then wmb is not going to help us here.

I guess the store could get reordered (and so delayed) a bit, but an wmb
is relatively expensive on some architectures.

> This is actually fairly pointless, because these fields are write-mostly and
> read-rarely.  But we need a spinlock anyway because of the concurrent
> modifiers problem.

It would be interesting to compare a spinlock or rwlock against a frlock
in a write mostly situation. Actually isnt it going to be slower because
we have to take a spinlock to serialise around the frlock write path?

Anton