Re: [PATCH v2 5/5] xfs: revalidate imap properly before writeback delalloc conversion

[Brian Foster <bfoster@redhat.com>]

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