Re: [RFC PATCH] xfs: fix cow_seq locking behavior

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jul 30, 2018 at 01:14:56AM -0700, Christoph Hellwig wrote:
> On Sun, Jul 29, 2018 at 10:55:39PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In Christoph Hellwig's patch "xfs: avoid COW fork extent lookups in
> > writeback if the fork didn't change" (which has not yet graduated to
> > for-next), we sample the COW fork sequence number without taking the
> > ilock.  This is a little strange, since in general we always take it
> > before accessing anything in a block mapping.  I think we get lucky in
> > that the unlocking during actual cow fork changes will erect the
> > necessary memory barriers (on x86 anyway) but let's not play fast and
> > loose with breaking everyone else's model of how locking works.
> 
> What exaxtly do you want to protect here?  It's not like we have any
> multiple fields we need to synchronize access to to.

Protecting against us screwing up the locking here some day due to a
subtlety that nobody will remember in 6 months. :)

> And it's not like this is superficials - in addition to not providing
> any actual synchronization this also means we have to take the ilock
> for every page, which reduces a large part of the improvements in the
> series.

Agreed!  I nearly tagged this RFCRAP instead.  Sometimes I send patches
to try to provoke a response... <cough>D :0

Now that I've had a night to think it over (and it's no longer 100F but
the sky is still blood red) I think we ought to have a comment
explaining how the synchronization works such that we don't need to take
the ILOCK before testing cow_seq....

/*
 * COW fork blocks can overlap data fork blocks even if the blocks
 * aren't shared.  COW I/O always takes precedent, so we must always
 * check for overlap on reflink inodes unless the mapping is already a
 * COW one.
 *
 * It's safe to check the COW fork if_seq here without the ILOCK because
 * we've indirectly protected against concurrent updates: writeback has
 * the page locked, which prevents concurrent invalidations by reflink
 * and directio and prevents concurrent buffered writes to the same
 * page.  Concurrent changes to other parts of the COW fork will drop
 * the i_lock on their way out, which provides the necessary memory
 * barrier to ensure that we see the updated if_seq.
 */

I'm not actually sure about the last sentence anymore -- that's what I
was thinking the first time I looked at this patch, before Dave spoke
up.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html