On Mon, 29 Jan 2024, Christoph Hellwig wrote:
> XFS is generally used on 64-bit, non-highmem platforms and xfile
> mappings are accessed all the time. Reduce our pain by not allowing
> any highmem mappings in the xfile page cache and remove all the kmap
> calls for it.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/xfarray.c | 3 +--
> fs/xfs/scrub/xfile.c | 21 +++++++++------------
> 2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/fs/xfs/scrub/xfarray.c b/fs/xfs/scrub/xfarray.c
> index 95ac14bceeadd6..d0f98a43b2ba0a 100644
> --- a/fs/xfs/scrub/xfarray.c
> +++ b/fs/xfs/scrub/xfarray.c
> @@ -580,7 +580,7 @@ xfarray_sort_get_page(
> * xfile pages must never be mapped into userspace, so we skip the
> * dcache flush when mapping the page.
> */
> - si->page_kaddr = kmap_local_page(si->xfpage.page);
> + si->page_kaddr = page_address(si->xfpage.page);
> return 0;
> }
>
> @@ -592,7 +592,6 @@ xfarray_sort_put_page(
> if (!si->page_kaddr)
> return 0;
>
> - kunmap_local(si->page_kaddr);
> si->page_kaddr = NULL;
>
> return xfile_put_page(si->array->xfile, &si->xfpage);
> diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c
> index 7e915385ef0011..a669ebbbc02d1d 100644
> --- a/fs/xfs/scrub/xfile.c
> +++ b/fs/xfs/scrub/xfile.c
> @@ -77,6 +77,12 @@ xfile_create(
> inode = file_inode(xf->file);
> lockdep_set_class(&inode->i_rwsem, &xfile_i_mutex_key);
>
> + /*
> + * We don't want to bother with kmapping data during repair, so don't
> + * allow highmem pages to back this mapping.
> + */
> + mapping_set_gfp_mask(inode->i_mapping, GFP_KERNEL);
I had been going to point you to inode_nohighmem(inode), which is how
that is usually done. But I share Matthew's uncertainty about cpusets
and the __GFP_HARDWALL inside GFP_USER: you may well be right to stick
with GFP_KERNEL, just as you've done it here. I suppose it depends on
whether you see this as a system operation or a user-invoked operation.
I did also think that perhaps you could mask off __GFP_FS here, to
avoid having to remember those memalloc_nofs_save()s elsewhere; but
that's probably a bad idea, I doubt anywhere else does it that way.
So, no change required, but I wanted to think about it.
Hugh
> +
> trace_xfile_create(xf);
>
> *xfilep = xf;
> @@ -126,7 +132,6 @@ xfile_load(
>
> pflags = memalloc_nofs_save();
> while (count > 0) {
> - void *p, *kaddr;
> unsigned int len;
>
> len = min_t(ssize_t, count, PAGE_SIZE - offset_in_page(pos));
> @@ -153,10 +158,7 @@ xfile_load(
> * xfile pages must never be mapped into userspace, so
> * we skip the dcache flush.
> */
> - kaddr = kmap_local_page(page);
> - p = kaddr + offset_in_page(pos);
> - memcpy(buf, p, len);
> - kunmap_local(kaddr);
> + memcpy(buf, page_address(page) + offset_in_page(pos), len);
> put_page(page);
>
> advance:
> @@ -221,14 +223,13 @@ xfile_store(
> * the dcache flush. If the page is not uptodate, zero it
> * before writing data.
> */
> - kaddr = kmap_local_page(page);
> + kaddr = page_address(page);
> if (!PageUptodate(page)) {
> memset(kaddr, 0, PAGE_SIZE);
> SetPageUptodate(page);
> }
> p = kaddr + offset_in_page(pos);
> memcpy(p, buf, len);
> - kunmap_local(kaddr);
>
> ret = aops->write_end(NULL, mapping, pos, len, len, page,
> fsdata);
> @@ -314,11 +315,7 @@ xfile_get_page(
> * to the caller and make sure the backing store will hold on to them.
> */
> if (!PageUptodate(page)) {
> - void *kaddr;
> -
> - kaddr = kmap_local_page(page);
> - memset(kaddr, 0, PAGE_SIZE);
> - kunmap_local(kaddr);
> + memset(page_address(page), 0, PAGE_SIZE);
> SetPageUptodate(page);
> }
>
> --
> 2.39.2
