| Message ID | 20231126124720.1249310-5-hch@lst.de (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [01/13] iomap: clear the per-folio dirty bits on all writeback failures | expand |
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
On Mon, Nov 27, 2023 at 12:09:08PM +0530, Ritesh Harjani wrote: > 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. Well, xfs/iomap never really cared about the flag, just that it didn't get called from direct reclaim or kswapd. If we think such a WARN_ON is still useful, the only caller of it in do_writepages might be a better place.
On Mon, Nov 27, 2023 at 07:41:07AM +0100, Christoph Hellwig wrote: > On Mon, Nov 27, 2023 at 12:09:08PM +0530, Ritesh Harjani wrote: > > 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. > > Well, xfs/iomap never really cared about the flag, just that it didn't > get called from direct reclaim or kswapd. If we think such a WARN_ON > is still useful, the only caller of it in do_writepages might be a better > place. I think that's a good idea, given the enormous complexity of modern day XFS and the sometimes rough quality of others. :P --D
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;
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(-)