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

[Brian Foster <bfoster@redhat.com>]

On Wed, Jan 23, 2019 at 10:14:12AM -0800, Christoph Hellwig wrote:
> On Tue, Jan 22, 2019 at 01:13:28PM -0500, Brian Foster wrote:
> > > 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..
> 
> I haven't managed to pinpint it mostly because xfs_bmapi_write is
> such an unreadable mess.  Based on that I decided to implement the idea
> of a separate xfs_bmapi_delalloc_convert helper again, mostly to try
> to understand what actuall is going on.  This seems to work pretty
> reasonable after initial testing, except that it seems to unhide an
> issue in xfs/109 which doesn't look directly related.  The current
> status is here:
> 
> http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-mapping-validation.2
> 

I have a v3 of this series that does something similar, but doesn't
quite take the refactoring as far. I just created an
xfs_bmapi_delalloc() wrapper over xfs_bmapi_write(). The changes to the
latter are minimal based on a tweak of XFS_BMAPI_DELALLOC behavior. The
change to xfs_iomap_write_allocate() is basically just to use the new
helper instead of xfs_bmapi_write().

The refactoring you have looks like the right direction to me, but I'm
wondering why we need to change so much all at once, particularly since
it's not clear that some of this -EAGAIN stuff is really necessary.
Pulling bits out of xfs_bmapi_write() is also inherently more difficult
to review than reusing it, IMO.

Hmm, I'm wondering if we could rebase your refactoring onto the simple
wrapper in my patch. You'd basically just reimplement the helper, kill
off the XFS_BMAPI_DELALLOC stuff as you already have, and the changes to
xfs_iomap_write_allocate() (notwithstanding moving/renaming the
function, which should really be split up into separate patches) would
probably be minimal (i.e., do the extent lookup).

I was still running some tests but I've got through a decent amount
already and have everything formatted to send out so I'll just post
it...

Brian