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

[Brian Foster <bfoster@redhat.com>]

On Mon, Oct 09, 2017 at 10:54:14AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Recently we've had warnings arise from the vm handing us pages
> without bufferheads attached to them. This should not ever occur
> in XFS, but we don't defend against it properly if it does. The only
> place where we remove bufferheads from a page is in
> xfs_vm_releasepage(), but we can't tell the difference here between
> "page is dirty so don't release" and "page is dirty but is being
> invalidated so release it".
> 

Ok, so we changed ->releasepage() in 99579ccec4 ("xfs: skip dirty pages
in ->releasepage()") to deal with the fact that the mm can send
legitimately dirty pages to ->releasepage(). That was apparently too
aggressive a change because truncated pages would also end up skipped
because the dirty bit is not cleared by the time the release occurs.
Commit 0a417b8dc1 ("xfs: Timely free truncated dirty pages") modified
->releasepage() again to not skip all dirty pages and only warn for
delalloc/unwritten blocks on clean pages.

That was apparently insufficient because we have some codepaths where
not skipping dirty pages can allow us to strip the buffers from a page
incorrectly...

> In some places that are invalidating pages ask for pages to be
> released and follow up afterward calling ->releasepage by checking
> whether the page was dirty and then aborting the invalidation. This
> is a possible vector for releasing buffers from a page but then
> leaving it in the mapping, so we really do need to avoid dirty pages
> in xfs_vm_releasepage().
> 

... but I'm having a hard time parsing that first sentence to understand
how that is. Can you elaborate on and/or give a specific reference to
this problematic invalidation abort sequence?

Also, it looks like truncate_complete_page() eventually and
unconditionally clears the page dirty bit, it just happens after the
invalidate -> release attempt sequence that occurs down through
do_invalidatepage(). IIUC, this was the problem fixed by Jan's patch
mentioned above. Is there any reason we can't do the dirty cancel a
little earlier there? Would that also address this problem?

Brian

> To differentiate between invalidated pages and normal pages, we need
> to clear the page dirty flag when invalidating the pages. This can
> be done through xfs_vm_invalidatepage(), and will result
> xfs_vm_releasepage() seeing the page as clean which matches the
> bufferhead state on the page after calling block_invalidatepage().
> 
> Hence we can re-add the page dirty check in xfs_vm_releasepage to
> catch the case where we might be releasing a page that is actually
> dirty and so should not have the bufferheads on it removed. This
> will remove one possible vector of "dirty page with no bufferheads"
> and so help narrow down the search for the root cause of that
> problem.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---

>  fs/xfs/xfs_aops.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index f18e5932aec4..067284d84d9e 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -735,6 +735,14 @@ xfs_vm_invalidatepage(
>  {
>  	trace_xfs_invalidatepage(page->mapping->host, page, offset,
>  				 length);
> +
> +	/*
> +	 * If we are invalidating the entire page, clear the dirty state from it
> +	 * so that we can check for attempts to release dirty cached pages in
> +	 * xfs_vm_releasepage().
> +	 */
> +	if (offset == 0 && length >= PAGE_SIZE)
> +		cancel_dirty_page(page);
>  	block_invalidatepage(page, offset, length);
>  }
>  
> @@ -1190,25 +1198,27 @@ xfs_vm_releasepage(
>  	 * mm accommodates an old ext3 case where clean pages might not have had
>  	 * the dirty bit cleared. Thus, it can send actual dirty pages to
>  	 * ->releasepage() via shrink_active_list(). Conversely,
> -	 * block_invalidatepage() can send pages that are still marked dirty
> -	 * but otherwise have invalidated buffers.
> +	 * block_invalidatepage() can send pages that are still marked dirty but
> +	 * otherwise have invalidated buffers.
>  	 *
>  	 * We want to release the latter to avoid unnecessary buildup of the
> -	 * LRU, skip the former and warn if we've left any lingering
> -	 * delalloc/unwritten buffers on clean pages. Skip pages with delalloc
> -	 * or unwritten buffers and warn if the page is not dirty. Otherwise
> -	 * try to release the buffers.
> +	 * LRU, so xfs_vm_invalidatepage() clears the page dirty flag on pages
> +	 * that are entirely invalidated and need to be released.  Hence the
> +	 * only time we should get dirty pages here is through
> +	 * shrink_active_list() and so we can simply skip those now.
> +	 *
> +	 * warn if we've left any lingering delalloc/unwritten buffers on clean
> +	 * or invalidated pages we are about to release.
>  	 */
> +	if (PageDirty(page))
> +		return 0;
> +
>  	xfs_count_page_state(page, &delalloc, &unwritten);
>  
> -	if (delalloc) {
> -		WARN_ON_ONCE(!PageDirty(page));
> +	if (WARN_ON_ONCE(delalloc))
>  		return 0;
> -	}
> -	if (unwritten) {
> -		WARN_ON_ONCE(!PageDirty(page));
> +	if (WARN_ON_ONCE(unwritten))
>  		return 0;
> -	}
>  
>  	return try_to_free_buffers(page);
>  }
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html