On Thu, Jul 12, 2018 at 03:49:10PM +0200, 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; Isn't this racy? It's not an atomic variable, we hold no locks, there are no memory barriers, etc. Hence we can miss changes made by concurrent mapping changes... Which makes me ask - is the sequence number bumped before or after we modify the extent map? It seems to me that it needs to be done before we make a modification so that we invalidate the current map before we change the extent map to avoid issuing IO to an extent map we are know we changing, right? /me had prototype seqno patches like this from way back and kept hitting races because of these "imap_valid checks are done without locks" issues... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
