Re: [PATCH 3/5] mm: Do not try to write pinned folio during memory cleaning writeback

[Jan Kara <jack@suse.cz>]

On Thu 09-02-23 17:54:14, John Hubbard wrote:
> On 2/9/23 04:31, Jan Kara wrote:
> > When a folio is pinned, there is no point in trying to write it during
> > memory cleaning writeback. We cannot reclaim the folio until it is
> > unpinned anyway and we cannot even be sure the folio is really clean.
> > On top of that writeback of such folio may be problematic as the data
> > can change while the writeback is running thus causing checksum or
> > DIF/DIX failures. So just don't bother doing memory cleaning writeback
> > for pinned folios.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >   fs/9p/vfs_addr.c            |  2 +-
> >   fs/afs/file.c               |  2 +-
> >   fs/afs/write.c              |  6 +++---
> >   fs/btrfs/extent_io.c        | 14 +++++++-------
> >   fs/btrfs/free-space-cache.c |  2 +-
> >   fs/btrfs/inode.c            |  2 +-
> >   fs/btrfs/subpage.c          |  2 +-
> 
> Hi Jan!
> 
> Just a quick note that this breaks the btrfs build in subpage.c.
> Because, unfortunately, btrfs creates 6 sets of functions via calls to a
> macro: IMPLEMENT_BTRFS_PAGE_OPS(). And that expects only one argument to
> the clear_page_func, and thus to clear_page_dirty_for_io().
> 
> It seems infeasible (right?) to add another argument to the other
> clear_page_func functions, which include:
> 
>    ClearPageUptodate
>    ClearPageError
>    end_page_writeback
>    ClearPageOrdered
>    ClearPageChecked
> 
> , so I expect IMPLEMENT_BTRFS_PAGE_OPS() may need to be partially
> unrolled, in order to pass in the new writeback control arg to
> clear_page_dirty_for_io().

Aha, thanks for catching this. So it is easy to fix this to make things
compile (just a wrapper around clear_page_dirty_for_io() - done now). It
will be a bit more challenging to propagate wbc into there for proper
decision - that will probably need these functions not to be defined by the
macros.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR