On Tue, Jan 22, 2019 at 09:14:45AM -0800, Christoph Hellwig wrote: > On Mon, Jan 21, 2019 at 12:42:56PM -0500, Brian Foster wrote: > > > It protects against the case where we migrate a COW fork mapping to the > > > data fork, which is not protected by the page lock. But I guess the > > > check warrants a comment and an assert. > > > > > > > Yeah, probably. It's not really clear to me what that means. > > > > > > 2. If so, then it also seems that the whole "eof:" thing in > > > > xfs_map_blocks() should never happen for data forks. If that's the case, > > > > the use of the eof label there seems gratuitous. > > > > > > Let me try with asserts enabled. > > > > > > > Ok, thanks. > > So, the nimaps == 0 case hits even for the data fork when running > xfs/442 on a 1k block size file system. That test has generally been > a fun source of bugs in the always_cow series. Hmm, Ok.. any idea how/why that is? I'm curious if this is an artifact of this code being racy (i.e., nimaps == 0 occurs somehow on a range external to the page) or there is some legitimate means to lose a delalloc block under the locked page. If the latter, we should at least document it in the code.. FWIW, I have some refactoring in place that basically turns nimaps == 0 into an error and I don't see an xfs/442 failure with 1k or 4k blocks, though that's only from a couple quick runs and I'm still working through some changes. The difference of course is that this code uses the range of the delalloc extent in the data fork rather than the (potentially already stale) range from wpc->imap. I'll let it spin for a bit I suppose and see if it explodes.. Brian
