On Tue, Jul 17, 2018 at 03:37:10PM +0200, Christoph Hellwig wrote: > On Sat, Jul 14, 2018 at 10:03:01AM +1000, Dave Chinner wrote: > > > 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? > > Right now they are bumped later, but they really should be first. > > I've got a series that bumps them first now. Cool. > That being said at least for the COW fork I don't think we care > about the races too much. All the actual updates to the COW fork > are under the page lock, so for a given page we are synchronized > already. And for the bigger picture we either convert a COW > fork to a real fork, in which case the change doesn't matter, or > we drop it, in which case we already have higher level synchronization. Ok. > > For the data fork things might be more nasty, and that could explain > why my trivial extension to that just didn't work at all.. Yeah, my patch was catching data fork modifications as well. IIRC I ended up putting the seqno bump and checks under the ip->i_flags_lock as a quick method of serialising updates, and that made the update vs check races go away. It didn't make all the problems go away - just the ones that were easy to reproduce - but those remaining bugs may have been in other parts of the patchset that I never got to the bottom of... 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
