On Thu, Jun 09, 2011 at 08:00:15PM +0200, David Sterba wrote: > On Thu, Jun 09, 2011 at 10:00:42AM -0400, Josef Bacik wrote: > > We're not trying to be perfect here, we're trying to be fast :). > > Be even faster with smp_rmb() :)
Arne made me think about this again. Let's analyze it in more detail: The read side, if(delalloc_bytes), utilizes a full barrier, which will force all cpus to flush pending reads and writes. This will ensure 'if' will see a fresh value. However, there is no pairing write barrier and the write flush will happen at some point in time, (delalloc_bytes += len), but completely unsynchronized with the read side. The smp_mb barrier has no desired synchonization effect. Moreover, it has a performance hit. Doing it right with barriers would mean to add smp_rmb before the if(...) and smp_wmb after the "delalloc_bytes =", but only in the case the variable is solely synchronized via barriers. Not our case. There is the spinlock. As strict correctness is not needed here, you admit that delalloc_bytes might not correspond to the state of fs_info->delalloc_inodes and this is not a problem. Fine. But then you do not need the smp_mb. The value of delalloc_bytes will be "random" (ie. unsynchronized), with or without the barrier. Please drop it from the patch. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
