On Mon, 21 Apr 2008 02:50:42 -0400
Erez Zadok <ezk@xxxxxxxxxxxxx> wrote:
>
> 1. remove the 3rd arg to fsstack_copy_attr_all. There are no users for it:
> ecryptfs never used the 3rd arg; unionfs stopped using it a long time
> ago. Halcrow ok'ed this patch some time ago.
>
> 2. add necessary locking for 32-bit smp systems in fsstack_copy_inode_size
> (courtesy Hugh Dickins).
>
> 3. minor commenting style changes, and addition of copyrights which were
> missing.
>
> Acked-by: Mike Halcrow <mhalcrow@xxxxxxxxxx>
> Signed-off-by: Erez Zadok <ezk@xxxxxxxxxxxxx>
>
> ...
>
> void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
> {
> - i_size_write(dst, i_size_read((struct inode *)src));
> +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> + spin_lock(&dst->i_lock);
> +#endif
The defined(CONFIG_SMP) is wrong. The spinlock is here to protect
dst->i_blocks, but it can be corrupted via preemption on uniprocessor as
well. So a plain old
#if BITS_PER_LONG == 32
would fix that.
> + i_size_write(dst, i_size_read(src));
> dst->i_blocks = src->i_blocks;
> +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> + spin_unlock(&dst->i_lock);
> +#endif
> }
However, what about src->i_blocks? It is protected by src->i_lock. The
code as you have it here could read transient values.
Furthermore, i_lock is defined as an innermost lock, for protection of
inode internals. But here we're proposing "taking" inode->i_size_seqcount
inside i_lock. Not necessarily a problem, but it broke the old rule.
We're also doing a read_seqlock of a _different_ inode inside this inode's
i_lock. Again, this is not necessarily a problem (but it might be!) but it
adds complexity and needs thought.
Can we avoid having to think?
void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
{
blkcnt_t i_blocks;
loff_t i_size;
i_size = i_size_read(src);
spin_lock_32bit(&src->i_lock);
i_blocks = src->i_blocks;
spin_unlock_32bit(&src->i_lock);
i_size_write(dst, i_size);
spin_lock_32bit(&dst->i_lock)
dst->i_blocks = i_blocks;
spin_unlock_32bit(&dst->i_lock)
}
--
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
