3

Could anyone explain why linux kernel's ext2 function int ext2_statfs (struct dentry * dentry, struct kstatfs * buf)

issues smp_rmb() andsmp_wmb() in else if (sbi->s_blocks_last != le32_to_cpu(es->s_blocks_count)) { case ?

This was added in the upstream kernel commit 2235219b7721b8e74de6841e79240936561a2b63 which omits unnecessary calculation of .statfs, but couldn't understand why memory barriers had been added.

1 Answers1

3

Since the function starts by doing a spin_lock (and unlocks at the end),

spin_lock(&sbi->s_lock);

it is protected against concurrent accesses. So why the need of smp_wmb and smp_rmb...

As for the write operation, 

    sbi->s_overhead_last = overhead;
    smp_wmb();
    sbi->s_blocks_last = le32_to_cpu(es->s_blocks_count);

it appears that the smp_wmb would prevent any compiler or processor optimization (since there is no dependency between the two writes in terms of memory accesses) that would change the order of the two assignments. Since there is no concurrent accesses in ext2_statfs, the problem could happen in another function that may have a similar behavior and check if there is a block count change and, if not, use the overhead from sbi->s_overhead_last - which would be wrong if another thread would be, at this very time, in another function, in the middle of the two write operations in the wrong order.

This is probably a precaution as any similar write access to the sbi properties would spin_lock it (with the same spinlock) beforehand. Also it enhances the fact that the two operations must be done in this order.

As for the read access the need is less obvious and would require to analyze further the dependencies between the sbi and es blocks.

Déjà vu
  • 28,223
  • 6
  • 72
  • 100
  • Due to the spinlock there is indeed no concurrency. However the result of a previous execution of the same function (or anything else that modified the data in question) might not yet be visible to the current cpu. The barriers make sure this function always sees the current state (which then can't change due to the lock) and that the result is committed before the lock is released. – Jester Aug 31 '14 at 12:14
  • Right, both of your answer/comment make good sense to me, but since spinlock (I believe) has effect of mb as well on many of the archs, shouldn't someone who writes a similar function (that does similar if check and update) possibly in the future just use the same primitive spinlock(superblock->lock) instead of caring about ordering that could have been optimized by compilers ? –  Aug 31 '14 at 12:43
  • # and I believe that's what later part of ring0's answer implies. -> "This is probably a precaution as any similar write access to the sbi properties would spin_lock it (with the same spinlock) beforehand." –  Aug 31 '14 at 12:55