Re: [PATCH 6/6] xfs: avoid COW fork extent lookups in writeback if the fork didn't change

[Dave Chinner <david@fromorbit.com>]

On Fri, Jul 27, 2018 at 05:10:39PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 24, 2018 at 03:35:23PM -0700, Darrick J. Wong wrote:
> > > mutex_unlock contains a barrier, and the sequence is a single register
> > > read.  There is nothing holding a lock here would help us with.
> > 
> > Which mutex_unlock is that?
> 
> Actually an up_write (on i_lock), but the result is the same.

I'm not sure it is - I've lost count of the number of times I've
heard the phrase "unlock does not provide a memory barrier, only
unlock to lock provides a full memory barrier".  As such, I've
always treated unlock as a store (release) barrier to keep stores
inside the critical section, and lock as the correspending load
barrier to keep loads inside the critical section.

I think that's irrelevant, anyway, because there is no memory
barrier implied until the ILOCK is dropped. i.e. we can change the
value before making the extent modification, but there's no
guarantee that it's visible to other CPUs until the store memory
barrier occurs when the ILOCK is dropped. i.e. after the extent map
has actually been changed.

And, FWIW, without read-side memory barriers before checking the
value, the load can be re-ordered and perhaps even elided by the
optimising compiler.

IOWs, I can't see any reliable access serialisation being provided
by holding the ILOCK while modifying the value. Perhaps this should
use WRITE_ONCE/READ_ONCE to ensure the correct memory barriers are
used to serialise tthe sequence count against itself rather than the
wider inode extent modification process?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com