Re: [PATCH 2/2] Btrfs: serialize flushers in reserve_metadata_bytes

[Josef Bacik <josef@redhat.com>]

On 06/10/2011 01:47 PM, David Sterba wrote:
> 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.

I just used the spin lock.

Josef