Re: [PATCH 4/4] xfs: cancel dirty pages on invalidation

[Dave Chinner <david@fromorbit.com>]

On Wed, Oct 11, 2017 at 05:02:20AM -0400, Brian Foster wrote:
> On Wed, Oct 11, 2017 at 11:04:00AM +1100, Dave Chinner wrote:
> > On Tue, Oct 10, 2017 at 08:29:45AM -0400, Brian Foster wrote:
> > > On Tue, Oct 10, 2017 at 07:48:16AM +1100, Dave Chinner wrote:
> > > > On Mon, Oct 09, 2017 at 10:24:46AM -0400, Brian Foster wrote:
> > > > > On Mon, Oct 09, 2017 at 10:54:14AM +1100, Dave Chinner wrote:
> > shrink_inactive_list
> >   shrink_list
> >     buffer_heads_over_limit
> >       try_to_release_page
> >         xfs_vm_releasepage()
> > 	  xfs_count_page_state   <<< finds delalloc/unwritten buffer
> > 	  WARN_ON(delalloc)
> > 
> 
> The xfs_vm_releasepage() code looks like this:
> 
> 	...
>         xfs_count_page_state(page, &delalloc, &unwritten);
> 
>         if (delalloc) {
>                 WARN_ON_ONCE(!PageDirty(page));
>                 return 0;
>         }
> 	...
> 
> So if we get here with a dirty page over a delalloc extent, for example,
> how exactly does that trigger a spurious warning? AFAICT the warning
> will not trigger because the page is dirty. We decide not to release the
> page precisely because it has delalloc buffers.

And, you know, it's not until you pasted it here that I saw the
"!" in that WARN_ON_ONCE.

I've looked at repeatedly over many weeks, and not *once* have I
registered that it's a "NOT page dirty" warning.

So we can ignore the spurious warning issue. You're right, that
clearly doesn't happen.

But, realistically, we shouldn't be relying on bufferhead state to
determine what the correct action to take is. We can still have
dirty pages that do not have delalloc or unwritten extents on them
that we should not be attempting to free. The current code happily
hands them to try_to_free_buffers() rather than says "no, we don't
free dirty pages".

i.e. if xfs_invalidatepage() is trashing the dirty state on the
buffers, then we should also be trashing the dirty state on the page
so they are clean and coherent when passed to xfs_vm_releasepage.
That leaves us with the simple rule in xfs_vm_releasepage():

	Never release a dirty page because they always contain valid
	data that needs to be written back first.

That's what I'm trying to do - move the control decisions to page
level, rather than having them split and, at times, be incoherent
at bufferhead level. We're wanting to get rid of bufferheads so we
should be making decisions in this code based on page state, not
the bufferhead state...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com