Re: [RFC PATCH v2 0/3] expose mapping wrprotect, fix fb_defio use

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

+cc Kajtar, who has kindly smoke tested this series on real hardware and
confirmed things are working ostensibly the same as before.

On this basis I will be un-RFC'ing this and, if Kajtar can reply to
confirm, will add a Tested-by tag to patch 3/3.

Again to emphasise - there is some urgency here - as struct page fields
that defio relies upon prior to this series are being removed
non-optionally.

Thanks, Lorenzo


On Mon, Jan 13, 2025 at 11:15:45PM +0000, Lorenzo Stoakes wrote:
> Right now the only means by which we can write-protect a range using the
> reverse mapping is via folio_mkclean().
>
> However this is not always the appropriate means of doing so, specifically
> in the case of the framebuffer deferred I/O logic (fb_defio enabled by
> CONFIG_FB_DEFERRED_IO). There, kernel pages are mapped read-only and
> write-protect faults used to batch up I/O operations.
>
> Each time the deferred work is done, folio_mkclean() is used to mark the
> framebuffer page as having had I/O performed on it. However doing so
> requires the kernel page (perhaps allocated via vmalloc()) to have its
> page->mapping, index fields set so the rmap can find everything that maps
> it in order to write-protect.
>
> This is problematic as firstly, these fields should not be set for
> kernel-allocated memory, and secondly these are not folios (it's not user
> memory) and page->index, mapping fields are now deprecated and soon to be
> removed.
>
> The implementers cannot be blamed for having used this however, as there is
> simply no other way of performing this operation correctly.
>
> This series fixes this - we provide the mapping_wrprotect_page() function
> to allow the reverse mapping to be used to look up mappings from the page
> cache object (i.e. its address_space pointer) at a specific offset.
>
> The fb_defio logic already stores this offset, and can simply be expanded
> to keep track of the page cache object, so the change then becomes
> straight-forward.
>
> This series should have no functional change.
>
> *** REVIEWERS NOTES: ***
>
> I do not have any hardware that uses fb_defio, so I'm asking for help with
> testing this series from those who do :) I have tested the mm side of this,
> and done a quick compile smoke test of the fb_defio side but this _very
> much_ requires testing on actual hardware to ensure everything behaves as
> expected.
>
> This is based on Andrew's tree [0] in the mm-unstable branch - I was
> thinking it'd be best to go through the mm tree (with fb_defio maintainer
> approval, of course!) as it relies upon the mm changes to work correctly.
>
> [0]: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/
>
> RFC v2:
> * Updated Jaya Kumar's email on cc - the MAINTAINERS section is apparently incorrect.
> * Corrected rmap_walk_file() comment to refer to folios as per Matthew.
> * Reference folio->mapping rather than folio_mapping(folio) in rmap_walk_file()
>   as per Matthew.
> * Reference folio->index rather than folio_pgoff(folio) in rmap_walk_file() as
>   per Matthew.
> * Renamed rmap_wrprotect_file_page() to mapping_wrprotect_page() as per Matthew.
> * Fixed kerneldoc and moved to implementation as per Matthew.
> * Updated mapping_wrprotect_page() to take a struct page pointer as per David.
> * Removed folio lock when invoking mapping_wrprotect_page() in
>   fb_deferred_io_work() as per Matthew.
> * Removed compound_nr() in fb_deferred_io_work() as per Matthew.
>
> RFC v1:
> https://lore.kernel.org/all/1e452b5b65f15a9a5d0c2ed3f5f812fdd1367603.1736352361.git.lorenzo.stoakes@oracle.com/
>
> Lorenzo Stoakes (3):
>   mm: refactor rmap_walk_file() to separate out traversal logic
>   mm: provide mapping_wrprotect_page() function
>   fb_defio: do not use deprecated page->mapping, index fields
>
>  drivers/video/fbdev/core/fb_defio.c |  38 ++-----
>  include/linux/fb.h                  |   1 +
>  include/linux/rmap.h                |   3 +
>  mm/rmap.c                           | 152 +++++++++++++++++++++++-----
>  4 files changed, 141 insertions(+), 53 deletions(-)
>
> --
> 2.48.0