Re: [PATCH 04/13] iomap: drop the obsolete PF_MEMALLOC check in iomap_do_writepage

From: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To: Christoph Hellwig <hch@lst.de>, Christian Brauner <brauner@kernel.org>
Cc: "Darrick J. Wong" <djwong@kernel.org>,
	Chandan Babu R <chandan.babu@oracle.com>,
	Zhang Yi <yi.zhang@huaweicloud.com>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 04/13] iomap: drop the obsolete PF_MEMALLOC check in iomap_do_writepage
Date: Mon, 27 Nov 2023 12:09:08 +0530	[thread overview]
Message-ID: <875y1nsnsj.fsf@doe.com> (raw)
In-Reply-To: <20231126124720.1249310-5-hch@lst.de>

Christoph Hellwig <hch@lst.de> writes:

> The iomap writepage implementation has been removed in commit
> 478af190cb6c ("iomap: remove iomap_writepage") and this code is now only
> called through ->writepages which never happens from memory reclaim.
> Remove the canary in the coal mine now that the coal mine has been shut
> down.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/buffered-io.c | 16 ----------------
>  1 file changed, 16 deletions(-)

Nice cleanup. As you explained, iomap_do_writepage() never gets called
from memory reclaim context. So it is unused code which can be removed. 

However, there was an instance when this WARN was hit by wrong
usage of PF_MEMALLOC flag [1], which was caught due to WARN_ON_ONCE.

[1]: https://lore.kernel.org/linux-xfs/20200309185714.42850-1-ebiggers@kernel.org/

Maybe we can just have a WARN_ON_ONCE() and update the comments?
We anyway don't require "goto redirty" anymore since we will never
actually get called from reclaim context.

Thoughts?

-ritesh

>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index b28c57f8603303..8148e4c9765dac 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1910,20 +1910,6 @@ static int iomap_do_writepage(struct folio *folio,
>  
>  	trace_iomap_writepage(inode, folio_pos(folio), folio_size(folio));
>  
> -	/*
> -	 * Refuse to write the folio out if we're called from reclaim context.
> -	 *
> -	 * This avoids stack overflows when called from deeply used stacks in
> -	 * random callers for direct reclaim or memcg reclaim.  We explicitly
> -	 * allow reclaim from kswapd as the stack usage there is relatively low.
> -	 *
> -	 * This should never happen except in the case of a VM regression so
> -	 * warn about it.
> -	 */
> -	if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD)) ==
> -			PF_MEMALLOC))
> -		goto redirty;
> -
>  	/*
>  	 * Is this folio beyond the end of the file?
>  	 *
> @@ -1989,8 +1975,6 @@ static int iomap_do_writepage(struct folio *folio,
>  
>  	return iomap_writepage_map(wpc, wbc, inode, folio, end_pos);
>  
> -redirty:
> -	folio_redirty_for_writepage(wbc, folio);
>  unlock:
>  	folio_unlock(folio);
>  	return 0;
> -- 
> 2.39.2