Re: kernel BUG at fs/xfs/xfs_aops.c:853! in kernel 4.13 rc6

[Dave Chinner <david@fromorbit.com>]

On Tue, Sep 05, 2017 at 06:17:34PM +0200, Jan Kara wrote:
> On Tue 05-09-17 08:36:48, Dave Chinner wrote:
> > On Mon, Sep 04, 2017 at 02:14:52PM +0200, Jan Kara wrote:
> > > > Seems like a reasonable revert/change, but given that ext3 was killed
> > > > off long ago, is it even still the case that the mm can feed releasepage
> > > > a dirty clean page?  If that is the case, then isn't it time to fix the
> > > > mm too?
> > > 
> > > Yes, ->releasepage() can still get PageDirty page. Whether the page can or
> > > cannot be reclaimed is still upto filesystem to decide.
> > 
> > Yes, and so we have to handle it.  For all I know right now we could
> > be chasing single bit memory error/corruptions....
> 
> Possibly, although I'm not convinced - as I've mentioned I've seen exact
> same assertion failure in XFS on our SLE12-SP2 kernel (4.4 based) in one of
> customers setup. And I've seen two or three times ext4 barfing for exactly
> same reason - buffers stripped from dirty page.

Yeah, we're chasing ghosts at the moment. :/

[....]
> > > Now XFS shouldn't
> > > really end up freeing such page - either because those delalloc / unwritten
> > > checks trigger or because try_to_free_buffers() refuses to free dirty
> > > buffers.
> > 
> > Except if the dirty page has come through the block_invalidation()
> > path, because all the buffers on the page have been invalidated and
> > cleaned. i.e. we've already removed BH_Dirty, BH_Delay and
> > BH_unwritten from all the buffer heads, so invalidated dirty pages
> > will run right through buffers will be removed.
> > 
> > Every caller to ->releasepage() - except the invalidatepage path and
> > the than the bufferhead stripper - checks PageDirty *after* the
> > ->releasepage call and return without doing anything because they
> > aren't supposed to be releasing dirty pages. So if XFS has decided
> > the page can be released, but a mapping invalidation call then notes
> > the page is dirty, it won't invalidate the pagei but it will have
> > had the bufferheads stripped. That's another possible vector, and
> > one that explicit checking of the page dirty flag will avoid.
> 
> Are you speaking about the PageDirty check in __remove_mapping()? I agree
> that checking PageDirty in releasepage would narrow that window for
> corruption although won't close it completely - there are places in the
> kernel that call set_page_dirty() without page lock held and can thus race
> with page invalidation. But I didn't find how any such callsite could race
> to cause what we are observing...

I was referring to invalidate_complete_page2() - I didn't look down
the __remove_mapping() path after I found the first example in
icp2....

> > Hence my question about XFS being able to cancel the page dirty flag
> > before calling block_invalidation() so that we can untangle the mess
> > where we can't tell the difference between a "must release a dirty
> > invalidated page because we've already invalidated the bufferheads"
> > context and the other "release page only if not dirty" caller
> > context?
> 
> Yeah, I agree that if you add cancel_dirty_page() into
> xfs_vm_invalidatepage() before calling block_invalidatepage() and then bail
> on dirty page in xfs_vm_releasepage(), things should work as well and they
> would be more robust.

Ok, I'll put together a patch to do that. Thanks Jan!

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com