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

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

On Mon, Jul 23, 2018 at 09:49:41AM +0200, Christoph Hellwig wrote:
> On Sun, Jul 22, 2018 at 09:23:11AM +1000, Dave Chinner wrote:
> > On Tue, Jul 17, 2018 at 04:24:05PM -0700, Christoph Hellwig wrote:
> > > @@ -333,12 +335,15 @@ xfs_map_blocks(
> > >  	 * 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.
> > > +	 * COW one, or the COW fork hasn't changed from the last time we looked
> > > +	 * at it.
> > >  	 */
> > >  	imap_valid = offset_fsb >= wpc->imap.br_startoff &&
> > >  		     offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount;
> > >  	if (imap_valid &&
> > > -	    (!xfs_inode_has_cow_data(ip) || wpc->io_type == XFS_IO_COW))
> > > +	    (!xfs_inode_has_cow_data(ip) ||
> > > +	     wpc->io_type == XFS_IO_COW ||
> > > +	     wpc->cow_seq == ip->i_cowfp->if_seq))
> > >  		return 0;
> > 
> > This seqno check is still racy against concurrent extent tree
> > modification. We aren't holding the ILOCK here at all, the sequence
> > numbers aren't atomic and there are no memory barriers to serialise
> > cross-cpu load/store operations...
> 
> 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?  I took a second look at the i_cowfp access
and realized that we don't take ILOCK_SHARED until after the comparison.
Writeback doesn't take any of the other inode locks AFAIK... so either
there's some locking subtlety here that ought to be explained in a
comment, or is this a bug waiting to happen?

--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